feat: Persist schema version at time of commit #1055
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AndrewSisley
added
feature
Codecov Report
@@ Coverage Diff @@
## develop #1055 +/- ##
===========================================
- Coverage 68.10% 68.09% -0.02%
===========================================
Files 172 172
Lines 16250 16246 -4
===========================================
- Hits 11067 11062 -5
- Misses 4251 4253 +2
+ Partials 932 931 -1
|
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
2 times, most recently
from
90db176 to
4a96aaa
Compare
shahzadlone
reviewed
Jan 26, 2023
client/descriptions.go
Outdated
| Name string | ||
| // SchemaId is the version agnostic identifier for this schema. | ||
| // | ||
| // It remains constant throught the lifetime of this schema. |
There was a problem hiding this comment.
:) Cheers
- typo
client/descriptions.go
Outdated
| @@ -100,7 +100,17 @@ func (index IndexDescription) IDString() string { | |||
|
|
|||
| // SchemaDescription describes a Schema and its associated metadata. | |||
| type SchemaDescription struct { | |||
| Name string | |||
| // SchemaId is the version agnostic identifier for this schema. | |||
There was a problem hiding this comment.
Suggested change
| // SchemaId is the version agnostic identifier for this schema. | |
| // SchemaID is the version agnostic identifier for this schema. |
There was a problem hiding this comment.
🤮 Will change, I think I find remembering this Go preference even harder than using US/Canadian English lol
- ID
There was a problem hiding this comment.
Lol... Of course this means updating all the test CIDs again...
- Update test cids
client/descriptions.go
Outdated
| // SchemaId is the version agnostic identifier for this schema. | ||
| // | ||
| // It remains constant throught the lifetime of this schema. | ||
| SchemaId string |
There was a problem hiding this comment.
Suggested change
| SchemaId string | |
| SchemaID string |
client/descriptions.go
Outdated
Comment on lines
108
to
113
| // VersionId is the version-specific identifier for this schema. | ||
| // | ||
| // It is generated on mutation of this schema and can be used to uniquely | ||
| // identify a schema at a specific version. | ||
| VersionId string |
There was a problem hiding this comment.
Suggested change
| // VersionId is the version-specific identifier for this schema. | |
| // | |
| // It is generated on mutation of this schema and can be used to uniquely | |
| // identify a schema at a specific version. | |
| VersionId string | |
| // VersionID is the version-specific identifier for this schema. | |
| // | |
| // It is generated on mutation of this schema and can be used to uniquely | |
| // identify a schema at a specific version. | |
| VersionID string |
fredcarle
reviewed
Jan 26, 2023
jsimnz
requested changes
Jan 26, 2023
core/crdt/composite.go
Outdated
| // SchemaVersionKey is the schema version datastore key at the time of commit. | ||
| // | ||
| // It can be used to identify the collection datastructure state at time of commit. | ||
| SchemaVersionKey string |
There was a problem hiding this comment.
question: Why use the CollectionSchemaVersionKey which when serialized into a string has the /collection/version prefix. That shouldn't be persisted into the DAG.
Should only be the ID (CID) part thats persisted.
There was a problem hiding this comment.
suggestion: Key isnt a needed suffix here on the field name. My understanding based on the discord conversation is that its used to refer to literal kvstore keys, but the CompositeDAGDelta shouldn't have kv information, as its all global data that is persisted into the DAG. Simply SchemaVersion or SchemaVersionID is sufficient.
There was a problem hiding this comment.
:) Makes a lot of sense - will change so that just the cid is stored. I probably ToStringed it on autopilot, it protects against the addition of any new props, but is quite wasteful here.
- only persist cid
db/collection.go
Outdated
| @@ -159,6 +154,16 @@ func (db *db) CreateCollection( | |||
|
|
|||
| // For new schemas the initial version id will match the schema id | |||
| schemaVersionId := schemaId | |||
|
|
|||
| col.desc.Schema.VersionId = schemaVersionId | |||
There was a problem hiding this comment.
question(out side of scope): Once migrations actually get added, is the schemaVersionID going to be the CID of the full schema after field changes, or is it going to be the CID of the patch difference?
There was a problem hiding this comment.
Full schema, unless that changes for some unforeseen reason.
I dont think that changing should affect the usage here though, and public function that fetches a schema by version id should be pretty explicit as to whether it returns full or patch, and it should be capable of constructing the full from patch in the off chance that it would need to do so on the fly.
Will be added to shortly
Breaks some tests as although the property is always empty it is accounted for in CID generation.
Note: The empty unused prop was used to construct some cids, removing it thus changes the cids even though it was always empty.
The primary driving change in this commit is the addition of SchemaVersionKey to CompositeDAGDelta in core/crdt/composite.go, this persists the schema state alongside the document change. All other changes in this commit are to facilitate this change. This commit got a bit more involved than I intended as getCollectionByVersionId was regenerating the schema id instead of taking it from the object it already had - possibly because the property on the object that it already had never actualy set the property. This got a bit awkward as the id lives on the object it is generated from - when the prop was populated on regeneration of itself it was taken into account as part of the regenerated key causing it to not match the originally (saved) key. Cids in the tests have been corrected in this commit for the production code changes in this commit and the prior BREAKING commits (to save fixing them 3 times).
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
from
4a96aaa to
5125e79
Compare
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
from
2e8e98b to
ce15526
Compare
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
4 times, most recently
from
b1fba09 to
6955ad2
Compare
jsimnz
approved these changes
Jan 26, 2023
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
from
6955ad2 to
83ef435
Compare
AndrewSisley
force-pushed
the
sisley/feat/I1006-schema-version-commit-q
branch
5 times, most recently
from
a1b6368 to
3904bf3
Compare
fredcarle
approved these changes
Jan 26, 2023
shahzadlone
pushed a commit
that referenced
this pull request
Apr 13, 2023
* Remove unused dag.GetSchemaID func * Break up long lines Will be added to shortly * BREAKING - Remove unused SchemaId prop Breaks some tests as although the property is always empty it is accounted for in CID generation. * BREAKING - Remove unused Schema.Key prop Note: The empty unused prop was used to construct some cids, removing it thus changes the cids even though it was always empty. * Replace commit schemaId with versionKey The primary driving change in this commit is the addition of SchemaVersionKey to CompositeDAGDelta in core/crdt/composite.go, this persists the schema state alongside the document change. All other changes in this commit are to facilitate this change. This commit got a bit more involved than I intended as getCollectionByVersionId was regenerating the schema id instead of taking it from the object it already had - possibly because the property on the object that it already had never actualy set the property. This got a bit awkward as the id lives on the object it is generated from - when the prop was populated on regeneration of itself it was taken into account as part of the regenerated key causing it to not match the originally (saved) key. Cids in the tests have been corrected in this commit for the production code changes in this commit and the prior BREAKING commits (to save fixing them 3 times).
shahzadlone
pushed a commit
to shahzadlone/defradb
that referenced
this pull request
Feb 23, 2024
* Remove unused dag.GetSchemaID func * Break up long lines Will be added to shortly * BREAKING - Remove unused SchemaId prop Breaks some tests as although the property is always empty it is accounted for in CID generation. * BREAKING - Remove unused Schema.Key prop Note: The empty unused prop was used to construct some cids, removing it thus changes the cids even though it was always empty. * Replace commit schemaId with versionKey The primary driving change in this commit is the addition of SchemaVersionKey to CompositeDAGDelta in core/crdt/composite.go, this persists the schema state alongside the document change. All other changes in this commit are to facilitate this change. This commit got a bit more involved than I intended as getCollectionByVersionId was regenerating the schema id instead of taking it from the object it already had - possibly because the property on the object that it already had never actualy set the property. This got a bit awkward as the id lives on the object it is generated from - when the prop was populated on regeneration of itself it was taken into account as part of the regenerated key causing it to not match the originally (saved) key. Cids in the tests have been corrected in this commit for the production code changes in this commit and the prior BREAKING commits (to save fixing them 3 times).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #1023
Description
Persists schema version at time of commit (property
SchemaVersionKeyonCompositeDAGDelta) .The driving change is within the commit
Replace commit schemaId with versionKey. The commits prior to that are just preliminary cleanup. Please read the commit body of that commit as it will help explain why some other stuff had to change alongside it.The important part of this PR is not integration testable at the moment (
SchemaVersionKeyonCompositeDAGDelta) as it is not read. This will change very soon as part of #1006 - I will work on that immediately after opening this PR, let me know if you prefer to review this as part of #1006 instead of in its current isolation.