Merge pull request #13660 from Automattic/IslandRhythms/gh-13369

BREAKING CHANGE: make model.prototype.deleteOne() return query, not promise

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', 'deleteOne']) {
+  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);

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');
@@ -1012,51 +1013,36 @@ Model.prototype.deleteOne = async function deleteOne(options) {
     this.$session(options.session);
   }
 
-  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 self = this;
   const where = this.$__where();
-  if (where instanceof MongooseError) {
-    return cb(where);
+  if (where instanceof Error) {
+    throw where;
   }
-
-  _applyCustomWhere(this, where);
-
-  const session = this.$session();
-  if (!options.hasOwnProperty('session')) {
-    options.session = session;
-  }
-
-  this[modelCollectionSymbol].deleteOne(where, options).then(
-    () => {
-      this.$__.isDeleted = true;
-      this.$emit('deleteOne', this);
-      this.constructor.emit('deleteOne', this);
-      return cb(null, this);
-    },
-    err => {
-      this.$__.isDeleted = false;
-      cb(err);
+  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);
+  });
+
+  return query;
 };
 
 /**

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');

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);

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

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);
   });

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() {