fix: Handle multiple nil values on unique indexed fields #2178
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2178 +/- ##
===========================================
+ Coverage 74.13% 74.16% +0.02%
===========================================
Files 256 256
Lines 25368 25372 +4
===========================================
+ Hits 18806 18815 +9
+ Misses 5279 5276 -3
+ Partials 1283 1281 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
islamaliev
changed the title
There was a problem hiding this comment.
This does feel like the wrong approach. I think users should still be allowed nil values in indexed fields, especially considering that the GQL types are nillable (Int vs Int!).
The SQL standards for this allow multiple nil values in a column with a unique constraint. Microsoft SQL Server naturally ignores this, and only permits a single nil value.
https://stackoverflow.com/questions/767657/how-do-i-create-a-unique-constraint-that-also-allows-nulls (check top answer).
|
added a task for multiple |
| testUtils.ExecuteTestCase(t, test) | ||
| } | ||
|
|
||
| func TestUniqueIndexCreate_UponAddingDocWithExistingNilValue_ReturnError(t *testing.T) { |
There was a problem hiding this comment.
todo: Can you please add a test that shows a single document with a nil indexed value succeeding? This way if this (multiple nil values) test is deleted (e.g. when switching to allowing multiple nils) we do not lose coverage of the single nil happy-case.
todo: Can you please add a test with a filter eq nil?
todo: Can you please add some tests for composite indexes with all-nil and mixed nil/non-nil values?
There was a problem hiding this comment.
test for a single nil value was always there
defradb/db/indexed_docs_test.go
Line 1031 in d2cfd87
There was a problem hiding this comment.
Could you please add it as an integration test please? It will make it much easier to discover, and it will be able to take advantage of all our integration test related perks (such as mutation and client types, change detection, etc)
There was a problem hiding this comment.
the current test already includes a successful addition of a document with a nil value:
testUtils.CreateDoc{
CollectionID: 0,
Doc: `
{
"name": "Keenan"
}`,
},
testUtils.CreateDoc{
CollectionID: 0,
Doc: `
{
"name": "Andy"
}`,
ExpectedError: db.NewErrCanNotIndexNonUniqueField("bae-2159860f-3cd1-59de-9440-71331e77cbb8", "age", nil).Error(),
},Adding another one would be redundant.
I will add a test with filtering on nil value.
todo: Can you please add some tests for composite indexes with all-nil and mixed nil/non-nil values?
composite indexes is WIP on another branch.
There was a problem hiding this comment.
the current test already includes a successful addition of a document with a nil value:
Yes I saw that, but an independent successful test would be useful. If this (multiple nil values) test is deleted (e.g. when switching to allowing multiple nils) we do not lose coverage of the single nil happy-case.
There was a problem hiding this comment.
added an explicit test
islamaliev
force-pushed
the
fix/unique-index-on-nil-field
branch
from
91fa97d
to
a21a95c
Compare
islamaliev
changed the title
…rk#2178) ## Relevant issue(s) Resolves sourcenetwork#2174 sourcenetwork#2175 ## Description This change fixes how nil values are handled on unique indexes
…rk#2178) ## Relevant issue(s) Resolves sourcenetwork#2174 sourcenetwork#2175 ## Description This change fixes how nil values are handled on unique indexes
Relevant issue(s)
Resolves #2174 #2175
Description
Forbid
nilvalues for fields that are unique-indexed.Tasks
How has this been tested?
Integration and unit tests
Specify the platform(s) on which this was tested: