feat: Auto-create relation id fields via PatchSchema #1807

AndrewSisley · 2023-08-22T17:01:45Z

Relevant issue(s)

Resolves #1773

Description

Auto-creates relation id fields via PatchSchema if not explicitly defined by the user.

Otherwise any mutations done within the updateCollection function will not be fully applied. This becomes important soon when we add fields within that function.

codecov · 2023-08-22T17:11:46Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (63a57ba) 75.74% compared to head (c681e71) 75.75%.

@@             Coverage Diff             @@
##           develop    #1807      +/-   ##
===========================================
+ Coverage    75.74%   75.75%   +0.01%     
===========================================
  Files          209      209              
  Lines        22200    22211      +11     
===========================================
+ Hits         16814    16825      +11     
- Misses        4222     4223       +1     
+ Partials      1164     1163       -1

Flag	Coverage Δ
all-tests	`75.75% <100.00%> (+0.01%)`	⬆️

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

Files Changed	Coverage Δ
db/collection.go	`72.57% <100.00%> (+0.26%)`	⬆️
db/schema.go	`78.05% <100.00%> (+0.27%)`	⬆️

... and 5 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 63a57ba...c681e71. Read the comment docs.

fredcarle

Approving as long as the comments get resolved :)

fredcarle · 2023-08-23T04:13:45Z

tests/integration/schema/updates/add/field/kind/foreign_object_array_test.go

+					Users {
+						name
+						foo {
+							name
+						}
+						foobar {
+							name
+						}
+					}


suggestion: To make it more explicit to the reader that foobar_id is set, I suggested returning it in the selection set.

Fair enough, will add.

Add foobar_id to selection set

fredcarle · 2023-08-23T04:16:41Z

tests/integration/schema/updates/add/field/kind/foreign_object_array_test.go

+				Schema: `
+					type Users {
+						name: String
+					}
+				`,


suggestion: Although it is interesting to show that this is possible, it would be nice to have a test also for two separate schemas. It would help to show that the relation_id field is only set on the primary.

Although it is interesting to show that this is possible, it would be nice to have a test also for two separate schemas

I'm not sure I follow what makes this interesting, it is just a normal one-many relationship and adding a second schema to the test just complicates things.

It would help to show that the relation_id field is only set on the primary.

I can't see how that would help, how do you see it? I do think it is worth adding a test though to verify that the _id field does not exist on the many side.

Assert _id field does not exist on many side.

I'm not sure I follow what makes this interesting, it is just a normal one-many relationship and adding a second schema to the test just complicates things.

It's interesting that we show that we can add self-referencing relationships. But that mean that everything is happening on the one collection. Having two separate collections would ensure that the operation isn't broken when it's not just one collection being manipulated.

I see what you mean, but that is not specific to the changes in this PR (this feature actually is one of the least relevant, as it never cares which object it is hosted on). I'll add a ticket to close the test gap for other related behaviour though.

Add test-ticket

In my mind it is very related as we need to ensure that the auto-creation is behaving properly for all objects involved.

integration test which involves 2 add operations

This is why it should be done in another ticket. It should be tested with a simpler use case that isn't cluttered with irrelevancies.

It is irrelevant to the changes in this PR.

Isn't too different to trying to force a developer working on filters group by to test with a range of groupby order configurations, just because orderby isnt properly tested yet. OrderBy should be tested, but in a simpler context where extras dont confuse the test. It also means that if groupBy-filter is removed as a feature, groupBy-orderby is still tested.

And please remember, I am not objecting to the testing of PatchSchema relation definitions for multiple objects (even though it is technically not needed given the implementation). I am say it should be done in another ticket, as it is not relevant here and will only complicate and dirty both work items.

I see it as related to the change in this PR. You're using an integration test to test the feature. The integration test doesn't care where the auto-create happens. It just cares that it happens. The most common way of having references is from one object to an other. Not from object to self. It would be quite appropriate to have a test that ensures that the feature works in the most common reference pattern where 2 objects are involved. It would also ensure that a change in the future doesn't result in only one side of the relationship having the relation id auto created.

If you really don't want to do it here, it should probably be done shortly so we don't forget about it.

fredcarle · 2023-08-23T04:17:18Z

tests/integration/schema/updates/add/field/kind/foreign_object_test.go

+				Schema: `
+					type Users {
+						name: String
+					}
+				`,


suggestion: Same as above. It would be nice to have a test for two separate schemas. In this case it would help show that the relation_id is set on both sides.

Same as above. It would be nice to have a test for two separate schemas

Leaving that convo against the other comment.

shahzadlone

LGTM.

Just 1 todo

shahzadlone · 2023-08-23T12:21:33Z

db/collection.go

@@ -244,6 +244,20 @@ func (db *db) updateCollection(
 		return db.getCollectionByName(ctx, txn, desc.Name)
 	}

+	for _, field := range desc.Schema.Fields {
+		if field.RelationType.IsSet(client.Relation_Type_ONE) {
+			idFieldName := field.Name + "_id"


Todo: instead use the defined id constant.

Will change, cheers.

const _id

) ## Relevant issue(s) Resolves sourcenetwork#1773 ## Description Auto-creates relation id fields via PatchSchema if not explicitly defined by the user.

AndrewSisley added 2 commits August 22, 2023 12:48

Use returned collection description

62800e0

Otherwise any mutations done within the updateCollection function will not be fully applied. This becomes important soon when we add fields within that function.

Autocreate relation id fields if not user-defined

de4c5a5

AndrewSisley added feature New feature or request area/schema Related to the schema system action/no-benchmark Skips the action that runs the benchmark. labels Aug 22, 2023

AndrewSisley added this to the DefraDB v0.7 milestone Aug 22, 2023

AndrewSisley requested a review from a team August 22, 2023 17:01

AndrewSisley self-assigned this Aug 22, 2023

fredcarle approved these changes Aug 23, 2023

View reviewed changes

shahzadlone approved these changes Aug 23, 2023

View reviewed changes

AndrewSisley added 3 commits August 23, 2023 12:16

PR FIXUP - Add foo_id to selection set

41ff69c

PR FIXUP - Use const for _id

bff876d

PR FIXUP - Assert id field not auto created on many side

c681e71

AndrewSisley merged commit 34329fd into sourcenetwork:develop Aug 23, 2023
12 checks passed

AndrewSisley deleted the 1773-auto-rel-id-fields branch August 23, 2023 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Auto-create relation id fields via PatchSchema #1807

feat: Auto-create relation id fields via PatchSchema #1807

AndrewSisley commented Aug 22, 2023

codecov bot commented Aug 22, 2023 •

edited

fredcarle left a comment

fredcarle Aug 23, 2023

AndrewSisley Aug 23, 2023 •

edited

fredcarle Aug 23, 2023

AndrewSisley Aug 23, 2023 •

edited

fredcarle Aug 23, 2023

AndrewSisley Aug 23, 2023 •

edited

fredcarle Aug 23, 2023

AndrewSisley Aug 23, 2023

AndrewSisley Aug 23, 2023

AndrewSisley Aug 23, 2023

AndrewSisley Aug 23, 2023

fredcarle Aug 23, 2023 •

edited

fredcarle Aug 23, 2023

AndrewSisley Aug 23, 2023

shahzadlone left a comment

shahzadlone Aug 23, 2023

AndrewSisley Aug 23, 2023 •

edited

feat: Auto-create relation id fields via PatchSchema #1807

feat: Auto-create relation id fields via PatchSchema #1807

Conversation

AndrewSisley commented Aug 22, 2023

Relevant issue(s)

Description

codecov bot commented Aug 22, 2023 • edited

Codecov Report

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle Aug 23, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Aug 23, 2023 • edited

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 •

edited

AndrewSisley Aug 23, 2023 •

edited

AndrewSisley Aug 23, 2023 •

edited

AndrewSisley Aug 23, 2023 •

edited

fredcarle Aug 23, 2023 •

edited

AndrewSisley Aug 23, 2023 •

edited