diff --git a/docs/migrating_to_9.md b/docs/migrating_to_9.md index 771839b60c2..537ba040438 100644 --- a/docs/migrating_to_9.md +++ b/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. diff --git a/lib/aggregate.js b/lib/aggregate.js index e475736da2e..aaaef070b28 100644 --- a/lib/aggregate.js +++ b/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'); diff --git a/lib/browserDocument.js b/lib/browserDocument.js index fcdf78a5cb9..7b7e6c1ccb6 100644 --- a/lib/browserDocument.js +++ b/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, []); }; /*! diff --git a/lib/cursor/aggregationCursor.js b/lib/cursor/aggregationCursor.js index 01cf961d5dd..a528076fe62 100644 --- a/lib/cursor/aggregationCursor.js +++ b/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); } } diff --git a/lib/cursor/queryCursor.js b/lib/cursor/queryCursor.js index f25a06f2fd1..18c5ba5836a 100644 --- a/lib/cursor/queryCursor.js +++ b/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); diff --git a/lib/document.js b/lib/document.js index 25572a34e06..a9ac7619505 100644 --- a/lib/document.js +++ b/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]); }; /*! diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 25ec30eb9e6..a1ee62f31ef 100644 --- a/lib/helpers/model/applyHooks.js +++ b/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; +} diff --git a/lib/helpers/model/applyStaticHooks.js b/lib/helpers/model/applyStaticHooks.js index 40116462f26..ec715f881e0 100644 --- a/lib/helpers/model/applyStaticHooks.js +++ b/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); }; } diff --git a/lib/model.js b/lib/model.js index ccf82910dec..ef437896f22 100644 --- a/lib/model.js +++ b/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) { diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index d4c899cba5e..744f8765fff 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -11,25 +11,17 @@ module.exports = function saveSubdocs(schema) { return; } - const _this = this; const subdocs = this.$getAllSubdocs({ useCache: true }); if (!subdocs.length) { return; } - await Promise.all(subdocs.map(async(subdoc) => { - return new Promise((resolve, reject) => { - subdoc.$__schema.s.hooks.execPre('save', subdoc, function(err) { - if (err) reject(err); - else resolve(); - }); - }); - })); + await Promise.all(subdocs.map(subdoc => subdoc._execDocumentPreHooks('save'))); // Invalidate subdocs cache because subdoc pre hooks can add new subdocuments - if (_this.$__.saveOptions) { - _this.$__.saveOptions.__subdocs = null; + if (this.$__.saveOptions) { + this.$__.saveOptions.__subdocs = null; } }, null, unshift); @@ -41,14 +33,7 @@ module.exports = function saveSubdocs(schema) { const promises = []; for (const subdoc of removedSubdocs) { - promises.push(new Promise((resolve, reject) => { - subdoc.$__schema.s.hooks.execPost('deleteOne', subdoc, [subdoc], function(err) { - if (err) { - return reject(err); - } - resolve(); - }); - })); + promises.push(subdoc._execDocumentPostHooks('deleteOne')); } this.$__.removedSubdocs = null; @@ -68,14 +53,7 @@ module.exports = function saveSubdocs(schema) { const promises = []; for (const subdoc of subdocs) { - promises.push(new Promise((resolve, reject) => { - subdoc.$__schema.s.hooks.execPost('save', subdoc, [subdoc], function(err) { - if (err) { - return reject(err); - } - resolve(); - }); - })); + promises.push(subdoc._execDocumentPostHooks('save')); } await Promise.all(promises); diff --git a/lib/plugins/sharding.js b/lib/plugins/sharding.js index 7d905f31c0f..76d95f9acfb 100644 --- a/lib/plugins/sharding.js +++ b/lib/plugins/sharding.js @@ -12,13 +12,11 @@ module.exports = function shardingPlugin(schema) { storeShard.call(this); return this; }); - schema.pre('save', function shardingPluginPreSave(next) { + schema.pre('save', function shardingPluginPreSave() { applyWhere.call(this); - next(); }); - schema.pre('remove', function shardingPluginPreRemove(next) { + schema.pre('remove', function shardingPluginPreRemove() { applyWhere.call(this); - next(); }); schema.post('save', function shardingPluginPostSave() { storeShard.call(this); diff --git a/lib/query.js b/lib/query.js index ea177e5dd5d..b4b81fb9e2b 100644 --- a/lib/query.js +++ b/lib/query.js @@ -4510,15 +4510,8 @@ function _executePostHooks(query, res, error, op) { * ignore */ -function _executePreExecHooks(query) { - return new Promise((resolve, reject) => { - query._hooks.execPre('exec', query, [], (error) => { - if (error != null) { - return reject(error); - } - resolve(); - }); - }); +async function _executePreExecHooks(query) { + return query._hooks.execPre('exec', query, []); } /*! @@ -4530,14 +4523,7 @@ function _executePreHooks(query, op) { return; } - return new Promise((resolve, reject) => { - query._queryMiddleware.execPre(op || query.op, query, [], (error) => { - if (error != null) { - return reject(error); - } - resolve(); - }); - }); + return query._queryMiddleware.execPre(op || query.op, query, []); } /** diff --git a/package.json b/package.json index 44e74f3452f..319708a39b4 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "license": "MIT", "dependencies": { "bson": "^6.10.3", - "kareem": "2.6.3", + "kareem": "git@github.com:mongoosejs/kareem.git#vkarpov15/v3", "mongodb": "~6.15.0", "mpath": "0.9.0", "mquery": "5.0.0", diff --git a/test/document.test.js b/test/document.test.js index adaacd3a10c..0b6397b5ce7 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -14424,6 +14424,20 @@ describe('document', function() { } }); + describe('async stack traces (gh-15317)', function() { + it('works with save() validation errors', async function() { + const userSchema = new mongoose.Schema({ + name: { type: String, required: true, validate: v => v.length > 3 }, + age: Number + }); + const User = db.model('User', userSchema); + const doc = new User({ name: 'A' }); + const err = await doc.save().then(() => null, err => err); + assert.ok(err instanceof Error); + assert.ok(err.stack.includes('document.test.js'), err.stack); + }); + }); + it('handles selected paths on root discriminator (gh-15308)', async function() { const CarSchema = new mongoose.Schema( { diff --git a/test/model.middleware.test.js b/test/model.middleware.test.js index 4aca8c5516c..ac2d68924f2 100644 --- a/test/model.middleware.test.js +++ b/test/model.middleware.test.js @@ -320,7 +320,7 @@ describe('model middleware', function() { schema.pre('save', function(next) { next(); - // This error will not get reported, because you already called next() + // Error takes precedence over next() throw new Error('woops!'); }); @@ -333,8 +333,8 @@ describe('model middleware', function() { const test = new TestMiddleware({ title: 'Test' }); - await test.save(); - assert.equal(called, 1); + await assert.rejects(test.save(), /woops!/); + assert.equal(called, 0); }); it('validate + remove', async function() { diff --git a/test/query.cursor.test.js b/test/query.cursor.test.js index e7265be1d06..3a736dd0f57 100644 --- a/test/query.cursor.test.js +++ b/test/query.cursor.test.js @@ -905,6 +905,10 @@ describe('QueryCursor', function() { it('returns the underlying Node driver cursor with getDriverCursor()', async function() { const schema = new mongoose.Schema({ name: String }); + // Add some middleware to ensure the cursor hasn't been created yet when `cursor()` is called. + schema.pre('find', async function() { + await new Promise(resolve => setTimeout(resolve, 10)); + }); const Movie = db.model('Movie', schema); @@ -927,7 +931,7 @@ describe('QueryCursor', function() { const TestModel = db.model('Test', mongoose.Schema({ name: String })); const stream = await TestModel.find().cursor(); - await once(stream, 'cursor'); + assert.ok(stream.cursor); assert.ok(!stream.cursor.closed); stream.destroy(); @@ -939,7 +943,9 @@ describe('QueryCursor', function() { it('handles destroy() before cursor is created (gh-14966)', async function() { db.deleteModel(/Test/); - const TestModel = db.model('Test', mongoose.Schema({ name: String })); + const schema = mongoose.Schema({ name: String }); + schema.pre('find', () => new Promise(resolve => setTimeout(resolve, 10))); + const TestModel = db.model('Test', schema); const stream = await TestModel.find().cursor(); assert.ok(!stream.cursor);