From 71e0779d901883bd6b02663757217020947fc9f3 Mon Sep 17 00:00:00 2001 From: Andrew Sisley Date: Fri, 4 Oct 2024 10:28:54 -0400 Subject: [PATCH] Treat explicit nil as implicit nil Field values should not behave differently depending on whether they were ommited from the initial create call. --- client/document.go | 25 ++++++++++----- internal/core/crdt/lwwreg.go | 17 ++++++++-- .../one_to_one/with_null_value_test.go | 26 +++++++++++++++- .../mutation/create/with_null_value_test.go | 31 ++++++------------- 4 files changed, 66 insertions(+), 33 deletions(-) diff --git a/client/document.go b/client/document.go index fa4c842343..9a969d46ed 100644 --- a/client/document.go +++ b/client/document.go @@ -697,7 +697,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 +718,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 +739,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 +763,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 +777,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..e6bdec0061 100644 --- a/internal/core/crdt/lwwreg.go +++ b/internal/core/crdt/lwwreg.go @@ -144,9 +144,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 len(val) <= 1 { + // 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/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 index 3b3df22acf..a428f76681 100644 --- 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 @@ -52,7 +52,31 @@ func TestMutationCreateOneToOne_WithExplicitNullOnPrimarySide(t *testing.T) { "name": "Will Ferguson", "published": testUtils.NewDocIndex(0, 0), }, - ExpectedError: "target document is already linked to another document", + }, + 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", + }, + }, + }, + }, }, }, } diff --git a/tests/integration/mutation/create/with_null_value_test.go b/tests/integration/mutation/create/with_null_value_test.go index ecdf28081b..97e1d6cb58 100644 --- a/tests/integration/mutation/create/with_null_value_test.go +++ b/tests/integration/mutation/create/with_null_value_test.go @@ -13,11 +13,19 @@ 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: ` @@ -37,28 +45,7 @@ func TestMutationCreate_WithOmittedValueAndExplicitNullValue(t *testing.T) { "name": "John", "age": null }`, - }, - testUtils.Request{ - Request: ` - query { - Users { - name - age - } - } - `, - Results: map[string]any{ - "Users": []map[string]any{ - { - "name": "John", - "age": nil, - }, - { - "name": "John", - "age": nil, - }, - }, - }, + ExpectedError: "a document with the given ID already exist", }, }, }