feat: Move relation field properties onto collection #2529

AndrewSisley · 2024-04-15T23:48:09Z

Relevant issue(s)

Resolves #2451 #1911 #2408

Description

Moves relation field properties onto collection description from schema description.

This allows for one-sided relations to be defined via PatchSchema. There is no way atm to create them using an SDL, but we could add a directive or something at somepoint if we want.

As the adding of fields via PatchCollection has not yet been enabled, this does prevent users from adding secondary relation fields to an existing schema. I'll create a ticket before merging this to allow this, it is my strong preference to not do that in this already large and fiddly PR though.

As the internal codebase relies on the setting of secondary fields via client.Document, client.Document now requires a collection, not just the schema, when being constructed. We will likely want to find a way to avoid that in the future.

This PR also moves some validation from the graphql package into db, not all of it has been moved, but that is a long term wish of mine. The db package validation can/should be improved further making rule reuse across PatchCollection, PatchSchema and CreateCollection (SDL), however I would rather not spend too much effort on that in this PR. This includes moving it out of the collection.go file.

It might resolve #2380 but I'd rather not bring that into scope. If I'm waiting around for reviews I might verify that here though.

This should conclude the transfer of local properties off of the schema object :)

Commits should be clean, however the vast majority of the work was done in Move relation field properties onto collection from schema.

Todos (now)

GQL mutations seem to be failing in CI

Todos (post review)

Create ticket to allow the adding of secondary relations via PatchCollection
Create ticket to further improve db schema/col validation

codecov · 2024-04-16T00:05:06Z

Codecov Report

Attention: Patch coverage is 88.93281% with 56 lines in your changes are missing coverage. Please review.

Project coverage is 75.59%. Comparing base (734b326) to head (4e5eec4).

❗ Current head 4e5eec4 differs from pull request most recent head 137e943. Consider uploading reports for the commit 137e943 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2529      +/-   ##
===========================================
- Coverage    75.81%   75.59%   -0.22%     
===========================================
  Files          293      292       -1     
  Lines        27924    28180     +256     
===========================================
+ Hits         21168    21301     +133     
- Misses        5411     5510      +99     
- Partials      1345     1369      +24

Flag	Coverage Δ
all-tests	`75.59% <88.93%> (-0.22%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
client/collection_description.go	`79.73% <100.00%> (+1.35%)`	⬆️
client/definitions.go	`100.00% <100.00%> (+6.06%)`	⬆️
client/document.go	`62.20% <100.00%> (-4.10%)`	⬇️
client/errors.go	`62.71% <ø> (-6.73%)`	⬇️
db/backup.go	`58.63% <100.00%> (-0.72%)`	⬇️
db/collection_get.go	`74.14% <100.00%> (+4.65%)`	⬆️
db/collection_index.go	`86.20% <100.00%> (-0.03%)`	⬇️
db/collection_update.go	`71.10% <100.00%> (-2.17%)`	⬇️
db/errors.go	`64.67% <100.00%> (-0.46%)`	⬇️
db/fetcher/encoded_doc.go	`76.14% <100.00%> (ø)`
... and 14 more

... and 47 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 734b326...137e943. Read the comment docs.

fredcarle · 2024-04-16T01:59:48Z

client/definitions.go

+		return NewLocalFieldDefinition(
+			collectionField,
+		), true
+	} else if !existsOnCollection && existsOnSchema {


question: Can you explain how this will ever be the case? Even the embedded object doesn't seem to make sense to me here.

We have this at the moment, e.g. related objects within a view (declared with interface instead of type in an SDL).

Embedded objects do not have 'stuff' declared on a collection description, they are schema-only.

(discussed in a call)

todo: The documentation for local-only vs local-global vs global-only fields is insufficient and dispersed. I will add more public documentation to the relevant client types/props.

Expand docs

nasdf

I don't see any blocking issues. Looks great considering the impact of the changes.

nasdf · 2024-04-16T18:05:54Z

db/collection.go

+	validateSingleSidePrimary,
+}
+
+func (db *db) validateNewCollection(


praise: this is a nice pattern

thought: may be worth moving to db_validators.go at some point

:) Cheers

thought: may be worth moving to db_validators.go at some point

I want to move them somewhere, and there are other improvements that can be made too, but I started doing it in here before pausing and deciding that it would require too much thought for an already fiddly PR (noted in the description, I'll be creating a ticket once people are happy with this PR being merged).

nasdf · 2024-04-16T18:16:19Z

request/graphql/schema/relations.go

praise: nice to not need this anymore

We'll need to call this somewhere else soon too.

Is really not worth having two slightly different messages for the same thing

Soon, secondary fields will not exist on the schema

Old name was incorrect, as it gets the field ID (hosted on collection) from the collection definition, not the schema. Should have been changed a while ago

Not from relation name, which is no longer required for relations

From schema.

Broken out of 'Move relation field properties onto collection' as to not clutter the diff for reviewers.

Secondary fields do not exist in the schema, and thus schema.fields is incorrect here.

fredcarle

LGTM. Right now I'm a little worried that we have made our Schema and Collection system more complex than it needs to be. Hopefully the future will put those worry to rest :)

fredcarle · 2024-04-19T18:29:23Z

request/graphql/schema/collection.go

+						// An _id field is added for every 1-1 or 1-N relationship from this object if the relation
+						// does not point to an embedded object.
+						//
+						// It is inserted immediately after the object field for make things nicer for the user.


typo: field to make

😁 will fix

Fix typo

AndrewSisley · 2024-04-19T19:18:25Z

Right now I'm a little worried that we have made our Schema and Collection system more complex than it needs to be.

I understand that, but I'm not really sure there is much we can do to simplify it without breaking the local/global boundary, or by falling back into the pointer-based problems we used to have.

I am a bit concerned at the increased prominence of CollectionDefinition, but will see how that plays out for a bit.

AndrewSisley added feature New feature or request area/schema Related to the schema system area/collections Related to the collections system refactor This issue specific to or requires *notable* refactoring of existing codebases and components labels Apr 15, 2024

AndrewSisley added this to the DefraDB v0.11 milestone Apr 15, 2024

AndrewSisley requested a review from a team April 15, 2024 23:48

AndrewSisley self-assigned this Apr 15, 2024

AndrewSisley changed the title ~~feat: Move relation field properties onto collection from schema~~ feat: Move relation field properties onto collection Apr 15, 2024

fredcarle reviewed Apr 16, 2024

View reviewed changes

AndrewSisley force-pushed the 2451-mv-sec-rel-fields branch 2 times, most recently from 6610a0a to 297faeb Compare April 16, 2024 15:33

AndrewSisley requested a review from fredcarle April 16, 2024 15:33

nasdf reviewed Apr 16, 2024

View reviewed changes

AndrewSisley force-pushed the 2451-mv-sec-rel-fields branch from 297faeb to e26ee8b Compare April 16, 2024 18:41

AndrewSisley added 10 commits April 19, 2024 13:39

Extract parsing of FieldKind to private func

32d9cec

We'll need to call this somewhere else soon too.

Remove duplicate error message

819c1cc

Is really not worth having two slightly different messages for the same thing

Resolve sec. mapping using ColDef

f917253

Soon, secondary fields will not exist on the schema

Rename tryGetFieldID

bfbfc6d

Old name was incorrect, as it gets the field ID (hosted on collection) from the collection definition, not the schema. Should have been changed a while ago

Determine relationshipness in mapper from Kind

0cfaaf4

Not from relation name, which is no longer required for relations

Move relation field properties onto collection

f168c1c

From schema.

FIXUP - Fix schema version id tests

76077b1

Broken out of 'Move relation field properties onto collection' as to not clutter the diff for reviewers.

Document breaking change

042c19a

PR FIXUP - Use field definitions generating gql inputs

d3a3371

Secondary fields do not exist in the schema, and thus schema.fields is incorrect here.

PR FIXUP - Expand documentation on local vs global fields

9c4fe72

AndrewSisley force-pushed the 2451-mv-sec-rel-fields branch from a759a7c to 9c4fe72 Compare April 19, 2024 17:46

AndrewSisley requested a review from nasdf April 19, 2024 18:09

fredcarle approved these changes Apr 19, 2024

View reviewed changes

PR FIXUP - Fix typo in code comment

137e943

AndrewSisley merged commit 4a75f23 into sourcenetwork:develop Apr 19, 2024
28 of 30 checks passed

AndrewSisley deleted the 2451-mv-sec-rel-fields branch April 19, 2024 19:46

AndrewSisley mentioned this pull request Jul 17, 2024

One sided primary relations are not queryable #2830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Move relation field properties onto collection #2529

feat: Move relation field properties onto collection #2529

AndrewSisley commented Apr 15, 2024 •

edited

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading

fredcarle Apr 16, 2024

AndrewSisley Apr 16, 2024 •

edited

Loading

AndrewSisley Apr 18, 2024 •

edited

Loading

nasdf left a comment

nasdf Apr 16, 2024

AndrewSisley Apr 16, 2024

nasdf Apr 16, 2024

fredcarle left a comment

fredcarle Apr 19, 2024

AndrewSisley Apr 19, 2024 •

edited

Loading

AndrewSisley commented Apr 19, 2024

feat: Move relation field properties onto collection #2529

feat: Move relation field properties onto collection #2529

Conversation

AndrewSisley commented Apr 15, 2024 • edited Loading

Relevant issue(s)

Description

Todos (now)

Todos (post review)

codecov bot commented Apr 16, 2024 • edited Loading

Codecov Report

fredcarle Apr 16, 2024

Choose a reason for hiding this comment

AndrewSisley Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

nasdf left a comment

Choose a reason for hiding this comment

nasdf Apr 16, 2024

Choose a reason for hiding this comment

AndrewSisley Apr 16, 2024

Choose a reason for hiding this comment

nasdf Apr 16, 2024

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

fredcarle Apr 19, 2024

Choose a reason for hiding this comment

AndrewSisley Apr 19, 2024 • edited Loading

Choose a reason for hiding this comment

AndrewSisley commented Apr 19, 2024

AndrewSisley commented Apr 15, 2024 •

edited

Loading

codecov bot commented Apr 16, 2024 •

edited

Loading

AndrewSisley Apr 16, 2024 •

edited

Loading

AndrewSisley Apr 18, 2024 •

edited

Loading

AndrewSisley Apr 19, 2024 •

edited

Loading