refactor: Merge collection UpdateWith and DeleteWith #2531
Conversation
| target any, | ||
| updater string, | ||
| ) (*UpdateResult, error) | ||
|
|
||
| // UpdateWithFilter updates using a filter to target documents for update. |
There was a problem hiding this comment.
suggestion: Just Update and Delete would be better names IMO, especially given that the filtering is optional - they might be calling UpdateWithFilter with no filter.
EDIT: Update and Delete were kept/already exist, ignore this :) Unless you want to unify them like GetCollections and GetSchemas do.
| @@ -72,7 +72,15 @@ func (n *updateNode) Next() (bool, error) { | |||
| if err != nil { | |||
| return false, err | |||
| } | |||
| _, err = n.collection.UpdateWithDocID(n.p.ctx, docID, string(patch)) | |||
| doc, err := n.collection.Get(n.p.ctx, docID, false) | |||
There was a problem hiding this comment.
todo: Why are you fetching the value here? You already have it n.currentValue = n.results.Value()
There was a problem hiding this comment.
I haven't worked with the planner much. Is there a way to create a client.Document from a core.Doc?
There was a problem hiding this comment.
I missed that the types here were different, but yes you can - easiest way would probably be:
docMap := n.currentValue.ToMap()
clientDoc := client.NewDocFromMap(docMap, colDef)
There might be something nicer somewhere, but I couldn't spot anything in a 30sec scan. I prefer the double-cast over fetching twice.
There was a problem hiding this comment.
Took a few tries but I think I found a good solution.
There was a problem hiding this comment.
@AndrewSisley It's not possible to update a document without fetching it first. Partial document updates break some parts of our indexing system.
It's also not possible to reconstruct the document from the planner node because it only selects the field values the user has requested be returned.
I've reverted back to fetching the document first and then updating with the patch fields.
There was a problem hiding this comment.
Partial document updates break some parts of our indexing system.
Thats not good, and a regression IMO. No worries though RE this PR, thanks for giving it a go :)
There was a problem hiding this comment.
@AndrewSisley That's exactly what UpdateWithDocID did so the pattern is the same. No regression in that sense.
There was a problem hiding this comment.
The regression was introduced with sec. indexes, I'm pretty sure that up until that point we could do partial updates via any public func, so long as the docID was provided.
nasdf
changed the title
cli/collection_update.go
Outdated
| res, err := col.UpdateWithFilter(cmd.Context(), filter, updater) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return writeJSON(cmd, res) | ||
| case len(argDocIDs) == 1 && len(args) == 1: | ||
| docID, err := client.NewDocIDFromString(argDocIDs[0]) | ||
| case len(argDocID) > 0 && len(args) == 1: |
There was a problem hiding this comment.
todo: The use of len(argDocID) > 0 is misleading here as it suggests that argDocID could be an array of docIDs but then used as a string bellow. Please change this to argDocID != "". Same thing in the collection_delete.go file :)
planner/update.go
Outdated
| } | ||
| patch, err := json.Marshal(n.input) | ||
| doc, err := client.NewDocFromMap(docMap, n.collection.Schema()) |
There was a problem hiding this comment.
todo: Now that the col vs schema PR is merged, this probably needs to change to:
doc, err := client.NewDocFromMap(docMap, n.collection)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2531 +/- ##
===========================================
+ Coverage 75.80% 76.20% +0.40%
===========================================
Files 292 292
Lines 28082 27755 -327
===========================================
- Hits 21285 21149 -136
+ Misses 5435 5248 -187
+ Partials 1362 1358 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Relevant issue(s)
Resolves #2457
Description
This PR merges the
DeleteWithandUpdateWithfunctions into a singularDeleteWithFilterandUpdateWithFilter.Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: