Merge pull request #14531 from Automattic/vkarpov15/gh-14502

fix(array): avoid converting to `$set` when calling `pull()` on an element in the middle of the array
Automattic · Apr 27, 2024 · c9293a2 · c9293a2
2 parents c97c060 + cadcf38
commit c9293a2
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 7 deletions.
diff --git a/lib/document.js b/lib/document.js
@@ -2381,15 +2381,29 @@ Document.prototype.isDirectModified = function(path) {
   }
 
   if (typeof path === 'string' && path.indexOf(' ') === -1) {
-    return this.$__.activePaths.getStatePaths('modify').hasOwnProperty(path);
+    const res = this.$__.activePaths.getStatePaths('modify').hasOwnProperty(path);
+    if (res || path.indexOf('.') === -1) {
+      return res;
+    }
+
+    const pieces = path.split('.');
+    for (let i = 0; i < pieces.length - 1; ++i) {
+      const subpath = pieces.slice(0, i + 1).join('.');
+      const subdoc = this.$get(subpath);
+      if (subdoc != null && subdoc.$__ != null && subdoc.isDirectModified(pieces.slice(i + 1).join('.'))) {
+        return true;
+      }
+    }
+
+    return false;
   }
 
   let paths = path;
-  if (!Array.isArray(paths)) {
+  if (typeof paths === 'string') {
     paths = paths.split(' ');
   }
 
-  return paths.some(path => this.$__.activePaths.getStatePaths('modify').hasOwnProperty(path));
+  return paths.some(path => this.isDirectModified(path));
 };
 
 /**

diff --git a/lib/types/array/methods/index.js b/lib/types/array/methods/index.js
@@ -603,7 +603,10 @@ const methods = {
 
   pull() {
     const values = [].map.call(arguments, (v, i) => this._cast(v, i, { defaults: false }), this);
-    const cur = this[arrayParentSymbol].get(this[arrayPathSymbol]);
+    let cur = this[arrayParentSymbol].get(this[arrayPathSymbol]);
+    if (utils.isMongooseArray(cur)) {
+      cur = cur.__array;
+    }
     let i = cur.length;
     let mem;
     this._markModified();
@@ -615,10 +618,10 @@ const methods = {
           return mem.equals(v);
         });
         if (some) {
-          [].splice.call(cur, i, 1);
+          cur.splice(i, 1);
         }
-      } else if (~cur.indexOf.call(values, mem)) {
-        [].splice.call(cur, i, 1);
+      } else if (~this.indexOf.call(values, mem)) {
+        cur.splice(i, 1);
       }
     }
 

diff --git a/test/document.test.js b/test/document.test.js
@@ -13055,6 +13055,64 @@ describe('document', function() {
     const obj = parent.toJSON({ getters: true });
     assert.equal(obj.children[0].name, 'Stefan');
   });
+
+  it('isDirectModified on paths underneath direct modified subdoc (gh-14502)', async function() {
+    const JsonFieldSchema = new Schema({
+      fieldA: String,
+      fieldB: String
+    });
+
+    const CommentSchema = new Schema({
+      title: String,
+      body: String,
+      jsonField: JsonFieldSchema
+    });
+
+    const BlogPostSchema = new Schema({
+      comment: CommentSchema
+    });
+
+    const Comments = db.model('Comments', CommentSchema);
+    const BlogPost = db.model('BlogPost', BlogPostSchema);
+
+    const comment1 = new Comments({});
+    comment1.init({
+      title: 'Test',
+      body: 'Test',
+      jsonField: {
+        fieldA: 'field A',
+        fieldB: 'field B'
+      }
+    });
+
+    const update = {
+      title: 'New test',
+      jsonField: {
+        fieldA: 'new Field A'
+      }
+    };
+    Object.assign(comment1, { ...update });
+
+    assert.ok(comment1.isDirectModified('jsonField.fieldA'));
+    assert.ok(comment1.jsonField.isDirectModified('fieldA'));
+
+    const blogPost = new BlogPost({});
+    blogPost.init({
+      comment: {
+        title: 'Test',
+        body: 'Test',
+        jsonField: {
+          fieldA: 'field A',
+          fieldB: 'field B'
+        }
+      }
+    });
+
+    Object.assign(blogPost.comment, { ...update });
+
+    assert.ok(blogPost.isDirectModified('comment.jsonField.fieldA'));
+    assert.ok(blogPost.comment.jsonField.isDirectModified('fieldA'));
+  });
 });
 
 describe('Check if instance function that is supplied in schema option is availabe', function() {

diff --git a/test/types.array.test.js b/test/types.array.test.js
@@ -597,6 +597,27 @@ describe('types array', function() {
       doc.a.pull(cat.id);
       assert.equal(doc.a.length, 0);
 
+      assert.ok(doc.getChanges().$pullAll.a);
+    });
+
+    it('registers $pull atomic if pulling from middle (gh-14502)', async function() {
+      const schema = new Schema({
+        a: [{ type: Schema.ObjectId, ref: 'Cat' }]
+      });
+      const A = db.model('Test', schema);
+
+      const oid1 = new mongoose.Types.ObjectId();
+      const oid2 = new mongoose.Types.ObjectId();
+      const oid3 = new mongoose.Types.ObjectId();
+      const a = new A({ a: [oid1, oid2, oid3] });
+      await a.save();
+
+      const doc = await A.findById(a);
+      assert.equal(doc.a.length, 3);
+      doc.a.pull(oid2);
+      assert.equal(doc.a.length, 2);
+
+      assert.ok(doc.getChanges().$pullAll.a);
     });
 
     it('handles pulling with no _id (gh-3341)', async function() {