From 2953ce16d04b338144dc0148c865f1697f3a7da1 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 14:32:35 -0400 Subject: [PATCH 01/25] Do not update crdt of existing fields on patch --- internal/db/schema.go | 8 +++- tests/integration/schema/get_schema_test.go | 12 +++--- .../schema/migrations/query/simple_test.go | 42 +++++++++---------- .../migrations/query/with_doc_id_test.go | 4 +- .../migrations/query/with_inverse_test.go | 6 +-- .../query/with_p2p_schema_branch_test.go | 2 +- .../schema/migrations/query/with_p2p_test.go | 14 +++---- .../migrations/query/with_restart_test.go | 4 +- .../migrations/query/with_set_default_test.go | 6 +-- .../schema/migrations/query/with_txn_test.go | 4 +- .../migrations/query/with_update_test.go | 4 +- .../schema/migrations/simple_test.go | 4 +- .../updates/add/field/create_update_test.go | 4 +- .../schema/updates/add/field/simple_test.go | 8 ++-- .../schema/updates/with_schema_branch_test.go | 28 ++++++------- .../schema/with_update_set_default_test.go | 2 +- 16 files changed, 79 insertions(+), 73 deletions(-) diff --git a/internal/db/schema.go b/internal/db/schema.go index 8c0ba074dc..1659cc434f 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -366,8 +366,14 @@ func (db *db) updateSchema( } } + previousSchema := existingSchemaByName[schema.Name] + previousFieldNames := make(map[string]struct{}, len(previousSchema.Fields)) + for _, field := range previousSchema.Fields { + previousFieldNames[field.Name] = struct{}{} + } + for i, field := range schema.Fields { - if field.Typ == client.NONE_CRDT { + if _, existed := previousFieldNames[field.Name]; !existed && field.Typ == client.NONE_CRDT { // If no CRDT Type has been provided, default to LWW_REGISTER. field.Typ = client.LWW_REGISTER schema.Fields[i] = field diff --git a/tests/integration/schema/get_schema_test.go b/tests/integration/schema/get_schema_test.go index a89f4a2eb9..7f04c99c9e 100644 --- a/tests/integration/schema/get_schema_test.go +++ b/tests/integration/schema/get_schema_test.go @@ -72,7 +72,7 @@ func TestGetSchema_GivenNoSchemaGivenUnknownName(t *testing.T) { func TestGetSchema_ReturnsAllSchema(t *testing.T) { usersSchemaVersion1ID := "bafkreia2jn5ecrhtvy4fravk6pm3wqiny46m7mqymvjkgat7xiqupgqoai" - usersSchemaVersion2ID := "bafkreibbsqjeladin2keszmja5kektzgi4eowb6m3oimxssiqge7mmvhva" + usersSchemaVersion2ID := "bafkreialnju2rez4t3quvpobf3463eai3lo64vdrdhdmunz7yy7sv3f5ce" booksSchemaVersion1ID := "bafkreibiu34zrehpq346pwp5z24qkderm7ibhnpcqalhkivhnf5e2afqoy" test := testUtils.TestCase{ @@ -116,7 +116,7 @@ func TestGetSchema_ReturnsAllSchema(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -146,7 +146,7 @@ func TestGetSchema_ReturnsAllSchema(t *testing.T) { func TestGetSchema_ReturnsSchemaForGivenRoot(t *testing.T) { usersSchemaVersion1ID := "bafkreia2jn5ecrhtvy4fravk6pm3wqiny46m7mqymvjkgat7xiqupgqoai" - usersSchemaVersion2ID := "bafkreibbsqjeladin2keszmja5kektzgi4eowb6m3oimxssiqge7mmvhva" + usersSchemaVersion2ID := "bafkreialnju2rez4t3quvpobf3463eai3lo64vdrdhdmunz7yy7sv3f5ce" test := testUtils.TestCase{ Actions: []any{ @@ -190,7 +190,7 @@ func TestGetSchema_ReturnsSchemaForGivenRoot(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -209,7 +209,7 @@ func TestGetSchema_ReturnsSchemaForGivenRoot(t *testing.T) { func TestGetSchema_ReturnsSchemaForGivenName(t *testing.T) { usersSchemaVersion1ID := "bafkreia2jn5ecrhtvy4fravk6pm3wqiny46m7mqymvjkgat7xiqupgqoai" - usersSchemaVersion2ID := "bafkreibbsqjeladin2keszmja5kektzgi4eowb6m3oimxssiqge7mmvhva" + usersSchemaVersion2ID := "bafkreialnju2rez4t3quvpobf3463eai3lo64vdrdhdmunz7yy7sv3f5ce" test := testUtils.TestCase{ Actions: []any{ @@ -253,7 +253,7 @@ func TestGetSchema_ReturnsSchemaForGivenName(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", diff --git a/tests/integration/schema/migrations/query/simple_test.go b/tests/integration/schema/migrations/query/simple_test.go index a588e70e87..4e0ca20f2b 100644 --- a/tests/integration/schema/migrations/query/simple_test.go +++ b/tests/integration/schema/migrations/query/simple_test.go @@ -46,7 +46,7 @@ func TestSchemaMigrationQuery(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -116,7 +116,7 @@ func TestSchemaMigrationQueryMultipleDocs(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -179,7 +179,7 @@ func TestSchemaMigrationQueryWithMigrationRegisteredBeforeSchemaPatch(t *testing testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -255,7 +255,7 @@ func TestSchemaMigrationQueryMigratesToIntermediaryVersion(t *testing.T) { // there should be no migration from version 2 to version 3. LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -325,8 +325,8 @@ func TestSchemaMigrationQueryMigratesFromIntermediaryVersion(t *testing.T) { // Register a migration from schema version 2 to schema version 3 **only** - // there should be no migration from version 1 to version 2. LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", - DestinationSchemaVersionID: "bafkreib65lld2tdyvlilbumlcccftqwvflpgutugghf5afrnlhdg7dgyv4", + SourceSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", + DestinationSchemaVersionID: "bafkreicpdtq27uclgcyeqivvyjvojtk57a573y3upfhi3lvteytktyhlva", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -395,7 +395,7 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersions(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -411,8 +411,8 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersions(t *testing.T) { }, testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", - DestinationSchemaVersionID: "bafkreib65lld2tdyvlilbumlcccftqwvflpgutugghf5afrnlhdg7dgyv4", + SourceSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", + DestinationSchemaVersionID: "bafkreicpdtq27uclgcyeqivvyjvojtk57a573y3upfhi3lvteytktyhlva", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -467,7 +467,7 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersionsBeforePatches(t *test testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -483,8 +483,8 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersionsBeforePatches(t *test }, testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", - DestinationSchemaVersionID: "bafkreib65lld2tdyvlilbumlcccftqwvflpgutugghf5afrnlhdg7dgyv4", + SourceSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", + DestinationSchemaVersionID: "bafkreicpdtq27uclgcyeqivvyjvojtk57a573y3upfhi3lvteytktyhlva", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -553,8 +553,8 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersionsBeforePatchesWrongOrd testUtils.ConfigureMigration{ // Declare the migration from v2=>v3 before declaring the migration from v1=>v2 LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", - DestinationSchemaVersionID: "bafkreib65lld2tdyvlilbumlcccftqwvflpgutugghf5afrnlhdg7dgyv4", + SourceSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", + DestinationSchemaVersionID: "bafkreicpdtq27uclgcyeqivvyjvojtk57a573y3upfhi3lvteytktyhlva", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -571,7 +571,7 @@ func TestSchemaMigrationQueryMigratesAcrossMultipleVersionsBeforePatchesWrongOrd testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -713,7 +713,7 @@ func TestSchemaMigrationQueryMigrationMutatesExistingScalarField(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -774,7 +774,7 @@ func TestSchemaMigrationQueryMigrationMutatesExistingInlineArrayField(t *testing testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreicn6ltdovb6y7g3ecoptqkvx2y5y5yntrb5uydmg3jiakskqva2ta", - DestinationSchemaVersionID: "bafkreifv4vhz3dw7upc5u3omsqi6klz3h3e54ogfskp72gtut62fuxqrcu", + DestinationSchemaVersionID: "bafkreigb473jarbms7de62ykdu5necvxukmb6zbzolp4szdjcwzjvomuiq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -837,7 +837,7 @@ func TestSchemaMigrationQueryMigrationRemovesExistingField(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreihhd6bqrjhl5zidwztgxzeseveplv3cj3fwtn3unjkdx7j2vr2vrq", - DestinationSchemaVersionID: "bafkreiegvk3fkcjxoqqpp7npxqjdjwijiwthvynzmsvtzajpjevgu2krku", + DestinationSchemaVersionID: "bafkreibbnm7nrtnvwo7hmjjxacx7nxlqkp6bfr24vtlbv5vhwttlhrbr4q", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -898,7 +898,7 @@ func TestSchemaMigrationQueryMigrationPreservesExistingFieldWhenFieldNotRequeste testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreihhd6bqrjhl5zidwztgxzeseveplv3cj3fwtn3unjkdx7j2vr2vrq", - DestinationSchemaVersionID: "bafkreiegvk3fkcjxoqqpp7npxqjdjwijiwthvynzmsvtzajpjevgu2krku", + DestinationSchemaVersionID: "bafkreibbnm7nrtnvwo7hmjjxacx7nxlqkp6bfr24vtlbv5vhwttlhrbr4q", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -972,7 +972,7 @@ func TestSchemaMigrationQueryMigrationCopiesExistingFieldWhenSrcFieldNotRequeste testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreihhd6bqrjhl5zidwztgxzeseveplv3cj3fwtn3unjkdx7j2vr2vrq", - DestinationSchemaVersionID: "bafkreidgnuvanzqur3pkp4mmrd77ojwvov2rlczraaks4435e6wsgxpwoq", + DestinationSchemaVersionID: "bafkreifhm3admsxmv3xsbxehfkmtfnxqaq5wchrx47e7zc6vaxr352b3om", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -1034,7 +1034,7 @@ func TestSchemaMigrationQueryMigrationCopiesExistingFieldWhenSrcAndDstFieldNotRe testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreihhd6bqrjhl5zidwztgxzeseveplv3cj3fwtn3unjkdx7j2vr2vrq", - DestinationSchemaVersionID: "bafkreidgnuvanzqur3pkp4mmrd77ojwvov2rlczraaks4435e6wsgxpwoq", + DestinationSchemaVersionID: "bafkreifhm3admsxmv3xsbxehfkmtfnxqaq5wchrx47e7zc6vaxr352b3om", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_doc_id_test.go b/tests/integration/schema/migrations/query/with_doc_id_test.go index a006441c4f..2bd34a6fd4 100644 --- a/tests/integration/schema/migrations/query/with_doc_id_test.go +++ b/tests/integration/schema/migrations/query/with_doc_id_test.go @@ -53,7 +53,7 @@ func TestSchemaMigrationQueryByDocID(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -159,7 +159,7 @@ func TestSchemaMigrationQueryMultipleQueriesByDocID(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_inverse_test.go b/tests/integration/schema/migrations/query/with_inverse_test.go index f436c332c0..11c83c5fd4 100644 --- a/tests/integration/schema/migrations/query/with_inverse_test.go +++ b/tests/integration/schema/migrations/query/with_inverse_test.go @@ -50,7 +50,7 @@ func TestSchemaMigrationQueryInversesAcrossMultipleVersions(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreicdkt3m6mgwuoix7qyijvwxwtj3dlre4a4c6mdnqbucbndwuxjsvi", - DestinationSchemaVersionID: "bafkreibpaw4dxy6bvmuoyegm7bwxyi24nubozmukemwiour4v62kz5ffuu", + DestinationSchemaVersionID: "bafkreigijxrkfpadmnkpagokjdy6zpwtryad32m6nkgsqrd452kjlfp46e", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -66,8 +66,8 @@ func TestSchemaMigrationQueryInversesAcrossMultipleVersions(t *testing.T) { }, testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreibpaw4dxy6bvmuoyegm7bwxyi24nubozmukemwiour4v62kz5ffuu", - DestinationSchemaVersionID: "bafkreickm4zodm2muw5qcctmssht63g57u7kxujqyoax4zb5c42zs4pdh4", + SourceSchemaVersionID: "bafkreigijxrkfpadmnkpagokjdy6zpwtryad32m6nkgsqrd452kjlfp46e", + DestinationSchemaVersionID: "bafkreibtmdbc3nbdt74xdwvfrez53fxwyz6nh4b6ppwsrxiqpj5zpwgole", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_p2p_schema_branch_test.go b/tests/integration/schema/migrations/query/with_p2p_schema_branch_test.go index b5e7bdde03..d7dc9f10dd 100644 --- a/tests/integration/schema/migrations/query/with_p2p_schema_branch_test.go +++ b/tests/integration/schema/migrations/query/with_p2p_schema_branch_test.go @@ -47,7 +47,7 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocOnOtherSchemaBranch(t *testing. // Register the migration on both nodes. LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce", - DestinationSchemaVersionID: "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm", + DestinationSchemaVersionID: "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_p2p_test.go b/tests/integration/schema/migrations/query/with_p2p_test.go index f8b0197d5d..39adf5a5a8 100644 --- a/tests/integration/schema/migrations/query/with_p2p_test.go +++ b/tests/integration/schema/migrations/query/with_p2p_test.go @@ -47,7 +47,7 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocAtOlderSchemaVersion(t *testing // Register the migration on both nodes. LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce", - DestinationSchemaVersionID: "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm", + DestinationSchemaVersionID: "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -146,7 +146,7 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocAtMuchOlderSchemaVersion(t *tes // Register the migration on both nodes. LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce", - DestinationSchemaVersionID: "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm", + DestinationSchemaVersionID: "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -163,8 +163,8 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocAtMuchOlderSchemaVersion(t *tes testUtils.ConfigureMigration{ // Register the migration on both nodes. LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm", - DestinationSchemaVersionID: "bafkreidiohu3klvu4f2fdqcywtpqild4v7spsn7ivsjtg6sea6ome2oc4i", + SourceSchemaVersionID: "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba", + DestinationSchemaVersionID: "bafkreiglqiiz6j7d5dokcle6juoz26uixxggc5zawqkgwcivmenvhob5jy", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -254,7 +254,7 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocAtNewerSchemaVersion(t *testing // Register the migration on both nodes. LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce", - DestinationSchemaVersionID: "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm", + DestinationSchemaVersionID: "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -355,8 +355,8 @@ func TestSchemaMigrationQueryWithP2PReplicatedDocAtMuchNewerSchemaVersionWithSch // Register a migration from version 2 to version 3 on both nodes. // There is no migration from version 1 to 2, thus node 1 has no knowledge of schema version 2. LensConfig: client.LensConfig{ - SourceSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", - DestinationSchemaVersionID: "bafkreib65lld2tdyvlilbumlcccftqwvflpgutugghf5afrnlhdg7dgyv4", + SourceSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", + DestinationSchemaVersionID: "bafkreicpdtq27uclgcyeqivvyjvojtk57a573y3upfhi3lvteytktyhlva", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_restart_test.go b/tests/integration/schema/migrations/query/with_restart_test.go index f44264312c..4f2c0f4ec7 100644 --- a/tests/integration/schema/migrations/query/with_restart_test.go +++ b/tests/integration/schema/migrations/query/with_restart_test.go @@ -46,7 +46,7 @@ func TestSchemaMigrationQueryWithRestart(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -100,7 +100,7 @@ func TestSchemaMigrationQueryWithRestartAndMigrationBeforeSchemaPatch(t *testing testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_set_default_test.go b/tests/integration/schema/migrations/query/with_set_default_test.go index 17c147338c..170a861d89 100644 --- a/tests/integration/schema/migrations/query/with_set_default_test.go +++ b/tests/integration/schema/migrations/query/with_set_default_test.go @@ -22,7 +22,7 @@ import ( ) func TestSchemaMigrationQuery_WithSetDefaultToLatest_AppliesForwardMigration(t *testing.T) { - schemaVersionID2 := "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm" + schemaVersionID2 := "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba" test := testUtils.TestCase{ Description: "Test schema migration", @@ -84,7 +84,7 @@ func TestSchemaMigrationQuery_WithSetDefaultToLatest_AppliesForwardMigration(t * func TestSchemaMigrationQuery_WithSetDefaultToOriginal_AppliesInverseMigration(t *testing.T) { schemaVersionID1 := "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce" - schemaVersionID2 := "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm" + schemaVersionID2 := "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba" test := testUtils.TestCase{ Description: "Test schema migration", @@ -159,7 +159,7 @@ func TestSchemaMigrationQuery_WithSetDefaultToOriginal_AppliesInverseMigration(t func TestSchemaMigrationQuery_WithSetDefaultToOriginalVersionThatDocWasCreatedAt_ClearsMigrations(t *testing.T) { schemaVersionID1 := "bafkreibpai5hfnalhtn5mgamzkgml4gwftow7pklmjcn6i4sqey6a5u5ce" - schemaVersionID2 := "bafkreidrbhf54zckhmchzw2ngbobfqtkt7sm6ihbliu2wtxesehz5g4xwm" + schemaVersionID2 := "bafkreif7z5sj2ehtmjenverki7c2hqfjgvbajqdlch6yk4kkbx7qvm2yba" test := testUtils.TestCase{ Description: "Test schema migration", diff --git a/tests/integration/schema/migrations/query/with_txn_test.go b/tests/integration/schema/migrations/query/with_txn_test.go index 880f9e01ed..79d2d9e825 100644 --- a/tests/integration/schema/migrations/query/with_txn_test.go +++ b/tests/integration/schema/migrations/query/with_txn_test.go @@ -48,7 +48,7 @@ func TestSchemaMigrationQueryWithTxn(t *testing.T) { TransactionID: immutable.Some(0), LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -110,7 +110,7 @@ func TestSchemaMigrationQueryWithTxnAndCommit(t *testing.T) { TransactionID: immutable.Some(0), LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/query/with_update_test.go b/tests/integration/schema/migrations/query/with_update_test.go index 93a2586e25..bbeabcd062 100644 --- a/tests/integration/schema/migrations/query/with_update_test.go +++ b/tests/integration/schema/migrations/query/with_update_test.go @@ -46,7 +46,7 @@ func TestSchemaMigrationQueryWithUpdateRequest(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -130,7 +130,7 @@ func TestSchemaMigrationQueryWithMigrationRegisteredAfterUpdate(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { diff --git a/tests/integration/schema/migrations/simple_test.go b/tests/integration/schema/migrations/simple_test.go index a7826f5366..e36c9ec836 100644 --- a/tests/integration/schema/migrations/simple_test.go +++ b/tests/integration/schema/migrations/simple_test.go @@ -107,7 +107,7 @@ func TestSchemaMigrationGetMigrationsReturnsMultiple(t *testing.T) { testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ SourceSchemaVersionID: "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe", - DestinationSchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + DestinationSchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Lens: model.Lens{ Lenses: []model.LensModule{ { @@ -158,7 +158,7 @@ func TestSchemaMigrationGetMigrationsReturnsMultiple(t *testing.T) { }, { ID: 4, - SchemaVersionID: "bafkreib5jaawobqqiu6frzacerlj55pxxxuql3igqj4ldmg2pgilke4bty", + SchemaVersionID: "bafkreiahhaeagyfsxaxmv3d665qvnbtyn3ts6jshhghy5bijwztbe7efpq", Sources: []any{ &client.CollectionSource{ SourceCollectionID: 3, diff --git a/tests/integration/schema/updates/add/field/create_update_test.go b/tests/integration/schema/updates/add/field/create_update_test.go index cd3a0b1267..53b892e0ae 100644 --- a/tests/integration/schema/updates/add/field/create_update_test.go +++ b/tests/integration/schema/updates/add/field/create_update_test.go @@ -18,7 +18,7 @@ import ( func TestSchemaUpdatesAddFieldWithCreateWithUpdateAfterSchemaUpdateAndVersionJoin(t *testing.T) { initialSchemaVersionID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - updatedSchemaVersionID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" + updatedSchemaVersionID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" test := testUtils.TestCase{ Description: "Test schema update, add field with update after schema update, version join", @@ -106,7 +106,7 @@ func TestSchemaUpdatesAddFieldWithCreateWithUpdateAfterSchemaUpdateAndVersionJoi func TestSchemaUpdatesAddFieldWithCreateWithUpdateAfterSchemaUpdateAndCommitQuery(t *testing.T) { initialSchemaVersionID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - updatedSchemaVersionID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" + updatedSchemaVersionID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" test := testUtils.TestCase{ Description: "Test schema update, add field with update after schema update, commits query", diff --git a/tests/integration/schema/updates/add/field/simple_test.go b/tests/integration/schema/updates/add/field/simple_test.go index 80aaec32d6..1288e00656 100644 --- a/tests/integration/schema/updates/add/field/simple_test.go +++ b/tests/integration/schema/updates/add/field/simple_test.go @@ -21,7 +21,7 @@ import ( func TestSchemaUpdatesAddFieldSimple(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" test := testUtils.TestCase{ Description: "Test schema update, add field", @@ -60,7 +60,7 @@ func TestSchemaUpdatesAddFieldSimple(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -116,7 +116,7 @@ func TestSchemaUpdates_AddFieldSimpleDoNotSetDefault_Errors(t *testing.T) { func TestSchemaUpdates_AddFieldSimpleDoNotSetDefault_VersionIsQueryable(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" test := testUtils.TestCase{ Description: "Test schema update, add field", @@ -149,7 +149,7 @@ func TestSchemaUpdates_AddFieldSimpleDoNotSetDefault_VersionIsQueryable(t *testi { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", diff --git a/tests/integration/schema/updates/with_schema_branch_test.go b/tests/integration/schema/updates/with_schema_branch_test.go index d8f7d1afc2..58759f3edd 100644 --- a/tests/integration/schema/updates/with_schema_branch_test.go +++ b/tests/integration/schema/updates/with_schema_branch_test.go @@ -21,8 +21,8 @@ import ( func TestSchemaUpdates_WithBranchingSchema(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" - schemaVersion3ID := "bafkreifswbi23wxvq2zpqnoldolsxk2fhtj5t6rs3pidil3j6tybc62q3m" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" + schemaVersion3ID := "bafkreifc46y7pk2xfwc3nc442r7iqf6cjixxerxrrnrsouky544gmz4zve" test := testUtils.TestCase{ Description: "Test schema update, with branching schema", @@ -74,7 +74,7 @@ func TestSchemaUpdates_WithBranchingSchema(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -112,7 +112,7 @@ func TestSchemaUpdates_WithBranchingSchema(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -170,9 +170,9 @@ func TestSchemaUpdates_WithBranchingSchema(t *testing.T) { func TestSchemaUpdates_WithPatchOnBranchedSchema(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" - schemaVersion3ID := "bafkreifswbi23wxvq2zpqnoldolsxk2fhtj5t6rs3pidil3j6tybc62q3m" - schemaVersion4ID := "bafkreid4ulxeclzgpzhznge7zdin6docxvklugvr6gt4jxfyanz5i2r2hu" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" + schemaVersion3ID := "bafkreifc46y7pk2xfwc3nc442r7iqf6cjixxerxrrnrsouky544gmz4zve" + schemaVersion4ID := "bafkreic2heai3vgufxcxs6bfvil2oyz27w3bzkwoqehjevlnkewq3ffp4e" test := testUtils.TestCase{ Description: "Test schema update, with patch on branching schema", @@ -234,7 +234,7 @@ func TestSchemaUpdates_WithPatchOnBranchedSchema(t *testing.T) { { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", @@ -308,8 +308,8 @@ func TestSchemaUpdates_WithPatchOnBranchedSchema(t *testing.T) { func TestSchemaUpdates_WithBranchingSchemaAndSetActiveSchemaToOtherBranch(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" - schemaVersion3ID := "bafkreifswbi23wxvq2zpqnoldolsxk2fhtj5t6rs3pidil3j6tybc62q3m" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" + schemaVersion3ID := "bafkreifc46y7pk2xfwc3nc442r7iqf6cjixxerxrrnrsouky544gmz4zve" test := testUtils.TestCase{ Description: "Test schema update, with branching schema toggling between branches", @@ -404,9 +404,9 @@ func TestSchemaUpdates_WithBranchingSchemaAndSetActiveSchemaToOtherBranch(t *tes func TestSchemaUpdates_WithBranchingSchemaAndSetActiveSchemaToOtherBranchThenPatch(t *testing.T) { schemaVersion1ID := "bafkreia3o3cetvcnnxyu5spucimoos77ifungfmacxdkva4zah2is3aooe" - schemaVersion2ID := "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4" - schemaVersion3ID := "bafkreifswbi23wxvq2zpqnoldolsxk2fhtj5t6rs3pidil3j6tybc62q3m" - schemaVersion4ID := "bafkreidjuyxhakc5yx7fucunoxijnfjvgqohf4sjoryzf27mqxidh37kne" + schemaVersion2ID := "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe" + schemaVersion3ID := "bafkreifc46y7pk2xfwc3nc442r7iqf6cjixxerxrrnrsouky544gmz4zve" + schemaVersion4ID := "bafkreifdkkauc4b4rkazmzijiu2nxlikqatxa5zbmjc4sn3wrtlcqqcrt4" test := testUtils.TestCase{ Description: "Test schema update, with branching schema toggling between branches then patch", @@ -472,7 +472,7 @@ func TestSchemaUpdates_WithBranchingSchemaAndSetActiveSchemaToOtherBranchThenPat { Name: "_docID", Kind: client.FieldKind_DocID, - Typ: client.LWW_REGISTER, + Typ: client.NONE_CRDT, }, { Name: "name", diff --git a/tests/integration/schema/with_update_set_default_test.go b/tests/integration/schema/with_update_set_default_test.go index f46e0540e3..22f05a4d73 100644 --- a/tests/integration/schema/with_update_set_default_test.go +++ b/tests/integration/schema/with_update_set_default_test.go @@ -129,7 +129,7 @@ func TestSchema_WithUpdateAndSetDefaultVersionToNew_AllowsQueryingOfNewField(t * SetAsDefaultVersion: immutable.Some(false), }, testUtils.SetActiveSchemaVersion{ - SchemaVersionID: "bafkreibz4g6rkxanzn6ro74ezmbwoe5hvcguwvi34judrk2kfuqqtk5ak4", + SchemaVersionID: "bafkreidt4i22v4bzga3aezlcxsrfbvuhzcbqo5bnfe2x2dgkpz3eds2afe", }, testUtils.Request{ Request: `query { From 4a3d257034c1f254ef9e1dc60c40531f49671b06 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Wed, 12 Jun 2024 13:55:07 -0400 Subject: [PATCH 02/25] Rename validator var --- internal/db/definition_validation.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 988ebeb15c..0139138a14 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -51,8 +51,8 @@ func (db *db) validateCollectionChanges( oldColsByID map[uint32]client.CollectionDescription, newColsByID map[uint32]client.CollectionDescription, ) error { - for _, validators := range patchCollectionValidators { - err := validators(oldColsByID, newColsByID) + for _, validator := range patchCollectionValidators { + err := validator(oldColsByID, newColsByID) if err != nil { return err } @@ -65,8 +65,8 @@ func (db *db) validateNewCollection( def client.CollectionDefinition, defsByName map[string]client.CollectionDefinition, ) error { - for _, validators := range newCollectionValidators { - err := validators(def, defsByName) + for _, validator := range newCollectionValidators { + err := validator(def, defsByName) if err != nil { return err } From c23c0eb4e018bc1685d4c3b432f33159c492acae Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Wed, 12 Jun 2024 15:22:41 -0400 Subject: [PATCH 03/25] Bulkify createCollection It doesn't make sense to handle these one by one, especially long term where a mix of multiple collections and schemas may be patched in the same call and validation must only take into account the final result. A happy and necessary side effect of this commit is the schema version id is now set before validating new collections - this will simplify the new validation framework. --- internal/db/collection_define.go | 179 +++++++++++++++++-------------- internal/db/schema.go | 19 ++-- internal/db/view.go | 57 ++++++---- 3 files changed, 142 insertions(+), 113 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 4712911399..0674d33cb7 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -25,113 +25,136 @@ import ( "github.com/sourcenetwork/defradb/internal/db/description" ) -func (db *db) createCollection( +func (db *db) createCollections( ctx context.Context, - def client.CollectionDefinition, newDefinitions []client.CollectionDefinition, -) (client.Collection, error) { - schema := def.Schema - desc := def.Description - txn := mustGetContextTxn(ctx) - - if desc.Name.HasValue() { - exists, err := description.HasCollectionByName(ctx, txn, desc.Name.Value()) - if err != nil { - return nil, err - } - if exists { - return nil, ErrCollectionAlreadyExists - } - } +) ([]client.CollectionDefinition, error) { + returnDescriptions := make([]client.CollectionDefinition, len(newDefinitions)) existingDefinitions, err := db.getAllActiveDefinitions(ctx) if err != nil { return nil, err } - schemaByName := map[string]client.SchemaDescription{} - for _, existingDefinition := range existingDefinitions { - schemaByName[existingDefinition.Schema.Name] = existingDefinition.Schema - } - for _, newDefinition := range newDefinitions { - schemaByName[newDefinition.Schema.Name] = newDefinition.Schema - } + for i, def := range newDefinitions { + schema := def.Schema + txn := mustGetContextTxn(ctx) - _, err = validateUpdateSchemaFields(schemaByName, client.SchemaDescription{}, schema) - if err != nil { - return nil, err - } + schemaByName := map[string]client.SchemaDescription{} + for _, existingDefinition := range existingDefinitions { + schemaByName[existingDefinition.Schema.Name] = existingDefinition.Schema + } + for _, newDefinition := range newDefinitions { + schemaByName[newDefinition.Schema.Name] = newDefinition.Schema + } - definitionsByName := map[string]client.CollectionDefinition{} - for _, existingDefinition := range existingDefinitions { - definitionsByName[existingDefinition.GetName()] = existingDefinition - } - for _, newDefinition := range newDefinitions { - definitionsByName[newDefinition.GetName()] = newDefinition - } - err = db.validateNewCollection(def, definitionsByName) - if err != nil { - return nil, err - } + _, err = validateUpdateSchemaFields(schemaByName, client.SchemaDescription{}, schema) + if err != nil { + return nil, err + } - colSeq, err := db.getSequence(ctx, core.CollectionIDSequenceKey{}) - if err != nil { - return nil, err - } - colID, err := colSeq.next(ctx) - if err != nil { - return nil, err + schema, err = description.CreateSchemaVersion(ctx, txn, schema) + if err != nil { + return nil, err + } + newDefinitions[i].Description.SchemaVersionID = schema.VersionID + newDefinitions[i].Schema = schema } - fieldSeq, err := db.getSequence(ctx, core.NewFieldIDSequenceKey(uint32(colID))) - if err != nil { - return nil, err - } + for _, def := range newDefinitions { + // Only accept the schema if policy description is valid, otherwise reject the schema. + err := db.validateCollectionDefinitionPolicyDesc(ctx, def.Description.Policy) + if err != nil { + return nil, err + } - desc.ID = uint32(colID) - desc.RootID = desc.ID + schema := def.Schema + desc := def.Description + txn := mustGetContextTxn(ctx) - schema, err = description.CreateSchemaVersion(ctx, txn, schema) - if err != nil { - return nil, err - } - desc.SchemaVersionID = schema.VersionID - for _, localField := range desc.Fields { - var fieldID uint64 - if localField.Name == request.DocIDFieldName { - // There is no hard technical requirement for this, we just think it looks nicer - // if the doc id is at the zero index. It makes it look a little nicer in commit - // queries too. - fieldID = 0 - } else { - fieldID, err = fieldSeq.next(ctx) + if desc.Name.HasValue() { + exists, err := description.HasCollectionByName(ctx, txn, desc.Name.Value()) if err != nil { return nil, err } + if exists { + return nil, ErrCollectionAlreadyExists + } + } + + definitionsByName := map[string]client.CollectionDefinition{} + for _, existingDefinition := range existingDefinitions { + definitionsByName[existingDefinition.GetName()] = existingDefinition + } + for _, newDefinition := range newDefinitions { + definitionsByName[newDefinition.GetName()] = newDefinition + } + err = db.validateNewCollection(def, definitionsByName) + if err != nil { + return nil, err + } + + colSeq, err := db.getSequence(ctx, core.CollectionIDSequenceKey{}) + if err != nil { + return nil, err + } + colID, err := colSeq.next(ctx) + if err != nil { + return nil, err + } + + fieldSeq, err := db.getSequence(ctx, core.NewFieldIDSequenceKey(uint32(colID))) + if err != nil { + return nil, err } - for i := range desc.Fields { - if desc.Fields[i].Name == localField.Name { - desc.Fields[i].ID = client.FieldID(fieldID) - break + desc.ID = uint32(colID) + desc.RootID = desc.ID + + for _, localField := range desc.Fields { + var fieldID uint64 + if localField.Name == request.DocIDFieldName { + // There is no hard technical requirement for this, we just think it looks nicer + // if the doc id is at the zero index. It makes it look a little nicer in commit + // queries too. + fieldID = 0 + } else { + fieldID, err = fieldSeq.next(ctx) + if err != nil { + return nil, err + } + } + + for i := range desc.Fields { + if desc.Fields[i].Name == localField.Name { + desc.Fields[i].ID = client.FieldID(fieldID) + break + } } } - } - desc, err = description.SaveCollection(ctx, txn, desc) - if err != nil { - return nil, err - } + desc, err = description.SaveCollection(ctx, txn, desc) + if err != nil { + return nil, err + } - col := db.newCollection(desc, schema) + col := db.newCollection(desc, schema) - for _, index := range desc.Indexes { - if _, err := col.createIndex(ctx, index); err != nil { + for _, index := range desc.Indexes { + if _, err := col.createIndex(ctx, index); err != nil { + return nil, err + } + } + + result, err := db.getCollectionByID(ctx, desc.ID) + if err != nil { return nil, err } + + returnDescriptions = append(returnDescriptions, result.Definition()) } - return db.getCollectionByID(ctx, desc.ID) + return returnDescriptions, nil } func (db *db) patchCollection( diff --git a/internal/db/schema.go b/internal/db/schema.go index 1659cc434f..daf69ec394 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -44,19 +44,14 @@ func (db *db) addSchema( return nil, err } - returnDescriptions := make([]client.CollectionDescription, len(newDefinitions)) - for i, definition := range newDefinitions { - // Only accept the schema if policy description is valid, otherwise reject the schema. - err := db.validateCollectionDefinitionPolicyDesc(ctx, definition.Description.Policy) - if err != nil { - return nil, err - } + returnDefinitions, err := db.createCollections(ctx, newDefinitions) + if err != nil { + return nil, err + } - col, err := db.createCollection(ctx, definition, newDefinitions) - if err != nil { - return nil, err - } - returnDescriptions[i] = col.Description() + returnDescriptions := make([]client.CollectionDescription, len(returnDefinitions)) + for i, def := range returnDefinitions { + returnDescriptions[i] = def.Description } err = db.loadSchema(ctx) diff --git a/internal/db/view.go b/internal/db/view.go index a663da7add..190f365b6f 100644 --- a/internal/db/view.go +++ b/internal/db/view.go @@ -68,30 +68,41 @@ func (db *db) addView( newDefinitions[i].Description.Sources = append(newDefinitions[i].Description.Sources, &source) } - returnDescriptions := make([]client.CollectionDefinition, len(newDefinitions)) - for i, definition := range newDefinitions { - if !definition.Description.Name.HasValue() { - schema, err := description.CreateSchemaVersion(ctx, txn, definition.Schema) - if err != nil { - return nil, err - } - returnDescriptions[i] = client.CollectionDefinition{ - // `Collection` is left as default for embedded types - Schema: schema, - } + returnDescriptions := make([]client.CollectionDefinition, 0, len(newDefinitions)) + collectionDefinitions := []client.CollectionDefinition{} + schemaOnlyDefinitions := []client.SchemaDescription{} + + for _, definition := range newDefinitions { + if definition.Description.Name.HasValue() { + collectionDefinitions = append(collectionDefinitions, definition) } else { - col, err := db.createCollection(ctx, definition, newDefinitions) - if err != nil { - return nil, err - } - returnDescriptions[i] = col.Definition() - - for _, source := range col.Description().QuerySources() { - if source.Transform.HasValue() { - err = db.LensRegistry().SetMigration(ctx, col.ID(), source.Transform.Value()) - if err != nil { - return nil, err - } + schemaOnlyDefinitions = append(schemaOnlyDefinitions, definition.Schema) + } + } + + for _, schema := range schemaOnlyDefinitions { + schema, err := description.CreateSchemaVersion(ctx, txn, schema) + if err != nil { + return nil, err + } + returnDescriptions = append(returnDescriptions, client.CollectionDefinition{ + // `Collection` is left as default for embedded types + Schema: schema, + }) + } + + returnColDescriptions, err := db.createCollections(ctx, collectionDefinitions) + if err != nil { + return nil, err + } + returnDescriptions = append(returnDescriptions, returnColDescriptions...) + + for _, definition := range returnColDescriptions { + for _, source := range definition.Description.QuerySources() { + if source.Transform.HasValue() { + err = db.LensRegistry().SetMigration(ctx, definition.Description.ID, source.Transform.Value()) + if err != nil { + return nil, err } } } From f6499b1397b065865f885c22f88efad51d10180a Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 13:31:23 -0400 Subject: [PATCH 04/25] Move validation to after field ids have been set This means that we can validate that they have been set correctly, as well as simplifying some rules. --- internal/db/collection_define.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 0674d33cb7..2e60b04085 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -89,10 +89,6 @@ func (db *db) createCollections( for _, newDefinition := range newDefinitions { definitionsByName[newDefinition.GetName()] = newDefinition } - err = db.validateNewCollection(def, definitionsByName) - if err != nil { - return nil, err - } colSeq, err := db.getSequence(ctx, core.CollectionIDSequenceKey{}) if err != nil { @@ -133,6 +129,11 @@ func (db *db) createCollections( } } + err = db.validateNewCollection(def, definitionsByName) + if err != nil { + return nil, err + } + desc, err = description.SaveCollection(ctx, txn, desc) if err != nil { return nil, err From 30bdc7ea447e68b5f737f460d5f6e085434a1d06 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Wed, 12 Jun 2024 17:16:48 -0400 Subject: [PATCH 05/25] Rework create and update collection validation The new types will be applied to schema updates soon too. A handful of tests have changed as the order in which rules execute has changed. --- internal/db/collection_define.go | 4 +- internal/db/definition_validation.go | 431 ++++++++++++------ .../updates/copy/name_test.go | 2 +- .../updates/replace/name_test.go | 2 +- tests/integration/schema/one_one_test.go | 4 +- 5 files changed, 297 insertions(+), 146 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 2e60b04085..1599c51157 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -129,7 +129,7 @@ func (db *db) createCollections( } } - err = db.validateNewCollection(def, definitionsByName) + err = db.validateNewCollection(ctx, definitionsByName) if err != nil { return nil, err } @@ -195,7 +195,7 @@ func (db *db) patchCollection( return err } - err = db.validateCollectionChanges(existingColsByID, newColsByID) + err = db.validateCollectionChanges(ctx, cols, newColsByID) if err != nil { return err } diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 0139138a14..95828a8303 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -20,12 +20,78 @@ import ( "github.com/sourcenetwork/defradb/client/request" ) -var patchCollectionValidators = []func( - map[uint32]client.CollectionDescription, - map[uint32]client.CollectionDescription, -) error{ - validateCollectionNameUnique, - validateSingleVersionActive, +// definitionState holds collection and schema descriptions in easily accessible +// sets. +// +// It is read only and will not and should not be mutated. +type definitionState struct { + collections []client.CollectionDescription + collectionsByID map[uint32]client.CollectionDescription + schemaByID map[string]client.SchemaDescription + + definitionsByName map[string]client.CollectionDefinition +} + +// newDefinitionState creates a new definitionState object given the provided +// descriptions. +func newDefinitionState( + collections []client.CollectionDescription, + schemasByID map[string]client.SchemaDescription, +) *definitionState { + collectionsByID := map[uint32]client.CollectionDescription{} + definitionsByName := map[string]client.CollectionDefinition{} + schemaVersionsAdded := map[string]struct{}{} + + for _, col := range collections { + if len(col.Fields) == 0 { + continue + } + + schema := schemasByID[col.SchemaVersionID] + definition := client.CollectionDefinition{ + Description: col, + Schema: schema, + } + + definitionsByName[definition.GetName()] = definition + schemaVersionsAdded[schema.VersionID] = struct{}{} + collectionsByID[col.ID] = col + } + + for _, schema := range schemasByID { + if _, ok := schemaVersionsAdded[schema.VersionID]; ok { + continue + } + + definitionsByName[schema.Name] = client.CollectionDefinition{ + Schema: schema, + } + } + + return &definitionState{ + collections: collections, + collectionsByID: collectionsByID, + schemaByID: schemasByID, + definitionsByName: definitionsByName, + } +} + +// definitionValidator aliases the signature that all schema and collection +// validation functions should follow. +type definitionValidator = func( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error + +// createOnlyValidators are executed on the creation of new descriptions only +// they will not be executed for updates to existing records. +var createOnlyValidators = []definitionValidator{} + +// createOnlyValidators are executed on the update of existing descriptions only +// they will not be executed for new records. +var updateOnlyValidators = []definitionValidator{ validateSourcesNotRedefined, validateIndexesNotModified, validateFieldsNotModified, @@ -36,23 +102,42 @@ var patchCollectionValidators = []func( validateRootIDNotMutated, validateSchemaVersionIDNotMutated, validateCollectionNotRemoved, + validateSingleVersionActive, } -var newCollectionValidators = []func( - client.CollectionDefinition, - map[string]client.CollectionDefinition, -) error{ - validateSecondaryFieldsPairUp, +// globalValidators are run on create and update of records. +var globalValidators = []definitionValidator{ + validateCollectionNameUnique, validateRelationPointsToValidKind, + validateSecondaryFieldsPairUp, validateSingleSidePrimary, } +var updateValidators = append( + append([]definitionValidator{}, updateOnlyValidators...), + globalValidators..., +) + +var createValidators = append( + append([]definitionValidator{}, createOnlyValidators...), + globalValidators..., +) + func (db *db) validateCollectionChanges( - oldColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + oldCols []client.CollectionDescription, newColsByID map[uint32]client.CollectionDescription, ) error { - for _, validator := range patchCollectionValidators { - err := validator(oldColsByID, newColsByID) + newCols := make([]client.CollectionDescription, 0, len(newColsByID)) + for _, col := range newColsByID { + newCols = append(newCols, col) + } + + newState := newDefinitionState(newCols, map[string]client.SchemaDescription{}) + oldState := newDefinitionState(oldCols, map[string]client.SchemaDescription{}) + + for _, validator := range updateValidators { + err := validator(ctx, db, newState, oldState) if err != nil { return err } @@ -62,11 +147,24 @@ func (db *db) validateCollectionChanges( } func (db *db) validateNewCollection( - def client.CollectionDefinition, - defsByName map[string]client.CollectionDefinition, + ctx context.Context, + newDefsByName map[string]client.CollectionDefinition, ) error { - for _, validator := range newCollectionValidators { - err := validator(def, defsByName) + collections := []client.CollectionDescription{} + schemasByID := map[string]client.SchemaDescription{} + + for _, def := range newDefsByName { + if len(def.Description.Fields) != 0 { + collections = append(collections, def.Description) + } + + schemasByID[def.Schema.VersionID] = def.Schema + } + + newState := newDefinitionState(collections, schemasByID) + + for _, validator := range createValidators { + err := validator(ctx, db, newState, &definitionState{}) if err != nil { return err } @@ -76,22 +174,26 @@ func (db *db) validateNewCollection( } func validateRelationPointsToValidKind( - def client.CollectionDefinition, - defsByName map[string]client.CollectionDefinition, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, field := range def.Description.Fields { - if !field.Kind.HasValue() { - continue - } + for _, newCollection := range newState.collections { + for _, field := range newCollection.Fields { + if !field.Kind.HasValue() { + continue + } - if !field.Kind.Value().IsObject() { - continue - } + if !field.Kind.Value().IsObject() { + continue + } - underlying := field.Kind.Value().Underlying() - _, ok := defsByName[underlying] - if !ok { - return NewErrFieldKindNotFound(field.Name, underlying) + underlying := field.Kind.Value().Underlying() + _, ok := newState.definitionsByName[underlying] + if !ok { + return NewErrFieldKindNotFound(field.Name, underlying) + } } } @@ -99,51 +201,65 @@ func validateRelationPointsToValidKind( } func validateSecondaryFieldsPairUp( - def client.CollectionDefinition, - defsByName map[string]client.CollectionDefinition, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, field := range def.Description.Fields { - if !field.Kind.HasValue() { + for _, newCollection := range newState.collections { + schema, ok := newState.schemaByID[newCollection.SchemaVersionID] + if !ok { continue } - if !field.Kind.Value().IsObject() { - continue + definition := client.CollectionDefinition{ + Description: newCollection, + Schema: schema, } - if !field.RelationName.HasValue() { - continue - } + for _, field := range newCollection.Fields { + if !field.Kind.HasValue() { + continue + } - _, hasSchemaField := def.Schema.GetFieldByName(field.Name) - if hasSchemaField { - continue - } + if !field.Kind.Value().IsObject() { + continue + } - underlying := field.Kind.Value().Underlying() - otherDef, ok := defsByName[underlying] - if !ok { - continue - } + if !field.RelationName.HasValue() { + continue + } - if len(otherDef.Description.Fields) == 0 { - // Views/embedded objects do not require both sides of the relation to be defined. - continue - } + _, hasSchemaField := schema.GetFieldByName(field.Name) + if hasSchemaField { + continue + } - otherField, ok := otherDef.Description.GetFieldByRelation( - field.RelationName.Value(), - def.GetName(), - field.Name, - ) - if !ok { - return NewErrRelationMissingField(underlying, field.RelationName.Value()) - } + underlying := field.Kind.Value().Underlying() + otherDef, ok := newState.definitionsByName[underlying] + if !ok { + continue + } - _, ok = otherDef.Schema.GetFieldByName(otherField.Name) - if !ok { - // This secondary is paired with another secondary, which is invalid - return NewErrRelationMissingField(underlying, field.RelationName.Value()) + if len(otherDef.Description.Fields) == 0 { + // Views/embedded objects do not require both sides of the relation to be defined. + continue + } + + otherField, ok := otherDef.Description.GetFieldByRelation( + field.RelationName.Value(), + definition.GetName(), + field.Name, + ) + if !ok { + return NewErrRelationMissingField(underlying, field.RelationName.Value()) + } + + _, ok = otherDef.Schema.GetFieldByName(otherField.Name) + if !ok { + // This secondary is paired with another secondary, which is invalid + return NewErrRelationMissingField(underlying, field.RelationName.Value()) + } } } @@ -151,48 +267,57 @@ func validateSecondaryFieldsPairUp( } func validateSingleSidePrimary( - def client.CollectionDefinition, - defsByName map[string]client.CollectionDefinition, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, field := range def.Description.Fields { - if !field.Kind.HasValue() { + for _, newCollection := range newState.collections { + schema, ok := newState.schemaByID[newCollection.SchemaVersionID] + if !ok { continue } - if !field.Kind.Value().IsObject() { - continue + definition := client.CollectionDefinition{ + Description: newCollection, + Schema: schema, } - if !field.RelationName.HasValue() { - continue - } + for _, field := range definition.GetFields() { + if !field.Kind.IsObject() { + continue + } - _, hasSchemaField := def.Schema.GetFieldByName(field.Name) - if !hasSchemaField { - // This is a secondary field and thus passes this rule - continue - } + if field.RelationName == "" { + continue + } - underlying := field.Kind.Value().Underlying() - otherDef, ok := defsByName[underlying] - if !ok { - continue - } + if !field.IsPrimaryRelation { + // This is a secondary field and thus passes this rule + continue + } - otherField, ok := otherDef.Description.GetFieldByRelation( - field.RelationName.Value(), - def.GetName(), - field.Name, - ) - if !ok { - // This must be a one-sided relation, in which case it passes this rule - continue - } + underlying := field.Kind.Underlying() + otherDef, ok := newState.definitionsByName[underlying] + if !ok { + continue + } + + otherField, ok := otherDef.Description.GetFieldByRelation( + field.RelationName, + definition.GetName(), + field.Name, + ) + if !ok { + // This must be a one-sided relation, in which case it passes this rule + continue + } - _, ok = otherDef.Schema.GetFieldByName(otherField.Name) - if ok { - // This primary is paired with another primary, which is invalid - return ErrMultipleRelationPrimaries + _, ok = otherDef.Schema.GetFieldByName(otherField.Name) + if ok { + // This primary is paired with another primary, which is invalid + return ErrMultipleRelationPrimaries + } } } @@ -200,11 +325,13 @@ func validateSingleSidePrimary( } func validateCollectionNameUnique( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { names := map[string]struct{}{} - for _, col := range newColsByID { + for _, col := range newState.collections { if !col.Name.HasValue() { continue } @@ -219,11 +346,13 @@ func validateCollectionNameUnique( } func validateSingleVersionActive( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { rootsWithActiveCol := map[uint32]struct{}{} - for _, col := range newColsByID { + for _, col := range newState.collections { if !col.Name.HasValue() { continue } @@ -243,11 +372,13 @@ func validateSingleVersionActive( // Currently new sources cannot be added, existing cannot be removed, and CollectionSources // cannot be redirected to other collections. func validateSourcesNotRedefined( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -281,11 +412,13 @@ func validateSourcesNotRedefined( } func validateIndexesNotModified( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -300,11 +433,13 @@ func validateIndexesNotModified( } func validateFieldsNotModified( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -319,11 +454,13 @@ func validateFieldsNotModified( } func validatePolicyNotModified( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -338,10 +475,12 @@ func validatePolicyNotModified( } func validateIDNotZero( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { + for _, newCol := range newState.collections { if newCol.ID == 0 { return ErrCollectionIDCannotBeZero } @@ -351,11 +490,13 @@ func validateIDNotZero( } func validateIDUnique( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { colIds := map[uint32]struct{}{} - for _, newCol := range newColsByID { + for _, newCol := range newState.collections { if _, ok := colIds[newCol.ID]; ok { return NewErrCollectionIDAlreadyExists(newCol.ID) } @@ -366,11 +507,13 @@ func validateIDUnique( } func validateIDExists( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - if _, ok := oldColsByID[newCol.ID]; !ok { + for _, newCol := range newState.collections { + if _, ok := oldState.collectionsByID[newCol.ID]; !ok { return NewErrAddCollectionIDWithPatch(newCol.ID) } } @@ -379,11 +522,13 @@ func validateIDExists( } func validateRootIDNotMutated( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -397,11 +542,13 @@ func validateRootIDNotMutated( } func validateSchemaVersionIDNotMutated( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { - for _, newCol := range newColsByID { - oldCol, ok := oldColsByID[newCol.ID] + for _, newCol := range newState.collections { + oldCol, ok := oldState.collectionsByID[newCol.ID] if !ok { continue } @@ -415,12 +562,14 @@ func validateSchemaVersionIDNotMutated( } func validateCollectionNotRemoved( - oldColsByID map[uint32]client.CollectionDescription, - newColsByID map[uint32]client.CollectionDescription, + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, ) error { oldLoop: - for _, oldCol := range oldColsByID { - for _, newCol := range newColsByID { + for _, oldCol := range oldState.collections { + for _, newCol := range newState.collectionsByID { // It is not enough to just match by the map index, in case the index does not pair // up with the ID (this can happen if a user moves the collection within the map) if newCol.ID == oldCol.ID { diff --git a/tests/integration/collection_description/updates/copy/name_test.go b/tests/integration/collection_description/updates/copy/name_test.go index b915d111ac..f5cbd3a83b 100644 --- a/tests/integration/collection_description/updates/copy/name_test.go +++ b/tests/integration/collection_description/updates/copy/name_test.go @@ -40,7 +40,7 @@ func TestColDescrUpdateCopyName_Errors(t *testing.T) { { "op": "copy", "from": "/1/Name", "path": "/2/Name" } ] `, - ExpectedError: "collection already exists. Name: Users", + ExpectedError: "multiple versions of same collection cannot be active. Name: Users, Root: 1", }, }, } diff --git a/tests/integration/collection_description/updates/replace/name_test.go b/tests/integration/collection_description/updates/replace/name_test.go index 98f1ba8c98..55e8160969 100644 --- a/tests/integration/collection_description/updates/replace/name_test.go +++ b/tests/integration/collection_description/updates/replace/name_test.go @@ -99,7 +99,7 @@ func TestColDescrUpdateReplaceName_GivenInactiveCollectionWithSameName_Errors(t { "op": "replace", "path": "/2/Name", "value": "Users" } ] `, - ExpectedError: "collection already exists. Name: Users", + ExpectedError: "multiple versions of same collection cannot be active. Name: Users, Root: 1", }, }, } diff --git a/tests/integration/schema/one_one_test.go b/tests/integration/schema/one_one_test.go index b5bc75bb48..8bc1e5a1fe 100644 --- a/tests/integration/schema/one_one_test.go +++ b/tests/integration/schema/one_one_test.go @@ -30,7 +30,9 @@ func TestSchemaOneOne_NoPrimary_Errors(t *testing.T) { owner: User } `, - ExpectedError: "relation missing field. Object: Dog, RelationName: dog_user", + // This error is dependent upon the order in which definitions are validated, so + // we only assert that the error is the correct type, and do not check the key-values + ExpectedError: "relation missing field", }, }, } From af07badca21636b85568182edfb06bffbae8dbfe Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Wed, 12 Jun 2024 17:24:55 -0400 Subject: [PATCH 06/25] Validate policy the same way as every other prop --- internal/db/collection_define.go | 6 --- internal/db/definition_validation.go | 59 ++++++++++++++++------------ 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 1599c51157..ba74b24862 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -62,12 +62,6 @@ func (db *db) createCollections( } for _, def := range newDefinitions { - // Only accept the schema if policy description is valid, otherwise reject the schema. - err := db.validateCollectionDefinitionPolicyDesc(ctx, def.Description.Policy) - if err != nil { - return nil, err - } - schema := def.Schema desc := def.Description txn := mustGetContextTxn(ctx) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 95828a8303..eb5eb1ec3d 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -14,8 +14,6 @@ import ( "context" "reflect" - "github.com/sourcenetwork/immutable" - "github.com/sourcenetwork/defradb/client" "github.com/sourcenetwork/defradb/client/request" ) @@ -111,6 +109,7 @@ var globalValidators = []definitionValidator{ validateRelationPointsToValidKind, validateSecondaryFieldsPairUp, validateSingleSidePrimary, + validateCollectionDefinitionPolicyDesc, } var updateValidators = append( @@ -587,31 +586,41 @@ oldLoop: // // Ensures that the information within the policy definition makes sense, // this function might also make relevant remote calls using the acp system. -func (db *db) validateCollectionDefinitionPolicyDesc( +func validateCollectionDefinitionPolicyDesc( ctx context.Context, - policyDesc immutable.Option[client.PolicyDescription], + db *db, + newState *definitionState, + oldState *definitionState, ) error { - if !policyDesc.HasValue() { - // No policy validation needed, whether acp exists or not doesn't matter. - return nil - } - - // If there is a policy specified, but the database does not have - // acp enabled/available return an error, database must have an acp available - // to enable access control (inorder to adhere to the policy specified). - if !db.acp.HasValue() { - return ErrCanNotHavePolicyWithoutACP - } - - // If we have the policy specified on the collection, and acp is available/enabled, - // then using the acp system we need to ensure the policy id specified - // actually exists as a policy, and the resource name exists on that policy - // and that the resource is a valid DPI. - return db.acp.Value().ValidateResourceExistsOnValidDPI( - ctx, - policyDesc.Value().ID, - policyDesc.Value().ResourceName, - ) + for _, newCol := range newState.collections { + if !newCol.Policy.HasValue() { + // No policy validation needed, whether acp exists or not doesn't matter. + continue + } + + // If there is a policy specified, but the database does not have + // acp enabled/available return an error, database must have an acp available + // to enable access control (inorder to adhere to the policy specified). + if !db.acp.HasValue() { + return ErrCanNotHavePolicyWithoutACP + } + + // If we have the policy specified on the collection, and acp is available/enabled, + // then using the acp system we need to ensure the policy id specified + // actually exists as a policy, and the resource name exists on that policy + // and that the resource is a valid DPI. + err := db.acp.Value().ValidateResourceExistsOnValidDPI( + ctx, + newCol.Policy.Value().ID, + newCol.Policy.Value().ResourceName, + ) + + if err != nil { + return err + } + } + + return nil } // validateUpdateSchema validates that the given schema description is a valid update. From 7811dc2eea6e71123dc77c5da57545bf2a0987f3 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 11:59:23 -0400 Subject: [PATCH 07/25] Extract validateSchemaFieldNotDeleted to new sys --- internal/db/definition_validation.go | 66 +++++++++++++++++++++++++--- internal/db/schema.go | 4 ++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index eb5eb1ec3d..089c208bec 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -25,7 +25,9 @@ import ( type definitionState struct { collections []client.CollectionDescription collectionsByID map[uint32]client.CollectionDescription - schemaByID map[string]client.SchemaDescription + + schemaByID map[string]client.SchemaDescription + schemaByName map[string]client.SchemaDescription definitionsByName map[string]client.CollectionDefinition } @@ -38,6 +40,7 @@ func newDefinitionState( ) *definitionState { collectionsByID := map[uint32]client.CollectionDescription{} definitionsByName := map[string]client.CollectionDefinition{} + schemaByName := map[string]client.SchemaDescription{} schemaVersionsAdded := map[string]struct{}{} for _, col := range collections { @@ -57,6 +60,8 @@ func newDefinitionState( } for _, schema := range schemasByID { + schemaByName[schema.Name] = schema + if _, ok := schemaVersionsAdded[schema.VersionID]; ok { continue } @@ -70,6 +75,7 @@ func newDefinitionState( collections: collections, collectionsByID: collectionsByID, schemaByID: schemasByID, + schemaByName: schemaByName, definitionsByName: definitionsByName, } } @@ -101,6 +107,7 @@ var updateOnlyValidators = []definitionValidator{ validateSchemaVersionIDNotMutated, validateCollectionNotRemoved, validateSingleVersionActive, + validateSchemaFieldNotDeleted, } // globalValidators are run on create and update of records. @@ -172,6 +179,33 @@ func (db *db) validateNewCollection( return nil } +func (db *db) validateSchemaUpdate( + ctx context.Context, + newSchemaByName map[string]client.SchemaDescription, + oldSchemaByName map[string]client.SchemaDescription, +) error { + newSchemaByID := make(map[string]client.SchemaDescription, len(newSchemaByName)) + oldSchemaByID := make(map[string]client.SchemaDescription, len(oldSchemaByName)) + for _, schema := range newSchemaByName { + newSchemaByID[schema.VersionID] = schema + } + for _, schema := range oldSchemaByName { + oldSchemaByID[schema.VersionID] = schema + } + + newState := newDefinitionState([]client.CollectionDescription{}, newSchemaByID) + oldState := newDefinitionState([]client.CollectionDescription{}, oldSchemaByID) + + for _, validator := range updateValidators { + err := validator(ctx, db, newState, oldState) + if err != nil { + return err + } + } + + return nil +} + func validateRelationPointsToValidKind( ctx context.Context, db *db, @@ -734,10 +768,32 @@ func validateUpdateSchemaFields( newFieldNames[proposedField.Name] = struct{}{} } - for _, field := range existingDesc.Fields { - if _, stillExists := newFieldNames[field.Name]; !stillExists { - return false, NewErrCannotDeleteField(field.Name) + return hasChanged, nil +} + +func validateSchemaFieldNotDeleted( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, newSchema := range newState.schemaByName { + oldSchema := oldState.schemaByName[newSchema.Name] + + for _, oldField := range oldSchema.Fields { + stillExists := false + for _, newField := range newSchema.Fields { + if newField.Name == oldField.Name { + stillExists = true + break + } + } + + if !stillExists { + return NewErrCannotDeleteField(oldField.Name) + } } } - return hasChanged, nil + + return nil } diff --git a/internal/db/schema.go b/internal/db/schema.go index daf69ec394..bd14b3865c 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -344,6 +344,10 @@ func (db *db) updateSchema( if err != nil { return err } + err = db.validateSchemaUpdate(ctx, proposedDescriptionsByName, existingSchemaByName) + if err != nil { + return err + } if !hasChanged { return nil From 3873eea54d7fd2607e550e090d645828aa2b5ca6 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 12:08:45 -0400 Subject: [PATCH 08/25] Extract validateTypeAndKindCompatible to new sys --- internal/db/definition_validation.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 089c208bec..ae337ec8f7 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -117,6 +117,7 @@ var globalValidators = []definitionValidator{ validateSecondaryFieldsPairUp, validateSingleSidePrimary, validateCollectionDefinitionPolicyDesc, + validateTypeAndKindCompatible, } var updateValidators = append( @@ -761,10 +762,6 @@ func validateUpdateSchemaFields( return false, client.NewErrInvalidCRDTType(proposedField.Name, proposedField.Typ.String()) } - if !proposedField.Typ.IsCompatibleWith(proposedField.Kind) { - return false, client.NewErrCRDTKindMismatch(proposedField.Typ.String(), proposedField.Kind.String()) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -797,3 +794,20 @@ func validateSchemaFieldNotDeleted( return nil } + +func validateTypeAndKindCompatible( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, newSchema := range newState.schemaByName { + for _, newField := range newSchema.Fields { + if !newField.Typ.IsCompatibleWith(newField.Kind) { + return client.NewErrCRDTKindMismatch(newField.Typ.String(), newField.Kind.String()) + } + } + } + + return nil +} From 0e48928800a5c83faa88a22fc44c3816cfd08df2 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 12:19:15 -0400 Subject: [PATCH 09/25] Extract validateTypeSupported to new sys --- internal/db/definition_validation.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index ae337ec8f7..4795fe41c7 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -117,6 +117,7 @@ var globalValidators = []definitionValidator{ validateSecondaryFieldsPairUp, validateSingleSidePrimary, validateCollectionDefinitionPolicyDesc, + validateTypeSupported, validateTypeAndKindCompatible, } @@ -758,10 +759,6 @@ func validateUpdateSchemaFields( return false, NewErrCannotMoveField(proposedField.Name, proposedIndex, existingIndex) } - if !proposedField.Typ.IsSupportedFieldCType() { - return false, client.NewErrInvalidCRDTType(proposedField.Name, proposedField.Typ.String()) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -811,3 +808,20 @@ func validateTypeAndKindCompatible( return nil } + +func validateTypeSupported( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, newSchema := range newState.schemaByName { + for _, newField := range newSchema.Fields { + if !newField.Typ.IsSupportedFieldCType() { + return client.NewErrInvalidCRDTType(newField.Name, newField.Typ.String()) + } + } + } + + return nil +} From 90bb075893e209977ca0883353d783635e1d1271 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 12:36:50 -0400 Subject: [PATCH 10/25] Extract validateFieldNotMoved to new sys --- internal/db/definition_validation.go | 32 ++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 4795fe41c7..b0baf399eb 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -108,6 +108,7 @@ var updateOnlyValidators = []definitionValidator{ validateCollectionNotRemoved, validateSingleVersionActive, validateSchemaFieldNotDeleted, + validateFieldNotMoved, } // globalValidators are run on create and update of records. @@ -718,7 +719,7 @@ func validateUpdateSchemaFields( } newFieldNames := map[string]struct{}{} - for proposedIndex, proposedField := range proposedDesc.Fields { + for _, proposedField := range proposedDesc.Fields { existingField, fieldAlreadyExists := existingFieldsByName[proposedField.Name] // If the field is new, then the collection has changed @@ -754,11 +755,6 @@ func validateUpdateSchemaFields( return false, NewErrCannotMutateField(proposedField.Name) } - if existingIndex := existingFieldIndexesByName[proposedField.Name]; fieldAlreadyExists && - proposedIndex != existingIndex { - return false, NewErrCannotMoveField(proposedField.Name, proposedIndex, existingIndex) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -825,3 +821,27 @@ func validateTypeSupported( return nil } + +func validateFieldNotMoved( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, oldSchema := range oldState.schemaByName { + oldFieldIndexesByName := map[string]int{} + for i, field := range oldSchema.Fields { + oldFieldIndexesByName[field.Name] = i + } + + newSchema := newState.schemaByName[oldSchema.Name] + + for newIndex, newField := range newSchema.Fields { + if existingIndex, exists := oldFieldIndexesByName[newField.Name]; exists && newIndex != existingIndex { + return NewErrCannotMoveField(newField.Name, newIndex, existingIndex) + } + } + } + + return nil +} From 78877e8fa1415d5e0944cd34c2cd9be6b08c2482 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 12:45:08 -0400 Subject: [PATCH 11/25] Extract validateFieldNotMutated to new sys --- internal/db/definition_validation.go | 32 +++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index b0baf399eb..1af7f6cc90 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -108,6 +108,7 @@ var updateOnlyValidators = []definitionValidator{ validateCollectionNotRemoved, validateSingleVersionActive, validateSchemaFieldNotDeleted, + validateFieldNotMutated, validateFieldNotMoved, } @@ -720,7 +721,7 @@ func validateUpdateSchemaFields( newFieldNames := map[string]struct{}{} for _, proposedField := range proposedDesc.Fields { - existingField, fieldAlreadyExists := existingFieldsByName[proposedField.Name] + _, fieldAlreadyExists := existingFieldsByName[proposedField.Name] // If the field is new, then the collection has changed hasChanged = hasChanged || !fieldAlreadyExists @@ -751,10 +752,6 @@ func validateUpdateSchemaFields( return false, NewErrDuplicateField(proposedField.Name) } - if fieldAlreadyExists && proposedField != existingField { - return false, NewErrCannotMutateField(proposedField.Name) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -845,3 +842,28 @@ func validateFieldNotMoved( return nil } + +func validateFieldNotMutated( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, oldSchema := range oldState.schemaByName { + oldFieldsByName := map[string]client.SchemaFieldDescription{} + for _, field := range oldSchema.Fields { + oldFieldsByName[field.Name] = field + } + + newSchema := newState.schemaByName[oldSchema.Name] + + for _, newField := range newSchema.Fields { + oldField, exists := oldFieldsByName[newField.Name] + if exists && oldField != newField { + return NewErrCannotMutateField(newField.Name) + } + } + } + + return nil +} From 48efc717857d0b98663f2d3b51d13b087c2615a8 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 12:56:18 -0400 Subject: [PATCH 12/25] Extract validateFieldNotDuplicated to new sys A handful of tests have changed due to the order of rule execution changing. --- internal/db/definition_validation.go | 25 ++++++++++++++++--- .../schema/updates/add/field/simple_test.go | 2 +- .../schema/updates/copy/field/simple_test.go | 2 +- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 1af7f6cc90..c73c6ac8b5 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -121,6 +121,7 @@ var globalValidators = []definitionValidator{ validateCollectionDefinitionPolicyDesc, validateTypeSupported, validateTypeAndKindCompatible, + validateFieldNotDuplicated, } var updateValidators = append( @@ -748,10 +749,6 @@ func validateUpdateSchemaFields( return false, NewErrSecondaryFieldOnSchema(proposedField.Name) } - if _, isDuplicate := newFieldNames[proposedField.Name]; isDuplicate { - return false, NewErrDuplicateField(proposedField.Name) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -867,3 +864,23 @@ func validateFieldNotMutated( return nil } + +func validateFieldNotDuplicated( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, schema := range newState.schemaByName { + fieldNames := map[string]struct{}{} + + for _, field := range schema.Fields { + if _, isDuplicate := fieldNames[field.Name]; isDuplicate { + return NewErrDuplicateField(field.Name) + } + fieldNames[field.Name] = struct{}{} + } + } + + return nil +} diff --git a/tests/integration/schema/updates/add/field/simple_test.go b/tests/integration/schema/updates/add/field/simple_test.go index 1288e00656..d315791dfa 100644 --- a/tests/integration/schema/updates/add/field/simple_test.go +++ b/tests/integration/schema/updates/add/field/simple_test.go @@ -362,7 +362,7 @@ func TestSchemaUpdatesAddFieldSimpleDuplicateOfExistingField(t *testing.T) { { "op": "add", "path": "/Users/Fields/-", "value": {"Name": "name", "Kind": 11} } ] `, - ExpectedError: "duplicate field. Name: name", + ExpectedError: "mutating an existing field is not supported. ProposedName: name", }, }, } diff --git a/tests/integration/schema/updates/copy/field/simple_test.go b/tests/integration/schema/updates/copy/field/simple_test.go index a2c631a515..5baf640ab8 100644 --- a/tests/integration/schema/updates/copy/field/simple_test.go +++ b/tests/integration/schema/updates/copy/field/simple_test.go @@ -34,7 +34,7 @@ func TestSchemaUpdatesCopyFieldErrors(t *testing.T) { { "op": "copy", "from": "/Users/Fields/1", "path": "/Users/Fields/2" } ] `, - ExpectedError: "duplicate field. Name: email", + ExpectedError: "moving fields is not currently supported. Name: email", }, testUtils.Request{ Request: `query { From ec7d3b9154b0e278eeee1e03541abc91e494da7f Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 13:14:37 -0400 Subject: [PATCH 13/25] Extract validateSecondaryNotOnSchema to new sys --- internal/db/definition_validation.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index c73c6ac8b5..a8396b0584 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -119,6 +119,7 @@ var globalValidators = []definitionValidator{ validateSecondaryFieldsPairUp, validateSingleSidePrimary, validateCollectionDefinitionPolicyDesc, + validateSecondaryNotOnSchema, validateTypeSupported, validateTypeAndKindCompatible, validateFieldNotDuplicated, @@ -745,10 +746,6 @@ func validateUpdateSchemaFields( } } - if proposedField.Kind.IsObjectArray() { - return false, NewErrSecondaryFieldOnSchema(proposedField.Name) - } - newFieldNames[proposedField.Name] = struct{}{} } @@ -884,3 +881,20 @@ func validateFieldNotDuplicated( return nil } + +func validateSecondaryNotOnSchema( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, newSchema := range newState.schemaByName { + for _, newField := range newSchema.Fields { + if newField.Kind.IsObjectArray() { + return NewErrSecondaryFieldOnSchema(newField.Name) + } + } + } + + return nil +} From ea17786c39d446cbc04257423efef405b13e8131 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 15:34:47 -0400 Subject: [PATCH 14/25] Extract validateRelationalFieldIDType to new sys --- internal/db/definition_validation.go | 40 +++++++++++++++++++++------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index a8396b0584..df40255dcd 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -119,6 +119,7 @@ var globalValidators = []definitionValidator{ validateSecondaryFieldsPairUp, validateSingleSidePrimary, validateCollectionDefinitionPolicyDesc, + validateRelationalFieldIDType, validateSecondaryNotOnSchema, validateTypeSupported, validateTypeAndKindCompatible, @@ -734,16 +735,6 @@ func validateUpdateSchemaFields( if !relatedDescFound { return false, NewErrFieldKindNotFound(proposedField.Name, proposedField.Kind.Underlying()) } - - if proposedField.Kind.IsObject() && !proposedField.Kind.IsArray() { - idFieldName := proposedField.Name + request.RelatedObjectID - idField, idFieldFound := proposedDesc.GetFieldByName(idFieldName) - if idFieldFound { - if idField.Kind != client.FieldKind_DocID { - return false, NewErrRelationalFieldIDInvalidType(idField.Name, client.FieldKind_DocID, idField.Kind) - } - } - } } newFieldNames[proposedField.Name] = struct{}{} @@ -898,3 +889,32 @@ func validateSecondaryNotOnSchema( return nil } + +func validateRelationalFieldIDType( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, schema := range newState.schemaByName { + fieldsByName := map[string]client.SchemaFieldDescription{} + + for _, field := range schema.Fields { + fieldsByName[field.Name] = field + } + + for _, field := range schema.Fields { + if field.Kind.IsObject() && !field.Kind.IsArray() { + idFieldName := field.Name + request.RelatedObjectID + idField, idFieldFound := fieldsByName[idFieldName] + if idFieldFound { + if idField.Kind != client.FieldKind_DocID { + return NewErrRelationalFieldIDInvalidType(idField.Name, client.FieldKind_DocID, idField.Kind) + } + } + } + } + } + + return nil +} From 734d0f9ea64b55eb61a11f80d5ba8e54b9444174 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 15:42:33 -0400 Subject: [PATCH 15/25] Extract schema field change detection to rule --- internal/db/definition_validation.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index df40255dcd..3bb2d06e56 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -237,6 +237,20 @@ func validateRelationPointsToValidKind( } } + for _, schema := range newState.schemaByName { + for _, field := range schema.Fields { + if !field.Kind.IsObject() { + continue + } + + underlying := field.Kind.Underlying() + _, ok := newState.definitionsByName[underlying] + if !ok { + return NewErrFieldKindNotFound(field.Name, underlying) + } + } + } + return nil } @@ -729,14 +743,6 @@ func validateUpdateSchemaFields( // If the field is new, then the collection has changed hasChanged = hasChanged || !fieldAlreadyExists - if !fieldAlreadyExists && proposedField.Kind.IsObject() { - _, relatedDescFound := descriptionsByName[proposedField.Kind.Underlying()] - - if !relatedDescFound { - return false, NewErrFieldKindNotFound(proposedField.Name, proposedField.Kind.Underlying()) - } - } - newFieldNames[proposedField.Name] = struct{}{} } From 116136cff842012883685289e039ff1cc9ce2db7 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 15:50:24 -0400 Subject: [PATCH 16/25] Remove impossible schema name rule They are mapped and interacted with by users by name, so this rule can never fail - changing the name would trip up other rules though --- internal/db/definition_validation.go | 6 ------ internal/db/errors.go | 9 --------- 2 files changed, 15 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 3bb2d06e56..2e5e7d8797 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -704,12 +704,6 @@ func (db *db) validateUpdateSchema( ) } - if proposedDesc.Name != existingDesc.Name { - // There is actually little reason to not support this atm besides controlling the surface area - // of the new feature. Changing this should not break anything, but it should be tested first. - return false, NewErrCannotModifySchemaName(existingDesc.Name, proposedDesc.Name) - } - if proposedDesc.VersionID != "" && proposedDesc.VersionID != existingDesc.VersionID { // If users specify this it will be overwritten, an error is preferred to quietly ignoring it. return false, ErrCannotSetVersionID diff --git a/internal/db/errors.go b/internal/db/errors.go index 8d3c770bd8..cb623b774f 100644 --- a/internal/db/errors.go +++ b/internal/db/errors.go @@ -26,7 +26,6 @@ const ( errAddCollectionWithPatch string = "adding collections via patch is not supported" errCollectionIDDoesntMatch string = "CollectionID does not match existing" errSchemaRootDoesntMatch string = "SchemaRoot does not match existing" - errCannotModifySchemaName string = "modifying the schema name is not supported" errCannotSetVersionID string = "setting the VersionID is not supported" errRelationalFieldInvalidRelationType string = "invalid RelationType" errRelationalFieldMissingIDField string = "missing id field for relation object field" @@ -256,14 +255,6 @@ func NewErrSchemaRootDoesntMatch(name, existingRoot, proposedRoot string) error ) } -func NewErrCannotModifySchemaName(existingName, proposedName string) error { - return errors.New( - errCannotModifySchemaName, - errors.NewKV("ExistingName", existingName), - errors.NewKV("ProposedName", proposedName), - ) -} - func NewErrRelationalFieldMissingIDField(name string, expectedName string) error { return errors.New( errRelationalFieldMissingIDField, From 48bdffbf4a00daf72ad9eaff7fbe532afbcdbc56 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 15:59:27 -0400 Subject: [PATCH 17/25] Extract schema version change detection to rule --- internal/db/definition_validation.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 2e5e7d8797..319de8a0b3 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -612,6 +612,14 @@ func validateSchemaVersionIDNotMutated( } } + for _, newSchema := range newState.schemaByName { + oldSchema := oldState.schemaByName[newSchema.Name] + if newSchema.VersionID != "" && newSchema.VersionID != oldSchema.VersionID { + // If users specify this it will be overwritten, an error is preferred to quietly ignoring it. + return ErrCannotSetVersionID + } + } + return nil } @@ -704,11 +712,6 @@ func (db *db) validateUpdateSchema( ) } - if proposedDesc.VersionID != "" && proposedDesc.VersionID != existingDesc.VersionID { - // If users specify this it will be overwritten, an error is preferred to quietly ignoring it. - return false, ErrCannotSetVersionID - } - hasChangedFields, err := validateUpdateSchemaFields(proposedDescriptionsByName, existingDesc, proposedDesc) if err != nil { return hasChangedFields, err From 89cba0056fe9b37a45d05dfb676127322b59fa75 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 15:59:48 -0400 Subject: [PATCH 18/25] Extract schema root change detection to rule --- internal/db/definition_validation.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 319de8a0b3..bba59c5a4a 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -592,6 +592,17 @@ func validateRootIDNotMutated( } } + for _, newSchema := range newState.schemaByName { + oldSchema := oldState.schemaByName[newSchema.Name] + if newSchema.Root != oldSchema.Root { + return NewErrSchemaRootDoesntMatch( + newSchema.Name, + oldSchema.Root, + newSchema.Root, + ) + } + } + return nil } @@ -704,14 +715,6 @@ func (db *db) validateUpdateSchema( return false, NewErrAddCollectionWithPatch(proposedDesc.Name) } - if proposedDesc.Root != existingDesc.Root { - return false, NewErrSchemaRootDoesntMatch( - proposedDesc.Name, - existingDesc.Root, - proposedDesc.Root, - ) - } - hasChangedFields, err := validateUpdateSchemaFields(proposedDescriptionsByName, existingDesc, proposedDesc) if err != nil { return hasChangedFields, err From 111bd6d965fa2db508ba50832abae2eb8931b660 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 17:38:24 -0400 Subject: [PATCH 19/25] Extract validateSchemaNotAdded to new sys A handful of tests have changed due to changes in the order in which rules are executed --- internal/db/definition_validation.go | 21 +++++++++++++++---- internal/db/errors.go | 5 +++-- .../schema/updates/add/simple_test.go | 2 +- .../schema/updates/copy/simple_test.go | 2 +- .../schema/updates/replace/simple_test.go | 2 +- 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index bba59c5a4a..c53ca2089e 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -107,6 +107,7 @@ var updateOnlyValidators = []definitionValidator{ validateSchemaVersionIDNotMutated, validateCollectionNotRemoved, validateSingleVersionActive, + validateSchemaNotAdded, validateSchemaFieldNotDeleted, validateFieldNotMutated, validateFieldNotMoved, @@ -710,10 +711,7 @@ func (db *db) validateUpdateSchema( return false, ErrSchemaNameEmpty } - existingDesc, collectionExists := existingDescriptionsByName[proposedDesc.Name] - if !collectionExists { - return false, NewErrAddCollectionWithPatch(proposedDesc.Name) - } + existingDesc := existingDescriptionsByName[proposedDesc.Name] hasChangedFields, err := validateUpdateSchemaFields(proposedDescriptionsByName, existingDesc, proposedDesc) if err != nil { @@ -924,3 +922,18 @@ func validateRelationalFieldIDType( return nil } + +func validateSchemaNotAdded( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, newSchema := range newState.schemaByName { + if _, exists := oldState.schemaByName[newSchema.Name]; !exists { + return NewErrAddSchemaWithPatch(newSchema.Name) + } + } + + return nil +} diff --git a/internal/db/errors.go b/internal/db/errors.go index cb623b774f..7a81824efe 100644 --- a/internal/db/errors.go +++ b/internal/db/errors.go @@ -24,6 +24,7 @@ const ( errAddingP2PCollection string = "cannot add collection ID" errRemovingP2PCollection string = "cannot remove collection ID" errAddCollectionWithPatch string = "adding collections via patch is not supported" + errAddSchemaWithPatch string = "adding schema via patch is not supported" errCollectionIDDoesntMatch string = "CollectionID does not match existing" errSchemaRootDoesntMatch string = "SchemaRoot does not match existing" errCannotSetVersionID string = "setting the VersionID is not supported" @@ -223,9 +224,9 @@ func NewErrRemovingP2PCollection(inner error) error { return errors.Wrap(errRemovingP2PCollection, inner) } -func NewErrAddCollectionWithPatch(name string) error { +func NewErrAddSchemaWithPatch(name string) error { return errors.New( - errAddCollectionWithPatch, + errAddSchemaWithPatch, errors.NewKV("Name", name), ) } diff --git a/tests/integration/schema/updates/add/simple_test.go b/tests/integration/schema/updates/add/simple_test.go index 88d36680b0..65d04a7af6 100644 --- a/tests/integration/schema/updates/add/simple_test.go +++ b/tests/integration/schema/updates/add/simple_test.go @@ -33,7 +33,7 @@ func TestSchemaUpdatesAddSimpleErrorsAddingSchema(t *testing.T) { { "op": "add", "path": "/-", "value": {"Name": "books"} } ] `, - ExpectedError: "adding collections via patch is not supported. Name: books", + ExpectedError: "adding schema via patch is not supported. Name: books", }, testUtils.Request{ Request: `query { diff --git a/tests/integration/schema/updates/copy/simple_test.go b/tests/integration/schema/updates/copy/simple_test.go index cdda8abaf8..0f5a691149 100644 --- a/tests/integration/schema/updates/copy/simple_test.go +++ b/tests/integration/schema/updates/copy/simple_test.go @@ -38,7 +38,7 @@ func TestSchemaUpdatesCopyCollectionWithRemoveIDAndReplaceName(t *testing.T) { { "op": "replace", "path": "/Book/Name", "value": "Book" } ] `, - ExpectedError: "adding collections via patch is not supported. Name: Book", + ExpectedError: "adding schema via patch is not supported. Name: Book", }, }, } diff --git a/tests/integration/schema/updates/replace/simple_test.go b/tests/integration/schema/updates/replace/simple_test.go index 722ff36f9b..7e403f7d03 100644 --- a/tests/integration/schema/updates/replace/simple_test.go +++ b/tests/integration/schema/updates/replace/simple_test.go @@ -44,7 +44,7 @@ func TestSchemaUpdatesReplaceCollectionErrors(t *testing.T) { // WARNING: An error is still expected if/when we allow the adding of collections, as this also // implies that the "Users" collection is to be deleted. Only once we support the adding *and* // removal of collections should this not error. - ExpectedError: "adding collections via patch is not supported. Name: Book", + ExpectedError: "adding schema via patch is not supported. Name: Book", }, }, } From 8161b2cfed9b79bcdb80368d6a57d688e0fa80f9 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 17:57:12 -0400 Subject: [PATCH 20/25] Extract validateSchemaNameNotEmpty to new sys --- internal/db/definition_validation.go | 20 +++++++++++++++---- .../schema/updates/remove/simple_test.go | 4 ++-- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index c53ca2089e..c958b0121b 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -120,6 +120,7 @@ var globalValidators = []definitionValidator{ validateSecondaryFieldsPairUp, validateSingleSidePrimary, validateCollectionDefinitionPolicyDesc, + validateSchemaNameNotEmpty, validateRelationalFieldIDType, validateSecondaryNotOnSchema, validateTypeSupported, @@ -707,10 +708,6 @@ func (db *db) validateUpdateSchema( proposedDescriptionsByName map[string]client.SchemaDescription, proposedDesc client.SchemaDescription, ) (bool, error) { - if proposedDesc.Name == "" { - return false, ErrSchemaNameEmpty - } - existingDesc := existingDescriptionsByName[proposedDesc.Name] hasChangedFields, err := validateUpdateSchemaFields(proposedDescriptionsByName, existingDesc, proposedDesc) @@ -937,3 +934,18 @@ func validateSchemaNotAdded( return nil } + +func validateSchemaNameNotEmpty( + ctx context.Context, + db *db, + newState *definitionState, + oldState *definitionState, +) error { + for _, schema := range newState.schemaByName { + if schema.Name == "" { + return ErrSchemaNameEmpty + } + } + + return nil +} diff --git a/tests/integration/schema/updates/remove/simple_test.go b/tests/integration/schema/updates/remove/simple_test.go index e9e4f139ae..d0343484e5 100644 --- a/tests/integration/schema/updates/remove/simple_test.go +++ b/tests/integration/schema/updates/remove/simple_test.go @@ -34,7 +34,7 @@ func TestSchemaUpdatesRemoveCollectionNameErrors(t *testing.T) { { "op": "remove", "path": "/Users/Name" } ] `, - ExpectedError: "schema name can't be empty", + ExpectedError: "SchemaRoot does not match existing. Name: ", }, }, } @@ -118,7 +118,7 @@ func TestSchemaUpdatesRemoveSchemaNameErrors(t *testing.T) { { "op": "remove", "path": "/Users/Name" } ] `, - ExpectedError: "schema name can't be empty", + ExpectedError: "SchemaRoot does not match existing. Name: ", }, }, } From fe8e3845ba9dc3c0e580eea94067e176c3e72393 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Thu, 13 Jun 2024 18:19:30 -0400 Subject: [PATCH 21/25] Replace validateUpdateSchema with areSchemasEqual --- internal/db/collection_define.go | 5 ---- internal/db/definition_validation.go | 45 ---------------------------- internal/db/schema.go | 36 ++++++++++++++-------- 3 files changed, 23 insertions(+), 63 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index ba74b24862..5f55b67b46 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -48,11 +48,6 @@ func (db *db) createCollections( schemaByName[newDefinition.Schema.Name] = newDefinition.Schema } - _, err = validateUpdateSchemaFields(schemaByName, client.SchemaDescription{}, schema) - if err != nil { - return nil, err - } - schema, err = description.CreateSchemaVersion(ctx, txn, schema) if err != nil { return nil, err diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index c958b0121b..158064997e 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -699,51 +699,6 @@ func validateCollectionDefinitionPolicyDesc( return nil } -// validateUpdateSchema validates that the given schema description is a valid update. -// -// Will return true if the given description differs from the current persisted state of the -// schema. Will return an error if it fails validation. -func (db *db) validateUpdateSchema( - existingDescriptionsByName map[string]client.SchemaDescription, - proposedDescriptionsByName map[string]client.SchemaDescription, - proposedDesc client.SchemaDescription, -) (bool, error) { - existingDesc := existingDescriptionsByName[proposedDesc.Name] - - hasChangedFields, err := validateUpdateSchemaFields(proposedDescriptionsByName, existingDesc, proposedDesc) - if err != nil { - return hasChangedFields, err - } - - return hasChangedFields, err -} - -func validateUpdateSchemaFields( - descriptionsByName map[string]client.SchemaDescription, - existingDesc client.SchemaDescription, - proposedDesc client.SchemaDescription, -) (bool, error) { - hasChanged := false - existingFieldsByName := map[string]client.SchemaFieldDescription{} - existingFieldIndexesByName := map[string]int{} - for i, field := range existingDesc.Fields { - existingFieldIndexesByName[field.Name] = i - existingFieldsByName[field.Name] = field - } - - newFieldNames := map[string]struct{}{} - for _, proposedField := range proposedDesc.Fields { - _, fieldAlreadyExists := existingFieldsByName[proposedField.Name] - - // If the field is new, then the collection has changed - hasChanged = hasChanged || !fieldAlreadyExists - - newFieldNames[proposedField.Name] = struct{}{} - } - - return hasChanged, nil -} - func validateSchemaFieldNotDeleted( ctx context.Context, db *db, diff --git a/internal/db/schema.go b/internal/db/schema.go index bd14b3865c..d2aeb8bcb9 100644 --- a/internal/db/schema.go +++ b/internal/db/schema.go @@ -336,23 +336,18 @@ func (db *db) updateSchema( migration immutable.Option[model.Lens], setAsActiveVersion bool, ) error { - hasChanged, err := db.validateUpdateSchema( - existingSchemaByName, - proposedDescriptionsByName, - schema, - ) - if err != nil { - return err + previousSchema := existingSchemaByName[schema.Name] + + areEqual := areSchemasEqual(schema, previousSchema) + if areEqual { + return nil } - err = db.validateSchemaUpdate(ctx, proposedDescriptionsByName, existingSchemaByName) + + err := db.validateSchemaUpdate(ctx, proposedDescriptionsByName, existingSchemaByName) if err != nil { return err } - if !hasChanged { - return nil - } - for _, field := range schema.Fields { if field.Kind.IsObject() && !field.Kind.IsArray() { idFieldName := field.Name + "_id" @@ -365,7 +360,6 @@ func (db *db) updateSchema( } } - previousSchema := existingSchemaByName[schema.Name] previousFieldNames := make(map[string]struct{}, len(previousSchema.Fields)) for _, field := range previousSchema.Fields { previousFieldNames[field.Name] = struct{}{} @@ -529,3 +523,19 @@ func (db *db) updateSchema( return nil } + +func areSchemasEqual(this client.SchemaDescription, that client.SchemaDescription) bool { + if len(this.Fields) != len(that.Fields) { + return false + } + + for i, thisField := range this.Fields { + if thisField != that.Fields[i] { + return false + } + } + + return this.Name == that.Name && + this.Root == that.Root && + this.VersionID == that.VersionID +} From d5725bd2bbecc0b7d63cfc295ce69b0e4f78cddd Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 14 Jun 2024 16:48:12 -0400 Subject: [PATCH 22/25] PR FIXUP - Cleanup schema vars --- internal/db/collection_define.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 5f55b67b46..43cff20ec1 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -37,7 +37,6 @@ func (db *db) createCollections( } for i, def := range newDefinitions { - schema := def.Schema txn := mustGetContextTxn(ctx) schemaByName := map[string]client.SchemaDescription{} @@ -48,7 +47,7 @@ func (db *db) createCollections( schemaByName[newDefinition.Schema.Name] = newDefinition.Schema } - schema, err = description.CreateSchemaVersion(ctx, txn, schema) + schema, err := description.CreateSchemaVersion(ctx, txn, def.Schema) if err != nil { return nil, err } @@ -57,7 +56,6 @@ func (db *db) createCollections( } for _, def := range newDefinitions { - schema := def.Schema desc := def.Description txn := mustGetContextTxn(ctx) @@ -128,7 +126,7 @@ func (db *db) createCollections( return nil, err } - col := db.newCollection(desc, schema) + col := db.newCollection(desc, def.Schema) for _, index := range desc.Indexes { if _, err := col.createIndex(ctx, index); err != nil { From 0798bb7ba6604b2932f086b826bafadf54b89e27 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 14 Jun 2024 16:49:23 -0400 Subject: [PATCH 23/25] PR FIXUP - Cleanup txn var --- internal/db/collection_define.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 43cff20ec1..53160eb324 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -36,9 +36,9 @@ func (db *db) createCollections( return nil, err } - for i, def := range newDefinitions { - txn := mustGetContextTxn(ctx) + txn := mustGetContextTxn(ctx) + for i, def := range newDefinitions { schemaByName := map[string]client.SchemaDescription{} for _, existingDefinition := range existingDefinitions { schemaByName[existingDefinition.Schema.Name] = existingDefinition.Schema @@ -57,7 +57,6 @@ func (db *db) createCollections( for _, def := range newDefinitions { desc := def.Description - txn := mustGetContextTxn(ctx) if desc.Name.HasValue() { exists, err := description.HasCollectionByName(ctx, txn, desc.Name.Value()) From 697adb5ef6b412aa98cdca9246ee5631200dea32 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 14 Jun 2024 17:25:51 -0400 Subject: [PATCH 24/25] PR FIXUP - Remove CollectionAlreadyExists check from col_define.go Is now handled by the (existing) validation rules. --- internal/db/collection_define.go | 29 +++++++++----------------- internal/db/definition_validation.go | 30 ++++++++++++++++++++------- internal/db/view.go | 31 ++-------------------------- 3 files changed, 34 insertions(+), 56 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index 53160eb324..dcb7301e43 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -56,24 +56,10 @@ func (db *db) createCollections( } for _, def := range newDefinitions { - desc := def.Description - - if desc.Name.HasValue() { - exists, err := description.HasCollectionByName(ctx, txn, desc.Name.Value()) - if err != nil { - return nil, err - } - if exists { - return nil, ErrCollectionAlreadyExists - } - } - - definitionsByName := map[string]client.CollectionDefinition{} - for _, existingDefinition := range existingDefinitions { - definitionsByName[existingDefinition.GetName()] = existingDefinition - } - for _, newDefinition := range newDefinitions { - definitionsByName[newDefinition.GetName()] = newDefinition + if len(def.Description.Fields) == 0 { + // This is a schema-only definition, we should not create a collection for it + returnDescriptions = append(returnDescriptions, def) + continue } colSeq, err := db.getSequence(ctx, core.CollectionIDSequenceKey{}) @@ -90,6 +76,7 @@ func (db *db) createCollections( return nil, err } + desc := def.Description desc.ID = uint32(colID) desc.RootID = desc.ID @@ -115,7 +102,11 @@ func (db *db) createCollections( } } - err = db.validateNewCollection(ctx, definitionsByName) + err = db.validateNewCollection( + ctx, + append(newDefinitions, existingDefinitions...), + existingDefinitions, + ) if err != nil { return nil, err } diff --git a/internal/db/definition_validation.go b/internal/db/definition_validation.go index 158064997e..08e6e603a7 100644 --- a/internal/db/definition_validation.go +++ b/internal/db/definition_validation.go @@ -163,23 +163,37 @@ func (db *db) validateCollectionChanges( func (db *db) validateNewCollection( ctx context.Context, - newDefsByName map[string]client.CollectionDefinition, + newDefinitions []client.CollectionDefinition, + oldDefinitions []client.CollectionDefinition, ) error { - collections := []client.CollectionDescription{} - schemasByID := map[string]client.SchemaDescription{} + newCollections := []client.CollectionDescription{} + newSchemasByID := map[string]client.SchemaDescription{} - for _, def := range newDefsByName { + for _, def := range newDefinitions { if len(def.Description.Fields) != 0 { - collections = append(collections, def.Description) + newCollections = append(newCollections, def.Description) } - schemasByID[def.Schema.VersionID] = def.Schema + newSchemasByID[def.Schema.VersionID] = def.Schema } - newState := newDefinitionState(collections, schemasByID) + newState := newDefinitionState(newCollections, newSchemasByID) + + oldCollections := []client.CollectionDescription{} + oldSchemasByID := map[string]client.SchemaDescription{} + + for _, def := range oldDefinitions { + if len(def.Description.Fields) != 0 { + oldCollections = append(oldCollections, def.Description) + } + + oldSchemasByID[def.Schema.VersionID] = def.Schema + } + + oldState := newDefinitionState(oldCollections, oldSchemasByID) for _, validator := range createValidators { - err := validator(ctx, db, newState, &definitionState{}) + err := validator(ctx, db, newState, oldState) if err != nil { return err } diff --git a/internal/db/view.go b/internal/db/view.go index 190f365b6f..2664dd4a57 100644 --- a/internal/db/view.go +++ b/internal/db/view.go @@ -20,7 +20,6 @@ import ( "github.com/sourcenetwork/defradb/client" "github.com/sourcenetwork/defradb/client/request" - "github.com/sourcenetwork/defradb/internal/db/description" ) func (db *db) addView( @@ -29,8 +28,6 @@ func (db *db) addView( sdl string, transform immutable.Option[model.Lens], ) ([]client.CollectionDefinition, error) { - txn := mustGetContextTxn(ctx) - // Wrap the given query as part of the GQL query object - this simplifies the syntax for users // and ensures that we can't be given mutations. In the future this line should disappear along // with the all calls to the parser appart from `ParseSDL` when we implement the DQL stuff. @@ -68,36 +65,12 @@ func (db *db) addView( newDefinitions[i].Description.Sources = append(newDefinitions[i].Description.Sources, &source) } - returnDescriptions := make([]client.CollectionDefinition, 0, len(newDefinitions)) - collectionDefinitions := []client.CollectionDefinition{} - schemaOnlyDefinitions := []client.SchemaDescription{} - - for _, definition := range newDefinitions { - if definition.Description.Name.HasValue() { - collectionDefinitions = append(collectionDefinitions, definition) - } else { - schemaOnlyDefinitions = append(schemaOnlyDefinitions, definition.Schema) - } - } - - for _, schema := range schemaOnlyDefinitions { - schema, err := description.CreateSchemaVersion(ctx, txn, schema) - if err != nil { - return nil, err - } - returnDescriptions = append(returnDescriptions, client.CollectionDefinition{ - // `Collection` is left as default for embedded types - Schema: schema, - }) - } - - returnColDescriptions, err := db.createCollections(ctx, collectionDefinitions) + returnDescriptions, err := db.createCollections(ctx, newDefinitions) if err != nil { return nil, err } - returnDescriptions = append(returnDescriptions, returnColDescriptions...) - for _, definition := range returnColDescriptions { + for _, definition := range returnDescriptions { for _, source := range definition.Description.QuerySources() { if source.Transform.HasValue() { err = db.LensRegistry().SetMigration(ctx, definition.Description.ID, source.Transform.Value()) From a55f317120d451dc9d78a76f300862f3c8012ca3 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 14 Jun 2024 17:37:34 -0400 Subject: [PATCH 25/25] PR FIXUP - Validate all definitions outside of loop Sorry, I spotted late that all the definitions were being validated on ever iteration --- internal/db/collection_define.go | 46 ++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 17 deletions(-) diff --git a/internal/db/collection_define.go b/internal/db/collection_define.go index dcb7301e43..a8b9fe9abd 100644 --- a/internal/db/collection_define.go +++ b/internal/db/collection_define.go @@ -55,10 +55,9 @@ func (db *db) createCollections( newDefinitions[i].Schema = schema } - for _, def := range newDefinitions { + for i, def := range newDefinitions { if len(def.Description.Fields) == 0 { // This is a schema-only definition, we should not create a collection for it - returnDescriptions = append(returnDescriptions, def) continue } @@ -76,11 +75,10 @@ func (db *db) createCollections( return nil, err } - desc := def.Description - desc.ID = uint32(colID) - desc.RootID = desc.ID + newDefinitions[i].Description.ID = uint32(colID) + newDefinitions[i].Description.RootID = newDefinitions[i].Description.ID - for _, localField := range desc.Fields { + for _, localField := range def.Description.Fields { var fieldID uint64 if localField.Name == request.DocIDFieldName { // There is no hard technical requirement for this, we just think it looks nicer @@ -94,24 +92,38 @@ func (db *db) createCollections( } } - for i := range desc.Fields { - if desc.Fields[i].Name == localField.Name { - desc.Fields[i].ID = client.FieldID(fieldID) + for j := range def.Description.Fields { + if def.Description.Fields[j].Name == localField.Name { + newDefinitions[i].Description.Fields[j].ID = client.FieldID(fieldID) break } } } + } - err = db.validateNewCollection( - ctx, - append(newDefinitions, existingDefinitions...), - existingDefinitions, - ) - if err != nil { - return nil, err + err = db.validateNewCollection( + ctx, + append( + append( + []client.CollectionDefinition{}, + newDefinitions..., + ), + existingDefinitions..., + ), + existingDefinitions, + ) + if err != nil { + return nil, err + } + + for _, def := range newDefinitions { + if len(def.Description.Fields) == 0 { + // This is a schema-only definition, we should not create a collection for it + returnDescriptions = append(returnDescriptions, def) + continue } - desc, err = description.SaveCollection(ctx, txn, desc) + desc, err := description.SaveCollection(ctx, txn, def.Description) if err != nil { return nil, err }