diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f078f0cf240..4552c62ed72 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -39,7 +39,7 @@ jobs: strategy: fail-fast: false matrix: - node: [16, 18, 20, 22] + node: [18, 20, 22] os: [ubuntu-22.04, ubuntu-24.04] mongodb: [6.0.15, 7.0.12, 8.0.0] include: diff --git a/docs/migrating_to_9.md b/docs/migrating_to_9.md index a55e923f32f..26a34b50df9 100644 --- a/docs/migrating_to_9.md +++ b/docs/migrating_to_9.md @@ -109,9 +109,101 @@ doc.mySubdoc[0].deleteOne(); await doc.save(); ``` +## Hooks for custom methods and statics no longer support callbacks + +Previously, you could use Mongoose middleware with custom methods and statics that took callbacks. +In Mongoose 9, this is no longer supported. +If you want to use Mongoose middleware with a custom method or static, that custom method or static must be an async function or return a Promise. + +```javascript +const mySchema = new Schema({ + name: String +}); + +// This is an example of a custom method that uses callbacks. While this method by itself still works in Mongoose 9, +// Mongoose 9 no longer supports hooks for this method. +mySchema.methods.foo = async function(cb) { + return cb(null, this.name); +}; +mySchema.statics.bar = async function(cb) { + return cb(null, 'bar'); +}; + +// This is no longer supported because `foo()` and `bar()` use callbacks. +mySchema.pre('foo', function() { + console.log('foo pre hook'); +}); +mySchema.pre('bar', function() { + console.log('bar pre hook'); +}); + +// The following code has a custom method and a custom static that use async functions. +// The following works correctly in Mongoose 9: `pre('bar')` is executed when you call `bar()` and +// `pre('qux')` is executed when you call `qux()`. +mySchema.methods.baz = async function baz(arg) { + return arg; +}; +mySchema.pre('baz', async function baz() { + console.log('baz pre hook'); +}); +mySchema.statics.qux = async function qux(arg) { + return arg; +}; +mySchema.pre('qux', async function qux() { + console.log('qux pre hook'); +}); +``` + +## Removed `promiseOrCallback` + +Mongoose 9 removed the `promiseOrCallback` helper function. + +```javascript +const { promiseOrCallback } = require('mongoose'); + +promiseOrCallback; // undefined in Mongoose 9 +``` + +## In `isAsync` middleware `next()` errors take priority over `done()` errors + +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. + +```javascript +const schema = new Schema({}); + +schema.pre('save', true, function(next, done) { + execed.first = true; + setTimeout( + function() { + done(new Error('first done() error')); + }, + 5); + + next(); +}); + +schema.pre('save', true, function(next, done) { + execed.second = true; + setTimeout( + function() { + next(new Error('second next() error')); + done(new Error('second done() error')); + }, + 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' +``` + ## Removed `skipOriginalStackTraces` option In Mongoose 8, Mongoose queries store an `_executionStack` property that stores the stack trace of where the query was originally executed for debugging `Query was already executed` errors. This behavior can cause performance issues with bundlers and source maps. `skipOriginalStackTraces` was added to work around this behavior. In Mongoose 9, this option is no longer necessary because Mongoose no longer stores the original stack trace. + +## Node.js version support + +Mongoose 9 requires Node.js 18 or higher. diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 08e42417f3c..95980adf204 100644 --- a/lib/helpers/model/applyHooks.js +++ b/lib/helpers/model/applyHooks.js @@ -1,7 +1,6 @@ 'use strict'; const symbols = require('../../schema/symbols'); -const promiseOrCallback = require('../promiseOrCallback'); /*! * ignore @@ -129,17 +128,8 @@ function applyHooks(model, schema, options) { continue; } const originalMethod = objToDecorate[method]; - objToDecorate[method] = function() { - const args = Array.prototype.slice.call(arguments); - const cb = args.slice(-1).pop(); - const argsWithoutCallback = typeof cb === 'function' ? - args.slice(0, args.length - 1) : args; - return promiseOrCallback(cb, callback => { - return this[`$__${method}`].apply(this, - argsWithoutCallback.concat([callback])); - }, model.events); - }; - objToDecorate[`$__${method}`] = middleware. + objToDecorate[`$__${method}`] = objToDecorate[method]; + objToDecorate[method] = middleware. createWrapper(method, originalMethod, null, customMethodOptions); } } diff --git a/lib/helpers/model/applyStaticHooks.js b/lib/helpers/model/applyStaticHooks.js index 46c8faaacbf..eb0caaff420 100644 --- a/lib/helpers/model/applyStaticHooks.js +++ b/lib/helpers/model/applyStaticHooks.js @@ -1,6 +1,5 @@ 'use strict'; -const promiseOrCallback = require('../promiseOrCallback'); const { queryMiddlewareFunctions, aggregateMiddlewareFunctions, modelMiddlewareFunctions, documentMiddlewareFunctions } = require('../../constants'); const middlewareFunctions = Array.from( @@ -28,40 +27,7 @@ module.exports = function applyStaticHooks(model, hooks, statics) { if (hooks.hasHooks(key)) { const original = model[key]; - model[key] = function() { - const numArgs = arguments.length; - const lastArg = numArgs > 0 ? arguments[numArgs - 1] : null; - const cb = typeof lastArg === 'function' ? lastArg : null; - const args = Array.prototype.slice. - call(arguments, 0, cb == null ? numArgs : numArgs - 1); - return promiseOrCallback(cb, callback => { - hooks.execPre(key, model, args).then(() => onPreComplete(null), err => onPreComplete(err)); - - function onPreComplete(err) { - if (err != null) { - return callback(err); - } - - let postCalled = 0; - const ret = original.apply(model, args.concat(post)); - if (ret != null && typeof ret.then === 'function') { - ret.then(res => post(null, res), err => post(err)); - } - - function post(error, res) { - if (postCalled++ > 0) { - return; - } - - if (error != null) { - return callback(error); - } - - hooks.execPost(key, model, [res]).then(() => callback(null, res), err => callback(err)); - } - } - }, model.events); - }; + model[key] = hooks.createWrapper(key, original); } } }; diff --git a/lib/helpers/promiseOrCallback.js b/lib/helpers/promiseOrCallback.js deleted file mode 100644 index 952eecf4bf8..00000000000 --- a/lib/helpers/promiseOrCallback.js +++ /dev/null @@ -1,54 +0,0 @@ -'use strict'; - -const immediate = require('./immediate'); - -const emittedSymbol = Symbol('mongoose#emitted'); - -module.exports = function promiseOrCallback(callback, fn, ee, Promise) { - if (typeof callback === 'function') { - try { - return fn(function(error) { - if (error != null) { - if (ee != null && ee.listeners != null && ee.listeners('error').length > 0 && !error[emittedSymbol]) { - error[emittedSymbol] = true; - ee.emit('error', error); - } - try { - callback(error); - } catch (error) { - return immediate(() => { - throw error; - }); - } - return; - } - callback.apply(this, arguments); - }); - } catch (error) { - if (ee != null && ee.listeners != null && ee.listeners('error').length > 0 && !error[emittedSymbol]) { - error[emittedSymbol] = true; - ee.emit('error', error); - } - - return callback(error); - } - } - - Promise = Promise || global.Promise; - - return new Promise((resolve, reject) => { - fn(function(error, res) { - if (error != null) { - if (ee != null && ee.listeners != null && ee.listeners('error').length > 0 && !error[emittedSymbol]) { - error[emittedSymbol] = true; - ee.emit('error', error); - } - return reject(error); - } - if (arguments.length > 2) { - return resolve(Array.prototype.slice.call(arguments, 1)); - } - resolve(res); - }); - }); -}; diff --git a/lib/utils.js b/lib/utils.js index e0cc40fc94c..4a0132ea18f 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -18,7 +18,6 @@ const isBsonType = require('./helpers/isBsonType'); const isPOJO = require('./helpers/isPOJO'); const getFunctionName = require('./helpers/getFunctionName'); const isMongooseObject = require('./helpers/isMongooseObject'); -const promiseOrCallback = require('./helpers/promiseOrCallback'); const schemaMerge = require('./helpers/schema/merge'); const specialProperties = require('./helpers/specialProperties'); const { trustedSymbol } = require('./helpers/query/trusted'); @@ -197,12 +196,6 @@ exports.last = function(arr) { return void 0; }; -/*! - * ignore - */ - -exports.promiseOrCallback = promiseOrCallback; - /*! * ignore */ diff --git a/package.json b/package.json index 29be1474ce3..0b4c50f3160 100644 --- a/package.json +++ b/package.json @@ -112,7 +112,7 @@ "main": "./index.js", "types": "./types/index.d.ts", "engines": { - "node": ">=16.20.1" + "node": ">=18.0.0" }, "bugs": { "url": "https://github.com/Automattic/mongoose/issues/new" diff --git a/test/document.test.js b/test/document.test.js index 1bd0ae78c6e..42042acda92 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -4534,13 +4534,13 @@ describe('document', function() { assert.equal(p.children[0].grandchild.foo(), 'bar'); }); - it('hooks/middleware for custom methods (gh-6385) (gh-7456)', async function() { + it('hooks/middleware for custom methods (gh-6385) (gh-7456)', async function hooksForCustomMethods() { const mySchema = new Schema({ name: String }); - mySchema.methods.foo = function(cb) { - return cb(null, this.name); + mySchema.methods.foo = function() { + return Promise.resolve(this.name); }; mySchema.methods.bar = function() { return this.name; @@ -4548,6 +4548,10 @@ describe('document', function() { mySchema.methods.baz = function(arg) { return Promise.resolve(arg); }; + mySchema.methods.qux = async function qux() { + await new Promise(resolve => setTimeout(resolve, 5)); + throw new Error('error!'); + }; let preFoo = 0; let postFoo = 0; @@ -4567,6 +4571,15 @@ describe('document', function() { ++postBaz; }); + let preQux = 0; + let postQux = 0; + mySchema.pre('qux', function() { + ++preQux; + }); + mySchema.post('qux', function() { + ++postQux; + }); + const MyModel = db.model('Test', mySchema); @@ -4588,6 +4601,12 @@ describe('document', function() { assert.equal(await doc.baz('foobar'), 'foobar'); assert.equal(preBaz, 1); assert.equal(preBaz, 1); + + const err = await doc.qux().then(() => null, err => err); + assert.equal(err.message, 'error!'); + assert.ok(err.stack.includes('hooksForCustomMethods')); + assert.equal(preQux, 1); + assert.equal(postQux, 0); }); it('custom methods with promises (gh-6385)', async function() { diff --git a/test/helpers/promiseOrCallback.test.js b/test/helpers/promiseOrCallback.test.js deleted file mode 100644 index 2ce3f7a3d3c..00000000000 --- a/test/helpers/promiseOrCallback.test.js +++ /dev/null @@ -1,110 +0,0 @@ -'use strict'; - -const assert = require('assert'); -const promiseOrCallback = require('../../lib/helpers/promiseOrCallback'); - -describe('promiseOrCallback()', () => { - const myError = new Error('This is My Error'); - const myRes = 'My Res'; - const myOtherArg = 'My Other Arg'; - - describe('apply callback', () => { - it('without error', (done) => { - promiseOrCallback( - (error, arg, otherArg) => { - assert.equal(arg, myRes); - assert.equal(otherArg, myOtherArg); - assert.equal(error, undefined); - done(); - }, - (fn) => { fn(null, myRes, myOtherArg); } - ); - }); - - describe('with error', () => { - it('without event emitter', (done) => { - promiseOrCallback( - (error) => { - assert.equal(error, myError); - done(); - }, - (fn) => { fn(myError); } - ); - }); - - it('with event emitter', (done) => { - promiseOrCallback( - () => { }, - (fn) => { return fn(myError); }, - { - listeners: () => [1], - emit: (eventType, error) => { - assert.equal(eventType, 'error'); - assert.equal(error, myError); - done(); - } - } - ); - }); - }); - }); - - describe('chain promise', () => { - describe('without error', () => { - it('two args', (done) => { - const promise = promiseOrCallback( - null, - (fn) => { fn(null, myRes); } - ); - promise.then((res) => { - assert.equal(res, myRes); - done(); - }); - }); - - it('more args', (done) => { - const promise = promiseOrCallback( - null, - (fn) => { fn(null, myRes, myOtherArg); } - ); - promise.then((args) => { - assert.equal(args[0], myRes); - assert.equal(args[1], myOtherArg); - done(); - }); - }); - }); - - describe('with error', () => { - it('without event emitter', (done) => { - const promise = promiseOrCallback( - null, - (fn) => { fn(myError); } - ); - promise.catch((error) => { - assert.equal(error, myError); - done(); - }); - }); - - - it('with event emitter', (done) => { - const promise = promiseOrCallback( - null, - (fn) => { return fn(myError); }, - { - listeners: () => [1], - emit: (eventType, error) => { - assert.equal(eventType, 'error'); - assert.equal(error, myError); - } - } - ); - promise.catch((error) => { - assert.equal(error, myError); - done(); - }); - }); - }); - }); -}); diff --git a/test/model.middleware.test.js b/test/model.middleware.test.js index ac2d68924f2..8e28512963b 100644 --- a/test/model.middleware.test.js +++ b/test/model.middleware.test.js @@ -416,6 +416,36 @@ describe('model middleware', function() { assert.equal(postCalled, 1); }); + it('static hooks async stack traces (gh-15317) (gh-5982)', async function staticHookAsyncStackTrace() { + const schema = new Schema({ + name: String + }); + + schema.statics.findByName = function() { + return this.find({ otherProp: { $notAnOperator: 'value' } }); + }; + + let preCalled = 0; + schema.pre('findByName', function() { + ++preCalled; + }); + + let postCalled = 0; + schema.post('findByName', function() { + ++postCalled; + }); + + const Model = db.model('Test', schema); + + await Model.create({ name: 'foo' }); + + const err = await Model.findByName('foo').then(() => null, err => err); + assert.equal(err.name, 'MongoServerError'); + assert.ok(err.stack.includes('staticHookAsyncStackTrace')); + assert.equal(preCalled, 1); + assert.equal(postCalled, 0); + }); + it('deleteOne hooks (gh-7538)', async function() { const schema = new Schema({ name: String diff --git a/test/query.test.js b/test/query.test.js index 63ae854f61d..43aa4ad35f1 100644 --- a/test/query.test.js +++ b/test/query.test.js @@ -4411,7 +4411,7 @@ describe('Query', function() { }); }); - it('throws an error if calling find(null), findOne(null), updateOne(null, update), etc. (gh-14948)', async function () { + it('throws an error if calling find(null), findOne(null), updateOne(null, update), etc. (gh-14948)', async function() { const userSchema = new Schema({ name: String });