From 4a6e816c60ff396f545b091ad2b05737aa2a330b Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 15 Sep 2023 15:23:27 -0400 Subject: [PATCH] Allow choice in PatchSchema to make default --- api/http/handlerfuncs.go | 5 ++- client/db.go | 5 +-- client/mocks/db.go | 21 +++++------ db/collection.go | 5 --- db/schema.go | 10 +++++- db/txn_db.go | 8 ++--- http/client.go | 14 ++++++-- http/handler_store.go | 6 ++-- http/wrapper.go | 4 +-- .../migrations/query/with_set_default_test.go | 4 +++ .../schema/updates/add/field/simple_test.go | 35 +++++++++++++++++++ .../schema/with_update_set_default_test.go | 4 +++ tests/integration/test_case.go | 8 +++-- tests/integration/utils2.go | 9 ++++- 14 files changed, 106 insertions(+), 32 deletions(-) diff --git a/api/http/handlerfuncs.go b/api/http/handlerfuncs.go index e4163de05f..2a248d7d81 100644 --- a/api/http/handlerfuncs.go +++ b/api/http/handlerfuncs.go @@ -271,7 +271,10 @@ func patchSchemaHandler(rw http.ResponseWriter, req *http.Request) { return } - err = db.PatchSchema(req.Context(), string(patch)) + // Hardcode setDefault to true here, as that preserves the existing behaviour. + // This function will be ripped out very shortly and I don't think it is worth + // spending time/thought here. The new http api handles this correctly. + err = db.PatchSchema(req.Context(), string(patch), true) if err != nil { handleErr(req.Context(), rw, err, http.StatusInternalServerError) return diff --git a/client/db.go b/client/db.go index 18f4df680c..47cd7d5a85 100644 --- a/client/db.go +++ b/client/db.go @@ -96,7 +96,8 @@ type Store interface { AddSchema(context.Context, string) ([]CollectionDescription, error) // PatchSchema takes the given JSON patch string and applies it to the set of CollectionDescriptions - // present in the database. + // present in the database. If true is provided, the new schema versions will be made default, otherwise + // [SetDefaultSchemaVersion] should be called to set them so. // // It will also update the GQL types used by the query system. It will error and not apply any of the // requested, valid updates should the net result of the patch result in an invalid state. The @@ -109,7 +110,7 @@ type Store interface { // // Field [FieldKind] values may be provided in either their raw integer form, or as string as per // [FieldKindStringToEnumMapping]. - PatchSchema(context.Context, string) error + PatchSchema(context.Context, string, bool) error // SetDefaultSchemaVersion sets the default schema version to the ID provided. It will be applied to all // collections using the schema. diff --git a/client/mocks/db.go b/client/mocks/db.go index afaa1503bf..02d32c4e8c 100644 --- a/client/mocks/db.go +++ b/client/mocks/db.go @@ -992,13 +992,13 @@ func (_c *DB_NewTxn_Call) RunAndReturn(run func(context.Context, bool) (datastor return _c } -// PatchSchema provides a mock function with given fields: _a0, _a1 -func (_m *DB) PatchSchema(_a0 context.Context, _a1 string) error { - ret := _m.Called(_a0, _a1) +// PatchSchema provides a mock function with given fields: _a0, _a1, _a2 +func (_m *DB) PatchSchema(_a0 context.Context, _a1 string, _a2 bool) error { + ret := _m.Called(_a0, _a1, _a2) var r0 error - if rf, ok := ret.Get(0).(func(context.Context, string) error); ok { - r0 = rf(_a0, _a1) + if rf, ok := ret.Get(0).(func(context.Context, string, bool) error); ok { + r0 = rf(_a0, _a1, _a2) } else { r0 = ret.Error(0) } @@ -1014,13 +1014,14 @@ type DB_PatchSchema_Call struct { // PatchSchema is a helper method to define mock.On call // - _a0 context.Context // - _a1 string -func (_e *DB_Expecter) PatchSchema(_a0 interface{}, _a1 interface{}) *DB_PatchSchema_Call { - return &DB_PatchSchema_Call{Call: _e.mock.On("PatchSchema", _a0, _a1)} +// - _a2 bool +func (_e *DB_Expecter) PatchSchema(_a0 interface{}, _a1 interface{}, _a2 interface{}) *DB_PatchSchema_Call { + return &DB_PatchSchema_Call{Call: _e.mock.On("PatchSchema", _a0, _a1, _a2)} } -func (_c *DB_PatchSchema_Call) Run(run func(_a0 context.Context, _a1 string)) *DB_PatchSchema_Call { +func (_c *DB_PatchSchema_Call) Run(run func(_a0 context.Context, _a1 string, _a2 bool)) *DB_PatchSchema_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(context.Context), args[1].(string)) + run(args[0].(context.Context), args[1].(string), args[2].(bool)) }) return _c } @@ -1030,7 +1031,7 @@ func (_c *DB_PatchSchema_Call) Return(_a0 error) *DB_PatchSchema_Call { return _c } -func (_c *DB_PatchSchema_Call) RunAndReturn(run func(context.Context, string) error) *DB_PatchSchema_Call { +func (_c *DB_PatchSchema_Call) RunAndReturn(run func(context.Context, string, bool) error) *DB_PatchSchema_Call { _c.Call.Return(run) return _c } diff --git a/db/collection.go b/db/collection.go index e8ed363a7b..52bc2a1d59 100644 --- a/db/collection.go +++ b/db/collection.go @@ -300,11 +300,6 @@ func (db *db) updateCollection( return nil, err } - err = db.setDefaultSchemaVersionExplicit(ctx, txn, desc.Name, desc.Schema.SchemaID, schemaVersionID) - if err != nil { - return nil, err - } - schemaVersionHistoryKey := core.NewSchemaHistoryKey(desc.Schema.SchemaID, previousSchemaVersionID) err = txn.Systemstore().Put(ctx, schemaVersionHistoryKey.ToDS(), []byte(schemaVersionID)) if err != nil { diff --git a/db/schema.go b/db/schema.go index 5c5c0568f8..bd0c45b13d 100644 --- a/db/schema.go +++ b/db/schema.go @@ -103,7 +103,7 @@ func (db *db) getCollectionDescriptions( // The collections (including the schema version ID) will only be updated if any changes have actually // been made, if the net result of the patch matches the current persisted description then no changes // will be applied. -func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString string) error { +func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString string, setAsDefaultVersion bool) error { patch, err := jsonpatch.DecodePatch([]byte(patchString)) if err != nil { return err @@ -148,6 +148,14 @@ func (db *db) patchSchema(ctx context.Context, txn datastore.Txn, patchString st if err != nil { return err } + + if setAsDefaultVersion { + err = db.setDefaultSchemaVersionExplicit(ctx, txn, desc.Name, desc.Schema.SchemaID, col.Schema().VersionID) + if err != nil { + return err + } + } + newDescriptions[i] = col.Description() } diff --git a/db/txn_db.go b/db/txn_db.go index 03afaeb34a..b4cc32dee1 100644 --- a/db/txn_db.go +++ b/db/txn_db.go @@ -250,14 +250,14 @@ func (db *explicitTxnDB) AddSchema(ctx context.Context, schemaString string) ([] // The collections (including the schema version ID) will only be updated if any changes have actually // been made, if the net result of the patch matches the current persisted description then no changes // will be applied. -func (db *implicitTxnDB) PatchSchema(ctx context.Context, patchString string) error { +func (db *implicitTxnDB) PatchSchema(ctx context.Context, patchString string, setAsDefaultVersion bool) error { txn, err := db.NewTxn(ctx, false) if err != nil { return err } defer txn.Discard(ctx) - err = db.patchSchema(ctx, txn, patchString) + err = db.patchSchema(ctx, txn, patchString, setAsDefaultVersion) if err != nil { return err } @@ -276,8 +276,8 @@ func (db *implicitTxnDB) PatchSchema(ctx context.Context, patchString string) er // The collections (including the schema version ID) will only be updated if any changes have actually // been made, if the net result of the patch matches the current persisted description then no changes // will be applied. -func (db *explicitTxnDB) PatchSchema(ctx context.Context, patchString string) error { - return db.patchSchema(ctx, db.txn, patchString) +func (db *explicitTxnDB) PatchSchema(ctx context.Context, patchString string, setAsDefaultVersion bool) error { + return db.patchSchema(ctx, db.txn, patchString, setAsDefaultVersion) } func (db *implicitTxnDB) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error { diff --git a/http/client.go b/http/client.go index d0a7dba37f..867cdc3bb1 100644 --- a/http/client.go +++ b/http/client.go @@ -212,10 +212,20 @@ func (c *Client) AddSchema(ctx context.Context, schema string) ([]client.Collect return cols, nil } -func (c *Client) PatchSchema(ctx context.Context, patch string) error { +type patchSchemaRequest struct { + Patch string + SetAsDefaultVersion bool +} + +func (c *Client) PatchSchema(ctx context.Context, patch string, setAsDefaultVersion bool) error { methodURL := c.http.baseURL.JoinPath("schema") - req, err := http.NewRequestWithContext(ctx, http.MethodPatch, methodURL.String(), strings.NewReader(patch)) + body, err := json.Marshal(patchSchemaRequest{patch, setAsDefaultVersion}) + if err != nil { + return err + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, methodURL.String(), bytes.NewBuffer(body)) if err != nil { return err } diff --git a/http/handler_store.go b/http/handler_store.go index 40a8762d5d..945f6115f8 100644 --- a/http/handler_store.go +++ b/http/handler_store.go @@ -151,12 +151,14 @@ func (s *storeHandler) AddSchema(rw http.ResponseWriter, req *http.Request) { func (s *storeHandler) PatchSchema(rw http.ResponseWriter, req *http.Request) { store := req.Context().Value(storeContextKey).(client.Store) - patch, err := io.ReadAll(req.Body) + var message patchSchemaRequest + err := requestJSON(req, &message) if err != nil { responseJSON(rw, http.StatusBadRequest, errorResponse{err}) return } - err = store.PatchSchema(req.Context(), string(patch)) + + err = store.PatchSchema(req.Context(), message.Patch, message.SetAsDefaultVersion) if err != nil { responseJSON(rw, http.StatusBadRequest, errorResponse{err}) return diff --git a/http/wrapper.go b/http/wrapper.go index 31dc37a74f..eb91ffdb7a 100644 --- a/http/wrapper.go +++ b/http/wrapper.go @@ -86,8 +86,8 @@ func (w *Wrapper) AddSchema(ctx context.Context, schema string) ([]client.Collec return w.client.AddSchema(ctx, schema) } -func (w *Wrapper) PatchSchema(ctx context.Context, patch string) error { - return w.client.PatchSchema(ctx, patch) +func (w *Wrapper) PatchSchema(ctx context.Context, patch string, setAsDefaultVersion bool) error { + return w.client.PatchSchema(ctx, patch, setAsDefaultVersion) } func (w *Wrapper) SetDefaultSchemaVersion(ctx context.Context, schemaVersionID string) error { 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 3f929da7f9..e276bcab24 100644 --- a/tests/integration/schema/migrations/query/with_set_default_test.go +++ b/tests/integration/schema/migrations/query/with_set_default_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/lens-vm/lens/host-go/config/model" + "github.com/sourcenetwork/immutable" "github.com/sourcenetwork/defradb/client" testUtils "github.com/sourcenetwork/defradb/tests/integration" @@ -45,6 +46,7 @@ func TestSchemaMigrationQuery_WithSetDefaultToLatest_AppliesForwardMigration(t * { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": "String"} } ] `, + SetAsDefaultVersion: immutable.Some(false), }, testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ @@ -107,6 +109,7 @@ func TestSchemaMigrationQuery_WithSetDefaultToOriginal_AppliesInverseMigration(t { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": "String"} } ] `, + SetAsDefaultVersion: immutable.Some(false), }, testUtils.SetDefaultSchemaVersion{ SchemaVersionID: schemaVersionID2, @@ -188,6 +191,7 @@ func TestSchemaMigrationQuery_WithSetDefaultToOriginalVersionThatDocWasCreatedAt { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": "String"} } ] `, + SetAsDefaultVersion: immutable.Some(true), }, testUtils.ConfigureMigration{ LensConfig: client.LensConfig{ diff --git a/tests/integration/schema/updates/add/field/simple_test.go b/tests/integration/schema/updates/add/field/simple_test.go index d64f9e3bbe..1fe6980a62 100644 --- a/tests/integration/schema/updates/add/field/simple_test.go +++ b/tests/integration/schema/updates/add/field/simple_test.go @@ -13,6 +13,8 @@ package field import ( "testing" + "github.com/sourcenetwork/immutable" + testUtils "github.com/sourcenetwork/defradb/tests/integration" ) @@ -48,6 +50,39 @@ func TestSchemaUpdatesAddFieldSimple(t *testing.T) { testUtils.ExecuteTestCase(t, test) } +func TestSchemaUpdates_AddFieldSimpleDoNotSetDefault_Errors(t *testing.T) { + test := testUtils.TestCase{ + Description: "Test schema update, add field", + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type Users { + name: String + } + `, + }, + testUtils.SchemaPatch{ + Patch: ` + [ + { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": 11} } + ] + `, + SetAsDefaultVersion: immutable.Some(false), + }, + testUtils.Request{ + Request: `query { + Users { + name + email + } + }`, + ExpectedError: `Cannot query field "email" on type "Users".`, + }, + }, + } + testUtils.ExecuteTestCase(t, test) +} + func TestSchemaUpdatesAddFieldSimpleErrorsAddingToUnknownCollection(t *testing.T) { test := testUtils.TestCase{ Description: "Test schema update, add to unknown collection fails", diff --git a/tests/integration/schema/with_update_set_default_test.go b/tests/integration/schema/with_update_set_default_test.go index ae92e556f9..3b365e0e5f 100644 --- a/tests/integration/schema/with_update_set_default_test.go +++ b/tests/integration/schema/with_update_set_default_test.go @@ -13,6 +13,8 @@ package schema import ( "testing" + "github.com/sourcenetwork/immutable" + testUtils "github.com/sourcenetwork/defradb/tests/integration" ) @@ -87,6 +89,7 @@ func TestSchema_WithUpdateAndSetDefaultVersionToOriginal_NewFieldIsNotQueriable( { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": 11} } ] `, + SetAsDefaultVersion: immutable.Some(false), }, testUtils.SetDefaultSchemaVersion{ SchemaVersionID: "bafkreihn4qameldz3j7rfundmd4ldhxnaircuulk6h2vcwnpcgxl4oqffq", @@ -123,6 +126,7 @@ func TestSchema_WithUpdateAndSetDefaultVersionToNew_AllowsQueryingOfNewField(t * { "op": "add", "path": "/Users/Schema/Fields/-", "value": {"Name": "email", "Kind": 11} } ] `, + SetAsDefaultVersion: immutable.Some(false), }, testUtils.SetDefaultSchemaVersion{ SchemaVersionID: "bafkreidejaxpsevyijnr4nah4e2l263emwhdaj57fwwv34eu5rea4ff54e", diff --git a/tests/integration/test_case.go b/tests/integration/test_case.go index c8580350b3..10f3cf7262 100644 --- a/tests/integration/test_case.go +++ b/tests/integration/test_case.go @@ -81,8 +81,12 @@ type SchemaPatch struct { // If a value is not provided the patch will be applied to all nodes. NodeID immutable.Option[int] - Patch string - ExpectedError string + Patch string + + // If SetAsDefaultVersion has a value, and that value is false then the schema version + // resulting from this patch will not be made default. + SetAsDefaultVersion immutable.Option[bool] + ExpectedError string } // SetDefaultSchemaVersion is an action that will set the default schema version to the diff --git a/tests/integration/utils2.go b/tests/integration/utils2.go index 717cfc0127..545e880f4f 100644 --- a/tests/integration/utils2.go +++ b/tests/integration/utils2.go @@ -1062,7 +1062,14 @@ func patchSchema( action SchemaPatch, ) { for _, node := range getNodes(action.NodeID, s.nodes) { - err := node.DB.PatchSchema(s.ctx, action.Patch) + var setAsDefaultVersion bool + if action.SetAsDefaultVersion.HasValue() { + setAsDefaultVersion = action.SetAsDefaultVersion.Value() + } else { + setAsDefaultVersion = true + } + + err := node.DB.PatchSchema(s.ctx, action.Patch, setAsDefaultVersion) expectedErrorRaised := AssertError(s.t, s.testCase.Description, err, action.ExpectedError) assertExpectedErrorRaised(s.t, s.testCase.Description, action.ExpectedError, expectedErrorRaised)