fix: Allow update when updating non-indexed field #2511
Conversation
fredcarle
force-pushed
the
fredcarle/fix/i2510-doc-update-with-unique-index
branch
2 times, most recently
from
1cf6d64
to
d3012ec
Compare
|
@islamaliev I've move the check like we discussed earlier. Let me know what you think. |
| testUtils "github.com/sourcenetwork/defradb/tests/integration" | ||
| ) | ||
|
|
||
| func TestUniqueCompositeIndexUpdate_UponUpdatingDocWithExistingFieldValue_ShouldSucceed(t *testing.T) { |
There was a problem hiding this comment.
praise: thanks for adding a test for the composite one.
fredcarle
force-pushed
the
fredcarle/fix/i2510-doc-update-with-unique-index
branch
2 times, most recently
from
ce9e34a
to
74a24bc
Compare
fredcarle
force-pushed
the
fredcarle/fix/i2510-doc-update-with-unique-index
branch
from
74a24bc
to
0e2eb12
Compare
db/collection_index.go
Outdated
| return err | ||
| // We only need to update the index if one of the indexed fields | ||
| // on the document has been changed. | ||
| if isUpdatingIndexedFields(index, oldDoc, doc) { |
There was a problem hiding this comment.
question(cc: @islamaliev): Should this be done inside the index.Update func, instead of here? This will A) allow different index types to handle this differently (if needed) B) Gurantees this behavior is consistent regardless of where index.Update is called from (vs doing it here which is only fixed for this particular call path).
There was a problem hiding this comment.
I also thought about this, but though it would be unlikely to have any other update behaviour.
Although, thinking about it again we don't know what will be in the future especially when we decide to index array or json fields or some other fancy stuff.
So having this encapsulated inside index itself would be more future-prove.
fredcarle
force-pushed
the
fredcarle/fix/i2510-doc-update-with-unique-index
branch
from
ffed1d0
to
7ff9422
Compare
fredcarle
force-pushed
the
fredcarle/fix/i2510-doc-update-with-unique-index
branch
from
7ff9422
to
1c3ebd7
Compare
| @@ -381,6 +381,11 @@ func (index *collectionUniqueIndex) Update( | |||
| oldDoc *client.Document, | |||
| newDoc *client.Document, | |||
| ) error { | |||
| // We only need to update the index if one of the indexed fields | |||
| // on the document has been changed. | |||
| if !isUpdatingIndexedFields(index, oldDoc, newDoc) { | |||
There was a problem hiding this comment.
suggestion: I think it also makes sense to do this check for regular indexes otherwise they will do unnecessary work with storage.
There was a problem hiding this comment.
I'll open a new PR to cover that case 👍
Relevant issue(s)
Resolves #2510
Description
This PR adds a check to the index validation step where if the resulting KV pair is the same on update as the previous KV pair, the outcome will be an no-op instead of an index existing error.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: