From 3c0cb464229394a383956d5d9dfdb1bb19f34dc0 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 7 Jul 2024 12:16:38 -0400 Subject: [PATCH 1/3] fix(document): ensure post('deleteOne') hooks are called when calling `save()` after `subdoc.deleteOne()` Fix #9885 --- lib/plugins/saveSubdocs.js | 21 ++++++++++++++ lib/types/subdocument.js | 7 +++-- test/document.test.js | 56 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 82 insertions(+), 2 deletions(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index 758acbbfe2e..ac46905aff6 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -36,6 +36,27 @@ module.exports = function saveSubdocs(schema) { }); }, null, unshift); + schema.s.hooks.post('save', function saveSubdocsPostDeleteOne(doc, next) { + const removedSubdocs = this.$__.removedSubdocs; + if (!removedSubdocs || !removedSubdocs.length) { + return next(); + } + + each(removedSubdocs, function(subdoc, cb) { + subdoc.$__schema.s.hooks.execPost('deleteOne', subdoc, [subdoc], function(err) { + cb(err); + }); + }, (error) => { + this.$__.removedSubdocs = null; + if (error) { + return this.$__schema.s.hooks.execPost('deleteOne:error', this, [this], { error: error }, function(error) { + next(error); + }); + } + next(); + }); + }); + schema.s.hooks.post('save', function saveSubdocsPostSave(doc, next) { if (this.$isSubdocument) { next(); diff --git a/lib/types/subdocument.js b/lib/types/subdocument.js index 014babe6caf..b1984d08ebf 100644 --- 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 index a160f2edb62..ba15804075c 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -13535,6 +13535,62 @@ 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 + }; + 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(); + }); + 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 + }); + }); }); describe('Check if instance function that is supplied in schema option is availabe', function() { From 5f55a30197c403a2b720337733356dbedd170c82 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 7 Jul 2024 19:16:58 -0400 Subject: [PATCH 2/3] fix: improve error handling and refactor to use async/await --- lib/plugins/saveSubdocs.js | 29 +++++++++++++++-------------- test/document.test.js | 11 ++++++++++- 2 files changed, 25 insertions(+), 15 deletions(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index ac46905aff6..52ea5a82e26 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -36,25 +36,26 @@ module.exports = function saveSubdocs(schema) { }); }, null, unshift); - schema.s.hooks.post('save', function saveSubdocsPostDeleteOne(doc, next) { + schema.s.hooks.post('save', async function saveSubdocsPostDeleteOne() { const removedSubdocs = this.$__.removedSubdocs; if (!removedSubdocs || !removedSubdocs.length) { - return next(); + return; } - each(removedSubdocs, function(subdoc, cb) { - subdoc.$__schema.s.hooks.execPost('deleteOne', subdoc, [subdoc], function(err) { - cb(err); - }); - }, (error) => { - this.$__.removedSubdocs = null; - if (error) { - return this.$__schema.s.hooks.execPost('deleteOne:error', this, [this], { error: error }, function(error) { - next(error); + 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(); }); - } - next(); - }); + })); + } + + this.$__.removedSubdocs = null; + await Promise.all(promises); }); schema.s.hooks.post('save', function saveSubdocsPostSave(doc, next) { diff --git a/test/document.test.js b/test/document.test.js index ba15804075c..b4349732250 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -13544,6 +13544,7 @@ describe('document', function() { preDeleteOne: 0, postDeleteOne: 0 }; + let postDeleteOneError = null; ChildSchema.pre('save', function(next) { ++called.preSave; next(); @@ -13558,7 +13559,7 @@ describe('document', function() { }); ChildSchema.post('deleteOne', { document: true, query: false }, function(subdoc, next) { ++called.postDeleteOne; - next(); + next(postDeleteOneError); }); const ParentSchema = new Schema({ name: String, children: [ChildSchema] }); const ParentModel = db.model('Parent', ParentSchema); @@ -13590,6 +13591,14 @@ describe('document', function() { 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/ + ); }); }); From 50f1a7385a840b47c0b1bc5ad8b7764cd113d0f1 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Sun, 7 Jul 2024 19:49:52 -0400 Subject: [PATCH 3/3] refactor: use async/await for subdoc post save --- lib/plugins/saveSubdocs.js | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index 52ea5a82e26..4b47bd73320 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -58,9 +58,8 @@ module.exports = function saveSubdocs(schema) { await Promise.all(promises); }); - schema.s.hooks.post('save', function saveSubdocsPostSave(doc, next) { + schema.s.hooks.post('save', async function saveSubdocsPostSave() { if (this.$isSubdocument) { - next(); return; } @@ -68,21 +67,32 @@ module.exports = function saveSubdocs(schema) { 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); };