Skip to content

Commit 9ddebb8

Browse files
vkarpov15vkarpov15
authored
Merge pull request #15682 from Automattic/vkarpov15/gh-15678
fix: correct handling of relative vs absolute paths with maps and subdocuments
2 parents a24dce3 + 3f0a9c9 commit 9ddebb8

7 files changed

Lines changed: 500 additions & 20 deletions

File tree

‎benchmarks/mapOfSubdocs.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ async function run() {
2828
);
2929
const MinisMap = mongoose.model('MinisMap', minisMap);
3030
await MinisMap.init();
31-
31+
3232
const mini = new Map();
3333
for (let i = 0; i < 2000; ++i) {
3434
const miniID = new mongoose.Types.ObjectId();
@@ -49,4 +49,4 @@ async function run() {
4949
};
5050

5151
console.log(JSON.stringify(results, null, ' '));
52-
}
52+
}

‎lib/schema/map.js‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,22 @@ class SchemaMap extends SchemaType {
2828
return val;
2929
}
3030

31-
const path = options?.path ?? this.path;
31+
const path = this.path;
3232

3333
if (init) {
34-
const map = new MongooseMap({}, path, doc, this.$__schemaType);
34+
const map = new MongooseMap({}, path, doc, this.$__schemaType, options);
35+
36+
// Use the map's path for passing to nested casts.
37+
// If map's parent is a subdocument, use the relative path so nested casts get relative paths.
38+
const mapPath = map.$__pathRelativeToParent != null ? map.$__pathRelativeToParent : map.$__path;
3539

3640
if (val instanceof global.Map) {
3741
for (const key of val.keys()) {
3842
let _val = val.get(key);
3943
if (_val == null) {
4044
_val = map.$__schemaType._castNullish(_val);
4145
} else {
42-
_val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: path + '.' + key });
46+
_val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: mapPath + '.' + key });
4347
}
4448
map.$init(key, _val);
4549
}
@@ -49,7 +53,7 @@ class SchemaMap extends SchemaType {
4953
if (_val == null) {
5054
_val = map.$__schemaType._castNullish(_val);
5155
} else {
52-
_val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: path + '.' + key });
56+
_val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: mapPath + '.' + key });
5357
}
5458
map.$init(key, _val);
5559
}
@@ -58,7 +62,7 @@ class SchemaMap extends SchemaType {
5862
return map;
5963
}
6064

61-
return new MongooseMap(val, path, doc, this.$__schemaType);
65+
return new MongooseMap(val, path, doc, this.$__schemaType, options);
6266
}
6367

6468
clone() {

‎lib/schema/subdocument.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,13 @@ SchemaSubdocument.prototype.cast = function(val, doc, init, priorVal, options) {
203203
if (init) {
204204
subdoc = new Constructor(void 0, selected, doc, false, { defaults: false });
205205
delete subdoc.$__.defaults;
206+
// Don't pass `path` to $init - it's only for the subdocument itself, not its fields.
207+
// For change tracking, subdocuments use relative paths internally.
208+
// Here, `options.path` contains the absolute path and is only used by the subdocument constructor, not by $init.
209+
if (options.path != null) {
210+
options = { ...options };
211+
delete options.path;
212+
}
206213
subdoc.$init(val, options);
207214
const exclude = isExclusive(selected);
208215
applyDefaults(subdoc, selected, exclude);

‎lib/types/array/methods/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ const methods = {
611611

612612
pull() {
613613
const values = [].map.call(arguments, (v, i) => this._cast(v, i, { defaults: false }), this);
614-
let cur = this[arrayParentSymbol].get(this[arrayPathSymbol]);
614+
let cur = this;
615615
if (utils.isMongooseArray(cur)) {
616616
cur = cur.__array;
617617
}

‎lib/types/map.js‎

Lines changed: 60 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,29 @@ const populateModelSymbol = require('../helpers/symbols').populateModelSymbol;
1818
*/
1919

2020
class MongooseMap extends Map {
21-
constructor(v, path, doc, schemaType) {
21+
constructor(v, path, doc, schemaType, options) {
2222
if (getConstructorName(v) === 'Object') {
2323
v = Object.keys(v).reduce((arr, key) => arr.concat([[key, v[key]]]), []);
2424
}
2525
super(v);
2626
this.$__parent = doc != null && doc.$__ != null ? doc : null;
27-
this.$__path = path;
27+
28+
// Calculate the full path from the root document
29+
// Priority: parent.$basePath (from subdoc) > options.path (from parent map/structure) > path (schema path)
30+
// Subdocuments have the most up-to-date path info, so prefer that over options.path
31+
if (this.$__parent?.$isSingleNested && this.$__parent.$basePath) {
32+
this.$__path = this.$__parent.$basePath + '.' + path;
33+
// Performance optimization: store path relative to parent subdocument
34+
// to avoid string operations in set() hot path
35+
this.$__pathRelativeToParent = path;
36+
} else if (options?.path) {
37+
this.$__path = options.path;
38+
this.$__pathRelativeToParent = null;
39+
} else {
40+
this.$__path = path;
41+
this.$__pathRelativeToParent = null;
42+
}
43+
2844
this.$__schemaType = schemaType == null ? new Mixed(path) : schemaType;
2945

3046
this.$__runDeferred();
@@ -37,6 +53,14 @@ class MongooseMap extends Map {
3753

3854
if (value != null && value.$isSingleNested) {
3955
value.$basePath = this.$__path + '.' + key;
56+
// Store the path relative to parent subdoc for efficient markModified()
57+
if (this.$__pathRelativeToParent != null) {
58+
// Map's parent is a subdocument, store path relative to that subdoc
59+
value.$pathRelativeToParent = this.$__pathRelativeToParent + '.' + key;
60+
} else {
61+
// Map's parent is root document, store the full path
62+
value.$pathRelativeToParent = this.$__path + '.' + key;
63+
}
4064
}
4165
}
4266

@@ -136,9 +160,16 @@ class MongooseMap extends Map {
136160
}
137161
} else {
138162
try {
139-
const options = this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested || this.$__schemaType.$isMongooseArray || this.$__schemaType.$isSchemaMap ?
140-
{ path: fullPath.call(this) } :
141-
null;
163+
let options = null;
164+
if (this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested || this.$__schemaType.$isMongooseArray || this.$__schemaType.$isSchemaMap) {
165+
options = { path: fullPath.call(this) };
166+
// For subdocuments, also pass the relative path to avoid string operations
167+
if (this.$__schemaType.$isSingleNested) {
168+
options.pathRelativeToParent = this.$__pathRelativeToParent != null ?
169+
this.$__pathRelativeToParent + '.' + key :
170+
this.$__path + '.' + key;
171+
}
172+
}
142173
value = this.$__schemaType.applySetters(
143174
value,
144175
this.$__parent,
@@ -157,13 +188,34 @@ class MongooseMap extends Map {
157188

158189
super.set(key, value);
159190

191+
// Set relative path on subdocuments to avoid string operations in markModified()
192+
// The path should be relative to the parent subdocument (if any), not just the key
193+
if (value != null && value.$isSingleNested) {
194+
if (this.$__pathRelativeToParent != null) {
195+
// Map's parent is a subdocument, store path relative to that subdoc (e.g., 'items.i2')
196+
value.$pathRelativeToParent = this.$__pathRelativeToParent + '.' + key;
197+
} else {
198+
// Map's parent is root document, store just the full path
199+
value.$pathRelativeToParent = this.$__path + '.' + key;
200+
}
201+
}
202+
160203
if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) {
161-
const path = fullPath.call(this);
162-
parent.markModified(path);
204+
// Optimization: if parent is a subdocument, use precalculated relative path
205+
// to avoid building a full path just to strip the parent's prefix
206+
let pathToMark;
207+
if (this.$__pathRelativeToParent != null) {
208+
// Parent is a subdocument - use precalculated relative path (e.g., 'items.i1')
209+
pathToMark = this.$__pathRelativeToParent + '.' + key;
210+
} else {
211+
// Parent is root document or map - use full path
212+
pathToMark = fullPath.call(this);
213+
}
214+
parent.markModified(pathToMark);
163215
// If overwriting the full document array or subdoc, make sure to clean up any paths that were modified
164216
// before re: #15108
165217
if (this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested) {
166-
cleanModifiedSubpaths(parent, path);
218+
cleanModifiedSubpaths(parent, pathToMark);
167219
}
168220
}
169221

‎lib/types/subdocument.js‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,21 @@ function Subdocument(value, fields, parent, skipId, options) {
3131
if (options != null && options.path != null) {
3232
this.$basePath = options.path;
3333
}
34-
Document.call(this, value, fields, skipId, options);
34+
if (options != null && options.pathRelativeToParent != null) {
35+
this.$pathRelativeToParent = options.pathRelativeToParent;
36+
}
37+
38+
// Don't pass `path` to Document constructor: path is used for storing the
39+
// absolute path to this schematype relative to the top-level document, but
40+
// subdocuments use relative paths (relative to the parent document) to track changes.
41+
// This avoids the subdocument's fields receiving the subdocument's path as options.path.
42+
let documentOptions = options;
43+
if (options != null && options.path != null) {
44+
documentOptions = Object.assign({}, options);
45+
delete documentOptions.path;
46+
}
47+
48+
Document.call(this, value, fields, skipId, documentOptions);
3549

3650
delete this.$__.priorDoc;
3751
}
@@ -123,9 +137,18 @@ Subdocument.prototype.$__fullPath = function(path) {
123137
*/
124138

125139
Subdocument.prototype.$__pathRelativeToParent = function(p) {
140+
// If this subdocument has a stored relative path (set by map when subdoc is created),
141+
// use it directly to avoid string operations
142+
if (this.$pathRelativeToParent != null) {
143+
return p == null ? this.$pathRelativeToParent : this.$pathRelativeToParent + '.' + p;
144+
}
145+
126146
if (p == null) {
127147
return this.$basePath;
128148
}
149+
if (!this.$basePath) {
150+
return p;
151+
}
129152
return [this.$basePath, p].join('.');
130153
};
131154

@@ -165,17 +188,22 @@ Subdocument.prototype.$isValid = function(path) {
165188
Subdocument.prototype.markModified = function(path) {
166189
Document.prototype.markModified.call(this, path);
167190
const parent = this.$parent();
168-
const fullPath = this.$__pathRelativeToParent(path);
169191

170-
if (parent == null || fullPath == null) {
192+
if (parent == null) {
193+
return;
194+
}
195+
196+
const pathToMark = this.$__pathRelativeToParent(path);
197+
if (pathToMark == null) {
171198
return;
172199
}
173200

174201
const myPath = this.$__pathRelativeToParent().replace(/\.$/, '');
175202
if (parent.isDirectModified(myPath) || this.isNew) {
176203
return;
177204
}
178-
this.$__parent.markModified(fullPath, this);
205+
206+
this.$__parent.markModified(pathToMark, this);
179207
};
180208

181209
/*!

0 commit comments

Comments
 (0)