Skip to content

Conversation

vkarpov15
Copy link
Collaborator

Fix #15678
Re: #15350

Summary

The core issue in #15678 and #15350 is that maps use absolute paths for change tracking (path relative to the top-level document), whereas subdocuments use relative paths (path relative to parent subdocument, change tracking bubbles up). This inconsistency becomes problematic when you have multiple layers of nesting - in #15678, the issue was a subdocument underneath a map underneath another map.

In this PR, maps and subdocuments now both track pathRelativeToParent as well as path (full/absolute path) and use the correct path for change tracking.

Examples

@vkarpov15 vkarpov15 added this to the 8.19.2 milestone Oct 9, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

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 a critical issue with change tracking in nested map and subdocument structures where paths were inconsistently handled between absolute and relative contexts. The fix ensures that both maps and subdocuments properly track both their full path (relative to root document) and their path relative to their parent, using the appropriate path type for change tracking operations.

Key changes:

  • Enhanced map constructor to calculate and store both full paths and relative-to-parent paths
  • Modified subdocument path handling to use stored relative paths when available
  • Updated change tracking logic to use the correct path type based on parent context

Reviewed Changes

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

Show a summary per file
File Description
test/types.map.test.js Adds comprehensive test case for nested maps with document arrays to verify correct change tracking
lib/types/subdocument.js Updates subdocument path handling and markModified logic to use relative paths efficiently
lib/types/map.js Enhances map constructor and set method to properly handle both absolute and relative paths
lib/types/array/methods/index.js Fixes array method path handling for subdocument parents
lib/schema/subdocument.js Passes path options through subdocument constructor
lib/schema/map.js Updates map casting to use calculated full paths and pass options correctly

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

LGTM

@kballen1
Copy link

kballen1 commented Oct 12, 2025

Thank you for your quick work on fixing this issue! I tested against your PR branch and it does fix the incorrect document paths.

However now that doc updates are able to be properly generated, I'm running into a few more issues with change tracking/delta calculations which appear to be related to array types:

  • Arrays in subdocuments (whether standalone or nested in a map) will not emit the proper update operator for push/pulls and instead use $set. Additionally, they seem to mark the entire parent doc as changed, resulting in undesired fields being included in the $set.
  • Array.pull() crashes for paths nested in a map.

See the following repro (against PR branch):

const itemSchema = new Schema({
  numField: Number,
  arrayField: [String],
}, { _id: false });

const groupSchema = new Schema({
  items: { type: Map, of: itemSchema },
  numField: Number,
  arrayField: [String],
}, { _id: false });

const parentSchema = new Schema({
  groups: { type: Map, of: groupSchema },
  singleItem: { type: itemSchema },
});

const M = model("parentSchema", parentSchema);

const x = new M().init({
    groups: {
        g1: {
            items: {
                i1: {
                    numField: 1,
                    arrayField: ["a"],
                },
            },
            numField: 2,
            arrayField: ["b"],
        },
    },
    singleItem: {
        numField: 3,
        arrayField: ["c"],
    },
});

x.singleItem.arrayField.push("val");
// entire subdocument is set
// {"$set":{"singleItem":{"numField":3,"arrayField":["c","val"]}}}

x.groups.get("g1").arrayField.push("val");
// entire subdocument is set and existing map entries are lost
// {"$set":{"groups.g1":{"items":{},"numField":2,"arrayField":["b","val"]}}}

x.groups.get("g1").items.get("i1").arrayField.push("val");
// {"$set":{"groups.g1.items.i1":{"numField":1,"arrayField":["a","val"]}}}

x.singleItem.arrayField.pull("c");
// when in a regular subdocument, doesn't use the correct operator but otherwise works
// {"$set":{"singleItem":{"numField":3,"arrayField":[]}}}

x.groups.get("g1").arrayField.pull("b");
// but in a map entry..
// ./node_modules/mongoose/lib/types/array/methods/index.js:623
//     let i = cur.length;
//                 ^
// TypeError: Cannot read properties of undefined (reading 'length')
//     at Proxy.pull (./node_modules/mongoose/lib/types/array/methods/index.js:623:17)

If you prefer I can open a new issue for this.

@vkarpov15 vkarpov15 requested review from Copilot and hasezoey October 13, 2025 18:35
@vkarpov15
Copy link
Collaborator Author

vkarpov15 commented Oct 13, 2025

@hasezoey @kballen1 please take another look, I fixed this to account for the new cases @kballen1 mentioned and a couple of others.

I double checked and performance on the mapOfSubdocs benchmark is no different on this branch than in master.

~/Desktop/MongoDB/mongoose$ git checkout master
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 192.3
}
^C
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 192.7
}
^C
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 205.5
}
^C
~/Desktop/MongoDB/mongoose$ 
~/Desktop/MongoDB/mongoose$ 
~/Desktop/MongoDB/mongoose$ git checkout vkarpov15/gh-15678 
Switched to branch 'vkarpov15/gh-15678'
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 192.9
}
^C
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 191.4
}
^C
~/Desktop/MongoDB/mongoose$ node ./benchmarks/mapOfSubdocs.js 
{
  "Average save time ms": 194.7
}
^C
~/Desktop/MongoDB/mongoose$ 

Copy link
Contributor

@Copilot Copilot AI left a comment

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 6 out of 6 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@kballen1
Copy link

All cases look good to me. Thank you!

@vkarpov15 vkarpov15 merged commit 9ddebb8 into master Oct 17, 2025
74 checks passed
@hasezoey hasezoey deleted the vkarpov15/gh-15678 branch October 18, 2025 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changed paths are incorrect in schemas with nested Maps

3 participants