support params.conditions in all putObject cases - #2691
Open
maeldonn wants to merge 5 commits into
Open
Conversation
Contributor
Hello maeldonn,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Contributor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/8.5 #2691 +/- ##
===================================================
+ Coverage 74.65% 74.70% +0.05%
===================================================
Files 227 227
Lines 18606 18644 +38
Branches 3850 3893 +43
===================================================
+ Hits 13890 13928 +38
Misses 4711 4711
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
maeldonn
force-pushed
the
improvement/ARSN-624
branch
2 times, most recently
from
August 31, 2026 13:09
ed67a15 to
17203c5
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
maeldonn
force-pushed
the
improvement/ARSN-624
branch
from
August 31, 2026 13:34
17203c5 to
7202dad
Compare
Factor the params.conditions translation used by putObjectVerCase3 into a shared buildConditionsFilter helper, so the other put cases can reuse it. Issue: ARSN-624
Apply the translated conditions to the master upsert filter of the suspended-versioning put. A failing condition turns the upsert into an insert of an existing key, which is reported as PreconditionFailed. Issue: ARSN-624
Apply the translated conditions to the master upsert filter of the non-versioned put, reporting a failing condition as PreconditionFailed. Issue: ARSN-624
Apply the translated conditions to the lookup filter of the oplog update path. Conditions are nested under $and so that a condition using $or does not collide with the deleted flag filter, and an unmatched condition is reported as PreconditionFailed instead of NoSuchKey. Issue: ARSN-624
Apply the translated conditions to the version upsert filter of the repairMaster put. A failing condition turns the upsert into an insert of an existing key, which is reported as PreconditionFailed. The master repair itself is left unconditional. Issue: ARSN-624
maeldonn
force-pushed
the
improvement/ARSN-624
branch
from
August 31, 2026 13:35
7202dad to
f5351d0
Compare
maeldonn
marked this pull request as ready for review
August 31, 2026 13:36
| objVal.versionId = versionId; | ||
| const masterKey = formatMasterKey(objName, params.vFormat); | ||
| c.updateOne({ _id: masterKey }, { $set: { value: objVal }, $setOnInsert: { _id: masterKey } }, { upsert: true }) | ||
| const filter = buildConditionsFilter('putObjectVerCase2', { _id: masterKey }, params.conditions, log); |
Contributor
There was a problem hiding this comment.
is this case (putObjectVerCase2) used for metadata updates as well, or just for creating new versions in the suspended-versioning case?
(conditional updates should only be used for, well, updates 😇... Not sure if I see an issue here, but I think it's better to double-think this, and make sure we have predictable outcome...)
| } | ||
| const key = formatMasterKey(objName, params.vFormat); | ||
| const putFilter = { _id: key }; | ||
| const putFilter = buildConditionsFilter('putObjectNoVer', { _id: key }, params?.conditions, log); |
Contributor
There was a problem hiding this comment.
same: can we distinguish object "replacement" from actual "metadata updates" ? do we need to, or can it be part of the "conditional update semantics" ?
Comment on lines
+1424
to
+1429
| const conditionsFilter = buildConditionsFilter('putObjectNoVerWithOplogUpdate', {}, params.conditions, log); | ||
| if (!conditionsFilter) { | ||
| return cb(errors.InternalError); | ||
| } | ||
| // conditions go under $and to leave the deleted flag $or above untouched | ||
| findFilter.$and = [conditionsFilter]; |
Contributor
There was a problem hiding this comment.
why not like the others:
Suggested change
| const conditionsFilter = buildConditionsFilter('putObjectNoVerWithOplogUpdate', {}, params.conditions, log); | |
| if (!conditionsFilter) { | |
| return cb(errors.InternalError); | |
| } | |
| // conditions go under $and to leave the deleted flag $or above untouched | |
| findFilter.$and = [conditionsFilter]; | |
| const findFilters = buildConditionsFilter('putObjectNoVerWithOplogUpdate', { | |
| ...putFilter, | |
| ...notDeletedFilter, | |
| }, params.conditions, log); | |
| if (!putFilter) { | |
| return cb(errors.InternalError); | |
| } |
| if (!doc?.value) { | ||
| if (hasConditions(params.conditions)) { | ||
| // a transiently deleted document is absent, not a condition mismatch | ||
| return collection.findOne({ ...putFilter, ...notDeletedFilter }).then(existing => { |
Contributor
There was a problem hiding this comment.
that looks fishy (if we can't find it, we can't).....and I think this is actually not correct : conditional updates is for updates (i.e. modifying metadata: transition, replication status...), which should never go through this code (which "builds" a fake oplog update entry, to simulate the ObjMD was deleted then created) ?
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
3 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.
Issue: ARSN-624