fix(update): preserve $setOnInsert.createdAt with overwriteImmutable: true#16278
Open
AbdelrahmanHafez wants to merge 5 commits into
Open
fix(update): preserve $setOnInsert.createdAt with overwriteImmutable: true#16278AbdelrahmanHafez wants to merge 5 commits into
AbdelrahmanHafez wants to merge 5 commits into
Conversation
The negative-case test in `describe('bulkWrite overwriteImmutable option
(gh-15781)')` placed `timestamps` as a sibling of `updateOne`/`updateMany`
instead of inside the op descriptor. Per-op `timestamps` is only honored
when nested inside the op (see `castBulkWrite.getTimestampsOpt`), so all
four loop iterations actually exercised the default path.
Moving `timestamps` inside the op descriptor surfaced that with
`timestamps: false`, the update became empty after immutable-field
stripping (`ssn` and the timestamps-plugin `createdAt` are both
immutable), causing `MongoInvalidArgumentError: Update document requires
atomic operators` from the driver. Added a mutable `name` field to the
update and asserted it changes, so all four iterations exercise genuine
immutable protection without producing an empty update.
Also remove a byte-identical duplicate of the `gh-15781` describe block.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a test regression around bulkWrite() per-operation timestamps handling (gh-15781) by ensuring the test inputs match how Mongoose reads timestamps for each bulk op.
Changes:
- Move per-op
timestampsinto theupdateOne/updateManyoperation descriptors so the test exercisescastBulkWrite.getTimestampsOptcorrectly. - Add a mutable field (
name) to the negative-case update payload to avoid the update becoming empty after immutable-path stripping. - Remove a duplicated
describeblock to avoid redundant coverage.
…n bulkWrite gh-15781 tests
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
test/model.updateOne.test.js:2849
- The
// Arrangecomment is mis-indented (it’s aligned with theit(line instead of the function body), which violates the repo’sindentESLint rule and may fail lint CI. Indent it to match the surrounding block (same level as theconst { User } ...line).
it(`can not update immutable fields without overwriteImmutable: true and timestamps: ${timestamps}`, async function() {
// Arrange
const { User } = createTestContext();
…erwriteImmutable: true`
`applyTimestampsToUpdate` only checked top-level and `$set` for a user-provided
`createdAt`, so a bulkWrite upsert with `$setOnInsert: { createdAt }` and
`overwriteImmutable: true` would silently overwrite the user's value with `now`.
AbdelrahmanHafez
changed the title
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.
applyTimestampsToUpdateonly checked top-level and$setfor a user-providedcreatedAt, so bulkWrite upserts with$setOnInsert: { createdAt }andoverwriteImmutable: truesilently overwrote the value withnow. Including$setOnInsertin the check preserves it.Also cleans up #15781 tests added in #15819:
timestampswas placed outside the op descriptor in the loop tests, so all four[true, false, null, undefined]iterations ran with the default. Moved inside.updatedAtactually varies withtimestamps. Previously assertions were invariant, so a regression would still pass.