fix: Correctly handle serialisation of nil field values (#1872)

## Relevant issue(s) Resolves #1842 ## Description Correctly handle serialisation of nil field values. Nil values are not empty byte arrays in CBOR, they need to be handled correctly.
sourcenetwork · Sep 14, 2023 · 3effe3a · 3effe3a
1 parent 482be74
commit 3effe3a
Show file tree

Hide file tree

Showing 6 changed files with 4 additions and 215 deletions.
diff --git a/client/document.go b/client/document.go
@@ -209,11 +209,7 @@ func (doc *Document) SetWithJSON(patch []byte) error {
 	}
 
 	for k, v := range patchObj {
-		if v == nil {
-			err = doc.Delete(k)
-		} else {
-			err = doc.Set(k, v)
-		}
+		err = doc.Set(k, v)
 		if err != nil {
 			return err
 		}
@@ -273,7 +269,7 @@ func (doc *Document) setObject(t CType, field string, val *Document) error {
 // @todo: Update with document schemas
 func (doc *Document) setAndParseType(field string, value any) error {
 	if value == nil {
-		return nil
+		return doc.setCBOR(LWW_REGISTER, field, value)
 	}
 
 	switch val := value.(type) {

diff --git a/client/document_test.go b/client/document_test.go
@@ -132,7 +132,8 @@ func TestSetWithJSON(t *testing.T) {
 	assert.Equal(t, doc.values[doc.fields["Name"]].IsDocument(), false)
 	assert.Equal(t, doc.values[doc.fields["Age"]].Value(), int64(27))
 	assert.Equal(t, doc.values[doc.fields["Age"]].IsDocument(), false)
-	assert.Equal(t, doc.values[doc.fields["Address"]].IsDelete(), true)
+	assert.Equal(t, doc.values[doc.fields["Address"]].Value(), nil)
+	assert.Equal(t, doc.values[doc.fields["Address"]].IsDocument(), false)
 
 	//subdoc fields
 	// subDoc := doc.values[doc.fields["Address"]].Value().(*Document)

diff --git a/tests/integration/mutation/update/field_kinds/array_bool_test.go b/tests/integration/mutation/update/field_kinds/array_bool_test.go
@@ -13,19 +13,12 @@ package field_kinds
 import (
 	"testing"
 
-	"github.com/sourcenetwork/immutable"
-
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
 func TestMutationUpdate_WithArrayOfBooleansToNil(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with boolean array, replace with nil",
-		// This restriction should be removed when we can, it is here because of
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.GQLRequestMutationType,
-		}),
 		Actions: []any{
 			testUtils.SchemaUpdate{
 				Schema: `
@@ -66,51 +59,6 @@ func TestMutationUpdate_WithArrayOfBooleansToNil(t *testing.T) {
 	testUtils.ExecuteTestCase(t, test)
 }
 
-func TestMutationUpdate_WithArrayOfBooleansToNil_Errors(t *testing.T) {
-	test := testUtils.TestCase{
-		Description: "Simple update mutation with boolean array, replace with nil",
-		// This is a bug, this test should be removed in
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.CollectionNamedMutationType,
-			testUtils.CollectionSaveMutationType,
-		}),
-		Actions: []any{
-			testUtils.SchemaUpdate{
-				Schema: `
-					type Users {
-						name: String
-						likedIndexes: [Boolean!]
-					}
-				`,
-			},
-			testUtils.CreateDoc{
-				Doc: `{
-					"name": "John",
-					"likedIndexes": [true, true, false, true]
-				}`,
-			},
-			testUtils.UpdateDoc{
-				Doc: `{
-					"likedIndexes": null
-				}`,
-			},
-			testUtils.Request{
-				Request: `
-					query {
-						Users {
-							likedIndexes
-						}
-					}
-				`,
-				ExpectedError: "EOF",
-			},
-		},
-	}
-
-	testUtils.ExecuteTestCase(t, test)
-}
-
 func TestMutationUpdate_WithArrayOfBooleansToEmpty(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with boolean array, replace with empty",

diff --git a/tests/integration/mutation/update/field_kinds/array_float_test.go b/tests/integration/mutation/update/field_kinds/array_float_test.go
@@ -13,19 +13,12 @@ package field_kinds
 import (
 	"testing"
 
-	"github.com/sourcenetwork/immutable"
-
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
 func TestMutationUpdate_WithArrayOfFloatsToNil(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with float array, replace with nil",
-		// This restriction should be removed when we can, it is here because of
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.GQLRequestMutationType,
-		}),
 		Actions: []any{
 			testUtils.SchemaUpdate{
 				Schema: `
@@ -66,51 +59,6 @@ func TestMutationUpdate_WithArrayOfFloatsToNil(t *testing.T) {
 	testUtils.ExecuteTestCase(t, test)
 }
 
-func TestMutationUpdate_WithArrayOfFloatsToNil_Errors(t *testing.T) {
-	test := testUtils.TestCase{
-		Description: "Simple update mutation with float array, replace with nil",
-		// This is a bug, this test should be removed in
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.CollectionNamedMutationType,
-			testUtils.CollectionSaveMutationType,
-		}),
-		Actions: []any{
-			testUtils.SchemaUpdate{
-				Schema: `
-					type Users {
-						name: String
-						favouriteFloats: [Float!]
-					}
-				`,
-			},
-			testUtils.CreateDoc{
-				Doc: `{
-					"name": "John",
-					"favouriteFloats": [3.1425, 0.00000000001, 10]
-				}`,
-			},
-			testUtils.UpdateDoc{
-				Doc: `{
-					"favouriteFloats": null
-				}`,
-			},
-			testUtils.Request{
-				Request: `
-					query {
-						Users {
-							favouriteFloats
-						}
-					}
-				`,
-				ExpectedError: "EOF",
-			},
-		},
-	}
-
-	testUtils.ExecuteTestCase(t, test)
-}
-
 func TestMutationUpdate_WithArrayOfFloatsToEmpty(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with float array, replace with empty",

diff --git a/tests/integration/mutation/update/field_kinds/array_int_test.go b/tests/integration/mutation/update/field_kinds/array_int_test.go
@@ -13,19 +13,12 @@ package field_kinds
 import (
 	"testing"
 
-	"github.com/sourcenetwork/immutable"
-
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
 func TestMutationUpdate_WithArrayOfIntsToNil(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with integer array, replace with nil",
-		// This restriction should be removed when we can, it is here because of
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.GQLRequestMutationType,
-		}),
 		Actions: []any{
 			testUtils.SchemaUpdate{
 				Schema: `
@@ -66,51 +59,6 @@ func TestMutationUpdate_WithArrayOfIntsToNil(t *testing.T) {
 	testUtils.ExecuteTestCase(t, test)
 }
 
-func TestMutationUpdate_WithArrayOfIntsToNil_Errors(t *testing.T) {
-	test := testUtils.TestCase{
-		Description: "Simple update mutation with integer array, replace with nil",
-		// This is a bug, this test should be removed in
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.CollectionNamedMutationType,
-			testUtils.CollectionSaveMutationType,
-		}),
-		Actions: []any{
-			testUtils.SchemaUpdate{
-				Schema: `
-					type Users {
-						name: String
-						favouriteIntegers: [Int!]
-					}
-				`,
-			},
-			testUtils.CreateDoc{
-				Doc: `{
-					"name": "John",
-					"favouriteIntegers": [1, 2, 3, 5, 8]
-				}`,
-			},
-			testUtils.UpdateDoc{
-				Doc: `{
-					"favouriteIntegers": null
-				}`,
-			},
-			testUtils.Request{
-				Request: `
-					query {
-						Users {
-							favouriteIntegers
-						}
-					}
-				`,
-				ExpectedError: "EOF",
-			},
-		},
-	}
-
-	testUtils.ExecuteTestCase(t, test)
-}
-
 func TestMutationUpdate_WithArrayOfIntsToEmpty(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with integer array, replace with empty",

diff --git a/tests/integration/mutation/update/field_kinds/array_string_test.go b/tests/integration/mutation/update/field_kinds/array_string_test.go
@@ -13,19 +13,12 @@ package field_kinds
 import (
 	"testing"
 
-	"github.com/sourcenetwork/immutable"
-
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
 func TestMutationUpdate_WithArrayOfStringsToNil(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with string array, replace with nil",
-		// This restriction should be removed when we can, it is here because of
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.GQLRequestMutationType,
-		}),
 		Actions: []any{
 			testUtils.SchemaUpdate{
 				Schema: `
@@ -66,51 +59,6 @@ func TestMutationUpdate_WithArrayOfStringsToNil(t *testing.T) {
 	testUtils.ExecuteTestCase(t, test)
 }
 
-func TestMutationUpdate_WithArrayOfStringsToNil_Errors(t *testing.T) {
-	test := testUtils.TestCase{
-		Description: "Simple update mutation with string array, replace with nil",
-		// This is a bug, this test should be removed in
-		// https://github.com/sourcenetwork/defradb/issues/1842
-		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
-			testUtils.CollectionNamedMutationType,
-			testUtils.CollectionSaveMutationType,
-		}),
-		Actions: []any{
-			testUtils.SchemaUpdate{
-				Schema: `
-					type Users {
-						name: String
-						preferredStrings: [String!]
-					}
-				`,
-			},
-			testUtils.CreateDoc{
-				Doc: `{
-					"name": "John",
-					"preferredStrings": ["", "the previous", "the first", "empty string"]
-				}`,
-			},
-			testUtils.UpdateDoc{
-				Doc: `{
-					"preferredStrings": null
-				}`,
-			},
-			testUtils.Request{
-				Request: `
-					query {
-						Users {
-							preferredStrings
-						}
-					}
-				`,
-				ExpectedError: "EOF",
-			},
-		},
-	}
-
-	testUtils.ExecuteTestCase(t, test)
-}
-
 func TestMutationUpdate_WithArrayOfStringsToEmpty(t *testing.T) {
 	test := testUtils.TestCase{
 		Description: "Simple update mutation with string array, replace with empty",