feat: Add support for one-one mutation from sec. side (#1247)

* Remove commented out code

* Remove commented out code

* Fix IsPrimaryRelation

Was inverted, and returned false for primary relations and true for secondaries.

* Remove doc.Clean from loop

There is no point doing this for each field, and it risks bugs by calling it so late within the loop where it may be skipped over by 'continue' controls

* Expand right-side create test to query both directions

Should make sure both work.

* Add test for update mutation from primary side

* Add support for update from sec. side

Note: This does not affect the collection.Update function, as that goes via `c.save`, and not `c.applyPatch`.  That behaviour will be changed with either mutation-create, or a different ticket for those calls. It might be nice to investigate unifying these at somepoint, but not now.

The two new functions isSecondaryIDField and patchPrimaryDoc will be used as-is when adding support for wrong-side save calls.  They are broken into two functions due to the differences in `c.save` and `c.applyPatch`.  It may be worth reviewing these two functions within the context of both commits.

* Add support for c.save-mutations from sec. side

This includes support for request create mutations, and collection.Save calls.

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.

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)
-// }

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

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)
+}

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)
+}