fix: Rework relation field kinds #2961
Conversation
AndrewSisley
added
bug
AndrewSisley
force-pushed
the
2619-self-field-kind
branch
from
7b33009 to
45d9dcc
Compare
AndrewSisley
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2961 +/- ##
===========================================
- Coverage 79.28% 79.26% -0.02%
===========================================
Files 326 328 +2
Lines 24778 25207 +429
===========================================
+ Hits 19644 19980 +336
- Misses 3715 3791 +76
- Partials 1419 1436 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
AndrewSisley
force-pushed
the
2619-self-field-kind
branch
from
45d9dcc to
d66f73f
Compare
There was a problem hiding this comment.
This is great work Andy! I like the complex schema being treated as a single schema ID with parts. The code is nice. There are only a few places where naming caused a bit of confusion on my end and I gave a few suggestion to alleviate it. Feel free to merge once the conversations are resolved.
internal/db/schema_id.go
Outdated
| return err | ||
| } | ||
|
|
||
| setIDs(setID, schemaSet) |
There was a problem hiding this comment.
thought: The close proximity of both set as a verb and set as a noun confused me a few time while reading this function. Maybe it would be worth changing the noun to something else. Maybe something like generateGroupID would be better?
There was a problem hiding this comment.
Lol - nice spot, yeah I'll change that to generateGroupID or similar.
- rename
setIDs
There was a problem hiding this comment.
Renamed to assignIDs, as the func does not generate the ids
client/definitions.go
Outdated
| // GetDefinitionUncached returns the definition that the given [FieldKind] points to, if it is found in the given store. | ||
| // | ||
| // If the related definition is not found, or an error occurs, default and false will be returned. | ||
| func GetDefinitionUncached( |
There was a problem hiding this comment.
suggestion: GetDefinitionUncached is a confusing name. I would suggest instead being more explicit with GetDefinitionFromCache and GetDefinitionFromStore.
There was a problem hiding this comment.
I like this, thanks.
- Rename GetDefinitionUncached
There was a problem hiding this comment.
Leaving GetDefinition as is, as it should be the default IMO. GetDefinitionUncached has been renamed to GetDefinitionFromStore
| if _, ok := schemaVersionsAdded[schema.VersionID]; ok { | ||
| continue | ||
| } | ||
| // newDefinitionState creates a new definitionState object given the provided |
There was a problem hiding this comment.
suggestion: The documentation for the function and the one above are too similar. This one should probably have the provided definition instead of description. Maybe more explicit names would help here as well: newDefinitionStateFromDef and newDefinitionStateFromDesc
There was a problem hiding this comment.
Not a problem, I'll have a look and tweak them a bit.
- Review
newDefinitionStates plus docs
There was a problem hiding this comment.
Docs tweaked, renamed the first col-only func to newDefinitionStateFromCols (newDefinitionState should be the default func to use)
There was a problem hiding this comment.
FromCols is not really clarifying anything though as they are both using collectionSomething.
| ctx context.Context, | ||
| db *db, | ||
| newState *definitionState, | ||
| oldState *definitionState, |
There was a problem hiding this comment.
suggestion: You can do _ *definitionState istead of oldSate *definitionState. This way it shows that this is there to follow a given function signature but that the field isn't used within.
There was a problem hiding this comment.
I'm probably going to leave this one as is, this func (and most other funcs in this file) satisfies an interface and they all have the same prop names and I think I prefer consistency over worrying about which funcs use which props (also aids copy-pasting the sig).
- Have a quick look when looking at
newDefinitionState
I stumbled across this accidentally, it is not relevant to this PR.
No tests have been modified. a 'relations' file does not scale particularly well, and there is already a one_one file.
They have since been replaced by integration tests and are really quite annoying to maintain.
The value implies it has meaning and is checked, which it does not
Before this fix the test framework would panic when using things like DocIndexes for renamed collections.
AndrewSisley
force-pushed
the
2619-self-field-kind
branch
from
d66f73f to
462e9ef
Compare
AndrewSisley
force-pushed
the
2619-self-field-kind
branch
from
462e9ef to
f0f1a10
Compare
## Relevant issue(s) Resolves #3074 ## Description Correctly handles validation of nearby relation fields in schema patch. Previously the equality check failed to account for `Kind` being a pointer and thus always flagged relation fields as having mutated. This was likely introduced in #2961 when `Kind` became a pointer. Collections referenced by relation fields were also not included in the validation set, causing the rules to think that the related object did not exist.
Relevant issue(s)
Resolves #2619 #2493
Description
Reworks relation field kinds so that relations are not defined by names (which are mutable).
This PR replaces the old
ObjectKindandObjectArrayKinds withCollectionKind,SchemaKindandSelfKind.NamedKindhas also been added - it allows users to interact with the definitions using their names (like before), before the kind gets translated into one of the other new kinds before storage.Because Schemas must form a DAG (because of their CIDs (SchemaVersionID and Root), Schemas defined within the same action that form circular relations (e.g. User=>Dog=>User) have their CIDs grouped into a set with the same base id, plus an index relative to their lexographical order (name) within the set suffixed onto their CIDs. This change is not user visible, besides the
-[index]suffix on their CIDs.Quite a lot had to change to facilitate this, and only a handful of things have been broken out to separate commits. Most of the work is in commit
Rework relation field kinds- when reviewing that commit, I suggest starting withtests/integration/collection_description/updates/replace/name_one_many_test.goto see the original bug, followed by a quick scan ofclient/schema_field_description.go, then a look attests/integration/schema/self_ref_test.gobefore properly reviewing the changes. The bulk of the self reference logic is ininternal/db/schema_id.go.Please give me a shout if you have any questions! I think this should be the last structural change to collection/schema definitions :)