fix: Preserve filter type/int precision in doc updates - #5192
Merged
Conversation
ChrisBQu
changed the title
ChrisBQu
marked this pull request as ready for review
August 24, 2026 21:08
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (41)
|
| Layer / File(s) | Summary |
|---|---|
Shared filter decoding and normalization internal/utils/util.go |
Adds DecodeJSONFilter with integer precision preservation and NormalizeFilterForJSON for optional request filters. |
Go, C, and CLI filter paths cbindings/document_delete.go, cbindings/document_update.go, cbindings/wrapper_document.go, cli/document_delete.go, cli/document_update.go, tests/clients/cli/wrapper_document.go |
Update and delete paths use shared decoding and normalize absent or optional filters before processing or serialization. |
HTTP request decoding and transport http/handler_collection.go, http/client_document.go |
HTTP requests decode filters with precision preservation, treat missing or null filters as match-all filters, and normalize outgoing filters. |
JavaScript filter arguments js/utils.go, js/client_document.go, tests/clients/js/wrapper_document.go |
JavaScript update and delete operations accept filter strings, structured objects, and nullish values. |
Cross-client update and delete validation tests/integration/mutation/delete/with_filter_action_test.go, tests/integration/mutation/update/with_filter_action_test.go |
Adds coverage for map filters, optional filters, match-all behavior, and exact large-integer matching. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Merge Risk: ⚪ Minimal · up to 9791b
The PR updates filter serialization and decoding across clients to preserve filter structure and integer precision; no actionable merge-blocking risk remains beyond normal checks and review.
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
Investigate update and delete filter parsing across affected clients [#5191] |
✅ | |
Preserve optional filter semantics, including Some and None [#5191] |
✅ | |
Accept map filters and preserve exact large integers [#5191] |
✅ |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
4 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5192 +/- ##
========================================
Coverage 76.31% 76.31%
========================================
Files 645 645
Lines 51050 51122 +72
========================================
+ Hits 38956 39011 +55
- Misses 8827 8832 +5
- Partials 3267 3279 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
AndrewSisley
approved these changes
Aug 25, 2026
AndrewSisley
left a comment
AndrewSisley
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thanks Chris :)
| state.HTTPClientType, | ||
| state.CLIClientType, | ||
| state.CClientType, | ||
| state.JSClientType, |
Contributor
There was a problem hiding this comment.
question: This looks like all of our client types are listed here, why have you done this?
todo: Please document why, or remove it. Please apply this to the other similar tests below.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Relevant issue(s)
Resolves #5191
Description
Filters sent to
UpdateDocumentsWithFilter/DeleteDocumentsWithFilterthrough any client that round-trips them as JSONcould silently target the wrong documents. This included the C bindings, as well as the HTTP client, CLI, and JS clients.There were actually a number of issues going wrong here.
immutable.Some(request.Filter{...})was marshaled to{"Conditions": ...}, which decoded back into a filter with a literalConditionsfield instead of the intended conditions. Further, rawmap[string]anyfilters lost integer precision on decode (encoding/json decodes numbers as float64), so a condition on an integer above 2^53 could match a different document than intended. This was an issue that appeared, and was addressed in, several other recent PRs.This PR introduces new functions,
NormalizeFilterrForJSONandDecodeJSONFilterused on the encoding and decoding sides, respectively, to properly wire the values into the clients code.To attain parity across the clients as much as possible, the JS client's filter parameter now also accepts an object as well as a filter string. However, because JavaScript does not support large integers, we are limited on that particular part off the issue. See: #5176
This PR contains new integration tests to illustrate the behavior. This includes two on which JS client support is marked as a to-do.
Tasks
How has this been tested?
Specify the platform(s) on which this was tested: