-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for one-one mutation from sec. side #1247
Changes from all commits
4b6c500
183ea83
7ac804a
ddc9c46
30682b4
fd989dc
3cf6f76
5d5bc69
8e0858f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: appreciate this comment |
||
// 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) | ||
// } |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
Comment on lines
+50
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Is there an issue for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is conceptually part of the problem that we dont really do much/any data validation on write, I'm not sure their is an existing issue There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #935 for write validation |
||
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I wonder why this was in the loop before? I feel like it would have been a redundant additions to the OnSuccess functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :P
I assume it was just a historical accident and replaced some logic that predated the
OnSuccess
funcs without moving itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just me being dumb 🙃