diff --git a/benchmarks/mapOfSubdocs.js b/benchmarks/mapOfSubdocs.js index dc450017448..52714a2ef8d 100644 --- a/benchmarks/mapOfSubdocs.js +++ b/benchmarks/mapOfSubdocs.js @@ -28,7 +28,7 @@ async function run() { ); const MinisMap = mongoose.model('MinisMap', minisMap); await MinisMap.init(); - + const mini = new Map(); for (let i = 0; i < 2000; ++i) { const miniID = new mongoose.Types.ObjectId(); @@ -49,4 +49,4 @@ async function run() { }; console.log(JSON.stringify(results, null, ' ')); -} \ No newline at end of file +} diff --git a/lib/schema/map.js b/lib/schema/map.js index 752fa07d949..cd1de644107 100644 --- a/lib/schema/map.js +++ b/lib/schema/map.js @@ -28,10 +28,14 @@ class SchemaMap extends SchemaType { return val; } - const path = options?.path ?? this.path; + const path = this.path; if (init) { - const map = new MongooseMap({}, path, doc, this.$__schemaType); + const map = new MongooseMap({}, path, doc, this.$__schemaType, options); + + // Use the map's path for passing to nested casts. + // If map's parent is a subdocument, use the relative path so nested casts get relative paths. + const mapPath = map.$__pathRelativeToParent != null ? map.$__pathRelativeToParent : map.$__path; if (val instanceof global.Map) { for (const key of val.keys()) { @@ -39,7 +43,7 @@ class SchemaMap extends SchemaType { if (_val == null) { _val = map.$__schemaType._castNullish(_val); } else { - _val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: path + '.' + key }); + _val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: mapPath + '.' + key }); } map.$init(key, _val); } @@ -49,7 +53,7 @@ class SchemaMap extends SchemaType { if (_val == null) { _val = map.$__schemaType._castNullish(_val); } else { - _val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: path + '.' + key }); + _val = map.$__schemaType.cast(_val, doc, true, null, { ...options, path: mapPath + '.' + key }); } map.$init(key, _val); } @@ -58,7 +62,7 @@ class SchemaMap extends SchemaType { return map; } - return new MongooseMap(val, path, doc, this.$__schemaType); + return new MongooseMap(val, path, doc, this.$__schemaType, options); } clone() { diff --git a/lib/schema/subdocument.js b/lib/schema/subdocument.js index b8a95a0d61c..c9e47944f3e 100644 --- a/lib/schema/subdocument.js +++ b/lib/schema/subdocument.js @@ -203,6 +203,13 @@ SchemaSubdocument.prototype.cast = function(val, doc, init, priorVal, options) { if (init) { subdoc = new Constructor(void 0, selected, doc, false, { defaults: false }); delete subdoc.$__.defaults; + // Don't pass `path` to $init - it's only for the subdocument itself, not its fields. + // For change tracking, subdocuments use relative paths internally. + // Here, `options.path` contains the absolute path and is only used by the subdocument constructor, not by $init. + if (options.path != null) { + options = { ...options }; + delete options.path; + } subdoc.$init(val, options); const exclude = isExclusive(selected); applyDefaults(subdoc, selected, exclude); diff --git a/lib/types/array/methods/index.js b/lib/types/array/methods/index.js index da26fcd2c4c..b7dff4d0080 100644 --- a/lib/types/array/methods/index.js +++ b/lib/types/array/methods/index.js @@ -611,7 +611,7 @@ const methods = { pull() { const values = [].map.call(arguments, (v, i) => this._cast(v, i, { defaults: false }), this); - let cur = this[arrayParentSymbol].get(this[arrayPathSymbol]); + let cur = this; if (utils.isMongooseArray(cur)) { cur = cur.__array; } diff --git a/lib/types/map.js b/lib/types/map.js index c0816bbf586..6703a932883 100644 --- a/lib/types/map.js +++ b/lib/types/map.js @@ -18,13 +18,29 @@ const populateModelSymbol = require('../helpers/symbols').populateModelSymbol; */ class MongooseMap extends Map { - constructor(v, path, doc, schemaType) { + constructor(v, path, doc, schemaType, options) { if (getConstructorName(v) === 'Object') { v = Object.keys(v).reduce((arr, key) => arr.concat([[key, v[key]]]), []); } super(v); this.$__parent = doc != null && doc.$__ != null ? doc : null; - this.$__path = path; + + // 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) { + this.$__path = this.$__parent.$basePath + '.' + path; + // Performance optimization: store path relative to parent subdocument + // to avoid string operations in set() hot path + this.$__pathRelativeToParent = path; + } else if (options?.path) { + this.$__path = options.path; + this.$__pathRelativeToParent = null; + } else { + this.$__path = path; + this.$__pathRelativeToParent = null; + } + this.$__schemaType = schemaType == null ? new Mixed(path) : schemaType; this.$__runDeferred(); @@ -37,6 +53,14 @@ class MongooseMap extends Map { if (value != null && value.$isSingleNested) { value.$basePath = this.$__path + '.' + key; + // Store the path relative to parent subdoc for efficient markModified() + if (this.$__pathRelativeToParent != null) { + // Map's parent is a subdocument, store path relative to that subdoc + value.$pathRelativeToParent = this.$__pathRelativeToParent + '.' + key; + } else { + // Map's parent is root document, store the full path + value.$pathRelativeToParent = this.$__path + '.' + key; + } } } @@ -136,9 +160,16 @@ class MongooseMap extends Map { } } else { try { - const options = this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested || this.$__schemaType.$isMongooseArray || this.$__schemaType.$isSchemaMap ? - { path: fullPath.call(this) } : - null; + let options = null; + if (this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested || this.$__schemaType.$isMongooseArray || this.$__schemaType.$isSchemaMap) { + options = { path: fullPath.call(this) }; + // For subdocuments, also pass the relative path to avoid string operations + if (this.$__schemaType.$isSingleNested) { + options.pathRelativeToParent = this.$__pathRelativeToParent != null ? + this.$__pathRelativeToParent + '.' + key : + this.$__path + '.' + key; + } + } value = this.$__schemaType.applySetters( value, this.$__parent, @@ -157,13 +188,34 @@ class MongooseMap extends Map { super.set(key, value); + // Set relative path on subdocuments to avoid string operations in markModified() + // The path should be relative to the parent subdocument (if any), not just the key + if (value != null && value.$isSingleNested) { + if (this.$__pathRelativeToParent != null) { + // Map's parent is a subdocument, store path relative to that subdoc (e.g., 'items.i2') + value.$pathRelativeToParent = this.$__pathRelativeToParent + '.' + key; + } else { + // Map's parent is root document, store just the full path + value.$pathRelativeToParent = this.$__path + '.' + key; + } + } + if (parent != null && parent.$__ != null && !deepEqual(value, priorVal)) { - const path = fullPath.call(this); - parent.markModified(path); + // Optimization: if parent is a subdocument, use precalculated relative path + // to avoid building a full path just to strip the parent's prefix + let pathToMark; + if (this.$__pathRelativeToParent != null) { + // Parent is a subdocument - use precalculated relative path (e.g., 'items.i1') + pathToMark = this.$__pathRelativeToParent + '.' + key; + } else { + // Parent is root document or map - use full path + pathToMark = fullPath.call(this); + } + parent.markModified(pathToMark); // If overwriting the full document array or subdoc, make sure to clean up any paths that were modified // before re: #15108 if (this.$__schemaType.$isMongooseDocumentArray || this.$__schemaType.$isSingleNested) { - cleanModifiedSubpaths(parent, path); + cleanModifiedSubpaths(parent, pathToMark); } } diff --git a/lib/types/subdocument.js b/lib/types/subdocument.js index 74b72ee66b8..2513319f227 100644 --- a/lib/types/subdocument.js +++ b/lib/types/subdocument.js @@ -31,7 +31,21 @@ function Subdocument(value, fields, parent, skipId, options) { if (options != null && options.path != null) { this.$basePath = options.path; } - Document.call(this, value, fields, skipId, options); + if (options != null && options.pathRelativeToParent != null) { + this.$pathRelativeToParent = options.pathRelativeToParent; + } + + // Don't pass `path` to Document constructor: path is used for storing the + // absolute path to this schematype relative to the top-level document, but + // subdocuments use relative paths (relative to the parent document) to track changes. + // This avoids the subdocument's fields receiving the subdocument's path as options.path. + let documentOptions = options; + if (options != null && options.path != null) { + documentOptions = Object.assign({}, options); + delete documentOptions.path; + } + + Document.call(this, value, fields, skipId, documentOptions); delete this.$__.priorDoc; } @@ -123,9 +137,18 @@ Subdocument.prototype.$__fullPath = function(path) { */ Subdocument.prototype.$__pathRelativeToParent = function(p) { + // If this subdocument has a stored relative path (set by map when subdoc is created), + // use it directly to avoid string operations + if (this.$pathRelativeToParent != null) { + return p == null ? this.$pathRelativeToParent : this.$pathRelativeToParent + '.' + p; + } + if (p == null) { return this.$basePath; } + if (!this.$basePath) { + return p; + } return [this.$basePath, p].join('.'); }; @@ -165,9 +188,13 @@ Subdocument.prototype.$isValid = function(path) { Subdocument.prototype.markModified = function(path) { Document.prototype.markModified.call(this, path); const parent = this.$parent(); - const fullPath = this.$__pathRelativeToParent(path); - if (parent == null || fullPath == null) { + if (parent == null) { + return; + } + + const pathToMark = this.$__pathRelativeToParent(path); + if (pathToMark == null) { return; } @@ -175,7 +202,8 @@ Subdocument.prototype.markModified = function(path) { if (parent.isDirectModified(myPath) || this.isNew) { return; } - this.$__parent.markModified(fullPath, this); + + this.$__parent.markModified(pathToMark, this); }; /*! diff --git a/test/types.map.test.js b/test/types.map.test.js index 85f1c8b99a7..ecf054b2a2a 100644 --- a/test/types.map.test.js +++ b/test/types.map.test.js @@ -1429,4 +1429,393 @@ describe('Map', function() { doc = await Debug.findById(empty._id); assert.strictEqual(doc.settings, undefined); }); + + it('handles maps with document arrays and maps of maps with document arrays (gh-15678)', async function() { + const itemSchema = new Schema({ + name: String, + itemNum: Number, + itemTags: [String] + }, { _id: false }); + + const groupSchema = new Schema({ + items: { type: Map, of: itemSchema, default: {} }, + groupNum: Number, + groupTags: [String] + }, { _id: false }); + + const parentSchema = new Schema({ + groups: { type: Map, of: groupSchema, default: {} } + }); + + const M = db.model('Test', parentSchema); + + // as if read from M.findOne() etc + const x = new M().init({ + groups: { + g1: { + items: { + i1: { + name: 'my item' + } + }, + groupTags: ['hi'] + } + } + }); + + // after each test below in isolation (others commented) + // console.log(x.getChanges()) + + x.groups.get('g1').items.set('i2', { name: 'second item' }); + assert.deepStrictEqual(x.getChanges(), { + $set: { 'groups.g1.items.i2': { name: 'second item', itemTags: [] } } + }); + + x.groups.get('g1').groupNum = 42; + assert.deepStrictEqual(x.getChanges(), { + $set: { + 'groups.g1.items.i2': { name: 'second item', itemTags: [] }, + 'groups.g1.groupNum': 42 + } + } + ); + + x.groups.get('g1').items.get('i1').name = 'different item'; + assert.deepStrictEqual(x.getChanges(), { + $set: { + 'groups.g1.items.i2': { name: 'second item', itemTags: [] }, + 'groups.g1.groupNum': 42, + 'groups.g1.items.i1.name': 'different item' + } + } + ); + + x.groups.get('g1').items.get('i1').itemNum = 20; + assert.deepStrictEqual(x.getChanges(), { + $set: { + 'groups.g1.items.i2': { name: 'second item', itemTags: [] }, + 'groups.g1.groupNum': 42, + 'groups.g1.items.i1.name': 'different item', + 'groups.g1.items.i1.itemNum': 20 + } + } + ); + + x.groups.get('g1').items.get('i1').itemTags.push('foo'); + assert.deepStrictEqual(x.getChanges(), { + $inc: { __v: 1 }, + $push: { + 'groups.g1.items.i1.itemTags': { $each: ['foo'] } + }, + $set: { + 'groups.g1.items.i2': { name: 'second item', itemTags: [] }, + 'groups.g1.groupNum': 42, + 'groups.g1.items.i1.itemNum': 20, + 'groups.g1.items.i1.name': 'different item' + } + }); + }); + + it('handles push() on arrays in subdocuments and maps correctly (gh-15678)', async function() { + 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 = db.model('Test', parentSchema); + + const x = new M().init({ + groups: { + g1: { + items: { + i1: { + numField: 1, + arrayField: ['a'] + } + }, + numField: 2, + arrayField: ['b'] + } + }, + singleItem: { + numField: 3, + arrayField: ['c'] + } + }); + + // Test 1: push to singleItem.arrayField should use $push, not $set on entire subdoc + x.singleItem.arrayField.push('val1'); + assert.deepStrictEqual(x.getChanges(), { + $push: { 'singleItem.arrayField': { $each: ['val1'] } }, + $inc: { __v: 1 } + }); + + // Test 2: push to groups.g1.arrayField should use $push, not $set on entire subdoc + x.groups.get('g1').arrayField.push('val2'); + assert.deepStrictEqual(x.getChanges(), { + $push: { + 'singleItem.arrayField': { $each: ['val1'] }, + 'groups.g1.arrayField': { $each: ['val2'] } + }, + $inc: { __v: 1 } + }); + + // Test 3: push to groups.g1.items.i1.arrayField should use $push + x.groups.get('g1').items.get('i1').arrayField.push('val3'); + assert.deepStrictEqual(x.getChanges(), { + $push: { + 'singleItem.arrayField': { $each: ['val1'] }, + 'groups.g1.arrayField': { $each: ['val2'] }, + 'groups.g1.items.i1.arrayField': { $each: ['val3'] } + }, + $inc: { __v: 1 } + }); + + // Test 4: pull from singleItem.arrayField + x.singleItem.arrayField.pull('c'); + assert.deepStrictEqual(x.getChanges(), { + $set: { 'singleItem.arrayField': ['val1'] }, + $push: { + 'groups.g1.arrayField': { $each: ['val2'] }, + 'groups.g1.items.i1.arrayField': { $each: ['val3'] } + }, + $inc: { __v: 1 } + }); + }); + + it('handles various array operations on subdocument arrays correctly (gh-15678)', async function() { + const itemSchema = new Schema({ + tags: [String] + }, { _id: false }); + + const schema = new Schema({ + items: { type: Map, of: itemSchema }, + directItem: itemSchema + }); + + const M = db.model('Test', schema); + + // Test non-atomic operations (pop, shift, unshift, splice) fall back to $set + const doc1 = new M().init({ + items: { i1: { tags: ['a', 'b', 'c'] } }, + directItem: { tags: ['x', 'y', 'z'] } + }); + + // pop() should use $set + doc1.items.get('i1').tags.pop(); + assert.deepStrictEqual(doc1.getChanges(), { + $set: { 'items.i1.tags': ['a', 'b'] }, + $inc: { __v: 1 } + }); + + // shift() should use $set + const doc2 = new M().init({ + items: { i1: { tags: ['a', 'b', 'c'] } } + }); + doc2.items.get('i1').tags.shift(); + assert.deepStrictEqual(doc2.getChanges(), { + $set: { 'items.i1.tags': ['b', 'c'] }, + $inc: { __v: 1 } + }); + + // unshift() should use $set + const doc3 = new M().init({ + items: { i1: { tags: ['a', 'b'] } } + }); + doc3.items.get('i1').tags.unshift('z'); + assert.deepStrictEqual(doc3.getChanges(), { + $set: { 'items.i1.tags': ['z', 'a', 'b'] }, + $inc: { __v: 1 } + }); + + // splice() should use $set + const doc4 = new M().init({ + items: { i1: { tags: ['a', 'b', 'c'] } } + }); + doc4.items.get('i1').tags.splice(1, 1); + assert.deepStrictEqual(doc4.getChanges(), { + $set: { 'items.i1.tags': ['a', 'c'] }, + $inc: { __v: 1 } + }); + + // addToSet() should use $addToSet + const doc5 = new M().init({ + items: { i1: { tags: ['a', 'b'] } } + }); + doc5.items.get('i1').tags.addToSet('c'); + assert.deepStrictEqual(doc5.getChanges(), { + $addToSet: { 'items.i1.tags': { $each: ['c'] } }, + $inc: { __v: 1 } + }); + + // Test direct subdocument (not in map) + const doc6 = new M().init({ + directItem: { tags: ['x', 'y'] } + }); + doc6.directItem.tags.push('z'); + assert.deepStrictEqual(doc6.getChanges(), { + $push: { 'directItem.tags': { $each: ['z'] } }, + $inc: { __v: 1 } + }); + + // Test mixing operations: push is atomic, then pop makes it $set + const doc7 = new M().init({ + items: { i1: { tags: ['a', 'b'] } } + }); + doc7.items.get('i1').tags.push('c'); + let changes = doc7.getChanges(); + assert.ok(changes.$push); + assert.deepStrictEqual(changes.$push['items.i1.tags'], { $each: ['c'] }); + + doc7.items.get('i1').tags.pop(); + changes = doc7.getChanges(); + // After pop(), should fall back to $set + assert.deepStrictEqual(changes, { + $set: { 'items.i1.tags': ['a', 'b'] }, + $inc: { __v: 1 } + }); + }); + + it('handles empty arrays and save/reload for subdocument arrays (gh-15678)', async function() { + const itemSchema = new Schema({ + tags: [String] + }, { _id: false }); + + const schema = new Schema({ + items: { type: Map, of: itemSchema } + }); + + const M = db.model('Test', schema); + + // Test with empty array + const doc = await M.create({ + items: new Map([['i1', { tags: [] }]]) + }); + + doc.items.get('i1').tags.push('first'); + assert.deepStrictEqual(doc.getChanges(), { + $push: { 'items.i1.tags': { $each: ['first'] } }, + $inc: { __v: 1 } + }); + + await doc.save(); + + // After save, changes should be cleared + assert.deepStrictEqual(doc.getChanges(), {}); + + // Reload and modify again + const reloaded = await M.findById(doc._id); + assert.deepStrictEqual(Array.from(reloaded.items.get('i1').tags), ['first']); + + reloaded.items.get('i1').tags.push('second'); + assert.deepStrictEqual(reloaded.getChanges(), { + $push: { 'items.i1.tags': { $each: ['second'] } }, + $inc: { __v: 1 } + }); + + await reloaded.save(); + + const final = await M.findById(doc._id); + assert.deepStrictEqual(Array.from(final.items.get('i1').tags), ['first', 'second']); + }); + + it('handles map of subdocument with map of arrays (gh-15678)', async function() { + // Map -> Subdoc -> Map -> Array + const innerSchema = new Schema({ + lists: { type: Map, of: [String] }, + name: String + }, { _id: false }); + + const outerSchema = new Schema({ + containers: { type: Map, of: innerSchema } + }); + + const M = db.model('Test', outerSchema); + + const doc = new M().init({ + containers: { + c1: { + name: 'container1', + lists: { + list1: ['a', 'b'], + list2: ['x', 'y'] + } + } + } + }); + + // Push to array in map within subdoc within map + doc.containers.get('c1').lists.get('list1').push('c'); + assert.deepStrictEqual(doc.getChanges(), { + $push: { 'containers.c1.lists.list1': { $each: ['c'] } }, + $inc: { __v: 1 } + }); + + // Push to different array in same nested structure + doc.containers.get('c1').lists.get('list2').push('z'); + assert.deepStrictEqual(doc.getChanges(), { + $push: { + 'containers.c1.lists.list1': { $each: ['c'] }, + 'containers.c1.lists.list2': { $each: ['z'] } + }, + $inc: { __v: 1 } + }); + + // Modify subdoc scalar field alongside array operations + doc.containers.get('c1').name = 'updated'; + assert.deepStrictEqual(doc.getChanges(), { + $push: { + 'containers.c1.lists.list1': { $each: ['c'] }, + 'containers.c1.lists.list2': { $each: ['z'] } + }, + $set: { + 'containers.c1.name': 'updated' + }, + $inc: { __v: 1 } + }); + + // Test with element modification instead of push + const doc2 = new M().init({ + containers: { + c1: { + lists: { + list1: ['a', 'b', 'c'] + } + } + } + }); + + doc2.containers.get('c1').lists.get('list1').set(1, 'modified'); + assert.deepStrictEqual(doc2.getChanges(), { + $set: { 'containers.c1.lists.list1.1': 'modified' } + }); + + // Test pop on deeply nested array + const doc3 = new M().init({ + containers: { + c1: { + lists: { + list1: ['a', 'b', 'c'] + } + } + } + }); + + doc3.containers.get('c1').lists.get('list1').pop(); + assert.deepStrictEqual(doc3.getChanges(), { + $set: { 'containers.c1.lists.list1': ['a', 'b'] }, + $inc: { __v: 1 } + }); + }); });