From 01030fd558e46b2ca9f18472dd91778fc0f278e7 Mon Sep 17 00:00:00 2001 From: AndrewSisley Date: Fri, 4 Oct 2024 12:50:40 -0400 Subject: [PATCH] fix: Treat explicitly set nil values like omitted values (#3101) ## Relevant issue(s) Resolves #3100 ## Description Treat explicitly set nil values like omitted values. --- client/document.go | 41 ++++- internal/core/crdt/lwwreg.go | 18 ++- internal/db/collection.go | 5 + tests/integration/events.go | 6 + .../one_to_one/with_null_value_test.go | 149 ++++++++++++++++++ .../mutation/create/with_null_value_test.go | 54 +++++++ 6 files changed, 263 insertions(+), 10 deletions(-) create mode 100644 tests/integration/mutation/create/field_kinds/one_to_one/with_null_value_test.go create mode 100644 tests/integration/mutation/create/with_null_value_test.go diff --git a/client/document.go b/client/document.go index fa4c842343..a49a60307b 100644 --- a/client/document.go +++ b/client/document.go @@ -27,6 +27,18 @@ import ( ccid "github.com/sourcenetwork/defradb/internal/core/cid" ) +func init() { + enc, err := CborEncodingOptions().EncMode() + if err != nil { + panic(err) + } + + CborNil, err = enc.Marshal(nil) + if err != nil { + panic(err) + } +} + // CborEncodingOptions returns the set of cbor encoding options to be used whenever // encoding defra documents. // @@ -42,6 +54,10 @@ func CborEncodingOptions() cbor.EncOptions { return opts } +// CborNil is the cbor encoded value of `nil` using the options returned from +// [CborEncodingOptions()] +var CborNil []byte + // This is the main implementation starting point for accessing the internal Document API // which provides API access to the various operations available for Documents, i.e. CRUD. // @@ -697,7 +713,12 @@ func (doc *Document) Values() map[Field]*FieldValue { // Bytes returns the document as a serialzed byte array using CBOR encoding. func (doc *Document) Bytes() ([]byte, error) { - docMap, err := doc.toMap() + // We want to ommit properties with nil values from the map, as setting a + // propery to nil should result in the same serialized value as ommiting the + // the property from the document. + // + // This is particularly important for docID generation. + docMap, err := doc.toMap(true) if err != nil { return nil, err } @@ -713,7 +734,7 @@ func (doc *Document) Bytes() ([]byte, error) { // Note: This representation should not be used for any cryptographic operations, // such as signatures, or hashes as it does not guarantee canonical representation or ordering. func (doc *Document) String() (string, error) { - docMap, err := doc.toMap() + docMap, err := doc.toMap(false) if err != nil { return "", err } @@ -734,7 +755,7 @@ func (doc *Document) ToMap() (map[string]any, error) { // ToJSONPatch returns a json patch that can be used to update // a document by calling SetWithJSON. func (doc *Document) ToJSONPatch() ([]byte, error) { - docMap, err := doc.toMap() + docMap, err := doc.toMap(false) if err != nil { return nil, err } @@ -758,9 +779,11 @@ func (doc *Document) Clean() { } } -// converts the document into a map[string]any -// including any sub documents -func (doc *Document) toMap() (map[string]any, error) { +// converts the document into a map[string]any including any sub documents. +// +// If `true` is provided, properties with nil values will be ommited from +// the result. +func (doc *Document) toMap(excludeEmpty bool) (map[string]any, error) { doc.mu.RLock() defer doc.mu.RUnlock() docMap := make(map[string]any) @@ -770,9 +793,13 @@ func (doc *Document) toMap() (map[string]any, error) { return nil, NewErrFieldNotExist(v.Name()) } + if excludeEmpty && value.Value() == nil { + continue + } + if value.IsDocument() { subDoc := value.Value().(*Document) - subDocMap, err := subDoc.toMap() + subDocMap, err := subDoc.toMap(excludeEmpty) if err != nil { return nil, err } diff --git a/internal/core/crdt/lwwreg.go b/internal/core/crdt/lwwreg.go index edfff9ca05..e27a4c5ace 100644 --- a/internal/core/crdt/lwwreg.go +++ b/internal/core/crdt/lwwreg.go @@ -16,6 +16,7 @@ import ( ds "github.com/ipfs/go-datastore" + "github.com/sourcenetwork/defradb/client" "github.com/sourcenetwork/defradb/datastore" "github.com/sourcenetwork/defradb/errors" "github.com/sourcenetwork/defradb/internal/core" @@ -144,9 +145,20 @@ func (reg LWWRegister) setValue(ctx context.Context, val []byte, priority uint64 } } - err = reg.store.Put(ctx, key.ToDS(), val) - if err != nil { - return NewErrFailedToStoreValue(err) + if bytes.Equal(val, client.CborNil) { + // If len(val) is 1 or less the property is nil and there is no reason for + // the field datastore key to exist. Ommiting the key saves space and is + // consistent with what would be found if the user omitted the property on + // create. + err = reg.store.Delete(ctx, key.ToDS()) + if err != nil { + return err + } + } else { + err = reg.store.Put(ctx, key.ToDS(), val) + if err != nil { + return NewErrFailedToStoreValue(err) + } } return reg.setPriority(ctx, reg.key, priority) diff --git a/internal/db/collection.go b/internal/db/collection.go index dd1a413946..785c96641e 100644 --- a/internal/db/collection.go +++ b/internal/db/collection.go @@ -657,6 +657,11 @@ func (c *collection) save( relationFieldDescription, isSecondaryRelationID := fieldDescription.GetSecondaryRelationField(c.Definition()) if isSecondaryRelationID { + if val.Value() == nil { + // If the value (relation) is nil, we don't need to check for any documents already linked to it + continue + } + primaryId := val.Value().(string) err = c.patchPrimaryDoc( diff --git a/tests/integration/events.go b/tests/integration/events.go index bf004b99aa..5e57a97294 100644 --- a/tests/integration/events.go +++ b/tests/integration/events.go @@ -335,6 +335,12 @@ func getEventsForCreateDoc(s *state, action CreateDoc) map[string]struct{} { // check for any secondary relation fields that could publish an event for f, v := range doc.Values() { + if v.Value() == nil { + // If the new relation value is nil there will be no related document + // to get an event for + continue + } + field, ok := def.GetFieldByName(f.Name()) if !ok { continue // ignore unknown field diff --git a/tests/integration/mutation/create/field_kinds/one_to_one/with_null_value_test.go b/tests/integration/mutation/create/field_kinds/one_to_one/with_null_value_test.go new file mode 100644 index 0000000000..a6421aec5c --- /dev/null +++ b/tests/integration/mutation/create/field_kinds/one_to_one/with_null_value_test.go @@ -0,0 +1,149 @@ +// Copyright 2024 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package one_to_one + +import ( + "testing" + + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +func TestMutationCreateOneToOne_WithExplicitNullOnPrimarySide(t *testing.T) { + test := testUtils.TestCase{ + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type Book { + name: String + author: Author @primary + } + + type Author { + name: String + published: Book + } + `, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "How to Be a Canadian", + "author": null + }`, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "Secrets at Maple Syrup Farm", + "author": null + }`, + }, + testUtils.CreateDoc{ + CollectionID: 1, + DocMap: map[string]any{ + "name": "Will Ferguson", + "published": testUtils.NewDocIndex(0, 0), + }, + }, + testUtils.Request{ + Request: ` + query { + Book { + name + author { + name + } + } + }`, + Results: map[string]any{ + "Book": []map[string]any{ + { + "name": "Secrets at Maple Syrup Farm", + "author": nil, + }, + { + "name": "How to Be a Canadian", + "author": map[string]any{ + "name": "Will Ferguson", + }, + }, + }, + }, + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +} + +func TestMutationCreateOneToOne_WithExplicitNullOnSecondarySide(t *testing.T) { + test := testUtils.TestCase{ + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type Book { + name: String + author: Author + } + + type Author { + name: String + published: Book @primary + } + `, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "How to Be a Canadian", + "author": null + }`, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "Secrets at Maple Syrup Farm", + "author": null + }`, + }, + testUtils.CreateDoc{ + CollectionID: 1, + DocMap: map[string]any{ + "name": "Will Ferguson", + "published": testUtils.NewDocIndex(0, 0), + }, + }, + testUtils.Request{ + Request: ` + query { + Book { + name + author { + name + } + } + }`, + Results: map[string]any{ + "Book": []map[string]any{ + { + "name": "Secrets at Maple Syrup Farm", + "author": nil, + }, + { + "name": "How to Be a Canadian", + "author": map[string]any{ + "name": "Will Ferguson", + }, + }, + }, + }, + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +} diff --git a/tests/integration/mutation/create/with_null_value_test.go b/tests/integration/mutation/create/with_null_value_test.go new file mode 100644 index 0000000000..97e1d6cb58 --- /dev/null +++ b/tests/integration/mutation/create/with_null_value_test.go @@ -0,0 +1,54 @@ +// Copyright 2024 Democratized Data Foundation +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package create + +import ( + "testing" + + "github.com/sourcenetwork/immutable" + + testUtils "github.com/sourcenetwork/defradb/tests/integration" +) + +func TestMutationCreate_WithOmittedValueAndExplicitNullValue(t *testing.T) { + test := testUtils.TestCase{ + SupportedMutationTypes: immutable.Some([]testUtils.MutationType{ + // Collection.Save would treat the second create as an update, and so + // is excluded from this test. + testUtils.CollectionNamedMutationType, + testUtils.GQLRequestMutationType, + }), + Actions: []any{ + testUtils.SchemaUpdate{ + Schema: ` + type Users { + name: String + age: Int + } + `, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "John" + }`, + }, + testUtils.CreateDoc{ + Doc: `{ + "name": "John", + "age": null + }`, + ExpectedError: "a document with the given ID already exist", + }, + }, + } + + testUtils.ExecuteTestCase(t, test) +}