fix: Error if given the wrong side of a one-one relationship #795

AndrewSisley · 2022-09-14T15:24:45Z

Relevant issue(s)

Resolves #513

Description

Errors nicely if given the wrong side of a one-one relationship (on write).

Specify the platform(s) on which this was tested:

Debian Linux

shahzadlone · 2022-09-15T05:26:50Z

client/errors.go

@@ -26,3 +25,7 @@ var (
 	ErrInvalidUpdater      = errors.New("The updater of a document is of invalid type")
 	ErrInvalidDeleteTarget = errors.New("The target document to delete is of invalid type")
 )
+
+func ErrFieldNotExist(name string) error {


praise: I Like this!

shahzadlone · 2022-09-15T05:31:28Z

db/collection.go

+}
+
+// isFieldRelationIdD returns true if the given field is the id field backing a relationship.
+func (c *collection) isFieldRelationIdD(fieldDescription *client.FieldDescription) bool {


nitpick: better name? does last character D stand for Description?

Yeah I have no idea, I am way to used to having these overloads use the same name, makes this kind of thing much cleaner IMO. Is not just Go that does this, I think Rust has this restriction too and I wonder why. What would you guys do?

you could alternatively do this:

func(fd *FieldDescription) isFieldRelationID(c *collection) bool {...}

lol, that is hacky pile of... 😆

hey... you wanted an alternative to D... there it is ;)

I don't have any good idea atm, as long as the name is meaningful haha, and we don't have to guess what D might mean for example.

Would suggest:

IsFieldRelationID
and
IsDescriptionRelationID

its not the best as they are both about fields, but one takes a string as a field, and the other a description as a field.

Really dont like the trailing D atm 😂

IsDescriptionRelationID

Description of what? :)

I tweaked the names slightly pre-merge, they are now a variant of John's suggestion

shahzadlone · 2022-09-15T05:34:59Z

tests/integration/collection/update/one_to_one/utils.go

+var schema = (`
+	type book {
+		name: String
+		rating: Float
+		author: author
+	}
+
+	type author {
+		name: String
+		age: Int
+		verified: Boolean
+		published: book @primary
+	}
+`)


Suggested change

var schema = (`

type book {

name: String

rating: Float

author: author

}

type author {

name: String

age: Int

verified: Boolean

published: book @primary

}

`)

var schema = `

type book {

name: String

rating: Float

author: author

}

type author {

name: String

age: Int

verified: Boolean

published: book @primary

}

`

Ops lol, will also change where I pasted this from 😆 Thanks

RM extra brackets

fredcarle

Looking good. Just a few comments.

fredcarle · 2022-09-15T21:34:13Z

db/collection.go

+		if !valid {
+			return false
+		}
+		if fd2.RelationType > 0 && fd2.RelationType&client.Relation_Type_Primary <= 0 {


thought: This looks like black magic. Can you add a comment to help with understanding?

I feel like fd2.RelationType&client.Relation_Type_Primary would be 0 or 128. Why <= and not ==?

Maybe:

// check that it's a relationship but not the primary.

cheers, I lazily copy-pasted this, I'll do it properly and document. I also thought I changed the variable names in here (from the WIP code), but looks like fd2 is still here lol

==

rename

fredcarle · 2022-09-15T21:38:41Z

db/collection.go

@@ -914,6 +919,31 @@ func (c *collection) tryGetSchemaFieldID(fieldName string) (uint32, bool) {
 	return uint32(0), false
 }

+// isFieldRelationId returns true if the given field is the id field backing a relationship.
+// Will return false if the field is not found at all.


nitpick: Will also return false.... It could lead to thinking that it returns false only if the field is not found.

will look at tweaking the wording

comment update

fredcarle · 2022-09-15T22:05:34Z

db/collection.go

+}
+
+// isFieldRelationIdD returns true if the given field is the id field backing a relationship.
+func (c *collection) isFieldRelationIdD(fieldDescription *client.FieldDescription) bool {


you could alternatively do this:

func(fd *FieldDescription) isFieldRelationID(c *collection) bool {...}

fredcarle · 2022-09-15T22:06:05Z

db/collection.go

@@ -914,6 +919,31 @@ func (c *collection) tryGetSchemaFieldID(fieldName string) (uint32, bool) {
 	return uint32(0), false
 }

+// isFieldRelationId returns true if the given field is the id field backing a relationship.
+// Will return false if the field is not found at all.
+func (c *collection) isFieldRelationId(fieldName string) bool {


nitpick: isFieldRelationID according to https://github.com/golang/go/wiki/CodeReviewComments#initialisms

Ah will leave as is :) We have so many like this, and I disagree with that anyway (makes it harder to spot the start/end of words)

nitpick: isFieldRelationID according to https://github.com/golang/go/wiki/CodeReviewComments#initialisms

LOL some of these go wiki code review comments crack me up, is there even any advantage of following minor subjective rules like those?

Consistency across the Go ecosystem.

This is why it's much easier, in most cases, to dive into new Go codebases than it is for most other languages.

This is why it's much easier, in most cases, to dive into new Go codebases than it is for most other languages.

I really disagree with that :) It is easier because it is a simple language, similar to JavaScript, with C-like typing. If they wanted to make things easier to pick up they should improve-the/write-some (non-blog-based) documentation instead of worrying about nitpicks like this

I think having guidelines like these help a lot. I find the Go documentation to be quite good. Much better than Rust (at least since the last time I looked at it) and incomparable to pretty much all the others. JavaScript is an easy language but there is no real unified guidelines on how to write it and it shows (same with python, ruby, etc.). Javascript codebases are usually as similar to one another as Quebec French and Belgium French dialects are to one another. Might as well learn a new language every time (I'm exaggerating here but barely). Following guideline also helps having a less developper preference discussions and less disparities between the different sections of the codebase. It makes the project approachable to newcomers and makes it easier to maintain open source projects.

It's important to look at this not from our own experienced developper's perspective, but from that of new devs that are potentially learning their first language. That's why I nitpick on those thing and I find it important to do so.

I think theres many aspects about Go that make it easy to jump into a new codebases. Eg, less features that are more orthogonal to eachother, leading to fewer options to implement various patterns. However nitpicking Id vs ID isn't really one of them. Its good to be consistent, but I've never been perplexed when seeing one vs the other.

I think we should be consistent, and I personally much prefer ID over Id but at the moment, its not 100% consistent across the codebase, so it should be a separate issue if we want it to be.

Yeah I don't care that much about ID vs Id or others similar capitalization guidelines specifically. I just care about the guidelines in general and I think we should follow them as much as possible for consistency of the ecosystem. I like to point them out to give people a reminder that they exist (And they have a good reason to exist). If we decide collectively that we prefer something else then so be it. I won't lose sleep on it 🙂

It is setting the author_id from the wrong side of the relationship, and is not relevant to the test anyway

jsimnz

Suggesting an alternative approach:

At the moment, you add a IsFieldRelation check to both the save and applyMerge as they are the two ways we actually mutate state.

A (potentially) more succinct way would be to change the code that generates the SchemaDescription to not generate the <type>_id field we don't care about. Which would result in not needed to do any of these checks, as both the tryGetField and the GetField calls would return false and we already return errors for !exists -> return ErrFieldNotExist.

We can go with this route youve implemented, but it
A) Requires making sure that the IsFieldRealtion check is always present in any future new mutation function (at the moment we have two, would need to be added for applyPatch which is todo).
B) Leaves the unused field on the SchemaDescription

jsimnz · 2022-09-16T20:44:08Z

client/errors.go

+func ErrFieldNotExist(name string) error {
+	return errors.New("The given field does not exist", errors.NewKV("Name", name))
+}


thought: Somewhat independant of this PR, We should make sure to establish a pattern for the codebase for "paramaterized" errors.

There is a consequence using a function like this as it won't allow a parent to use errors.Is(err, ErrFieldNotExist)

There is a consequence using a function like this as it won't allow a parent to use

Good point, would want to keep the string publicly available for that, might tweak pre merge

changed to allow for Is.

jsimnz · 2022-09-16T20:53:50Z

db/collection.go

@@ -914,6 +919,31 @@ func (c *collection) tryGetSchemaFieldID(fieldName string) (uint32, bool) {
 	return uint32(0), false
 }

+// isFieldRelationId returns true if the given field is the id field backing a relationship.
+// Will return false if the field is not found at all.
+func (c *collection) isFieldRelationId(fieldName string) bool {


I think theres many aspects about Go that make it easy to jump into a new codebases. Eg, less features that are more orthogonal to eachother, leading to fewer options to implement various patterns. However nitpicking Id vs ID isn't really one of them. Its good to be consistent, but I've never been perplexed when seeing one vs the other.

I think we should be consistent, and I personally much prefer ID over Id but at the moment, its not 100% consistent across the codebase, so it should be a separate issue if we want it to be.

jsimnz · 2022-09-16T20:58:08Z

db/collection.go

+}
+
+// isFieldRelationIdD returns true if the given field is the id field backing a relationship.
+func (c *collection) isFieldRelationIdD(fieldDescription *client.FieldDescription) bool {


Would suggest:

IsFieldRelationID
and
IsDescriptionRelationID

its not the best as they are both about fields, but one takes a string as a field, and the other a description as a field.

Really dont like the trailing D atm 😂

AndrewSisley · 2022-09-16T21:18:44Z

A (potentially) more succinct way would be to change the code that generates the SchemaDescription to not generate the _id field we don't care about.

Could be good, but I don't know what else that might impact, and if you really want that route it can go in for 0.4.0 (is 5:20pm on Friday of the release lol)

jsimnz · 2022-09-16T21:23:18Z

Could be good, but I don't know what else that might impact, and if you really want that route it can go in for 0.4.0 (is 5:20pm on Friday of the release lol)

LOL. Fair ;)

jsimnz · 2022-09-16T21:24:25Z

LGTM. Merge at your convenience based on whatever you wanted to change pre-merge.

codecov · 2022-09-16T21:39:12Z

Codecov Report

Merging #795 (919ddb4) into develop (0fe9dd3) will increase coverage by 0.06%.
The diff coverage is 66.66%.

@@             Coverage Diff             @@
##           develop     #795      +/-   ##
===========================================
+ Coverage    59.61%   59.67%   +0.06%     
===========================================
  Files          154      155       +1     
  Lines        17257    17283      +26     
===========================================
+ Hits         10287    10313      +26     
  Misses        6044     6044              
  Partials       926      926

Impacted Files	Coverage Δ
client/document.go	`64.72% <20.00%> (ø)`
db/collection.go	`65.99% <70.00%> (+0.07%)`	⬆️
db/collection_update.go	`56.48% <75.00%> (+1.00%)`	⬆️
client/descriptions.go	`84.37% <100.00%> (+1.04%)`	⬆️
client/errors.go	`100.00% <100.00%> (ø)`
query/graphql/planner/update.go	`75.00% <0.00%> (+3.40%)`	⬆️

…etwork#795) * Remove extra brackets * Move collection update simple tests into dir * Remove invalid and irrelevant test data It is setting the author_id from the wrong side of the relationship, and is not relevant to the test anyway * Include field name in missing field error * Error if creating 1-1 from wrong side

AndrewSisley added bug Something isn't working area/query Related to the query component area/collections Related to the collections system action/no-benchmark Skips the action that runs the benchmark. labels Sep 14, 2022

AndrewSisley added this to the DefraDB v0.3.1 milestone Sep 14, 2022

AndrewSisley requested a review from a team September 14, 2022 15:24

AndrewSisley self-assigned this Sep 14, 2022

AndrewSisley changed the title ~~fix: Error nicely if given the wrong side of a one-one relationship~~ fix: Error if given the wrong side of a one-one relationship Sep 14, 2022

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch 2 times, most recently from 7ff3df2 to 914cb92 Compare September 14, 2022 18:34

shahzadlone reviewed Sep 15, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch from 914cb92 to efe08ae Compare September 15, 2022 15:40

AndrewSisley requested a review from shahzadlone September 15, 2022 15:41

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch from efe08ae to 9576fec Compare September 15, 2022 19:38

fredcarle reviewed Sep 15, 2022

View reviewed changes

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch 3 times, most recently from 2c0991b to b8d713c Compare September 16, 2022 16:46

AndrewSisley requested a review from fredcarle September 16, 2022 16:59

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch from b8d713c to 5153e8c Compare September 16, 2022 18:39

AndrewSisley added 4 commits September 16, 2022 15:21

Remove extra brackets

bbdb62d

Move collection update simple tests into dir

a4999d6

Remove invalid and irrelevant test data

707451f

It is setting the author_id from the wrong side of the relationship, and is not relevant to the test anyway

Include field name in missing field error

a274d12

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch from 5153e8c to dd287b8 Compare September 16, 2022 19:21

Error if creating 1-1 from wrong side

4c0c538

jsimnz requested changes Sep 16, 2022

View reviewed changes

jsimnz approved these changes Sep 16, 2022

View reviewed changes

FIXUP - re-expose ErrFieldNotExist without param

1b8af36

AndrewSisley force-pushed the sisley/fix/I513-one-one-wrong-side branch from dd287b8 to 1b8af36 Compare September 16, 2022 21:35

FIXUP - tweak func names

919ddb4

AndrewSisley merged commit f294f19 into develop Sep 16, 2022

AndrewSisley deleted the sisley/fix/I513-one-one-wrong-side branch September 16, 2022 21:48

fix: Error if given the wrong side of a one-one relationship #795

fix: Error if given the wrong side of a one-one relationship #795

Conversation

AndrewSisley commented Sep 14, 2022

Relevant issue(s)

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Sep 15, 2022 • edited Loading

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

shahzadlone Sep 16, 2022 • edited Loading

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

AndrewSisley Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Sep 15, 2022 • edited Loading

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

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

jsimnz left a 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

Choose a reason for hiding this comment

AndrewSisley commented Sep 16, 2022

jsimnz commented Sep 16, 2022

jsimnz commented Sep 16, 2022

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

AndrewSisley Sep 15, 2022 •

edited

Loading

shahzadlone Sep 16, 2022 •

edited

Loading

AndrewSisley Sep 15, 2022 •

edited

Loading

AndrewSisley Sep 15, 2022 •

edited

Loading

AndrewSisley Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading