Fix #2570,#2572: Allow upserts to recognize $and clauses - #2571
Draft
toptobes wants to merge 7 commits into
Draft
Conversation
Contributor
📈 Unit Test Coverage Delta vs Main Branch
|
Contributor
Unit Test Coverage Report
|
Contributor
📉 Integration Test Coverage Delta vs Main Branch (dse69-it)
|
Contributor
Integration Test Coverage Report (dse69-it)
|
Contributor
📉 Integration Test Coverage Delta vs Main Branch (hcd-it)
|
Contributor
Integration Test Coverage Report (hcd-it)
|
There was a problem hiding this comment.
Pull request overview
Enables upserts to reconstruct documents from equality filters nested in $and clauses.
Changes:
- Recursively reconstructs upsert documents and validates overlapping paths.
- Moves reconstruction into
DocumentUpdater, with_id-only reconstruction for replacements. - Adds error handling and unit tests for reconstruction.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
DocumentUpdaterTest.java |
Updates calls for the new reconstructor parameter. |
FindCollectionOperationTest.java |
Tests reconstruction and path validation. |
errors.yaml |
Defines the overlapping-upsert-path error. |
DocumentUpdater.java |
Integrates operation-specific reconstruction. |
ReadAndUpdateCollectionOperation.java |
Delegates upsert reconstruction. |
FindCollectionOperation.java |
Traverses $and filters and validates paths. |
UpdateException.java |
Registers the new error code. |
UpdateOperation.java |
Changes the action accessor return type. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+460
to
+461
| public ReadDocument newEmptyDocument() { | ||
| return ReadDocument.from(null, null, objectMapper().createObjectNode()); |
Author
There was a problem hiding this comment.
This was a bug before my PR as well (as it wasn't guaranteed for an IDCollectionFilter to be present in getNewDocument(). My PR just amplifies the issue by never having the id set in the ReadDocument at all.
The issue is a deeper one which I'm not 100% sure how to solve
Author
There was a problem hiding this comment.
I think we may need to save writableShreddedDocument.id() and use that in readDocumentAgain()? I'm really not sure though
toptobes
changed the title
$and clauses$and clauses
…s behavior w/ findOneAndReplace
toptobes
force-pushed
the
KG-upserts-recognize-and
branch
from
August 25, 2026 22:41
be38117 to
373fd29
Compare
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
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.
What this PR does:
Updates Collection update operations to allow
$eqfilters nested inside of$andsto be included in the reconstructed document. See #2570 for a lot more information.$andnodes, and "validated" when being reconstructed, throwing a newUNSUPPORTED_OVERLAPPING_UPSERT_PATHSerror when contradictory paths are given, similar to Mongo's implementation.This PR also addresses the TODO to have document reconstruction be inside of
documentUpdaterrather than split into two places._id. This preventsFindOneAndReplaceoperations from unnecessarily/wrongly throwing errors when contradictory filters exist outside the_idfield.Finally, this PR fixes #2572 which wrongly allows certain non-
$eqfields to be included in the reconstructed document.P.S. I left a comment for a minor optimization which could address one of aaron's comments + would remove an extra deepCopy in replace operations. Lmk if you just want me to do it in this PR, it's like a 4 line change
it should work, I just need to finish testing
Which issue(s) this PR fixes:
Fixes #2570 and #2572
Checklist