diff --git a/client/descriptions.go b/client/descriptions.go index 8acc8b0e8f..0c85e87338 100644 --- a/client/descriptions.go +++ b/client/descriptions.go @@ -238,7 +238,7 @@ func (f FieldDescription) IsObjectArray() bool { // IsPrimaryRelation returns true if this field is a relation, and is the primary side. func (f FieldDescription) IsPrimaryRelation() bool { - return f.RelationType > 0 && f.RelationType&Relation_Type_Primary == 0 + return f.RelationType > 0 && f.RelationType&Relation_Type_Primary != 0 } // IsSet returns true if the target relation type is set. diff --git a/db/collection.go b/db/collection.go index c354e4f004..9e588b0597 100644 --- a/db/collection.go +++ b/db/collection.go @@ -15,7 +15,6 @@ import ( "encoding/json" "fmt" "strconv" - "strings" "github.com/fxamacker/cbor/v2" "github.com/ipfs/go-cid" @@ -738,6 +737,15 @@ func (c *collection) save( txn datastore.Txn, doc *client.Document, ) (cid.Cid, error) { + // NOTE: We delay the final Clean() call until we know + // the commit on the transaction is successful. If we didn't + // wait, and just did it here, then *if* the commit fails down + // the line, then we have no way to roll back the state + // side-effect on the document func called here. + txn.OnSuccess(func() { + doc.Clean() + }) + // New batch transaction/store (optional/todo) // Ensute/Set doc object marker // Loop through doc values @@ -758,10 +766,25 @@ func (c *collection) save( return cid.Undef, client.NewErrFieldNotExist(k) } - if c.isFieldNameRelationID(k) { + fieldDescription, valid := c.desc.GetField(k) + if !valid { return cid.Undef, client.NewErrFieldNotExist(k) } + relationFieldDescription, isSecondaryRelationID := c.isSecondaryIDField(fieldDescription) + if isSecondaryRelationID { + primaryId := val.Value().(string) + + err = c.patchPrimaryDoc(ctx, txn, relationFieldDescription, primaryKey.DocKey, primaryId) + if err != nil { + return cid.Undef, err + } + + // If this field was a secondary relation ID the related document will have been + // updated instead and we should discard this value + continue + } + node, _, err := c.saveDocValue(ctx, txn, fieldKey, val) if err != nil { return cid.Undef, err @@ -772,15 +795,6 @@ func (c *collection) save( docProperties[k] = val.Value() } - // NOTE: We delay the final Clean() call until we know - // the commit on the transaction is successful. If we didn't - // wait, and just did it here, then *if* the commit fails down - // the line, then we have no way to roll back the state - // side-effect on the document func called here. - txn.OnSuccess(func() { - doc.Clean() - }) - link := core.DAGLink{ Name: k, Cid: node.Cid(), @@ -1116,33 +1130,3 @@ func (c *collection) tryGetSchemaFieldID(fieldName string) (uint32, bool) { } return uint32(0), false } - -// isFieldNameRelationID returns true if the given field is the id field backing a relationship. -func (c *collection) isFieldNameRelationID(fieldName string) bool { - fieldDescription, valid := c.desc.GetField(fieldName) - if !valid { - return false - } - - return c.isFieldDescriptionRelationID(&fieldDescription) -} - -// isFieldDescriptionRelationID returns true if the given field is the id field backing a relationship. -func (c *collection) isFieldDescriptionRelationID(fieldDescription *client.FieldDescription) bool { - if fieldDescription.RelationType == client.Relation_Type_INTERNAL_ID { - relationDescription, valid := c.desc.GetField(strings.TrimSuffix(fieldDescription.Name, "_id")) - if !valid { - return false - } - if relationDescription.IsPrimaryRelation() { - return true - } - } - return false -} - -// makeCollectionKey returns a formatted collection key for the system data store. -// it assumes the name of the collection is non-empty. -// func makeCollectionDataKey(collectionID uint32) core.Key { -// return collectionNs.ChildString(name) -// } diff --git a/db/collection_update.go b/db/collection_update.go index 61c0cdf611..59e3fec03d 100644 --- a/db/collection_update.go +++ b/db/collection_update.go @@ -12,6 +12,8 @@ package db import ( "context" + "fmt" + "strings" cbor "github.com/fxamacker/cbor/v2" "github.com/sourcenetwork/immutable" @@ -302,8 +304,21 @@ func (c *collection) applyMerge( return client.NewErrFieldNotExist(mfield) } - if c.isFieldDescriptionRelationID(&fd) { - return client.NewErrFieldNotExist(mfield) + relationFieldDescription, isSecondaryRelationID := c.isSecondaryIDField(fd) + if isSecondaryRelationID { + primaryId, err := getString(mval) + if err != nil { + return err + } + + err = c.patchPrimaryDoc(ctx, txn, relationFieldDescription, keyStr, primaryId) + if err != nil { + return err + } + + // If this field was a secondary relation ID the related document will have been + // updated instead and we should discard this merge item + continue } cborVal, err := validateFieldSchema(mval, fd) @@ -322,7 +337,7 @@ func (c *collection) applyMerge( if err != nil { return err } - // links[mfield] = c + links = append(links, core.DAGLink{ Name: mfield, Cid: c.Cid(), @@ -370,6 +385,54 @@ func (c *collection) applyMerge( return nil } +// isSecondaryIDField returns true if the given field description represents a secondary relation field ID. +func (c *collection) isSecondaryIDField(fieldDesc client.FieldDescription) (client.FieldDescription, bool) { + if fieldDesc.RelationType != client.Relation_Type_INTERNAL_ID { + return client.FieldDescription{}, false + } + + relationFieldDescription, valid := c.Description().GetField(strings.TrimSuffix(fieldDesc.Name, "_id")) + return relationFieldDescription, valid && !relationFieldDescription.IsPrimaryRelation() +} + +// patchPrimaryDoc patches the (primary) document linked to from the document of the given dockey via the +// given (secondary) relationship field description (hosted on the collection of the document matching the +// given dockey). +// +// The given field value should be the string representation of the dockey of the primary document to be +// patched. +func (c *collection) patchPrimaryDoc( + ctx context.Context, + txn datastore.Txn, + relationFieldDescription client.FieldDescription, + docKey string, + fieldValue string, +) error { + primaryDockey, err := client.NewDocKeyFromString(fieldValue) + if err != nil { + return err + } + + primaryCol, err := c.db.getCollectionByName(ctx, txn, relationFieldDescription.Schema) + if err != nil { + return err + } + primaryCol = primaryCol.WithTxn(txn) + + primaryField, _ := primaryCol.Description().GetRelation(relationFieldDescription.RelationName) + + _, err = primaryCol.UpdateWithKey( + ctx, + primaryDockey, + fmt.Sprintf(`{"%s": "%s"}`, primaryField.Name+"_id", docKey), + ) + if err != nil { + return err + } + + return nil +} + // validateFieldSchema takes a given value as an interface, // and ensures it matches the supplied field description. // It will do any minor parsing, like dates, and return diff --git a/tests/integration/collection/create/one_to_one/save_test.go b/tests/integration/collection/create/one_to_one/save_test.go index fa39114cf0..6981aef6bb 100644 --- a/tests/integration/collection/create/one_to_one/save_test.go +++ b/tests/integration/collection/create/one_to_one/save_test.go @@ -20,7 +20,7 @@ import ( testUtils "github.com/sourcenetwork/defradb/tests/integration/collection" ) -func TestCollectionCreateSaveErrorsGivenRelationSetOnWrongSide(t *testing.T) { +func TestCollectionCreateSaveErrorsNonExistantKeyViaSecondarySide(t *testing.T) { doc, err := client.NewDocFromJSON( []byte( `{ @@ -41,12 +41,15 @@ func TestCollectionCreateSaveErrorsGivenRelationSetOnWrongSide(t *testing.T) { }, }, }, - ExpectedError: "The given field does not exist", + ExpectedError: "no document for the given key exists", } executeTestCase(t, test) } +// Note: This test should probably not pass, as it contains a +// reference to a document that doesnt exist. It is doubly odd +// given that saving from the secondary side errors as expected func TestCollectionCreateSaveCreatesDoc(t *testing.T) { doc, err := client.NewDocFromJSON( []byte( @@ -89,3 +92,36 @@ func TestCollectionCreateSaveCreatesDoc(t *testing.T) { executeTestCase(t, test) } + +func TestCollectionCreateSaveFromSecondarySide(t *testing.T) { + doc, err := client.NewDocFromJSON( + []byte( + `{ + "name": "Painted House", + "author_id": "bae-2edb7fdd-cad7-5ad4-9c7d-6920245a96ed" + }`, + ), + ) + if err != nil { + assert.Fail(t, err.Error()) + } + + test := testUtils.TestCase{ + Docs: map[string][]string{ + "author": { + `{ + "name": "John Grisham" + }`, + }, + }, + CollectionCalls: map[string][]func(client.Collection) error{ + "book": []func(c client.Collection) error{ + func(c client.Collection) error { + return c.Save(context.Background(), doc) + }, + }, + }, + } + + executeTestCase(t, test) +} diff --git a/tests/integration/collection/update/one_to_one/save_test.go b/tests/integration/collection/update/one_to_one/save_test.go index 631de53f0d..44407a65d6 100644 --- a/tests/integration/collection/update/one_to_one/save_test.go +++ b/tests/integration/collection/update/one_to_one/save_test.go @@ -20,7 +20,7 @@ import ( testUtils "github.com/sourcenetwork/defradb/tests/integration/collection" ) -func TestUpdateOneToOneSaveErrorsGivenWrongSideOfRelation(t *testing.T) { +func TestUpdateOneToOneSaveErrorsGivenNonExistantKeyViaSecondarySide(t *testing.T) { doc, err := client.NewDocFromJSON( []byte( `{ @@ -35,7 +35,7 @@ func TestUpdateOneToOneSaveErrorsGivenWrongSideOfRelation(t *testing.T) { err = doc.SetWithJSON( []byte( `{ - "author_id": "ValueDoesntMatter" + "author_id": "bae-fd541c25-229e-5280-b44b-e5c2af3e374d" }`, ), ) @@ -58,14 +58,15 @@ func TestUpdateOneToOneSaveErrorsGivenWrongSideOfRelation(t *testing.T) { }, }, }, - ExpectedError: "The given field does not exist", + ExpectedError: "no document for the given key exists", } executeTestCase(t, test) } // Note: This test should probably not pass, as it contains a -// reference to a document that doesnt exist. +// reference to a document that doesnt exist. It is doubly odd +// given that saving from the secondary side errors as expected func TestUpdateOneToOneSavesGivenNewRelationValue(t *testing.T) { doc, err := client.NewDocFromJSON( []byte( @@ -108,3 +109,51 @@ func TestUpdateOneToOneSavesGivenNewRelationValue(t *testing.T) { executeTestCase(t, test) } + +func TestUpdateOneToOneSaveFromSecondarySide(t *testing.T) { + doc, err := client.NewDocFromJSON( + []byte( + `{ + "name": "Painted House" + }`, + ), + ) + if err != nil { + assert.Fail(t, err.Error()) + } + + err = doc.SetWithJSON( + []byte( + `{ + "author_id": "bae-2edb7fdd-cad7-5ad4-9c7d-6920245a96ed" + }`, + ), + ) + if err != nil { + assert.Fail(t, err.Error()) + } + + test := testUtils.TestCase{ + Docs: map[string][]string{ + "author": { + `{ + "name": "John Grisham" + }`, + }, + "book": { + `{ + "name": "Painted House" + }`, + }, + }, + CollectionCalls: map[string][]func(client.Collection) error{ + "book": []func(c client.Collection) error{ + func(c client.Collection) error { + return c.Save(context.Background(), doc) + }, + }, + }, + } + + executeTestCase(t, test) +} diff --git a/tests/integration/mutation/one_to_one/create/with_simple_test.go b/tests/integration/mutation/one_to_one/create/with_simple_test.go index 2d60c2058f..4520ce326f 100644 --- a/tests/integration/mutation/one_to_one/create/with_simple_test.go +++ b/tests/integration/mutation/one_to_one/create/with_simple_test.go @@ -18,79 +18,155 @@ import ( simpleTests "github.com/sourcenetwork/defradb/tests/integration/mutation/one_to_one" ) -// This test documents incorrect behaviour. It should be possible to create author then book, -// linking in the second create step (like in [TestMutationCreateOneToOne]). -// https://github.com/sourcenetwork/defradb/issues/1213 -func TestMutationCreateOneToOneWrongSide(t *testing.T) { +// Note: This test should probably not pass, as it contains a +// reference to a document that doesnt exist. +func TestMutationCreateOneToOneNoChild(t *testing.T) { test := testUtils.TestCase{ Description: "One to one create mutation, from the wrong side", Actions: []any{ testUtils.Request{ Request: `mutation { - create_book(data: "{\"name\": \"Painted House\",\"author_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { - _key + create_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { + name } }`, - ExpectedError: "The given field does not exist", + Results: []map[string]any{ + { + "name": "John Grisham", + }, + }, }, }, } - simpleTests.ExecuteTestCase(t, test) } -// Note: This test should probably not pass, as it contains a -// reference to a document that doesnt exist. -func TestMutationCreateOneToOneNoChild(t *testing.T) { +func TestMutationCreateOneToOne(t *testing.T) { + bookKey := "bae-3d236f89-6a31-5add-a36a-27971a2eac76" + test := testUtils.TestCase{ - Description: "One to one create mutation, from the wrong side", + Description: "One to one create mutation", Actions: []any{ testUtils.Request{ Request: `mutation { - create_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { + create_book(data: "{\"name\": \"Painted House\"}") { + _key + } + }`, + Results: []map[string]any{ + { + "_key": bookKey, + }, + }, + }, + testUtils.Request{ + Request: fmt.Sprintf( + `mutation { + create_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"%s\"}") { + name + } + }`, + bookKey, + ), + Results: []map[string]any{ + { + "name": "John Grisham", + }, + }, + }, + testUtils.Request{ + Request: ` + query { + book { + name + author { name } - }`, + } + }`, + Results: []map[string]any{ + { + "name": "Painted House", + "author": map[string]any{ + "name": "John Grisham", + }, + }, + }, + }, + testUtils.Request{ + Request: ` + query { + author { + name + published { + name + } + } + }`, Results: []map[string]any{ { "name": "John Grisham", + "published": map[string]any{ + "name": "Painted House", + }, }, }, }, }, } + simpleTests.ExecuteTestCase(t, test) } -func TestMutationCreateOneToOne(t *testing.T) { - bookKey := "bae-3d236f89-6a31-5add-a36a-27971a2eac76" +func TestMutationCreateOneToOneSecondarySide(t *testing.T) { + authorKey := "bae-2edb7fdd-cad7-5ad4-9c7d-6920245a96ed" test := testUtils.TestCase{ - Description: "One to one create mutation", + Description: "One to one create mutation from secondary side", Actions: []any{ testUtils.Request{ Request: `mutation { - create_book(data: "{\"name\": \"Painted House\"}") { + create_author(data: "{\"name\": \"John Grisham\"}") { _key } }`, Results: []map[string]any{ { - "_key": bookKey, + "_key": authorKey, }, }, }, testUtils.Request{ Request: fmt.Sprintf( `mutation { - create_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"%s\"}") { + create_book(data: "{\"name\": \"Painted House\",\"author_id\": \"%s\"}") { name } }`, - bookKey), + authorKey, + ), + Results: []map[string]any{ + { + "name": "Painted House", + }, + }, + }, + testUtils.Request{ + Request: ` + query { + author { + name + published { + name + } + } + }`, Results: []map[string]any{ { "name": "John Grisham", + "published": map[string]any{ + "name": "Painted House", + }, }, }, }, diff --git a/tests/integration/mutation/one_to_one/update/with_simple_test.go b/tests/integration/mutation/one_to_one/update/with_simple_test.go index e564e74ae9..10ecff8509 100644 --- a/tests/integration/mutation/one_to_one/update/with_simple_test.go +++ b/tests/integration/mutation/one_to_one/update/with_simple_test.go @@ -17,52 +17,172 @@ import ( simpleTests "github.com/sourcenetwork/defradb/tests/integration/mutation/one_to_one" ) -// This test documents incorrect behaviour. It should be possible to update book to point to author. -// https://github.com/sourcenetwork/defradb/issues/1214 -func TestMutationUpdateOneToOneWrongSide(t *testing.T) { +// Note: This test should probably not pass, as it contains a +// reference to a document that doesnt exist. +func TestMutationUpdateOneToOneNoChild(t *testing.T) { test := testUtils.TestCase{ Description: "One to one create mutation, from the wrong side", Actions: []any{ testUtils.CreateDoc{ - CollectionID: 0, + CollectionID: 1, Doc: `{ - "name": "Theif Lord" + "name": "John" }`, }, testUtils.Request{ Request: `mutation { - update_book(data: "{\"name\": \"Painted House\",\"author_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { - _key + update_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { + name } }`, - ExpectedError: "The given field does not exist", + Results: []map[string]any{ + { + "name": "John Grisham", + }, + }, }, }, } simpleTests.ExecuteTestCase(t, test) } -// Note: This test should probably not pass, as it contains a -// reference to a document that doesnt exist. -func TestMutationUpdateOneToOneNoChild(t *testing.T) { +func TestMutationUpdateOneToOne(t *testing.T) { test := testUtils.TestCase{ - Description: "One to one create mutation, from the wrong side", + Description: "One to one update mutation", Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "Painted House" + }`, + }, testUtils.CreateDoc{ CollectionID: 1, Doc: `{ - "name": "John" + "name": "John Grisham" }`, }, testUtils.Request{ - Request: `mutation { - update_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"bae-fd541c25-229e-5280-b44b-e5c2af3e374d\"}") { + Request: ` + mutation { + update_author(data: "{\"name\": \"John Grisham\",\"published_id\": \"bae-3d236f89-6a31-5add-a36a-27971a2eac76\"}") { + name + } + }`, + Results: []map[string]any{ + { + "name": "John Grisham", + }, + }, + }, + testUtils.Request{ + Request: ` + query { + book { + name + author { name } - }`, + } + }`, + Results: []map[string]any{ + { + "name": "Painted House", + "author": map[string]any{ + "name": "John Grisham", + }, + }, + }, + }, + testUtils.Request{ + Request: ` + query { + author { + name + published { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "John Grisham", + "published": map[string]any{ + "name": "Painted House", + }, + }, + }, + }, + }, + } + + simpleTests.ExecuteTestCase(t, test) +} + +func TestMutationUpdateOneToOneSecondarySide(t *testing.T) { + test := testUtils.TestCase{ + Description: "One to one create mutation, from the secondary side", + Actions: []any{ + testUtils.CreateDoc{ + CollectionID: 0, + Doc: `{ + "name": "Painted House" + }`, + }, + testUtils.CreateDoc{ + CollectionID: 1, + Doc: `{ + "name": "John Grisham" + }`, + }, + testUtils.Request{ + Request: ` + mutation { + update_book(data: "{\"name\": \"Painted House\",\"author_id\": \"bae-2edb7fdd-cad7-5ad4-9c7d-6920245a96ed\"}") { + name + } + }`, + Results: []map[string]any{ + { + "name": "Painted House", + }, + }, + }, + testUtils.Request{ + Request: ` + query { + book { + name + author { + name + } + } + }`, + Results: []map[string]any{ + { + "name": "Painted House", + "author": map[string]any{ + "name": "John Grisham", + }, + }, + }, + }, + testUtils.Request{ + Request: ` + query { + author { + name + published { + name + } + } + }`, Results: []map[string]any{ { "name": "John Grisham", + "published": map[string]any{ + "name": "Painted House", + }, }, }, },