Skip to content

fix(documentarray): reindex subdocs after reordering - #16282

Merged
vkarpov15 merged 18 commits into
Automattic:masterfrom
AbdelrahmanHafez:fix/document-array-reindex-after-removal
Jun 22, 2026
Merged

fix(documentarray): reindex subdocs after reordering#16282
vkarpov15 merged 18 commits into
Automattic:masterfrom
AbdelrahmanHafez:fix/document-array-reindex-after-removal

Conversation

@AbdelrahmanHafez

@AbdelrahmanHafez AbdelrahmanHafez commented May 13, 2026

Copy link
Copy Markdown
Collaborator

Document array operations that remove or reorder entries can leave surviving subdocs with stale __index values. After operations like pull(), shift(), splice(), $shift(), unshift(), positioned push(), sort(), or reverse(), a subdoc can still think it lives at its previous array index. A later nested change then marks the wrong path, and since the delta serializes the value from whatever subdoc currently sits at that stale index, the update writes a no-op to the wrong element and the user's change is silently lost:

const user = await User.create({
  addresses: [{ city: 'Boston' }, { city: 'Chicago' }, { city: 'Denver' }]
});
user.addresses.pull(user.addresses[0]._id);
await user.save();

user.addresses[0].city = 'New York';
user.$getChanges(); // { $set: { 'addresses.1.city': 'Denver' } }, 'New York' never reaches the DB

This PR:

  • restamps document-array subdoc indexes after operations that can move existing subdocs: pull(), shift(), $shift(), splice(), unshift(), positioned push(), sort(), reverse()
  • keeps append-only push() on the fast path, the new subdoc is already cast with the correct appended index and existing indexes can't move, so repeated appends don't become O(n²)
  • refreshes the parent populated cache in operations that previously skipped it: $shift(), unshift(), sort(), reverse(), pop(), $pop()
  • makes document-array reverse() register a full-array $set, matching sort()/splice()/unshift() and avoiding stale pending atomics after a prior $push
  • adds regression coverage asserting the exact $getChanges() and modifiedPaths() after each operation, persistence after save, populated-cache order, and reverse atomics
  • fixes the MDN links in the touched doc comments, shift() was pointing at the unshift article

addToSet(), nonAtomicPush(), and set() have the same populated cache gap, I'll send that as a follow-up PR once this one lands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes document array subdocument index bookkeeping after removals so later nested mutations mark the correct path.

Changes:

  • Reindexes surviving subdocuments after pull(), shift(), and splice().
  • Preserves populated cache updates in the same wrappers.
  • Adds regression coverage for saving after removal and mutating the first remaining subdocument.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/types/documentArray/methods/index.js Adds subdocument index restamping after selected document-array removal operations.
test/types.documentarray.test.js Adds regression tests for pull(), splice(), and shift() reindex behavior.

Comment thread lib/types/documentArray/methods/index.js
@AbdelrahmanHafez
AbdelrahmanHafez force-pushed the fix/document-array-reindex-after-removal branch from 14e4e57 to f1f0559 Compare May 13, 2026 19:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread lib/types/documentArray/methods/index.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread lib/types/documentArray/methods/index.js Outdated
Comment thread test/types.documentarray.test.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

*/

reverse() {
const ret = _baseReverse.call(this);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@AbdelrahmanHafez AbdelrahmanHafez changed the title fix(documentarray): reindex subdocs after removal fix(documentarray): reindex subdocs after reordering May 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread lib/types/documentArray/methods/index.js
Comment thread lib/types/documentArray/methods/index.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@hasezoey hasezoey left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vkarpov15 vkarpov15 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spotted one issue to follow up on, but otherwise LGTM this is a meaningful improvement


for (let i = 0; i < rawArray.length; ++i) {
if (typeof rawArray[i]?.$setIndex === 'function') {
rawArray[i].$setIndex(i);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a related error with $setIndex(): $setIndex() adds new validation error keys, but retains existing ones. For example:

user.addresses[1].invalidate('city', 'invalid city');
user.addresses.sort((a, b) => a.sequence - b.sequence);
user.$__.validationError.errors; // Has both `addresses.1.city` and `addresses.0.city` presuming that the invalid city was sorted into position 0

@vkarpov15 vkarpov15 added this to the 9.7.1 milestone Jun 22, 2026
@vkarpov15

Copy link
Copy Markdown
Collaborator

Gonna squash this to make it easier to backport to 8.x.

@vkarpov15
vkarpov15 merged commit d48fee3 into Automattic:master Jun 22, 2026
26 checks passed
@AbdelrahmanHafez AbdelrahmanHafez modified the milestones: 9.7.1, 9.7.2 Jun 22, 2026
@AbdelrahmanHafez AbdelrahmanHafez added the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Jun 22, 2026
vkarpov15 pushed a commit that referenced this pull request Jun 22, 2026
* test(documentarray): cover subdoc indexes after removal

* fix(documentarray): reindex subdocs after removal

* test(documentarray): cover subdoc indexes after atomic shift

* fix(documentarray): reindex subdocs after atomic shift

* test(documentarray): cover indexes after reordering

* fix(documentarray): reindex subdocs after reordering

* test(documentarray): cover populated paths after sort

* fix(documentarray): refresh populated paths after sort

* test(documentarray): cover append and reverse reordering

* fix(documentarray): handle reverse and positioned push reindexing

* test(documentarray): cover reverse atomics after push

* fix(documentarray): register reverse as full array update

* test(documentarray): move initial save into createTestContext

* test(documentarray): assert exact update deltas and modified paths

* test(documentarray): cover populated paths after pop

* fix(documentarray): refresh populated paths after pop

* docs(documentarray): normalize MDN links and fix shift() link target
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confirmed-bug We've confirmed this is a bug in Mongoose and will fix it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants