Merge pull request #15324 from Automattic/vkarpov15/async-savesubdocs

BREAKING CHANGE: use kareem@v3 promise-based execPre() for document hooks

Author: vkarpov15

docs/migrating_to_9.md

@@ -36,3 +36,34 @@ try {
   // Handle validation error
 }
 ```
+
+## Errors in middleware functions take priority over `next()` calls
+
+In Mongoose 8.x, if a middleware function threw an error after calling `next()`, that error would be ignored.
+
+```javascript
+schema.pre('save', function(next) {
+  next();
+  // In Mongoose 8, this error will not get reported, because you already called next()
+  throw new Error('woops!');
+});
+```
+
+In Mongoose 9, errors in the middleware function take priority, so the above `save()` would throw an error.
+
+## `next()` no longer supports passing arguments to the next middleware
+
+Previously, you could call `next(null, 'new arg')` in a hook and the args to the next middleware would get overwritten by 'new arg'.
+
+```javascript
+schema.pre('save', function(next, options) {
+  options; // options passed to `save()`
+  next(null, 'new arg');
+});
+
+schema.pre('save', function(next, arg) {
+  arg; // In Mongoose 8, this would be 'new arg', overwrote the options passed to `save()`
+});
+```
+
+In Mongoose 9, `next(null, 'new arg')` doesn't overwrite the args to the next middleware.

lib/aggregate.js

@@ -800,18 +800,19 @@ Aggregate.prototype.explain = async function explain(verbosity) {
 
   prepareDiscriminatorPipeline(this._pipeline, this._model.schema);
 
-  await new Promise((resolve, reject) => {
-    model.hooks.execPre('aggregate', this, error => {
-      if (error) {
-        const _opts = { error: error };
-        return model.hooks.execPost('aggregate', this, [null], _opts, error => {
-          reject(error);
-        });
-      } else {
+  try {
+    await model.hooks.execPre('aggregate', this);
+  } catch (error) {
+    const _opts = { error: error };
+    return await new Promise((resolve, reject) => {
+      model.hooks.execPost('aggregate', this, [null], _opts, error => {
+        if (error) {
+          return reject(error);
+        }
         resolve();
-      }
+      });
     });
-  });
+  }
 
   const cursor = model.collection.aggregate(this._pipeline, this.options);
 
@@ -1079,18 +1080,19 @@ Aggregate.prototype.exec = async function exec() {
   prepareDiscriminatorPipeline(this._pipeline, this._model.schema);
   stringifyFunctionOperators(this._pipeline);
 
-  await new Promise((resolve, reject) => {
-    model.hooks.execPre('aggregate', this, error => {
-      if (error) {
-        const _opts = { error: error };
-        return model.hooks.execPost('aggregate', this, [null], _opts, error => {
-          reject(error);
-        });
-      } else {
+  try {
+    await model.hooks.execPre('aggregate', this);
+  } catch (error) {
+    const _opts = { error: error };
+    return await new Promise((resolve, reject) => {
+      model.hooks.execPost('aggregate', this, [null], _opts, error => {
+        if (error) {
+          return reject(error);
+        }
         resolve();
-      }
+      });
     });
-  });
+  }
 
   if (!this._pipeline.length) {
     throw new MongooseError('Aggregate has empty pipeline');

lib/browserDocument.js

@@ -97,16 +97,8 @@ Document.$emitter = new EventEmitter();
  * ignore
  */
 
-Document.prototype._execDocumentPreHooks = function _execDocumentPreHooks(opName) {
-  return new Promise((resolve, reject) => {
-    this._middleware.execPre(opName, this, [], (error) => {
-      if (error != null) {
-        reject(error);
-        return;
-      }
-      resolve();
-    });
-  });
+Document.prototype._execDocumentPreHooks = async function _execDocumentPreHooks(opName) {
+  return this._middleware.execPre(opName, this, []);
 };
 
 /*!

lib/cursor/aggregationCursor.js

@@ -63,34 +63,24 @@ util.inherits(AggregationCursor, Readable);
 
 function _init(model, c, agg) {
   if (!model.collection.buffer) {
-    model.hooks.execPre('aggregate', agg, function(err) {
-      if (err != null) {
-        _handlePreHookError(c, err);
-        return;
-      }
-      if (typeof agg.options?.cursor?.transform === 'function') {
-        c._transforms.push(agg.options.cursor.transform);
-      }
-
-      c.cursor = model.collection.aggregate(agg._pipeline, agg.options || {});
-      c.emit('cursor', c.cursor);
-    });
+    model.hooks.execPre('aggregate', agg).then(() => onPreComplete(null), err => onPreComplete(err));
   } else {
     model.collection.emitter.once('queue', function() {
-      model.hooks.execPre('aggregate', agg, function(err) {
-        if (err != null) {
-          _handlePreHookError(c, err);
-          return;
-        }
+      model.hooks.execPre('aggregate', agg).then(() => onPreComplete(null), err => onPreComplete(err));
+    });
+  }
 
-        if (typeof agg.options?.cursor?.transform === 'function') {
-          c._transforms.push(agg.options.cursor.transform);
-        }
+  function onPreComplete(err) {
+    if (err != null) {
+      _handlePreHookError(c, err);
+      return;
+    }
+    if (typeof agg.options?.cursor?.transform === 'function') {
+      c._transforms.push(agg.options.cursor.transform);
+    }
 
-        c.cursor = model.collection.aggregate(agg._pipeline, agg.options || {});
-        c.emit('cursor', c.cursor);
-      });
-    });
+    c.cursor = model.collection.aggregate(agg._pipeline, agg.options || {});
+    c.emit('cursor', c.cursor);
   }
 }
 

lib/cursor/queryCursor.js

@@ -49,7 +49,8 @@ function QueryCursor(query) {
   this._transforms = [];
   this.model = model;
   this.options = {};
-  model.hooks.execPre('find', query, (err) => {
+
+  const onPreComplete = (err) => {
     if (err != null) {
       if (err instanceof kareem.skipWrappedFunction) {
         const resultValue = err.args[0];
@@ -91,7 +92,9 @@ function QueryCursor(query) {
     } else {
       _getRawCursor(query, this);
     }
-  });
+  };
+
+  model.hooks.execPre('find', query).then(() => onPreComplete(null), err => onPreComplete(err));
 }
 
 util.inherits(QueryCursor, Readable);

lib/document.js

@@ -847,8 +847,8 @@ function init(self, obj, doc, opts, prefix) {
 Document.prototype.updateOne = function updateOne(doc, options, callback) {
   const query = this.constructor.updateOne({ _id: this._doc._id }, doc, options);
   const self = this;
-  query.pre(function queryPreUpdateOne(cb) {
-    self.constructor._middleware.execPre('updateOne', self, [self], cb);
+  query.pre(function queryPreUpdateOne() {
+    return self.constructor._middleware.execPre('updateOne', self, [self]);
   });
   query.post(function queryPostUpdateOne(cb) {
     self.constructor._middleware.execPost('updateOne', self, [self], {}, cb);
@@ -2880,16 +2880,8 @@ function _pushNestedArrayPaths(val, paths, path) {
  * ignore
  */
 
-Document.prototype._execDocumentPreHooks = function _execDocumentPreHooks(opName, ...args) {
-  return new Promise((resolve, reject) => {
-    this.constructor._middleware.execPre(opName, this, [...args], (error) => {
-      if (error != null) {
-        reject(error);
-        return;
-      }
-      resolve();
-    });
-  });
+Document.prototype._execDocumentPreHooks = async function _execDocumentPreHooks(opName, ...args) {
+  return this.constructor._middleware.execPre(opName, this, [...args]);
 };
 
 /*!

lib/helpers/model/applyHooks.js

@@ -51,25 +51,11 @@ function applyHooks(model, schema, options) {
   for (const key of Object.keys(schema.paths)) {
     let type = schema.paths[key];
     let childModel = null;
-    if (type.$isSingleNested) {
-      childModel = type.caster;
-    } else if (type.$isMongooseDocumentArray) {
-      childModel = type.Constructor;
-    } else if (type.instance === 'Array') {
-      let curType = type;
-      // Drill into nested arrays to check if nested array contains document array
-      while (curType.instance === 'Array') {
-        if (curType.$isMongooseDocumentArray) {
-          childModel = curType.Constructor;
-          type = curType;
-          break;
-        }
-        curType = curType.getEmbeddedSchemaType();
-      }
 
-      if (childModel == null) {
-        continue;
-      }
+    const result = findChildModel(type);
+    if (result) {
+      childModel = result.childModel;
+      type = result.type;
     } else {
       continue;
     }
@@ -164,3 +150,33 @@ function applyHooks(model, schema, options) {
       createWrapper(method, originalMethod, null, customMethodOptions);
   }
 }
+
+/**
+ * Check if there is an embedded schematype in the given schematype. Handles drilling down into primitive
+ * arrays and maps in case of array of array of subdocs or map of subdocs.
+ *
+ * @param {SchemaType} curType
+ * @returns {{ childModel: Model | typeof Subdocument, curType: SchemaType } | null}
+ */
+
+function findChildModel(curType) {
+  if (curType.$isSingleNested) {
+    return { childModel: curType.caster, type: curType };
+  }
+  if (curType.$isMongooseDocumentArray) {
+    return { childModel: curType.Constructor, type: curType };
+  }
+  if (curType.instance === 'Array') {
+    const embedded = curType.getEmbeddedSchemaType();
+    if (embedded) {
+      return findChildModel(embedded);
+    }
+  }
+  if (curType.instance === 'Map') {
+    const mapType = curType.getEmbeddedSchemaType();
+    if (mapType) {
+      return findChildModel(mapType);
+    }
+  }
+  return null;
+}

lib/helpers/model/applyStaticHooks.js

@@ -42,10 +42,10 @@ module.exports = function applyStaticHooks(model, hooks, statics) {
         const cb = typeof lastArg === 'function' ? lastArg : null;
         const args = Array.prototype.slice.
           call(arguments, 0, cb == null ? numArgs : numArgs - 1);
-        // Special case: can't use `Kareem#wrap()` because it doesn't currently
-        // support wrapped functions that return a promise.
         return promiseOrCallback(cb, callback => {
-          hooks.execPre(key, model, args, function(err) {
+          hooks.execPre(key, model, args).then(() => onPreComplete(null), err => onPreComplete(err));
+
+          function onPreComplete(err) {
             if (err != null) {
               return callback(err);
             }
@@ -72,7 +72,7 @@ module.exports = function applyStaticHooks(model, hooks, statics) {
                 callback(null, res);
               });
             }
-          });
+          }
         }, model.events);
       };
     }

lib/model.js

@@ -810,19 +810,16 @@ Model.prototype.deleteOne = function deleteOne(options) {
     }
   }
 
-  query.pre(function queryPreDeleteOne(cb) {
-    self.constructor._middleware.execPre('deleteOne', self, [self], cb);
+  query.pre(function queryPreDeleteOne() {
+    return self.constructor._middleware.execPre('deleteOne', self, [self]);
   });
-  query.pre(function callSubdocPreHooks(cb) {
-    each(self.$getAllSubdocs(), (subdoc, cb) => {
-      subdoc.constructor._middleware.execPre('deleteOne', subdoc, [subdoc], cb);
-    }, cb);
+  query.pre(function callSubdocPreHooks() {
+    return Promise.all(self.$getAllSubdocs().map(subdoc => subdoc.constructor._middleware.execPre('deleteOne', subdoc, [subdoc])));
   });
-  query.pre(function skipIfAlreadyDeleted(cb) {
+  query.pre(function skipIfAlreadyDeleted() {
     if (self.$__.isDeleted) {
-      return cb(Kareem.skipWrappedFunction());
+      throw new Kareem.skipWrappedFunction();
     }
-    return cb();
   });
   query.post(function callSubdocPostHooks(cb) {
     each(self.$getAllSubdocs(), (subdoc, cb) => {
@@ -1185,16 +1182,11 @@ Model.createCollection = async function createCollection(options) {
     throw new MongooseError('Model.createCollection() no longer accepts a callback');
   }
 
-  const shouldSkip = await new Promise((resolve, reject) => {
-    this.hooks.execPre('createCollection', this, [options], (err) => {
-      if (err != null) {
-        if (err instanceof Kareem.skipWrappedFunction) {
-          return resolve(true);
-        }
-        return reject(err);
-      }
-      resolve();
-    });
+  const shouldSkip = await this.hooks.execPre('createCollection', this, [options]).catch(err => {
+    if (err instanceof Kareem.skipWrappedFunction) {
+      return true;
+    }
+    throw err;
   });
 
   const collectionOptions = this &&
@@ -3380,17 +3372,13 @@ Model.bulkWrite = async function bulkWrite(ops, options) {
   }
   options = options || {};
 
-  const shouldSkip = await new Promise((resolve, reject) => {
-    this.hooks.execPre('bulkWrite', this, [ops, options], (err) => {
-      if (err != null) {
-        if (err instanceof Kareem.skipWrappedFunction) {
-          return resolve(err);
-        }
-        return reject(err);
-      }
-      resolve();
-    });
-  });
+  const shouldSkip = await this.hooks.execPre('bulkWrite', this, [ops, options]).catch(err => {
+    if (err instanceof Kareem.skipWrappedFunction) {
+      return err;
+    }
+    throw err;
+  }
+  );
 
   if (shouldSkip) {
     return shouldSkip.args[0];
@@ -3621,16 +3609,8 @@ Model.bulkSave = async function bulkSave(documents, options) {
   return bulkWriteResult;
 };
 
-function buildPreSavePromise(document, options) {
-  return new Promise((resolve, reject) => {
-    document.schema.s.hooks.execPre('save', document, [options], (err) => {
-      if (err) {
-        reject(err);
-        return;
-      }
-      resolve();
-    });
-  });
+async function buildPreSavePromise(document, options) {
+  return document.schema.s.hooks.execPre('save', document, [options]);
 }
 
 function handleSuccessfulWrite(document) {