Skip to content
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

fix: Treat explicitly set nil values like omitted values #3101

Merged
merged 4 commits into from
Oct 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@
ccid "github.com/sourcenetwork/defradb/internal/core/cid"
)

func init() {
enc, err := CborEncodingOptions().EncMode()
if err != nil {
panic(err)

Check warning on line 33 in client/document.go

View check run for this annotation

Codecov / codecov/patch

client/document.go#L33

Added line #L33 was not covered by tests
}

CborNil, err = enc.Marshal(nil)
if err != nil {
panic(err)

Check warning on line 38 in client/document.go

View check run for this annotation

Codecov / codecov/patch

client/document.go#L38

Added line #L38 was not covered by tests
}
}

// CborEncodingOptions returns the set of cbor encoding options to be used whenever
// encoding defra documents.
//
Expand All @@ -42,6 +54,10 @@
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 @@

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

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

Check warning on line 802 in client/document.go

View check run for this annotation

Codecov / codecov/patch

client/document.go#L802

Added line #L802 was not covered by tests
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 @@

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 @@
}
}

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

Check warning on line 155 in internal/core/crdt/lwwreg.go

View check run for this annotation

Codecov / codecov/patch

internal/core/crdt/lwwreg.go#L155

Added line #L155 was not covered by tests
}
} else {
err = reg.store.Put(ctx, key.ToDS(), val)
if err != nil {
return NewErrFailedToStoreValue(err)

Check warning on line 160 in internal/core/crdt/lwwreg.go

View check run for this annotation

Codecov / codecov/patch

internal/core/crdt/lwwreg.go#L160

Added line #L160 was not covered by tests
}
}

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