From 61e90417a2b58f87285768abea55c09b3d2eb3d5 Mon Sep 17 00:00:00 2001 From: Daniel Diaz <39510674+IslandRhythms@users.noreply.github.com> Date: Mon, 17 Jul 2023 16:14:25 -0400 Subject: [PATCH 1/2] made the changes but some failing tests --- lib/helpers/model/applyHooks.js | 2 +- lib/model.js | 56 +++++++++------------------------ 2 files changed, 15 insertions(+), 43 deletions(-) diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 7ed7895d4b3..093b037b39a 100644 --- a/lib/helpers/model/applyHooks.js +++ b/lib/helpers/model/applyHooks.js @@ -104,7 +104,7 @@ function applyHooks(model, schema, options) { objToDecorate.$__originalValidate = objToDecorate.$__originalValidate || objToDecorate.$__validate; - for (const method of ['save', 'validate', 'remove', 'deleteOne']) { + for (const method of ['save', 'validate', 'remove']) { const toWrap = method === 'validate' ? '$__originalValidate' : `$__${method}`; const wrapped = middleware. createWrapper(method, objToDecorate[toWrap], null, kareemOptions); diff --git a/lib/model.js b/lib/model.js index 052414d2b76..72e59acf279 100644 --- a/lib/model.js +++ b/lib/model.js @@ -1009,53 +1009,25 @@ Model.prototype.deleteOne = async function deleteOne(options) { options = {}; } - if (options.hasOwnProperty('session')) { - this.$session(options.session); + if (!this._id) { + throw new Error('No _id found on document!') } - - const res = await new Promise((resolve, reject) => { - this.$__deleteOne(options, (err, res) => { - if (err != null) { - return reject(err); - } - resolve(res); - }); - }); - - return res; -}; - -/*! - * ignore - */ - -Model.prototype.$__deleteOne = function $__deleteOne(options, cb) { if (this.$__.isDeleted) { - return immediate(() => cb(null, this)); - } - - const where = this.$__where(); - if (where instanceof MongooseError) { - return cb(where); + return; } + const self = this; + const query = self.constructor.deleteOne(options); + query.pre(function queryPreDeleteOne(cb) { + self.constructor._middleware.execPre('deleteOne', self, [self], cb); + }); + query.post(function queryPostDeleteOne(cb) { + self.constructor._middleware.execPost('deleteOne', self, [self], {}, cb); + }); - _applyCustomWhere(this, where); - - const session = this.$session(); - if (!options.hasOwnProperty('session')) { - options.session = session; + if (options.hasOwnProperty('session')) { + self.$session(options.session); } - - this[modelCollectionSymbol].deleteOne(where, options, err => { - if (!err) { - this.$__.isDeleted = true; - this.$emit('deleteOne', this); - this.constructor.emit('deleteOne', this); - return cb(null, this); - } - this.$__.isDeleted = false; - cb(err); - }); + return query; }; /** From 9fbed3269b1108718027ba05fa217ca34851e534 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 26 Jul 2023 16:13:11 -0400 Subject: [PATCH 2/2] BREAKING CHANGE: make model.prototype.deleteOne() return query, not promise Fix #13369 --- lib/helpers/model/applyHooks.js | 5 ++-- lib/model.js | 42 +++++++++++++++++++++++---------- lib/plugins/index.js | 1 - lib/plugins/removeSubdocs.js | 35 --------------------------- lib/query.js | 13 ++++++++-- test/document.test.js | 7 +++--- test/model.middleware.test.js | 4 ++-- test/model.test.js | 9 ++++--- 8 files changed, 55 insertions(+), 61 deletions(-) delete mode 100644 lib/plugins/removeSubdocs.js diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 093b037b39a..998da62f42a 100644 --- a/lib/helpers/model/applyHooks.js +++ b/lib/helpers/model/applyHooks.js @@ -64,7 +64,7 @@ function applyHooks(model, schema, options) { continue; } - applyHooks(childModel, type.schema, options); + applyHooks(childModel, type.schema, { ...options, isChildSchema: true }); if (childModel.discriminators != null) { const keys = Object.keys(childModel.discriminators); for (const key of keys) { @@ -104,7 +104,8 @@ function applyHooks(model, schema, options) { objToDecorate.$__originalValidate = objToDecorate.$__originalValidate || objToDecorate.$__validate; - for (const method of ['save', 'validate', 'remove']) { + const internalMethodsToWrap = options && options.isChildSchema ? ['save', 'validate', 'deleteOne'] : ['save', 'validate']; + for (const method of internalMethodsToWrap) { const toWrap = method === 'validate' ? '$__originalValidate' : `$__${method}`; const wrapped = middleware. createWrapper(method, objToDecorate[toWrap], null, kareemOptions); diff --git a/lib/model.js b/lib/model.js index c275bbb3007..350067ff1fb 100644 --- a/lib/model.js +++ b/lib/model.js @@ -10,6 +10,7 @@ const Document = require('./document'); const DocumentNotFoundError = require('./error/notFound'); const DivergentArrayError = require('./error/divergentArray'); const EventEmitter = require('events').EventEmitter; +const Kareem = require('kareem'); const MongooseBuffer = require('./types/buffer'); const MongooseError = require('./error/index'); const OverwriteModelError = require('./error/overwriteModel'); @@ -987,18 +988,18 @@ Model.prototype.$__where = function _where(where) { }; /** - * Removes this document from the db. Equivalent to `.remove()`. + * Delete this document from the db. * * #### Example: * - * product = await product.deleteOne(); + * await product.deleteOne(); * await Product.findById(product._id); // null * - * @return {Promise} Promise + * @return {Query} Query * @api public */ -Model.prototype.deleteOne = async function deleteOne(options) { +Model.prototype.deleteOne = function deleteOne(options) { if (typeof options === 'function' || typeof arguments[1] === 'function') { throw new MongooseError('Model.prototype.deleteOne() no longer accepts a callback'); @@ -1008,24 +1009,39 @@ Model.prototype.deleteOne = async function deleteOne(options) { options = {}; } - if (!this._id) { - throw new Error('No _id found on document!') - } - if (this.$__.isDeleted) { - return; + if (options.hasOwnProperty('session')) { + this.$session(options.session); } + const self = this; - const query = self.constructor.deleteOne(options); + const where = this.$__where(); + if (where instanceof Error) { + throw where; + } + const query = self.constructor.deleteOne(where, options); query.pre(function queryPreDeleteOne(cb) { self.constructor._middleware.execPre('deleteOne', self, [self], cb); }); + query.pre(function callSubdocPreHooks(cb) { + each(self.$getAllSubdocs(), (subdoc, cb) => { + subdoc.constructor._middleware.execPre('deleteOne', subdoc, [subdoc], cb); + }, cb); + }); + query.pre(function skipIfAlreadyDeleted(cb) { + if (self.$__.isDeleted) { + return cb(Kareem.skipWrappedFunction()); + } + return cb(); + }); + query.post(function callSubdocPostHooks(cb) { + each(self.$getAllSubdocs(), (subdoc, cb) => { + subdoc.constructor._middleware.execPost('deleteOne', subdoc, [subdoc], {}, cb); + }, cb); + }); query.post(function queryPostDeleteOne(cb) { self.constructor._middleware.execPost('deleteOne', self, [self], {}, cb); }); - if (options.hasOwnProperty('session')) { - self.$session(options.session); - } return query; }; diff --git a/lib/plugins/index.js b/lib/plugins/index.js index 69fa6ad284c..a8a6c044240 100644 --- a/lib/plugins/index.js +++ b/lib/plugins/index.js @@ -1,6 +1,5 @@ 'use strict'; -exports.removeSubdocs = require('./removeSubdocs'); exports.saveSubdocs = require('./saveSubdocs'); exports.sharding = require('./sharding'); exports.trackTransaction = require('./trackTransaction'); diff --git a/lib/plugins/removeSubdocs.js b/lib/plugins/removeSubdocs.js deleted file mode 100644 index 85c7de0ff20..00000000000 --- a/lib/plugins/removeSubdocs.js +++ /dev/null @@ -1,35 +0,0 @@ -'use strict'; - -const each = require('../helpers/each'); - -/*! - * ignore - */ - -module.exports = function removeSubdocs(schema) { - const unshift = true; - schema.s.hooks.pre('deleteOne', { document: true, query: false }, function removeSubDocsPreRemove(next) { - if (this.$isSubdocument) { - next(); - return; - } - if (this.$__ == null) { - next(); - return; - } - - const _this = this; - const subdocs = this.$getAllSubdocs(); - - each(subdocs, function(subdoc, cb) { - subdoc.$__deleteOne(cb); - }, function(error) { - if (error) { - return _this.$__schema.s.hooks.execPost('deleteOne:error', _this, [_this], { error: error }, function(error) { - next(error); - }); - } - next(); - }); - }, null, unshift); -}; diff --git a/lib/query.js b/lib/query.js index 756b19992f2..959bd9e3b0b 100644 --- a/lib/query.js +++ b/lib/query.js @@ -4336,14 +4336,23 @@ Query.prototype.exec = async function exec(op) { this._executionStack = new Error(); } - await _executePreExecHooks(this); + let skipWrappedFunction = null; + try { + await _executePreExecHooks(this); + } catch (err) { + if (err instanceof Kareem.skipWrappedFunction) { + skipWrappedFunction = err; + } else { + throw err; + } + } let res; let error = null; try { await _executePreHooks(this); - res = await this[thunk](); + res = skipWrappedFunction ? skipWrappedFunction.args[0] : await this[thunk](); for (const fn of this._transforms) { res = fn(res); diff --git a/test/document.test.js b/test/document.test.js index 4207ebec68a..789fa2e7f67 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -153,10 +153,11 @@ describe('document', function() { const test = new Test({ x: 'test' }); const doc = await test.save(); - await doc.deleteOne(); + const q = doc.deleteOne(); + assert.ok(q instanceof mongoose.Query, `Expected query, got ${q.constructor.name}`); + await q; const found = await Test.findOne({ _id: doc._id }); assert.strictEqual(found, null); - }); }); @@ -9819,7 +9820,7 @@ describe('document', function() { assert.ok(doc); }); - it('Makes sure pre remove hook is executed gh-9885', async function() { + it('Makes sure pre deleteOne hook is executed (gh-9885)', async function() { const SubSchema = new Schema({ myValue: { type: String diff --git a/test/model.middleware.test.js b/test/model.middleware.test.js index 645a6beb1d2..7747167bb4b 100644 --- a/test/model.middleware.test.js +++ b/test/model.middleware.test.js @@ -447,13 +447,13 @@ describe('model middleware', function() { await doc.deleteOne(); - assert.equal(queryPreCalled, 0); + assert.equal(queryPreCalled, 1); assert.equal(preCalled, 1); assert.equal(postCalled, 1); await Model.deleteOne(); - assert.equal(queryPreCalled, 1); + assert.equal(queryPreCalled, 2); assert.equal(preCalled, 1); assert.equal(postCalled, 1); }); diff --git a/test/model.test.js b/test/model.test.js index 7d362a6797c..64894faef78 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -1177,8 +1177,12 @@ describe('Model', function() { it('errors when id deselected (gh-3118)', async function() { await BlogPost.create({ title: 1 }, { title: 2 }); const doc = await BlogPost.findOne({ title: 1 }, { _id: 0 }); - const err = await doc.deleteOne().then(() => null, err => err); - assert.equal(err.message, 'No _id found on document!'); + try { + await doc.deleteOne(); + assert.ok(false); + } catch (err) { + assert.equal(err.message, 'No _id found on document!'); + } }); it('should not remove any records when deleting by id undefined', async function() { @@ -5458,7 +5462,6 @@ describe('Model', function() { await doc.deleteOne({ session }); assert.equal(sessions.length, 1); assert.strictEqual(sessions[0], session); - }); it('set $session() before pre validate hooks run on bulkWrite and insertMany (gh-7769)', async function() {