Merge pull request #14760 from Automattic/vkarpov15/gh-14759

fix(model+document): avoid depopulating manually populated doc as getter value
Automattic · Jul 26, 2024 · 93684f8 · 93684f8
2 parents 2badd9e + 1ad7531
commit 93684f8
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 13 deletions.
diff --git a/lib/document.js b/lib/document.js
@@ -1393,8 +1393,8 @@ Document.prototype.$set = function $set(path, val, type, options) {
     })();
 
     let didPopulate = false;
-    if (refMatches && val instanceof Document && (!val.$__.wasPopulated || utils.deepEqual(val.$__.wasPopulated.value, val._id))) {
-      const unpopulatedValue = (schema && schema.$isSingleNested) ? schema.cast(val, this) : val._id;
+    if (refMatches && val instanceof Document && (!val.$__.wasPopulated || utils.deepEqual(val.$__.wasPopulated.value, val._doc._id))) {
+      const unpopulatedValue = (schema && schema.$isSingleNested) ? schema.cast(val, this) : val._doc._id;
       this.$populated(path, unpopulatedValue, { [populateModelSymbol]: val.constructor });
       val.$__.wasPopulated = { value: unpopulatedValue };
       didPopulate = true;
@@ -1409,10 +1409,10 @@ Document.prototype.$set = function $set(path, val, type, options) {
         schema.options[typeKey][0].ref &&
         _isManuallyPopulatedArray(val, schema.options[typeKey][0].ref)) {
       popOpts = { [populateModelSymbol]: val[0].constructor };
-      this.$populated(path, val.map(function(v) { return v._id; }), popOpts);
+      this.$populated(path, val.map(function(v) { return v._doc._id; }), popOpts);
 
       for (const doc of val) {
-        doc.$__.wasPopulated = { value: doc._id };
+        doc.$__.wasPopulated = { value: doc._doc._id };
       }
       didPopulate = true;
     }
@@ -1455,7 +1455,7 @@ Document.prototype.$set = function $set(path, val, type, options) {
       if (Array.isArray(val) && this.$__.populated[path]) {
         for (let i = 0; i < val.length; ++i) {
           if (val[i] instanceof Document) {
-            val.set(i, val[i]._id, true);
+            val.set(i, val[i]._doc._id, true);
           }
         }
       }
@@ -1628,7 +1628,7 @@ Document.prototype.$__shouldModify = function(pathToMark, path, options, constru
   // if they have the same _id
   if (this.$populated(path) &&
       val instanceof Document &&
-      deepEqual(val._id, priorVal)) {
+      deepEqual(val._doc._id, priorVal)) {
     return false;
   }
 
@@ -3840,7 +3840,7 @@ Document.prototype.$toObject = function(options, json) {
   // _isNested will only be true if this is not the top level document, we
   // should never depopulate the top-level document
   if (depopulate && options._isNested && this.$__.wasPopulated) {
-    return clone(this.$__.wasPopulated.value || this._id, options);
+    return clone(this.$__.wasPopulated.value || this._doc._id, options);
   }
 
   // merge default options with input options.

diff --git a/lib/model.js b/lib/model.js
@@ -4459,7 +4459,7 @@ function _assign(model, vals, mod, assignmentOpts) {
         }
       } else {
         if (_val instanceof Document) {
-          _val = _val._id;
+          _val = _val._doc._id;
         }
         key = String(_val);
         if (rawDocs[key]) {
@@ -4468,7 +4468,7 @@ function _assign(model, vals, mod, assignmentOpts) {
             rawOrder[key].push(i);
           } else if (isVirtual ||
             rawDocs[key].constructor !== val.constructor ||
-            String(rawDocs[key]._id) !== String(val._id)) {
+            String(rawDocs[key]._doc._id) !== String(val._doc._id)) {
             // May need to store multiple docs with the same id if there's multiple models
             // if we have discriminators or a ref function. But avoid converting to an array
             // if we have multiple queries on the same model because of `perDocumentLimit` re: gh-9906

diff --git a/lib/schemaType.js b/lib/schemaType.js
@@ -1542,7 +1542,7 @@ SchemaType.prototype._castRef = function _castRef(value, doc, init) {
   }
 
   if (value.$__ != null) {
-    value.$__.wasPopulated = value.$__.wasPopulated || { value: value._id };
+    value.$__.wasPopulated = value.$__.wasPopulated || { value: value._doc._id };
     return value;
   }
 
@@ -1568,7 +1568,7 @@ SchemaType.prototype._castRef = function _castRef(value, doc, init) {
     !doc.$__.populated[path].options.options ||
     !doc.$__.populated[path].options.options.lean) {
     ret = new pop.options[populateModelSymbol](value);
-    ret.$__.wasPopulated = { value: ret._id };
+    ret.$__.wasPopulated = { value: ret._doc._id };
   }
 
   return ret;

diff --git a/lib/types/map.js b/lib/types/map.js
@@ -116,15 +116,15 @@ class MongooseMap extends Map {
             v = new populated.options[populateModelSymbol](v);
           }
           // Doesn't support single nested "in-place" populate
-          v.$__.wasPopulated = { value: v._id };
+          v.$__.wasPopulated = { value: v._doc._id };
           return v;
         });
       } else if (value != null) {
         if (value.$__ == null) {
           value = new populated.options[populateModelSymbol](value);
         }
         // Doesn't support single nested "in-place" populate
-        value.$__.wasPopulated = { value: value._id };
+        value.$__.wasPopulated = { value: value._doc._id };
       }
     } else {
       try {

diff --git a/test/model.populate.test.js b/test/model.populate.test.js
@@ -11014,4 +11014,38 @@ describe('model: populate:', function() {
     assert.equal(latestClass.students[1].name, 'Robert');
     assert.equal(latestClass.students[1].grade.grade, 'B');
   });
+
+  it('avoids depopulating manually populated doc as getter value (gh-14759)', async function() {
+    const ownerSchema = new mongoose.Schema({
+      _id: {
+        type: 'ObjectId',
+        get(value) {
+          return value == null ? value : value.toString();
+        }
+      },
+      name: 'String'
+    });
+    const petSchema = new mongoose.Schema({
+      name: 'String',
+      owner: { type: 'ObjectId', ref: 'Owner' }
+    });
+
+    const Owner = db.model('Owner', ownerSchema);
+    const Pet = db.model('Pet', petSchema);
+
+    const ownerId = new mongoose.Types.ObjectId();
+    const owner = await Owner.create({
+      _id: ownerId,
+      name: 'Alice'
+    });
+    await Pet.create({ name: 'Kitty', owner: owner });
+
+    const fromDb = await Pet.findOne({ owner: ownerId }).lean().orFail();
+    assert.ok(fromDb.owner instanceof mongoose.Types.ObjectId);
+
+    const pet1 = new Pet({ name: 'Kitty1', owner: owner });
+    const pet2 = new Pet({ name: 'Kitty2', owner: owner });
+    assert.equal(pet1.owner.name, 'Alice');
+    assert.equal(pet2.owner.name, 'Alice');
+  });
 });