-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: correct handling of relative vs absolute paths with maps and subdocuments #15682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ss a relative path to the subdocument markModified
There was a problem hiding this 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.
// Calculate the full path from the root document | ||
// Priority: parent.$basePath (from subdoc) > options.path (from parent map/structure) > path (schema path) | ||
// Subdocuments have the most up-to-date path info, so prefer that over options.path | ||
if (this.$__parent?.$isSingleNested && this.$__parent.$basePath) { |
Copilot
AI
Oct 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Using optional chaining with logical AND can be simplified. Consider this.$__parent?.$isSingleNested && this.$__parent?.$basePath
to be more explicit about null safety for both property accesses.
if (this.$__parent?.$isSingleNested && this.$__parent.$basePath) { | |
if (this.$__parent?.$isSingleNested && this.$__parent?.$basePath) { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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:
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. |
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