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: Correctly handle serialisation of nil field values #1872

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

AndrewSisley
Copy link
Contributor

@AndrewSisley AndrewSisley commented Sep 13, 2023

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.

Nil json values were being treated as delete operations.  Setting a value to nil is not the same as deleting it.
@AndrewSisley AndrewSisley added bug Something isn't working area/collections Related to the collections system action/no-benchmark Skips the action that runs the benchmark. labels Sep 13, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.7 milestone Sep 13, 2023
@AndrewSisley AndrewSisley requested a review from a team September 13, 2023 17:54
@AndrewSisley AndrewSisley self-assigned this Sep 13, 2023
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.04% ⚠️

Comparison is base (482be74) 70.44% compared to head (a32e689) 70.39%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1872      +/-   ##
===========================================
- Coverage    70.44%   70.39%   -0.04%     
===========================================
  Files          232      232              
  Lines        24184    24180       -4     
===========================================
- Hits         17034    17021      -13     
- Misses        5993     5999       +6     
- Partials      1157     1160       +3     
Flag Coverage Δ
all-tests 70.39% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
client/document.go 65.40% <100.00%> (-0.43%) ⬇️

... and 4 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 482be74...a32e689. Read the comment docs.

Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

question: I see most tests were removed and TestMutationUpdate_WithArrayOfBooleansToNil removed the restriction only, if this bug was introduced again, are the existing tests enough to start failing / detect this?

Comment on lines 138 to 139
//subdoc fields
// subDoc := doc.values[doc.fields["Address"]].Value().(*Document)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: suggest removal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't further tempt me to remove this file completely :)

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Just to confirm the following is what we would expect if we tested it right?

assert.Equal(t, doc.values[doc.fields["Address"]].IsDelete(), false)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@AndrewSisley
Copy link
Contributor Author

I see most tests were removed and TestMutationUpdate_WithArrayOfBooleansToNil removed the restriction only, if this bug was introduced again, are the existing tests enough to start failing / detect this?

Yes, the tests would fail if the bug was re-introduced. There tests that were removed were just variants of the tests who's restrictions were removed. The setup and act were the same, but the assert documented the bug.

@AndrewSisley AndrewSisley merged commit 3effe3a into sourcenetwork:develop Sep 14, 2023
15 checks passed
@AndrewSisley AndrewSisley deleted the 1842-nil-arrays branch September 14, 2023 12:25
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…k#1872)

## Relevant issue(s)

Resolves sourcenetwork#1842

## Description

Correctly handle serialisation of nil field values.

Nil values are not empty byte arrays in CBOR, they need to be handled
correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/collections Related to the collections system bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EOF error when reading field set to nil via collection.Save
2 participants