Merge pull request #15599 from Automattic/vkarpov15/gh-11531

BREAKING CHANGE: remove support for callbacks in pre middleware

Author: vkarpov15

docs/middleware.md

@@ -128,19 +128,17 @@ childSchema.pre('findOneAndUpdate', function() {
 
 ## Pre {#pre}
 
-Pre middleware functions are executed one after another, when each
-middleware calls `next`.
+Pre middleware functions are executed one after another.
 
 ```javascript
 const schema = new Schema({ /* ... */ });
-schema.pre('save', function(next) {
+schema.pre('save', function() {
   // do stuff
-  next();
 });
 ```
 
-In [mongoose 5.x](http://thecodebarbarian.com/introducing-mongoose-5.html#promises-and-async-await-with-middleware), instead of calling `next()` manually, you can use a
-function that returns a promise. In particular, you can use [`async/await`](http://thecodebarbarian.com/common-async-await-design-patterns-in-node.js.html).
+You can also use a function that returns a promise, including async functions.
+Mongoose will wait until the promise resolves to move on to the next middleware.
 
 ```javascript
 schema.pre('save', function() {
@@ -153,33 +151,22 @@ schema.pre('save', async function() {
   await doStuff();
   await doMoreStuff();
 });
-```
-
-If you use `next()`, the `next()` call does **not** stop the rest of the code in your middleware function from executing. Use
-[the early `return` pattern](https://www.bennadel.com/blog/2323-use-a-return-statement-when-invoking-callbacks-especially-in-a-guard-statement.htm)
-to prevent the rest of your middleware function from running when you call `next()`.
 
-```javascript
-const schema = new Schema({ /* ... */ });
-schema.pre('save', function(next) {
-  if (foo()) {
-    console.log('calling next!');
-    // `return next();` will make sure the rest of this function doesn't run
-    /* return */ next();
-  }
-  // Unless you comment out the `return` above, 'after next' will print
-  console.log('after next');
+schema.pre('save', function() {
+  // Will execute **after** `await doMoreStuff()` is done
 });
 ```
 
 ### Use Cases
 
-Middleware are useful for atomizing model logic. Here are some other ideas:
+Middleware is useful for atomizing model logic. Here are some other ideas:
 
 * complex validation
 * removing dependent documents (removing a user removes all their blogposts)
 * asynchronous defaults
 * asynchronous tasks that a certain action triggers
+* updating denormalized data on other documents
+* saving change records
 
 ### Errors in Pre Hooks {#error-handling}
 
@@ -189,11 +176,9 @@ and/or reject the returned promise. There are several ways to report an
 error in middleware:
 
 ```javascript
-schema.pre('save', function(next) {
+schema.pre('save', function() {
   const err = new Error('something went wrong');
-  // If you call `next()` with an argument, that argument is assumed to be
-  // an error.
-  next(err);
+  throw err;
 });
 
 schema.pre('save', function() {
@@ -222,9 +207,6 @@ myDoc.save(function(err) {
 });
 ```
 
-Calling `next()` multiple times is a no-op. If you call `next()` with an
-error `err1` and then throw an error `err2`, mongoose will report `err1`.
-
 ## Post middleware {#post}
 
 [post](api.html#schema_Schema-post) middleware are executed *after*
@@ -373,16 +355,13 @@ const User = mongoose.model('User', userSchema);
 await User.findOneAndUpdate({ name: 'John' }, { $set: { age: 30 } });
 ```
 
-For document middleware, like `pre('save')`, Mongoose passes the 1st parameter to `save()` as the 2nd argument to your `pre('save')` callback.
-You should use the 2nd argument to get access to the `save()` call's `options`, because Mongoose documents don't store all the options you can pass to `save()`.
+Mongoose also passes the 1st parameter to the hooked function, like `save()`, as the 1st argument to your `pre('save')` function.
+You should use the argument to get access to the `save()` call's `options`, because Mongoose documents don't store all the options you can pass to `save()`.
 
 ```javascript
 const userSchema = new Schema({ name: String, age: Number });
-userSchema.pre('save', function(next, options) {
+userSchema.pre('save', function(options) {
   options.validateModifiedOnly; // true
-
-  // Remember to call `next()` unless you're using an async function or returning a promise
-  next();
 });
 const User = mongoose.model('User', userSchema);
 
@@ -513,10 +492,9 @@ await Model.updateOne({}, { $set: { name: 'test' } });
 
 ## Error Handling Middleware {#error-handling-middleware}
 
-Middleware execution normally stops the first time a piece of middleware
-calls `next()` with an error. However, there is a special kind of post
-middleware called "error handling middleware" that executes specifically
-when an error occurs. Error handling middleware is useful for reporting
+Middleware execution normally stops the first time a piece of middleware throws an error, or returns a promise that rejects.
+However, there is a special kind of post middleware called "error handling middleware" that executes specifically when an error occurs.
+Error handling middleware is useful for reporting
 errors and making error messages more readable.
 
 Error handling middleware is defined as middleware that takes one extra
@@ -553,13 +531,13 @@ errors.
 
 ```javascript
 // The same E11000 error can occur when you call `updateOne()`
-// This function **must** take 4 parameters.
+// This function **must** take exactly 3 parameters.
 
-schema.post('updateOne', function(passRawResult, error, res, next) {
+schema.post('updateOne', function(error, res, next) {
   if (error.name === 'MongoServerError' && error.code === 11000) {
-    next(new Error('There was a duplicate key error'));
+    throw new Error('There was a duplicate key error');
   } else {
-    next(); // The `updateOne()` call will still error out.
+    next();
   }
 });
 
@@ -570,9 +548,8 @@ await Person.create(people);
 await Person.updateOne({ name: 'Slash' }, { $set: { name: 'Axl Rose' } });
 ```
 
-Error handling middleware can transform an error, but it can't remove the
-error. Even if you call `next()` with no error as shown above, the
-function call will still error out.
+Error handling middleware can transform an error, but it can't remove the error.
+Even if the error handling middleware succeeds, the function call will still error out.
 
 ## Aggregation Hooks {#aggregate}
 
@@ -598,10 +575,9 @@ pipeline from middleware.
 
 ## Synchronous Hooks {#synchronous}
 
-Certain Mongoose hooks are synchronous, which means they do **not** support
-functions that return promises or receive a `next()` callback. Currently,
-only `init` hooks are synchronous, because the [`init()` function](api/document.html#document_Document-init)
-is synchronous. Below is an example of using pre and post init hooks.
+Certain Mongoose hooks are synchronous, which means they do **not** support functions that return promises.
+Currently, only `init` hooks are synchronous, because the [`init()` function](api/document.html#document_Document-init) is synchronous.
+Below is an example of using pre and post init hooks.
 
 ```acquit
 [require:post init hooks.*success]

docs/migrating_to_9.md

@@ -185,12 +185,14 @@ const { promiseOrCallback } = require('mongoose');
 promiseOrCallback; // undefined in Mongoose 9
 ```
 
-## In `isAsync` middleware `next()` errors take priority over `done()` errors
+## `isAsync` middleware no longer supported
 
-Due to Mongoose middleware now relying on promises and async/await, `next()` errors take priority over `done()` errors.
-If you use `isAsync` middleware, any errors in `next()` will be thrown first, and `done()` errors will only be thrown if there are no `next()` errors.
+Mongoose 9 no longer supports `isAsync` middleware. Middleware functions that use the legacy signature with both `next` and `done` callbacks (i.e., `function(next, done)`) are not supported. We recommend middleware now use promises or async/await.
+
+If you have code that uses `isAsync` middleware, you must refactor it to use async functions or return a promise instead.
 
 ```javascript
+// ❌ Not supported in Mongoose 9
 const schema = new Schema({});
 
 schema.pre('save', true, function(next, done) {
@@ -214,8 +216,16 @@ schema.pre('save', true, function(next, done) {
     25);
 });
 
-// In Mongoose 8, with the above middleware, `save()` would error with 'first done() error'
-// In Mongoose 9, with the above middleware, `save()` will error with 'second next() error'
+// ✅ Supported in Mongoose 9: use async functions or return a promise
+schema.pre('save', async function() {
+  execed.first = true;
+  await new Promise(resolve => setTimeout(resolve, 5));
+});
+
+schema.pre('save', async function() {
+  execed.second = true;
+  await new Promise(resolve => setTimeout(resolve, 25));
+});
 ```
 
 ## Removed `skipOriginalStackTraces` option

lib/helpers/timestamps/setupTimestamps.js

@@ -42,15 +42,13 @@ module.exports = function setupTimestamps(schema, timestamps) {
 
   schema.add(schemaAdditions);
 
-  schema.pre('save', function timestampsPreSave(next) {
+  schema.pre('save', function timestampsPreSave() {
     const timestampOption = get(this, '$__.saveOptions.timestamps');
     if (timestampOption === false) {
-      return next();
+      return;
     }
 
     setDocumentTimestamps(this, timestampOption, currentTime, createdAt, updatedAt);
-
-    next();
   });
 
   schema.methods.initializeTimestamps = function() {
@@ -88,7 +86,7 @@ module.exports = function setupTimestamps(schema, timestamps) {
   schema.pre('updateOne', opts, _setTimestampsOnUpdate);
   schema.pre('updateMany', opts, _setTimestampsOnUpdate);
 
-  function _setTimestampsOnUpdate(next) {
+  function _setTimestampsOnUpdate() {
     const now = currentTime != null ?
       currentTime() :
       this.model.base.now();
@@ -105,6 +103,5 @@ module.exports = function setupTimestamps(schema, timestamps) {
       replaceOps.has(this.op)
     );
     applyTimestampsToChildren(now, this.getUpdate(), this.model.schema);
-    next();
   }
 };

lib/model.js

@@ -2914,7 +2914,6 @@ Model.insertMany = async function insertMany(arr, options) {
     await this._middleware.execPost('insertMany', this, [arr], { error });
   }
 
-
   options = options || {};
   const ThisModel = this;
   const limit = options.limit || 1000;

lib/plugins/validateBeforeSave.js

@@ -6,7 +6,7 @@
 
 module.exports = function validateBeforeSave(schema) {
   const unshift = true;
-  schema.pre('save', false, async function validateBeforeSave(_next, options) {
+  schema.pre('save', false, async function validateBeforeSave(options) {
     // Nested docs have their own presave
     if (this.$isSubdocument) {
       return;

lib/schema.js

@@ -2081,23 +2081,21 @@ Schema.prototype.queue = function(name, args) {
  *
  *     const toySchema = new Schema({ name: String, created: Date });
  *
- *     toySchema.pre('save', function(next) {
+ *     toySchema.pre('save', function() {
  *       if (!this.created) this.created = new Date;
- *       next();
  *     });
  *
- *     toySchema.pre('validate', function(next) {
+ *     toySchema.pre('validate', function() {
  *       if (this.name !== 'Woody') this.name = 'Woody';
- *       next();
  *     });
  *
  *     // Equivalent to calling `pre()` on `find`, `findOne`, `findOneAndUpdate`.
- *     toySchema.pre(/^find/, function(next) {
+ *     toySchema.pre(/^find/, function() {
  *       console.log(this.getFilter());
  *     });
  *
  *     // Equivalent to calling `pre()` on `updateOne`, `findOneAndUpdate`.
- *     toySchema.pre(['updateOne', 'findOneAndUpdate'], function(next) {
+ *     toySchema.pre(['updateOne', 'findOneAndUpdate'], function() {
  *       console.log(this.getFilter());
  *     });
  *

package.json

@@ -20,7 +20,7 @@
   "type": "commonjs",
   "license": "MIT",
   "dependencies": {
-    "kareem": "git+https://github.com/mongoosejs/kareem.git#vkarpov15/v3",
+    "kareem": "git+https://github.com/mongoosejs/kareem.git#vkarpov15/remove-isasync",
     "mongodb": "~6.18.0",
     "mpath": "0.9.0",
     "mquery": "5.0.0",

test/aggregate.test.js

@@ -886,9 +886,9 @@ describe('aggregate: ', function() {
         const s = new Schema({ name: String });
 
         let called = 0;
-        s.pre('aggregate', function(next) {
+        s.pre('aggregate', function() {
           ++called;
-          next();
+          return Promise.resolve();
         });
 
         const M = db.model('Test', s);
@@ -902,9 +902,9 @@ describe('aggregate: ', function() {
       it('setting option in pre (gh-7606)', async function() {
         const s = new Schema({ name: String });
 
-        s.pre('aggregate', function(next) {
+        s.pre('aggregate', function() {
           this.options.collation = { locale: 'en_US', strength: 1 };
-          next();
+          return Promise.resolve();
         });
 
         const M = db.model('Test', s);
@@ -920,9 +920,9 @@ describe('aggregate: ', function() {
       it('adding to pipeline in pre (gh-8017)', async function() {
         const s = new Schema({ name: String });
 
-        s.pre('aggregate', function(next) {
+        s.pre('aggregate', function() {
           this.append({ $limit: 1 });
-          next();
+          return Promise.resolve();
         });
 
         const M = db.model('Test', s);
@@ -980,8 +980,8 @@ describe('aggregate: ', function() {
         const s = new Schema({ name: String });
 
         const calledWith = [];
-        s.pre('aggregate', function(next) {
-          next(new Error('woops'));
+        s.pre('aggregate', function() {
+          throw new Error('woops');
         });
         s.post('aggregate', function(error, res, next) {
           calledWith.push(error);
@@ -1003,9 +1003,9 @@ describe('aggregate: ', function() {
 
         let calledPre = 0;
         let calledPost = 0;
-        s.pre('aggregate', function(next) {
+        s.pre('aggregate', function() {
           ++calledPre;
-          next();
+          return Promise.resolve();
         });
         s.post('aggregate', function(res, next) {
           ++calledPost;
@@ -1030,9 +1030,9 @@ describe('aggregate: ', function() {
 
         let calledPre = 0;
         const calledPost = [];
-        s.pre('aggregate', function(next) {
+        s.pre('aggregate', function() {
           ++calledPre;
-          next();
+          return Promise.resolve();
         });
         s.post('aggregate', function(res, next) {
           calledPost.push(res);
@@ -1295,11 +1295,10 @@ describe('aggregate: ', function() {
   it('cursor() errors out if schema pre aggregate hook throws an error (gh-15279)', async function() {
     const schema = new Schema({ name: String });
 
-    schema.pre('aggregate', function(next) {
+    schema.pre('aggregate', function() {
       if (!this.options.allowed) {
         throw new Error('Unauthorized aggregate operation: only allowed operations are permitted');
       }
-      next();
     });
 
     const Test = db.model('Test', schema);

test/docs/discriminators.test.js

@@ -164,17 +164,15 @@ describe('discriminator docs', function() {
 
     const eventSchema = new mongoose.Schema({ time: Date }, options);
     let eventSchemaCalls = 0;
-    eventSchema.pre('validate', function(next) {
+    eventSchema.pre('validate', function() {
       ++eventSchemaCalls;
-      next();
     });
     const Event = mongoose.model('GenericEvent', eventSchema);
 
     const clickedLinkSchema = new mongoose.Schema({ url: String }, options);
     let clickedSchemaCalls = 0;
-    clickedLinkSchema.pre('validate', function(next) {
+    clickedLinkSchema.pre('validate', function() {
       ++clickedSchemaCalls;
-      next();
     });
     const ClickedLinkEvent = Event.discriminator('ClickedLinkEvent',
       clickedLinkSchema);