Merge pull request #14732 from Automattic/vkarpov15/gh-9885

fix(document): ensure post('deleteOne') hooks are called when calling `save()` after `subdoc.deleteOne()`
Automattic · Jul 8, 2024 · 761e551 · 761e551
2 parents 9623653 + 50f1a73
commit 761e551
Show file tree

Hide file tree

Showing 3 changed files with 116 additions and 16 deletions.
diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js
@@ -36,31 +36,63 @@ module.exports = function saveSubdocs(schema) {
     });
   }, null, unshift);
 
-  schema.s.hooks.post('save', function saveSubdocsPostSave(doc, next) {
+  schema.s.hooks.post('save', async function saveSubdocsPostDeleteOne() {
+    const removedSubdocs = this.$__.removedSubdocs;
+    if (!removedSubdocs || !removedSubdocs.length) {
+      return;
+    }
+
+    const promises = [];
+    for (const subdoc of removedSubdocs) {
+      promises.push(new Promise((resolve, reject) => {
+        subdoc.$__schema.s.hooks.execPost('deleteOne', subdoc, [subdoc], function(err) {
+          if (err) {
+            return reject(err);
+          }
+          resolve();
+        });
+      }));
+    }
+
+    this.$__.removedSubdocs = null;
+    await Promise.all(promises);
+  });
+
+  schema.s.hooks.post('save', async function saveSubdocsPostSave() {
     if (this.$isSubdocument) {
-      next();
       return;
     }
 
     const _this = this;
     const subdocs = this.$getAllSubdocs();
 
     if (!subdocs.length) {
-      next();
       return;
     }
 
-    each(subdocs, function(subdoc, cb) {
-      subdoc.$__schema.s.hooks.execPost('save', subdoc, [subdoc], function(err) {
-        cb(err);
-      });
-    }, function(error) {
-      if (error) {
-        return _this.$__schema.s.hooks.execPost('save:error', _this, [_this], { error: error }, function(error) {
-          next(error);
+    const promises = [];
+    for (const subdoc of subdocs) {
+      promises.push(new Promise((resolve, reject) => {
+        subdoc.$__schema.s.hooks.execPost('save', subdoc, [subdoc], function(err) {
+          if (err) {
+            return reject(err);
+          }
+          resolve();
         });
-      }
-      next();
-    });
+      }));
+    }
+
+    try {
+      await Promise.all(promises);
+    } catch (error) {
+      await new Promise((resolve, reject) => {
+        this.$__schema.s.hooks.execPost('save:error', _this, [_this], { error: error }, function(error) {
+          if (error) {
+            return reject(error);
+          }
+          resolve();
+        });
+      });
+    }
   }, null, unshift);
 };
diff --git a/lib/types/subdocument.js b/lib/types/subdocument.js
@@ -378,6 +378,10 @@ Subdocument.prototype.deleteOne = function(options, callback) {
   // If removing entire doc, no need to remove subdoc
   if (!options || !options.noop) {
     this.$__removeFromParent();
+
+    const owner = this.ownerDocument();
+    owner.$__.removedSubdocs = owner.$__.removedSubdocs || [];
+    owner.$__.removedSubdocs.push(this);
   }
 
   return this.$__deleteOne(callback);
@@ -417,14 +421,13 @@ if (util.inspect.custom) {
  */
 
 function registerRemoveListener(sub) {
-  let owner = sub.ownerDocument();
+  const owner = sub.ownerDocument();
 
   function emitRemove() {
     owner.$removeListener('save', emitRemove);
     owner.$removeListener('deleteOne', emitRemove);
     sub.emit('deleteOne', sub);
     sub.constructor.emit('deleteOne', sub);
-    owner = sub = null;
   }
 
   owner.$on('save', emitRemove);

diff --git a/test/document.test.js b/test/document.test.js
@@ -13535,6 +13535,71 @@ describe('document', function() {
     assert.ok(blogPost.isDirectModified('comment.jsonField.fieldA'));
     assert.ok(blogPost.comment.jsonField.isDirectModified('fieldA'));
   });
+
+  it('post deleteOne hook (gh-9885)', async function() {
+    const ChildSchema = new Schema({ name: String });
+    const called = {
+      preSave: 0,
+      postSave: 0,
+      preDeleteOne: 0,
+      postDeleteOne: 0
+    };
+    let postDeleteOneError = null;
+    ChildSchema.pre('save', function(next) {
+      ++called.preSave;
+      next();
+    });
+    ChildSchema.post('save', function(subdoc, next) {
+      ++called.postSave;
+      next();
+    });
+    ChildSchema.pre('deleteOne', { document: true, query: false }, function(next) {
+      ++called.preDeleteOne;
+      next();
+    });
+    ChildSchema.post('deleteOne', { document: true, query: false }, function(subdoc, next) {
+      ++called.postDeleteOne;
+      next(postDeleteOneError);
+    });
+    const ParentSchema = new Schema({ name: String, children: [ChildSchema] });
+    const ParentModel = db.model('Parent', ParentSchema);
+
+    const doc = new ParentModel({ name: 'Parent' });
+    await doc.save();
+    assert.deepStrictEqual(called, {
+      preSave: 0,
+      postSave: 0,
+      preDeleteOne: 0,
+      postDeleteOne: 0
+    });
+    doc.children.push({ name: 'Child 1' });
+    doc.children.push({ name: 'Child 2' });
+    doc.children.push({ name: 'Child 3' });
+    await doc.save();
+    assert.deepStrictEqual(called, {
+      preSave: 3,
+      postSave: 3,
+      preDeleteOne: 0,
+      postDeleteOne: 0
+    });
+    const child2 = doc.children[1];
+    child2.deleteOne();
+    await doc.save();
+    assert.deepStrictEqual(called, {
+      preSave: 5,
+      postSave: 5,
+      preDeleteOne: 1,
+      postDeleteOne: 1
+    });
+
+    postDeleteOneError = new Error('Test error in post deleteOne hook');
+    const child3 = doc.children[1];
+    child3.deleteOne();
+    await assert.rejects(
+      () => doc.save(),
+      /Test error in post deleteOne hook/
+    );
+  });
 });
 
 describe('Check if instance function that is supplied in schema option is availabe', function() {