Skip to content

Commit

Permalink
fix: Treat explicitly set nil values like omitted values (#3101)
Browse files Browse the repository at this point in the history
## Relevant issue(s)

Resolves #3100

## Description

Treat explicitly set nil values like omitted values.
  • Loading branch information
AndrewSisley authored Oct 4, 2024
1 parent 4e5470c commit 01030fd
Show file tree
Hide file tree
Showing 6 changed files with 263 additions and 10 deletions.
41 changes: 34 additions & 7 deletions client/document.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand All @@ -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.
//
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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)
Expand All @@ -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
}
Expand Down
18 changes: 15 additions & 3 deletions internal/core/crdt/lwwreg.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions internal/db/collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 6 additions & 0 deletions tests/integration/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
54 changes: 54 additions & 0 deletions tests/integration/mutation/create/with_null_value_test.go
Original file line number Diff line number Diff line change
@@ -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)
}

0 comments on commit 01030fd

Please sign in to comment.