Merge pull request #14883 from Automattic/vkarpov15/gh-1635-2

fix: depopulate if `push()` or `addToSet()` with an ObjectId on a populated array

Author: vkarpov15

docs/populate.md

@@ -119,6 +119,33 @@ story.author = author;
 console.log(story.author.name); // prints "Ian Fleming"
 ```
 
+You can also push documents or POJOs onto a populated array, and Mongoose will add those documents if their `ref` matches.
+
+```javascript
+const fan1 = await Person.create({ name: 'Sean' });
+await Story.updateOne({ title: 'Casino Royale' }, { $push: { fans: { $each: [fan1._id] } } });
+
+const story = await Story.findOne({ title: 'Casino Royale' }).populate('fans');
+story.fans[0].name; // 'Sean'
+
+const fan2 = await Person.create({ name: 'George' });
+story.fans.push(fan2);
+story.fans[1].name; // 'George'
+
+story.fans.push({ name: 'Roger' });
+story.fans[2].name; // 'Roger'
+```
+
+If you push a non-POJO and non-document value, like an ObjectId, Mongoose `>= 8.7.0` will depopulate the entire array.
+
+```javascript
+const fan4 = await Person.create({ name: 'Timothy' });
+story.fans.push(fan4._id); // Push the `_id`, not the full document
+
+story.fans[0].name; // undefined, `fans[0]` is now an ObjectId
+story.fans[0].toString() === fan1._id.toString(); // true
+```
+
 ## Checking Whether a Field is Populated {#checking-populated}
 
 You can call the `populated()` function to check whether a field is populated.

lib/types/array/methods/index.js

@@ -410,6 +410,7 @@ const methods = {
 
   addToSet() {
     _checkManualPopulation(this, arguments);
+    _depopulateIfNecessary(this, arguments);
 
     const values = [].map.call(arguments, this._mapCast, this);
     const added = [];
@@ -691,6 +692,7 @@ const methods = {
     }
 
     _checkManualPopulation(this, values);
+    _depopulateIfNecessary(this, values);
 
     values = [].map.call(values, this._mapCast, this);
     let ret;
@@ -1009,6 +1011,30 @@ function _checkManualPopulation(arr, docs) {
   }
 }
 
+/*!
+ * If `docs` isn't all instances of the right model, depopulate `arr`
+ */
+
+function _depopulateIfNecessary(arr, docs) {
+  const ref = arr == null ?
+    null :
+    arr[arraySchemaSymbol] && arr[arraySchemaSymbol].caster && arr[arraySchemaSymbol].caster.options && arr[arraySchemaSymbol].caster.options.ref || null;
+  const parentDoc = arr[arrayParentSymbol];
+  const path = arr[arrayPathSymbol];
+  if (!ref || !parentDoc.populated(path)) {
+    return;
+  }
+  for (const doc of docs) {
+    if (doc == null) {
+      continue;
+    }
+    if (typeof doc !== 'object' || doc instanceof String || doc instanceof Number || doc instanceof Buffer || utils.isMongooseType(doc)) {
+      parentDoc.depopulate(path);
+      break;
+    }
+  }
+}
+
 const returnVanillaArrayMethods = [
   'filter',
   'flat',

test/model.populate.setting.test.js

@@ -152,7 +152,7 @@ describe('model: populate:', function() {
           assert.equal(doc.fans[6], null);
 
           const _id = construct[id]();
-          doc.fans.addToSet(_id);
+          doc.fans.addToSet({ _id });
           if (Buffer.isBuffer(_id)) {
             assert.equal(doc.fans[7]._id.toString('utf8'), _id.toString('utf8'));
           } else {

test/model.populate.test.js

@@ -11131,4 +11131,30 @@ describe('model: populate:', function() {
     }
     assert.equal(posts.length, 2);
   });
+
+  it('depopulates if pushing ObjectId to a populated array (gh-1635)', async function() {
+    const ParentModel = db.model('Test', mongoose.Schema({
+      name: String,
+      children: [{ type: 'ObjectId', ref: 'Child' }]
+    }));
+    const ChildModel = db.model('Child', mongoose.Schema({ name: String }));
+
+    const children = await ChildModel.create([{ name: 'Luke' }, { name: 'Leia' }]);
+    const newChild = await ChildModel.create({ name: 'Taco' });
+    const { _id } = await ParentModel.create({ name: 'Anakin', children });
+
+    const doc = await ParentModel.findById(_id).populate('children');
+    doc.children.push(newChild._id);
+
+    assert.ok(doc.children[0] instanceof mongoose.Types.ObjectId);
+    assert.ok(doc.children[1] instanceof mongoose.Types.ObjectId);
+    assert.ok(doc.children[2] instanceof mongoose.Types.ObjectId);
+
+    await doc.save();
+
+    const fromDb = await ParentModel.findById(_id);
+    assert.equal(fromDb.children[0].toHexString(), children[0]._id.toHexString());
+    assert.equal(fromDb.children[1].toHexString(), children[1]._id.toHexString());
+    assert.equal(fromDb.children[2].toHexString(), newChild._id.toHexString());
+  });
 });