refactor: Remove SchemaDescription #3842
Conversation
4884b5e to
7e9a68f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #3842 +/- ##
===========================================
+ Coverage 72.76% 73.02% +0.26%
===========================================
Files 462 463 +1
Lines 42621 42068 -553
===========================================
- Hits 31009 30716 -293
+ Misses 9521 9317 -204
+ Partials 2091 2035 -56
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
e11c4f6 to
1686c11
Compare
3e94bff to
5bca807
Compare
http/handler_ccip_test.go
Outdated
| require.NoError(t, err) | ||
|
|
||
| cdb, err := db.NewDB(ctx, memory.NewDatastore(ctx), dac.NoDocumentACP, nil) | ||
| cdb, err := db.NewDB(ctx, store, dac.NoDocumentACP, nil) |
There was a problem hiding this comment.
note: The compiler decided to hit an existing corekv mem store deadlock bug due to unrelated changes, so I moved it to badger. Most our other tests had already moved to badger because of this but this one had managed to stay passing up until now.
5bca807 to
289200a
Compare
| // | ||
| // For example Field [FieldKind] string representations will be replaced by the raw integer | ||
| // value. | ||
| func substituteCollectionPatch( |
There was a problem hiding this comment.
note: This has been copy-pasted from the schema code, very few changes have been made - IIRC the only new feature is support for substitution in "from" paths, which had been missed in the original.
| // by the Apache License, Version 2.0, included in the file | ||
| // licenses/APL.txt. | ||
|
|
||
| package db |
There was a problem hiding this comment.
note: This is the old db/schema_id file but renamed, and modified.
The circular detection and grouping is largely unchanged. sortCollectionSets is new, as the sets need to be handled in the order that they exist within the dependency/relationship DAG so that the CollectionIDs can be added to the CollectionKind, before creating the host blocks.
substituteSecondaryRelationFieldKinds is also new.
023a53f to
ec9d8b9
Compare
| fieldID string, | ||
| ) (uint32, error) { | ||
| // This concatenation is temporary, soon we can just use the field CID | ||
| uniqueKey := strconv.Itoa(int(collectionShortID)) + ":" + fieldID |
There was a problem hiding this comment.
Note: This turned out to be not so temporary, as the short ids are collection specific, but the field ids are not. Removing the collection short id prefix results in the wrong field short ids being returned.
| return err | ||
| } | ||
|
|
||
| if dstCol.CollectionID != "" { // todo- this makes no sense |
There was a problem hiding this comment.
note: The old todo I accidentally left in from an old PR that should have dealt with it proved correct - it made no sense, and did nothing useful, so now it is gone.
bb6c2f5 to
e1005a7
Compare
53b30d5 to
38928cd
Compare
38928cd to
0a3aa50
Compare
fredcarle
left a comment
There was a problem hiding this comment.
Great work on this Andy!
Will always have a value now
By removing weird and incorrect single sided rel check
0a3aa50 to
3e15f0e
Compare
Relevant issue(s)
Resolves #3698
Resolves #1520
To reviewers
This is a disgustingly large PR and I am sorry that anyone has to try and review it. About 3000 lines of the diff are CID test changes - I doubt it is worth reviewing any of the test changes outside of the
tests/integration/schemaand thetests/integration/collection_versiondirectories - many of the lines within those directories are CID changes, but many of them are not.I suggest reading this PR description carefully, and consider reviewing and commenting on it's contents before bothering with the code.
When reviewing the code, I suggest starting with the changes in the
clientpackage, followed by thecrdtpackage, and then maybe thedbpackage.db/collection_id.gocontains some useful explanations as to why some stuff needs to exist and do what it does (particularly RE CollectionSets), it might be worth scanning the function level documentation within that file if anything inclientconfuses you initially.Description
Removes SchemaDescription from user space.
Schema properties are now saved in the blockstore
The global properties (previously on schema and co) are now saved in the blockstore.
The blocks are defined in the
crdtpackage in order to piggyback off of the existing block logic, even though atm they are not true CRDTs (they could become so later if we want). I would like to avoid a further refactor of the existing crdt code in this PR.Only properties that have been mutated are stored in the blocks (a delta).
The CollectionSet concept that was largely contained within the old
db/schema_id.gofile is now also in the blockstore. This is required for circular subsets of collections, as they must reference each other by their relative location within the collection set instead of the related CollectionID, as otherwise we would be trying to form a circular DAG (dog=>cat=>dog=>cat=>...). There is more documentation on this indb/collection_id.go(renamed from the originaldb/schema_id.go).Schema properties are now hosted on Collection
The properties on SchemaDescription, and SchemaFieldDescription have been moved onto CollectionVersion and CollectionFieldDescription. The following changes were made to facilitate this:
IsPlaceholderonCollectionVersionhas been added, as we can no longer use the lack of aSchemaDescriptionto determine whether a migration was uploaded before the schema versions were defined.IsPrimaryhas been added toCollectionFieldDefinitionas we can no longer use the lack of aSchemaFieldDescriptionto determine whether a field is the primary side of a relation.This also means that
PatchSchemahas been removed, and the properties ported over are now available viaPatchCollection.PatchCollectionhas gained some of the substitution logic thatPatchSchemahad, such as referencing by collection name instead of ID. It also activates new versions by default - given the way patching works, I think this will make a lot more sense to users, and the oldsetActiveboolean is very confusing with edits to multiple collections, and those that do not result in new versions. The new version may still be explicitly set to inactive, by patching theIsActiveproperty.The internal logic around orphan collection versions (when a migration between versions was added before the versions existed locally) had to change slightly because of this.
Collection IDs are now the Block CIDs
The IDs on CollectionVersion and its children are no longer formed by hashing the json representation of the
clientobjects, instead they are now the CIDs of the blocks. This includesCollectionID,CollectionVersion.VersionID,CollectionFieldDescription.FieldID, andCollectionVersion.CollectionSetID.This gives fields a stable, deterministic, global, field ID. This FieldID is not unique to the collection, and may be found on fields belonging to other collections - to do otherwise would create a circular dependency between field and collection.
Collection Fields can now be deleted via patch
Collection Fields can now be deleted via patch - the blocking off of this was already broken, as users were able to add a field, then toggle the version back to the previous (essentially removing the field). This became even more flimsy with the other changes in this PR, and fairly easy to handle, thanks to the new
FieldID.Collection short field ids are indexed by FieldID now in order to prevent their reuse, as well as just being nicer IMO.
Follow-up changes
The following changes should be done soon, but were excluded from this silly-sized PR in order to not make it even bigger.
schemadirectory (after converting them toPatchCollection, as moving them and de-duplicating them with the existing patch collection tests would significantly increase the line count, and obfuscate the behavioural test changes made to them within this PR. Merge and review patch schema/collection tests #3843client.CollectionDefinitionhas not been removed in this PR - it is referenced in a large number of places throughout the repo - removing it here would increase the line count, obsfucating changes that should be reviewed more carefully, and increasing the likelihood of merge conflicts. Removeclient.CollectionDefinition#3845schemas, small change, but easy to break out Http collection component refs are all underschema#3865