From b20df583056e9afcd53599ce84970de67aec1780 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 10 Mar 2025 14:00:41 -0400 Subject: [PATCH 1/7] BREAKING CHANGE: make SchemaType.prototype.doValidate() an async function for cleaner stack traces --- docs/migrating_to_9.md | 13 + lib/document.js | 27 +- lib/helpers/updateValidators.js | 162 ++++-------- lib/model.js | 57 ++-- lib/query.js | 9 +- lib/schema/documentArray.js | 77 ++---- lib/schema/documentArrayElement.js | 10 +- lib/schema/subdocument.js | 22 +- lib/schemaType.js | 83 +++--- test/document.test.js | 6 +- test/schema.documentarray.test.js | 9 +- test/schema.number.test.js | 21 +- test/schema.validation.test.js | 405 ++++++++++------------------- test/updateValidators.unit.test.js | 111 -------- 14 files changed, 325 insertions(+), 687 deletions(-) create mode 100644 docs/migrating_to_9.md delete mode 100644 test/updateValidators.unit.test.js diff --git a/docs/migrating_to_9.md b/docs/migrating_to_9.md new file mode 100644 index 00000000000..9dd3dbbfa88 --- /dev/null +++ b/docs/migrating_to_9.md @@ -0,0 +1,13 @@ +# Migrating from 8.x to 9.x + + + +There are several backwards-breaking changes you should be aware of when migrating from Mongoose 8.x to Mongoose 9.x. + +If you're still on Mongoose 7.x or earlier, please read the [Mongoose 7.x to 8.x migration guide](migrating_to_8.html) and upgrade to Mongoose 8.x first before upgrading to Mongoose 9. + +## `Schema.prototype.doValidate()` now returns a promise diff --git a/lib/document.js b/lib/document.js index 31cb0263f7d..d4935c33e0a 100644 --- a/lib/document.js +++ b/lib/document.js @@ -3085,22 +3085,17 @@ Document.prototype.$__validate = async function $__validate(pathsToValidate, opt _nestedValidate: true }; - await new Promise((resolve) => { - schemaType.doValidate(val, function doValidateCallback(err) { - if (err) { - const isSubdoc = schemaType.$isSingleNested || - schemaType.$isArraySubdocument || - schemaType.$isMongooseDocumentArray; - if (isSubdoc && err instanceof ValidationError) { - return resolve(); - } - _this.invalidate(path, err, undefined, true); - resolve(); - } else { - resolve(); - } - }, scope, doValidateOptions); - }); + try { + await schemaType.doValidate(val, scope, doValidateOptions); + } catch (err) { + const isSubdoc = schemaType.$isSingleNested || + schemaType.$isArraySubdocument || + schemaType.$isMongooseDocumentArray; + if (isSubdoc && err instanceof ValidationError) { + return; + } + _this.invalidate(path, err, undefined, true); + } } }; diff --git a/lib/helpers/updateValidators.js b/lib/helpers/updateValidators.js index 176eff26e16..f819074d48a 100644 --- a/lib/helpers/updateValidators.js +++ b/lib/helpers/updateValidators.js @@ -21,7 +21,7 @@ const modifiedPaths = require('./common').modifiedPaths; * @api private */ -module.exports = function(query, schema, castedDoc, options, callback) { +module.exports = async function updateValidators(query, schema, castedDoc, options) { const keys = Object.keys(castedDoc || {}); let updatedKeys = {}; let updatedValues = {}; @@ -32,9 +32,8 @@ module.exports = function(query, schema, castedDoc, options, callback) { const modified = {}; let currentUpdate; let key; - let i; - for (i = 0; i < numKeys; ++i) { + for (let i = 0; i < numKeys; ++i) { if (keys[i].startsWith('$')) { hasDollarUpdate = true; if (keys[i] === '$push' || keys[i] === '$addToSet') { @@ -89,161 +88,108 @@ module.exports = function(query, schema, castedDoc, options, callback) { const alreadyValidated = []; const context = query; - function iter(i, v) { + for (let i = 0; i < numUpdates; ++i) { + const v = updatedValues[updates[i]]; const schemaPath = schema._getSchema(updates[i]); if (schemaPath == null) { - return; + continue; } if (schemaPath.instance === 'Mixed' && schemaPath.path !== updates[i]) { - return; + continue; } if (v && Array.isArray(v.$in)) { v.$in.forEach((v, i) => { - validatorsToExecute.push(function(callback) { - schemaPath.doValidate( - v, - function(err) { - if (err) { - err.path = updates[i] + '.$in.' + i; - validationErrors.push(err); - } - callback(null); - }, - context, - { updateValidator: true }); - }); + validatorsToExecute.push( + schemaPath.doValidate(v, context, { updateValidator: true }).catch(err => { + err.path = updates[i] + '.$in.' + i; + validationErrors.push(err); + }) + ); }); } else { if (isPull[updates[i]] && schemaPath.$isMongooseArray) { - return; + continue; } if (schemaPath.$isMongooseDocumentArrayElement && v != null && v.$__ != null) { alreadyValidated.push(updates[i]); - validatorsToExecute.push(function(callback) { - schemaPath.doValidate(v, function(err) { - if (err) { - if (err.errors) { - for (const key of Object.keys(err.errors)) { - const _err = err.errors[key]; - _err.path = updates[i] + '.' + key; - validationErrors.push(_err); - } - } else { - err.path = updates[i]; - validationErrors.push(err); + validatorsToExecute.push( + schemaPath.doValidate(v, context, { updateValidator: true }).catch(err => { + if (err.errors) { + for (const key of Object.keys(err.errors)) { + const _err = err.errors[key]; + _err.path = updates[i] + '.' + key; + validationErrors.push(_err); } + } else { + err.path = updates[i]; + validationErrors.push(err); } - - return callback(null); - }, context, { updateValidator: true }); - }); + }) + ); } else { - validatorsToExecute.push(function(callback) { - for (const path of alreadyValidated) { - if (updates[i].startsWith(path + '.')) { - return callback(null); - } + for (const path of alreadyValidated) { + if (updates[i].startsWith(path + '.')) { + continue; } - - schemaPath.doValidate(v, function(err) { + } + validatorsToExecute.push( + schemaPath.doValidate(v, context, { updateValidator: true }).catch(err => { if (schemaPath.schema != null && schemaPath.schema.options.storeSubdocValidationError === false && err instanceof ValidationError) { - return callback(null); + return; } if (err) { err.path = updates[i]; validationErrors.push(err); } - callback(null); - }, context, { updateValidator: true }); - }); + }) + ); } } } - for (i = 0; i < numUpdates; ++i) { - iter(i, updatedValues[updates[i]]); - } const arrayUpdates = Object.keys(arrayAtomicUpdates); for (const arrayUpdate of arrayUpdates) { let schemaPath = schema._getSchema(arrayUpdate); if (schemaPath && schemaPath.$isMongooseDocumentArray) { - validatorsToExecute.push(function(callback) { + validatorsToExecute.push( schemaPath.doValidate( arrayAtomicUpdates[arrayUpdate], - getValidationCallback(arrayUpdate, validationErrors, callback), - options && options.context === 'query' ? query : null); - }); + options && options.context === 'query' ? query : null + ).catch(err => { + err.path = arrayUpdate; + validationErrors.push(err); + }) + ); } else { schemaPath = schema._getSchema(arrayUpdate + '.0'); for (const atomicUpdate of arrayAtomicUpdates[arrayUpdate]) { - validatorsToExecute.push(function(callback) { + validatorsToExecute.push( schemaPath.doValidate( atomicUpdate, - getValidationCallback(arrayUpdate, validationErrors, callback), options && options.context === 'query' ? query : null, - { updateValidator: true }); - }); + { updateValidator: true } + ).catch(err => { + err.path = arrayUpdate; + validationErrors.push(err); + }) + ); } } } - if (callback != null) { - let numValidators = validatorsToExecute.length; - if (numValidators === 0) { - return _done(callback); - } - for (const validator of validatorsToExecute) { - validator(function() { - if (--numValidators <= 0) { - _done(callback); - } - }); - } - - return; - } - - return function(callback) { - let numValidators = validatorsToExecute.length; - if (numValidators === 0) { - return _done(callback); - } - for (const validator of validatorsToExecute) { - validator(function() { - if (--numValidators <= 0) { - _done(callback); - } - }); - } - }; + await Promise.all(validatorsToExecute); + if (validationErrors.length) { + const err = new ValidationError(null); - function _done(callback) { - if (validationErrors.length) { - const err = new ValidationError(null); - - for (const validationError of validationErrors) { - err.addError(validationError.path, validationError); - } - - return callback(err); + for (const validationError of validationErrors) { + err.addError(validationError.path, validationError); } - callback(null); - } - - function getValidationCallback(arrayUpdate, validationErrors, callback) { - return function(err) { - if (err) { - err.path = arrayUpdate; - validationErrors.push(err); - } - callback(null); - }; + throw err; } }; - diff --git a/lib/model.js b/lib/model.js index e39ea7d4b52..4c2247a4ea1 100644 --- a/lib/model.js +++ b/lib/model.js @@ -4289,43 +4289,34 @@ Model.validate = async function validate(obj, pathsOrOptions, context) { } } - let remaining = paths.size; - - return new Promise((resolve, reject) => { - for (const path of paths) { - const schemaType = schema.path(path); - if (schemaType == null) { - _checkDone(); - continue; - } + const promises = []; + for (const path of paths) { + const schemaType = schema.path(path); + if (schemaType == null) { + continue; + } - const pieces = path.indexOf('.') === -1 ? [path] : path.split('.'); - let cur = obj; - for (let i = 0; i < pieces.length - 1; ++i) { - cur = cur[pieces[i]]; - } + const pieces = path.indexOf('.') === -1 ? [path] : path.split('.'); + let cur = obj; + for (let i = 0; i < pieces.length - 1; ++i) { + cur = cur[pieces[i]]; + } - const val = get(obj, path, void 0); + const val = get(obj, path, void 0); + promises.push( + schemaType.doValidate(val, context, { path: path }).catch(err => { + error = error || new ValidationError(); + error.addError(path, err); + }) + ); + } - schemaType.doValidate(val, err => { - if (err) { - error = error || new ValidationError(); - error.addError(path, err); - } - _checkDone(); - }, context, { path: path }); - } + await Promise.all(promises); + if (error != null) { + throw error; + } - function _checkDone() { - if (--remaining <= 0) { - if (error) { - reject(error); - } else { - resolve(obj); - } - } - } - }); + return obj; }; /** diff --git a/lib/query.js b/lib/query.js index 245554c5be7..ea177e5dd5d 100644 --- a/lib/query.js +++ b/lib/query.js @@ -3945,14 +3945,7 @@ Query.prototype.validate = async function validate(castedDoc, options, isOverwri if (isOverwriting) { await castedDoc.$validate(); } else { - await new Promise((resolve, reject) => { - updateValidators(this, this.model.schema, castedDoc, options, (err) => { - if (err != null) { - return reject(err); - } - resolve(); - }); - }); + await updateValidators(this, this.model.schema, castedDoc, options); } await _executePostHooks(this, null, null, 'validate'); diff --git a/lib/schema/documentArray.js b/lib/schema/documentArray.js index cf84b303f51..c117cb1c6a6 100644 --- a/lib/schema/documentArray.js +++ b/lib/schema/documentArray.js @@ -220,72 +220,45 @@ SchemaDocumentArray.prototype.discriminator = function(name, schema, options) { /** * Performs local validations first, then validations on each embedded doc * - * @api private + * @api public */ -SchemaDocumentArray.prototype.doValidate = function(array, fn, scope, options) { +SchemaDocumentArray.prototype.doValidate = async function doValidate(array, scope, options) { // lazy load MongooseDocumentArray || (MongooseDocumentArray = require('../types/documentArray')); - const _this = this; - try { - SchemaType.prototype.doValidate.call(this, array, cb, scope); - } catch (err) { - return fn(err); + await SchemaType.prototype.doValidate.call(this, array, scope); + if (options?.updateValidator) { + return; + } + if (!utils.isMongooseDocumentArray(array)) { + array = new MongooseDocumentArray(array, this.path, scope); } - function cb(err) { - if (err) { - return fn(err); - } - - let count = array && array.length; - let error; - - if (!count) { - return fn(); - } - if (options && options.updateValidator) { - return fn(); - } - if (!utils.isMongooseDocumentArray(array)) { - array = new MongooseDocumentArray(array, _this.path, scope); - } - + const promises = []; + for (let i = 0; i < array.length; ++i) { // handle sparse arrays, do not use array.forEach which does not // iterate over sparse elements yet reports array.length including // them :( - - function callback(err) { - if (err != null) { - error = err; - } - --count || fn(error); + let doc = array[i]; + if (doc == null) { + continue; + } + // If you set the array index directly, the doc might not yet be + // a full fledged mongoose subdoc, so make it into one. + if (!(doc instanceof Subdocument)) { + const Constructor = getConstructor(this.casterConstructor, array[i]); + doc = array[i] = new Constructor(doc, array, undefined, undefined, i); } - for (let i = 0, len = count; i < len; ++i) { - // sidestep sparse entries - let doc = array[i]; - if (doc == null) { - --count || fn(error); - continue; - } - - // If you set the array index directly, the doc might not yet be - // a full fledged mongoose subdoc, so make it into one. - if (!(doc instanceof Subdocument)) { - const Constructor = getConstructor(_this.casterConstructor, array[i]); - doc = array[i] = new Constructor(doc, array, undefined, undefined, i); - } - - if (options != null && options.validateModifiedOnly && !doc.$isModified()) { - --count || fn(error); - continue; - } - - doc.$__validate(null, options).then(() => callback(), err => callback(err)); + if (options != null && options.validateModifiedOnly && !doc.$isModified()) { + continue; } + + promises.push(doc.$__validate(null, options)); } + + await Promise.all(promises); }; /** diff --git a/lib/schema/documentArrayElement.js b/lib/schema/documentArrayElement.js index 5250b74b505..552dd94a428 100644 --- a/lib/schema/documentArrayElement.js +++ b/lib/schema/documentArrayElement.js @@ -58,21 +58,19 @@ SchemaDocumentArrayElement.prototype.cast = function(...args) { }; /** - * Casts contents for queries. + * Async validation on this individual array element * - * @param {String} $cond - * @param {any} [val] - * @api private + * @api public */ -SchemaDocumentArrayElement.prototype.doValidate = function(value, fn, scope, options) { +SchemaDocumentArrayElement.prototype.doValidate = async function doValidate(value, scope, options) { const Constructor = getConstructor(this.caster, value); if (value && !(value instanceof Constructor)) { value = new Constructor(value, scope, null, null, options && options.index != null ? options.index : null); } - return SchemaSubdocument.prototype.doValidate.call(this, value, fn, scope, options); + return SchemaSubdocument.prototype.doValidate.call(this, value, scope, options); }; /** diff --git a/lib/schema/subdocument.js b/lib/schema/subdocument.js index 3afdb8ee281..affe228c3d1 100644 --- a/lib/schema/subdocument.js +++ b/lib/schema/subdocument.js @@ -246,10 +246,10 @@ SchemaSubdocument.prototype.castForQuery = function($conditional, val, context, /** * Async validation on this single nested doc. * - * @api private + * @api public */ -SchemaSubdocument.prototype.doValidate = function(value, fn, scope, options) { +SchemaSubdocument.prototype.doValidate = async function doValidate(value, scope, options) { const Constructor = getConstructor(this.caster, value); if (value && !(value instanceof Constructor)) { @@ -258,21 +258,15 @@ SchemaSubdocument.prototype.doValidate = function(value, fn, scope, options) { if (options && options.skipSchemaValidators) { if (!value) { - return fn(null); + return; } - return value.validate().then(() => fn(null), err => fn(err)); + return value.validate(); } - SchemaType.prototype.doValidate.call(this, value, function(error) { - if (error) { - return fn(error); - } - if (!value) { - return fn(null); - } - - value.validate().then(() => fn(null), err => fn(err)); - }, scope, options); + await SchemaType.prototype.doValidate.call(this, value, scope, options); + if (value != null) { + await value.validate(); + } }; /** diff --git a/lib/schemaType.js b/lib/schemaType.js index aae3e244423..688a433f923 100644 --- a/lib/schemaType.js +++ b/lib/schemaType.js @@ -1307,7 +1307,6 @@ SchemaType.prototype.select = function select(val) { * Performs a validation of `value` using the validators declared for this SchemaType. * * @param {Any} value - * @param {Function} callback * @param {Object} scope * @param {Object} [options] * @param {String} [options.path] @@ -1315,28 +1314,20 @@ SchemaType.prototype.select = function select(val) { * @api public */ -SchemaType.prototype.doValidate = function(value, fn, scope, options) { +SchemaType.prototype.doValidate = async function doValidate(value, scope, options) { let err = false; const path = this.path; - if (typeof fn !== 'function') { - throw new TypeError(`Must pass callback function to doValidate(), got ${typeof fn}`); - } // Avoid non-object `validators` const validators = this.validators. filter(v => typeof v === 'object' && v !== null); - let count = validators.length; - - if (!count) { - return fn(null); + if (!validators.length) { + return; } + const promises = []; for (let i = 0, len = validators.length; i < len; ++i) { - if (err) { - break; - } - const v = validators[i]; const validator = v.validator; let ok; @@ -1346,17 +1337,18 @@ SchemaType.prototype.doValidate = function(value, fn, scope, options) { validatorProperties.fullPath = this.$fullPath; validatorProperties.value = value; - if (validator instanceof RegExp) { - validate(validator.test(value), validatorProperties, scope); - continue; - } - - if (typeof validator !== 'function') { + if (value === undefined && validator !== this.requiredValidator) { continue; } - - if (value === undefined && validator !== this.requiredValidator) { - validate(true, validatorProperties, scope); + if (validator instanceof RegExp) { + ok = validator.test(value); + if (ok === false) { + const ErrorConstructor = validatorProperties.ErrorConstructor || ValidatorError; + err = new ErrorConstructor(validatorProperties, scope); + err[validatorErrorSymbol] = true; + throw err; + } + } else if (typeof validator !== 'function') { continue; } @@ -1375,34 +1367,35 @@ SchemaType.prototype.doValidate = function(value, fn, scope, options) { } if (ok != null && typeof ok.then === 'function') { - ok.then( - function(ok) { validate(ok, validatorProperties, scope); }, - function(error) { - validatorProperties.reason = error; - validatorProperties.message = error.message; - ok = false; - validate(ok, validatorProperties, scope); - }); - } else { - validate(ok, validatorProperties, scope); - } - } - - function validate(ok, validatorProperties, scope) { - if (err) { - return; - } - if (ok === undefined || ok) { - if (--count <= 0) { - fn(null); - } - } else { + promises.push( + ok.then( + function(ok) { + if (ok === false) { + const ErrorConstructor = validatorProperties.ErrorConstructor || ValidatorError; + err = new ErrorConstructor(validatorProperties, scope); + err[validatorErrorSymbol] = true; + throw err; + } + }, + function(error) { + validatorProperties.reason = error; + validatorProperties.message = error.message; + ok = false; + const ErrorConstructor = validatorProperties.ErrorConstructor || ValidatorError; + err = new ErrorConstructor(validatorProperties, scope); + err[validatorErrorSymbol] = true; + throw err; + }) + ); + } else if (ok !== undefined && !ok) { const ErrorConstructor = validatorProperties.ErrorConstructor || ValidatorError; err = new ErrorConstructor(validatorProperties, scope); err[validatorErrorSymbol] = true; - fn(err); + throw err; } } + + await Promise.all(promises); }; diff --git a/test/document.test.js b/test/document.test.js index b52f522d6ca..c7463e01229 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -6502,14 +6502,16 @@ describe('document', function() { }); const Model = db.model('Test', schema); - await Model.create({ + let doc = new Model({ roles: [ { name: 'admin' }, { name: 'mod', folders: [{ folderId: 'foo' }] } ] }); + await doc.validate().then(() => null, err => console.log(err)); + await doc.save(); - const doc = await Model.findOne(); + doc = await Model.findOne(); doc.roles[1].folders.push({ folderId: 'bar' }); diff --git a/test/schema.documentarray.test.js b/test/schema.documentarray.test.js index d9ccb6c1f6b..92eee9a4320 100644 --- a/test/schema.documentarray.test.js +++ b/test/schema.documentarray.test.js @@ -150,14 +150,7 @@ describe('schema.documentarray', function() { const TestModel = mongoose.model('Test', testSchema); const testDoc = new TestModel(); - const err = await new Promise((resolve, reject) => { - testSchema.path('comments').$embeddedSchemaType.doValidate({}, err => { - if (err != null) { - return reject(err); - } - resolve(); - }, testDoc.comments, { index: 1 }); - }).then(() => null, err => err); + const err = await testSchema.path('comments').$embeddedSchemaType.doValidate({}, testDoc.comments, { index: 1 }).then(() => null, err => err); assert.equal(err.name, 'ValidationError'); assert.equal(err.message, 'Validation failed: text: Path `text` is required.'); }); diff --git a/test/schema.number.test.js b/test/schema.number.test.js index 99c69ca1540..461873535dc 100644 --- a/test/schema.number.test.js +++ b/test/schema.number.test.js @@ -10,35 +10,20 @@ describe('SchemaNumber', function() { it('allows 0 with required: true and ref set (gh-11912)', async function() { const schema = new Schema({ x: { type: Number, required: true, ref: 'Foo' } }); - await new Promise((resolve, reject) => { - schema.path('x').doValidate(0, err => { - if (err != null) { - return reject(err); - } - resolve(); - }); - }); + await schema.path('x').doValidate(0); }); it('allows calling `min()` with no message arg (gh-15236)', async function() { const schema = new Schema({ x: { type: Number } }); schema.path('x').min(0); - const err = await new Promise((resolve) => { - schema.path('x').doValidate(-1, err => { - resolve(err); - }); - }); + const err = await schema.path('x').doValidate(-1).then(() => null, err => err); assert.ok(err); assert.equal(err.message, 'Path `x` (-1) is less than minimum allowed value (0).'); schema.path('x').min(0, 'Invalid value!'); - const err2 = await new Promise((resolve) => { - schema.path('x').doValidate(-1, err => { - resolve(err); - }); - }); + const err2 = await schema.path('x').doValidate(-1).then(() => null, err => err); assert.equal(err2.message, 'Invalid value!'); }); }); diff --git a/test/schema.validation.test.js b/test/schema.validation.test.js index d70643c1825..243328c2f76 100644 --- a/test/schema.validation.test.js +++ b/test/schema.validation.test.js @@ -48,7 +48,7 @@ describe('schema', function() { done(); }); - it('string enum', function(done) { + it('string enum', async function() { const Test = new Schema({ complex: { type: String, enum: ['a', 'b', undefined, 'c', null] }, state: { type: String } @@ -71,92 +71,58 @@ describe('schema', function() { assert.equal(Test.path('state').validators.length, 1); assert.deepEqual(Test.path('state').enumValues, ['opening', 'open', 'closing', 'closed']); - Test.path('complex').doValidate('x', function(err) { - assert.ok(err instanceof ValidatorError); - }); + await assert.rejects(Test.path('complex').doValidate('x'), ValidatorError); // allow unsetting enums - Test.path('complex').doValidate(undefined, function(err) { - assert.ifError(err); - }); + await Test.path('complex').doValidate(undefined); - Test.path('complex').doValidate(null, function(err) { - assert.ifError(err); - }); - - Test.path('complex').doValidate('da', function(err) { - assert.ok(err instanceof ValidatorError); - }); + await Test.path('complex').doValidate(null); - Test.path('state').doValidate('x', function(err) { - assert.ok(err instanceof ValidatorError); - assert.equal(err.message, - 'enum validator failed for path `state`: test'); - }); + await assert.rejects( + Test.path('complex').doValidate('da'), + ValidatorError + ); - Test.path('state').doValidate('opening', function(err) { - assert.ifError(err); - }); + await assert.rejects( + Test.path('state').doValidate('x'), + err => { + assert.ok(err instanceof ValidatorError); + assert.equal(err.message, + 'enum validator failed for path `state`: test'); + return true; + } + ); - Test.path('state').doValidate('open', function(err) { - assert.ifError(err); - }); + await Test.path('state').doValidate('opening'); - done(); + await Test.path('state').doValidate('open'); }); - it('string regexp', function(done) { - let remaining = 10; + it('string regexp', async function() { const Test = new Schema({ simple: { type: String, match: /[a-z]/ } }); assert.equal(Test.path('simple').validators.length, 1); - Test.path('simple').doValidate('az', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate('az'); Test.path('simple').match(/[0-9]/); assert.equal(Test.path('simple').validators.length, 2); - Test.path('simple').doValidate('12', function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects(Test.path('simple').doValidate('12'), ValidatorError); - Test.path('simple').doValidate('a12', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate('a12'); - Test.path('simple').doValidate('', function(err) { - assert.ifError(err); - --remaining || done(); - }); - Test.path('simple').doValidate(null, function(err) { - assert.ifError(err); - --remaining || done(); - }); - Test.path('simple').doValidate(undefined, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate(''); + await Test.path('simple').doValidate(null); + await Test.path('simple').doValidate(undefined); Test.path('simple').validators = []; Test.path('simple').match(/[1-9]/); - Test.path('simple').doValidate(0, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects(Test.path('simple').doValidate(0), ValidatorError); Test.path('simple').match(null); - Test.path('simple').doValidate(0, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); - - done(); + await assert.rejects(Test.path('simple').doValidate(0), ValidatorError); }); describe('non-required fields', function() { @@ -198,39 +164,32 @@ describe('schema', function() { }); }); - it('number min and max', function(done) { - let remaining = 4; + it('number min and max', async function() { const Tobi = new Schema({ friends: { type: Number, max: 15, min: 5 } }); assert.equal(Tobi.path('friends').validators.length, 2); - Tobi.path('friends').doValidate(10, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Tobi.path('friends').doValidate(10); - Tobi.path('friends').doValidate(100, function(err) { + await assert.rejects(Tobi.path('friends').doValidate(100), (err) => { assert.ok(err instanceof ValidatorError); assert.equal(err.path, 'friends'); assert.equal(err.kind, 'max'); assert.equal(err.value, 100); - --remaining || done(); + return true; }); - Tobi.path('friends').doValidate(1, function(err) { + await assert.rejects(Tobi.path('friends').doValidate(1), (err) => { assert.ok(err instanceof ValidatorError); assert.equal(err.path, 'friends'); assert.equal(err.kind, 'min'); - --remaining || done(); + return true; }); // null is allowed - Tobi.path('friends').doValidate(null, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Tobi.path('friends').doValidate(null); Tobi.path('friends').min(); Tobi.path('friends').max(); @@ -240,8 +199,7 @@ describe('schema', function() { }); describe('required', function() { - it('string required', function(done) { - let remaining = 4; + it('string required', async function() { const Test = new Schema({ simple: String }); @@ -249,29 +207,16 @@ describe('schema', function() { Test.path('simple').required(true); assert.equal(Test.path('simple').validators.length, 1); - Test.path('simple').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects(Test.path('simple').doValidate(null), ValidatorError); - Test.path('simple').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects(Test.path('simple').doValidate(undefined), ValidatorError); - Test.path('simple').doValidate('', function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects(Test.path('simple').doValidate(''), ValidatorError); - Test.path('simple').doValidate('woot', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate('woot'); }); - it('string conditional required', function(done) { - let remaining = 8; + it('string conditional required', async function() { const Test = new Schema({ simple: String }); @@ -284,240 +229,172 @@ describe('schema', function() { Test.path('simple').required(isRequired); assert.equal(Test.path('simple').validators.length, 1); - Test.path('simple').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Test.path('simple').doValidate(null), + ValidatorError + ); - Test.path('simple').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Test.path('simple').doValidate(undefined), + ValidatorError + ); - Test.path('simple').doValidate('', function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Test.path('simple').doValidate(''), + ValidatorError + ); - Test.path('simple').doValidate('woot', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate('woot'); required = false; - Test.path('simple').doValidate(null, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate(null); - Test.path('simple').doValidate(undefined, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate(undefined); - Test.path('simple').doValidate('', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate(''); - Test.path('simple').doValidate('woot', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Test.path('simple').doValidate('woot'); }); - it('number required', function(done) { - let remaining = 3; + it('number required', async function() { const Edwald = new Schema({ friends: { type: Number, required: true } }); - Edwald.path('friends').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Edwald.path('friends').doValidate(null), + ValidatorError + ); - Edwald.path('friends').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Edwald.path('friends').doValidate(undefined), + ValidatorError + ); - Edwald.path('friends').doValidate(0, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Edwald.path('friends').doValidate(0); }); - it('date required', function(done) { - let remaining = 3; + it('date required', async function() { const Loki = new Schema({ birth_date: { type: Date, required: true } }); - Loki.path('birth_date').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Loki.path('birth_date').doValidate(null), + ValidatorError + ); - Loki.path('birth_date').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Loki.path('birth_date').doValidate(undefined), + ValidatorError + ); - Loki.path('birth_date').doValidate(new Date(), function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Loki.path('birth_date').doValidate(new Date()); }); - it('date not empty string (gh-3132)', function(done) { + it('date not empty string (gh-3132)', async function() { const HappyBirthday = new Schema({ date: { type: Date, required: true } }); - HappyBirthday.path('date').doValidate('', function(err) { - assert.ok(err instanceof ValidatorError); - done(); - }); + await assert.rejects( + HappyBirthday.path('date').doValidate(''), + ValidatorError + ); }); - it('objectid required', function(done) { - let remaining = 3; + it('objectid required', async function() { const Loki = new Schema({ owner: { type: ObjectId, required: true } }); - Loki.path('owner').doValidate(new DocumentObjectId(), function(err) { - assert.ifError(err); - --remaining || done(); - }); + await assert.rejects( + Loki.path('owner').doValidate(null), + ValidatorError + ); - Loki.path('owner').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); - - Loki.path('owner').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Loki.path('owner').doValidate(undefined), + ValidatorError + ); }); - it('array required', function(done) { + it('array required', async function() { const Loki = new Schema({ likes: { type: Array, required: true } }); - let remaining = 2; - - Loki.path('likes').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Loki.path('likes').doValidate(null), + ValidatorError + ); - Loki.path('likes').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Loki.path('likes').doValidate(undefined), + ValidatorError + ); }); - it('array required custom required', function(done) { + it('array required custom required', async function() { const requiredOrig = mongoose.Schema.Types.Array.checkRequired(); mongoose.Schema.Types.Array.checkRequired(v => Array.isArray(v) && v.length); - const doneWrapper = (err) => { - mongoose.Schema.Types.Array.checkRequired(requiredOrig); - done(err); - }; - - const Loki = new Schema({ - likes: { type: Array, required: true } - }); - - let remaining = 2; + try { + const Loki = new Schema({ + likes: { type: Array, required: true } + }); - Loki.path('likes').doValidate([], function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || doneWrapper(); - }); + await assert.rejects( + Loki.path('likes').doValidate([]), + ValidatorError + ); - Loki.path('likes').doValidate(['cake'], function(err) { - assert(!err); - --remaining || doneWrapper(); - }); + await Loki.path('likes').doValidate(['cake']); + } finally { + mongoose.Schema.Types.Array.checkRequired(requiredOrig); + } }); - it('boolean required', function(done) { + it('boolean required', async function() { const Animal = new Schema({ isFerret: { type: Boolean, required: true } }); - let remaining = 4; - - Animal.path('isFerret').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); - - Animal.path('isFerret').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); - - Animal.path('isFerret').doValidate(true, function(err) { - assert.ifError(err); - --remaining || done(); - }); - - Animal.path('isFerret').doValidate(false, function(err) { - assert.ifError(err); - --remaining || done(); - }); + await assert.rejects(Animal.path('isFerret').doValidate(null), ValidatorError); + await assert.rejects(Animal.path('isFerret').doValidate(undefined), ValidatorError); + await Animal.path('isFerret').doValidate(true); + await Animal.path('isFerret').doValidate(false); }); - it('mixed required', function(done) { + it('mixed required', async function() { const Animal = new Schema({ characteristics: { type: Mixed, required: true } }); - let remaining = 4; + await assert.rejects( + Animal.path('characteristics').doValidate(null), + ValidatorError + ); - Animal.path('characteristics').doValidate(null, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); + await assert.rejects( + Animal.path('characteristics').doValidate(undefined), + ValidatorError + ); - Animal.path('characteristics').doValidate(undefined, function(err) { - assert.ok(err instanceof ValidatorError); - --remaining || done(); - }); - - Animal.path('characteristics').doValidate({ + await Animal.path('characteristics').doValidate({ aggresive: true - }, function(err) { - assert.ifError(err); - --remaining || done(); }); - Animal.path('characteristics').doValidate('none available', function(err) { - assert.ifError(err); - --remaining || done(); - }); + await Animal.path('characteristics').doValidate('none available'); }); }); describe('async', function() { - it('works', function(done) { - let executed = 0; - + it('works', async function() { function validator(value) { return new Promise(function(resolve) { setTimeout(function() { - executed++; resolve(value === true); - if (executed === 2) { - done(); - } }, 5); }); } @@ -526,16 +403,15 @@ describe('schema', function() { ferret: { type: Boolean, validate: validator } }); - Animal.path('ferret').doValidate(true, function(err) { - assert.ifError(err); - }); + await Animal.path('ferret').doValidate(true); - Animal.path('ferret').doValidate(false, function(err) { - assert.ok(err instanceof Error); - }); + await assert.rejects( + Animal.path('ferret').doValidate(false), + ValidatorError + ); }); - it('scope', function(done) { + it('scope', async function() { let called = false; function validator() { @@ -555,11 +431,8 @@ describe('schema', function() { } }); - Animal.path('ferret').doValidate(true, function(err) { - assert.ifError(err); - assert.equal(called, true); - done(); - }, { a: 'b' }); + await Animal.path('ferret').doValidate(true, { a: 'b' }); + assert.equal(called, true); }); it('doValidateSync should ignore async function and script waiting for promises (gh-4885)', function(done) { diff --git a/test/updateValidators.unit.test.js b/test/updateValidators.unit.test.js deleted file mode 100644 index 62a545261e6..00000000000 --- a/test/updateValidators.unit.test.js +++ /dev/null @@ -1,111 +0,0 @@ -'use strict'; - -require('./common'); - -const Schema = require('../lib/schema'); -const assert = require('assert'); -const updateValidators = require('../lib/helpers/updateValidators'); -const emitter = require('events').EventEmitter; - -describe('updateValidators', function() { - let schema; - - beforeEach(function() { - schema = {}; - schema._getSchema = function(p) { - schema._getSchema.calls.push(p); - return schema; - }; - schema._getSchema.calls = []; - schema.doValidate = function(v, cb) { - schema.doValidate.calls.push({ v: v, cb: cb }); - schema.doValidate.emitter.emit('called', { v: v, cb: cb }); - }; - schema.doValidate.calls = []; - schema.doValidate.emitter = new emitter(); - }); - - describe('validators', function() { - it('flattens paths', function(done) { - const fn = updateValidators({}, schema, { test: { a: 1, b: null } }, {}); - schema.doValidate.emitter.on('called', function(args) { - args.cb(); - }); - fn(function(err) { - assert.ifError(err); - assert.equal(schema._getSchema.calls.length, 3); - assert.equal(schema.doValidate.calls.length, 3); - assert.equal(schema._getSchema.calls[0], 'test'); - assert.equal(schema._getSchema.calls[1], 'test.a'); - assert.equal(schema._getSchema.calls[2], 'test.b'); - assert.deepEqual(schema.doValidate.calls[0].v, { - a: 1, - b: null - }); - assert.equal(schema.doValidate.calls[1].v, 1); - assert.equal(schema.doValidate.calls[2].v, null); - done(); - }); - }); - - it('doesnt flatten dates (gh-3194)', function(done) { - const dt = new Date(); - const fn = updateValidators({}, schema, { test: dt }, {}); - schema.doValidate.emitter.on('called', function(args) { - args.cb(); - }); - fn(function(err) { - assert.ifError(err); - assert.equal(schema._getSchema.calls.length, 1); - assert.equal(schema.doValidate.calls.length, 1); - assert.equal(schema._getSchema.calls[0], 'test'); - assert.equal(dt, schema.doValidate.calls[0].v); - done(); - }); - }); - - it('doesnt flatten empty arrays (gh-3554)', function(done) { - const fn = updateValidators({}, schema, { test: [] }, {}); - schema.doValidate.emitter.on('called', function(args) { - args.cb(); - }); - fn(function(err) { - assert.ifError(err); - assert.equal(schema._getSchema.calls.length, 1); - assert.equal(schema.doValidate.calls.length, 1); - assert.equal(schema._getSchema.calls[0], 'test'); - assert.deepEqual(schema.doValidate.calls[0].v, []); - done(); - }); - }); - - it('doesnt flatten decimal128 (gh-7561)', function(done) { - const Decimal128Type = require('../lib/types/decimal128'); - const schema = Schema({ test: { type: 'Decimal128', required: true } }); - const fn = updateValidators({}, schema, { - test: new Decimal128Type('33.426') - }, {}); - fn(function(err) { - assert.ifError(err); - done(); - }); - }); - - it('handles nested paths correctly (gh-3587)', function(done) { - const schema = Schema({ - nested: { - a: { type: String, required: true }, - b: { type: String, required: true } - } - }); - const fn = updateValidators({}, schema, { - nested: { b: 'test' } - }, {}); - fn(function(err) { - assert.ok(err); - assert.deepEqual(Object.keys(err.errors), ['nested.a']); - done(); - }); - }); - }); -}); From 50bca1fc2d8714629f7983a393139a78ade34e60 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 10 Mar 2025 14:06:16 -0400 Subject: [PATCH 2/7] refactor: make validateBeforeSave async for better stack traces --- lib/plugins/validateBeforeSave.js | 16 +++------------- test/schema.validation.test.js | 1 - 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/lib/plugins/validateBeforeSave.js b/lib/plugins/validateBeforeSave.js index c55824184ac..6d1cebdd9d5 100644 --- a/lib/plugins/validateBeforeSave.js +++ b/lib/plugins/validateBeforeSave.js @@ -6,11 +6,10 @@ module.exports = function validateBeforeSave(schema) { const unshift = true; - schema.pre('save', false, function validateBeforeSave(next, options) { - const _this = this; + schema.pre('save', false, async function validateBeforeSave(_next, options) { // Nested docs have their own presave if (this.$isSubdocument) { - return next(); + return; } const hasValidateBeforeSaveOption = options && @@ -32,20 +31,11 @@ module.exports = function validateBeforeSave(schema) { const validateOptions = hasValidateModifiedOnlyOption ? { validateModifiedOnly: options.validateModifiedOnly } : null; - this.$validate(validateOptions).then( + await this.$validate(validateOptions).then( () => { this.$op = 'save'; - next(); - }, - error => { - _this.$__schema.s.hooks.execPost('save:error', _this, [_this], { error: error }, function(error) { - _this.$op = 'save'; - next(error); - }); } ); - } else { - next(); } }, null, unshift); }; diff --git a/test/schema.validation.test.js b/test/schema.validation.test.js index 243328c2f76..d246630c4fd 100644 --- a/test/schema.validation.test.js +++ b/test/schema.validation.test.js @@ -16,7 +16,6 @@ const ValidatorError = mongoose.Error.ValidatorError; const SchemaTypes = Schema.Types; const ObjectId = SchemaTypes.ObjectId; const Mixed = SchemaTypes.Mixed; -const DocumentObjectId = mongoose.Types.ObjectId; describe('schema', function() { describe('validation', function() { From fae948fc97fac3eaf587fbe5867553f989bfd91b Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 11 Mar 2025 18:09:58 -0400 Subject: [PATCH 3/7] BREAKING CHANGE: make validateBeforeSave async and stop relying on Kareem wrappers for save hooks --- lib/document.js | 11 ++- lib/helpers/model/applyHooks.js | 4 +- lib/model.js | 153 +++++++++++++++++--------------- lib/types/subdocument.js | 21 +++-- test/document.test.js | 4 +- test/model.middleware.test.js | 2 +- 6 files changed, 106 insertions(+), 89 deletions(-) diff --git a/lib/document.js b/lib/document.js index d4935c33e0a..afd652db08e 100644 --- a/lib/document.js +++ b/lib/document.js @@ -2880,9 +2880,9 @@ function _pushNestedArrayPaths(val, paths, path) { * ignore */ -Document.prototype._execDocumentPreHooks = function _execDocumentPreHooks(opName) { +Document.prototype._execDocumentPreHooks = function _execDocumentPreHooks(opName, ...args) { return new Promise((resolve, reject) => { - this.constructor._middleware.execPre(opName, this, [], (error) => { + this.constructor._middleware.execPre(opName, this, [...args], (error) => { if (error != null) { reject(error); return; @@ -2912,7 +2912,12 @@ Document.prototype._execDocumentPostHooks = function _execDocumentPostHooks(opNa */ Document.prototype.$__validate = async function $__validate(pathsToValidate, options) { - await this._execDocumentPreHooks('validate'); + try { + await this._execDocumentPreHooks('validate'); + } catch (error) { + await this._execDocumentPostHooks('validate', error); + return; + } if (this.$__.saveOptions && this.$__.saveOptions.pathsToSave && !pathsToValidate) { pathsToValidate = [...this.$__.saveOptions.pathsToSave]; diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 00262792bf6..87edc322ded 100644 --- a/lib/helpers/model/applyHooks.js +++ b/lib/helpers/model/applyHooks.js @@ -15,8 +15,8 @@ module.exports = applyHooks; applyHooks.middlewareFunctions = [ 'deleteOne', - 'save', 'remove', + 'save', 'updateOne', 'init' ]; @@ -119,7 +119,7 @@ function applyHooks(model, schema, options) { model._middleware = middleware; - const internalMethodsToWrap = options && options.isChildSchema ? ['save', 'deleteOne'] : ['save']; + const internalMethodsToWrap = options && options.isChildSchema ? ['deleteOne'] : []; for (const method of internalMethodsToWrap) { const toWrap = `$__${method}`; const wrapped = middleware. diff --git a/lib/model.js b/lib/model.js index 4c2247a4ea1..19f9cfd0f2f 100644 --- a/lib/model.js +++ b/lib/model.js @@ -493,70 +493,84 @@ Model.prototype.$__handleSave = function(options, callback) { * ignore */ -Model.prototype.$__save = function(options, callback) { - this.$__handleSave(options, (error, result) => { - if (error) { - error = this.$__schema._transformDuplicateKeyError(error); - const hooks = this.$__schema.s.hooks; - return hooks.execPost('save:error', this, [this], { error: error }, (error) => { - callback(error, this); - }); - } - let numAffected = 0; - const writeConcern = options != null ? - options.writeConcern != null ? - options.writeConcern.w : - options.w : - 0; - if (writeConcern !== 0) { - // Skip checking if write succeeded if writeConcern is set to - // unacknowledged writes, because otherwise `numAffected` will always be 0 - if (result != null) { - if (Array.isArray(result)) { - numAffected = result.length; - } else if (result.matchedCount != null) { - numAffected = result.matchedCount; - } else { - numAffected = result; - } - } +Model.prototype.$__save = async function $__save(options) { + try { + await this._execDocumentPreHooks('save', options); + } catch (error) { + await this._execDocumentPostHooks('save', error); + return; + } - const versionBump = this.$__.version; - // was this an update that required a version bump? - if (versionBump && !this.$__.inserting) { - const doIncrement = VERSION_INC === (VERSION_INC & this.$__.version); - this.$__.version = undefined; - const key = this.$__schema.options.versionKey; - const version = this.$__getValue(key) || 0; - if (numAffected <= 0) { - // the update failed. pass an error back - this.$__undoReset(); - const err = this.$__.$versionError || - new VersionError(this, version, this.$__.modifiedPaths); - return callback(err); - } - // increment version if was successful - if (doIncrement) { - this.$__setValue(key, version + 1); + let result = null; + try { + result = await new Promise((resolve, reject) => { + this.$__handleSave(options, (error, result) => { + if (error) { + return reject(error); } + resolve(result); + }); + }); + } catch (err) { + const error = this.$__schema._transformDuplicateKeyError(err); + await this._execDocumentPostHooks('save', error); + return; + } + + let numAffected = 0; + const writeConcern = options != null ? + options.writeConcern != null ? + options.writeConcern.w : + options.w : + 0; + if (writeConcern !== 0) { + // Skip checking if write succeeded if writeConcern is set to + // unacknowledged writes, because otherwise `numAffected` will always be 0 + if (result != null) { + if (Array.isArray(result)) { + numAffected = result.length; + } else if (result.matchedCount != null) { + numAffected = result.matchedCount; + } else { + numAffected = result; } - if (result != null && numAffected <= 0) { + } + + const versionBump = this.$__.version; + // was this an update that required a version bump? + if (versionBump && !this.$__.inserting) { + const doIncrement = VERSION_INC === (VERSION_INC & this.$__.version); + this.$__.version = undefined; + const key = this.$__schema.options.versionKey; + const version = this.$__getValue(key) || 0; + if (numAffected <= 0) { + // the update failed. pass an error back this.$__undoReset(); - error = new DocumentNotFoundError(result.$where, - this.constructor.modelName, numAffected, result); - const hooks = this.$__schema.s.hooks; - return hooks.execPost('save:error', this, [this], { error: error }, (error) => { - callback(error, this); - }); + const err = this.$__.$versionError || + new VersionError(this, version, this.$__.modifiedPaths); + await this._execDocumentPostHooks('save', err); + return; + } + + // increment version if was successful + if (doIncrement) { + this.$__setValue(key, version + 1); } } - this.$__.saving = undefined; - this.$__.savedState = {}; - this.$emit('save', this, numAffected); - this.constructor.emit('save', this, numAffected); - callback(null, this); - }); + if (result != null && numAffected <= 0) { + this.$__undoReset(); + const error = new DocumentNotFoundError(result.$where, + this.constructor.modelName, numAffected, result); + await this._execDocumentPostHooks('save', error); + return; + } + } + this.$__.saving = undefined; + this.$__.savedState = {}; + this.$emit('save', this, numAffected); + this.constructor.emit('save', this, numAffected); + await this._execDocumentPostHooks('save'); }; /*! @@ -636,20 +650,17 @@ Model.prototype.save = async function save(options) { this.$__.saveOptions = options; - await new Promise((resolve, reject) => { - this.$__save(options, error => { - this.$__.saving = null; - this.$__.saveOptions = null; - this.$__.$versionError = null; - this.$op = null; - if (error != null) { - this.$__handleReject(error); - return reject(error); - } - - resolve(); - }); - }); + try { + await this.$__save(options); + } catch (error) { + this.$__handleReject(error); + throw error; + } finally { + this.$__.saving = null; + this.$__.saveOptions = null; + this.$__.$versionError = null; + this.$op = null; + } return this; }; diff --git a/lib/types/subdocument.js b/lib/types/subdocument.js index b1984d08ebf..550471a59db 100644 --- a/lib/types/subdocument.js +++ b/lib/types/subdocument.js @@ -1,7 +1,6 @@ 'use strict'; const Document = require('../document'); -const immediate = require('../helpers/immediate'); const internalToObjectOptions = require('../options').internalToObjectOptions; const util = require('util'); const utils = require('../utils'); @@ -80,14 +79,7 @@ Subdocument.prototype.save = async function save(options) { 'if you\'re sure this behavior is right for your app.'); } - return new Promise((resolve, reject) => { - this.$__save((err) => { - if (err != null) { - return reject(err); - } - resolve(this); - }); - }); + return await this.$__save(); }; /** @@ -141,8 +133,15 @@ Subdocument.prototype.$__pathRelativeToParent = function(p) { * @api private */ -Subdocument.prototype.$__save = function(fn) { - return immediate(() => fn(null, this)); +Subdocument.prototype.$__save = async function $__save() { + try { + await this._execDocumentPreHooks('save'); + } catch (error) { + await this._execDocumentPostHooks('save', error); + return; + } + + await this._execDocumentPostHooks('save'); }; /*! diff --git a/test/document.test.js b/test/document.test.js index c7463e01229..06ac93e0459 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -9750,7 +9750,7 @@ describe('document', function() { const schema = Schema({ name: String }); let called = 0; - schema.pre(/.*/, { document: true, query: false }, function() { + schema.pre(/.*/, { document: true, query: false }, function testPreSave9190() { ++called; }); const Model = db.model('Test', schema); @@ -9765,7 +9765,9 @@ describe('document', function() { await Model.countDocuments(); assert.equal(called, 0); + console.log('-----'); const docs = await Model.create([{ name: 'test' }], { validateBeforeSave: false }); + console.log('------'); assert.equal(called, 1); await docs[0].validate(); diff --git a/test/model.middleware.test.js b/test/model.middleware.test.js index 92ca5224dee..4aca8c5516c 100644 --- a/test/model.middleware.test.js +++ b/test/model.middleware.test.js @@ -293,7 +293,7 @@ describe('model middleware', function() { title: String }); - schema.post('save', function() { + schema.post('save', function postSaveTestError() { throw new Error('woops!'); }); From 0f6b4aadb91984db9cf0d5f73d6c11746eaa4a83 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 12 Mar 2025 10:48:09 -0400 Subject: [PATCH 4/7] address code review comments --- docs/migrating_to_9.md | 25 +++++++++++++++++++++++++ lib/helpers/model/applyHooks.js | 1 + lib/schema.js | 2 +- test/document.test.js | 2 -- 4 files changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/migrating_to_9.md b/docs/migrating_to_9.md index 9dd3dbbfa88..771839b60c2 100644 --- a/docs/migrating_to_9.md +++ b/docs/migrating_to_9.md @@ -11,3 +11,28 @@ There are several backwards-breaking changes you should be aware of when migrati If you're still on Mongoose 7.x or earlier, please read the [Mongoose 7.x to 8.x migration guide](migrating_to_8.html) and upgrade to Mongoose 8.x first before upgrading to Mongoose 9. ## `Schema.prototype.doValidate()` now returns a promise + +`Schema.prototype.doValidate()` now returns a promise that rejects with a validation error if one occurred. +In Mongoose 8.x, `doValidate()` took a callback and did not return a promise. + +```javascript +// Mongoose 8.x function signature +function doValidate(value, cb, scope, options) {} + +// Mongoose 8.x example usage +schema.doValidate(value, function(error) { + if (error) { + // Handle validation error + } +}, scope, options); + +// Mongoose 9.x function signature +async function doValidate(value, scope, options) {} + +// Mongoose 9.x example usage +try { + await schema.doValidate(value, scope, options); +} catch (error) { + // Handle validation error +} +``` diff --git a/lib/helpers/model/applyHooks.js b/lib/helpers/model/applyHooks.js index 87edc322ded..25ec30eb9e6 100644 --- a/lib/helpers/model/applyHooks.js +++ b/lib/helpers/model/applyHooks.js @@ -18,6 +18,7 @@ applyHooks.middlewareFunctions = [ 'remove', 'save', 'updateOne', + 'validate', 'init' ]; diff --git a/lib/schema.js b/lib/schema.js index f7528f6b4b4..a9e795120d9 100644 --- a/lib/schema.js +++ b/lib/schema.js @@ -31,7 +31,7 @@ const hasNumericSubpathRegex = /\.\d+(\.|$)/; let MongooseTypes; const queryHooks = require('./constants').queryMiddlewareFunctions; -const documentHooks = require('./helpers/model/applyHooks').middlewareFunctions; +const documentHooks = require('./constants').documentMiddlewareFunctions; const hookNames = queryHooks.concat(documentHooks). reduce((s, hook) => s.add(hook), new Set()); diff --git a/test/document.test.js b/test/document.test.js index 06ac93e0459..0d958f409f8 100644 --- a/test/document.test.js +++ b/test/document.test.js @@ -9765,9 +9765,7 @@ describe('document', function() { await Model.countDocuments(); assert.equal(called, 0); - console.log('-----'); const docs = await Model.create([{ name: 'test' }], { validateBeforeSave: false }); - console.log('------'); assert.equal(called, 1); await docs[0].validate(); From 59e9381def353633dd7df5e7e9050ac5c96e92e8 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 12 Mar 2025 14:54:30 -0400 Subject: [PATCH 5/7] remove vestigial :error post hook calls --- lib/plugins/saveSubdocs.js | 18 ++---------------- test/model.test.js | 6 ++++++ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index bb88db59f85..6a163762ed2 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -32,9 +32,7 @@ module.exports = function saveSubdocs(schema) { _this.$__.saveOptions.__subdocs = null; } if (error) { - return _this.$__schema.s.hooks.execPost('save:error', _this, [_this], { error: error }, function(error) { - next(error); - }); + return next(error); } next(); }); @@ -67,7 +65,6 @@ module.exports = function saveSubdocs(schema) { return; } - const _this = this; const subdocs = this.$getAllSubdocs({ useCache: true }); if (!subdocs.length) { @@ -86,17 +83,6 @@ module.exports = function saveSubdocs(schema) { })); } - try { - await Promise.all(promises); - } catch (error) { - await new Promise((resolve, reject) => { - this.$__schema.s.hooks.execPost('save:error', _this, [_this], { error: error }, function(error) { - if (error) { - return reject(error); - } - resolve(); - }); - }); - } + await Promise.all(promises); }, null, unshift); }; diff --git a/test/model.test.js b/test/model.test.js index d4aa35ec686..7546313e1a7 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -2154,17 +2154,23 @@ describe('Model', function() { next(); }); + let schemaPostSaveCalls = 0; const schema = new Schema({ name: String, child: [childSchema] }); schema.pre('save', function(next) { this.name = 'parent'; next(); }); + schema.post('save', function testSchemaPostSave(err, res, next) { + ++schemaPostSaveCalls; + next(err); + }); const S = db.model('Test', schema); const s = new S({ name: 'a', child: [{ name: 'b', grand: [{ name: 'c' }] }] }); const err = await s.save().then(() => null, err => err); assert.equal(err.message, 'Error 101'); + assert.equal(schemaPostSaveCalls, 1); }); describe('init', function() { From 92d6ca5055eef2e91d75c755ea615a459e9c027b Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 12 Mar 2025 15:12:33 -0400 Subject: [PATCH 6/7] refactor: make saveSubdocsPreSave async --- lib/plugins/saveSubdocs.js | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index 6a163762ed2..22d8e2a8046 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -1,16 +1,13 @@ 'use strict'; -const each = require('../helpers/each'); - /*! * ignore */ module.exports = function saveSubdocs(schema) { const unshift = true; - schema.s.hooks.pre('save', false, function saveSubdocsPreSave(next) { + schema.s.hooks.pre('save', false, async function saveSubdocsPreSave() { if (this.$isSubdocument) { - next(); return; } @@ -18,24 +15,22 @@ module.exports = function saveSubdocs(schema) { const subdocs = this.$getAllSubdocs({ useCache: true }); if (!subdocs.length) { - next(); return; } - each(subdocs, function(subdoc, cb) { - subdoc.$__schema.s.hooks.execPre('save', subdoc, function(err) { - cb(err); + 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(); + }); }); - }, function(error) { - // Invalidate subdocs cache because subdoc pre hooks can add new subdocuments - if (_this.$__.saveOptions) { - _this.$__.saveOptions.__subdocs = null; - } - if (error) { - return next(error); - } - next(); - }); + })); + + // Invalidate subdocs cache because subdoc pre hooks can add new subdocuments + if (_this.$__.saveOptions) { + _this.$__.saveOptions.__subdocs = null; + } }, null, unshift); schema.s.hooks.post('save', async function saveSubdocsPostDeleteOne() { From 80a2ed33fadc03bad28bcbe16be16dd5f271f6d7 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 12 Mar 2025 15:14:20 -0400 Subject: [PATCH 7/7] style: fix lint --- lib/plugins/saveSubdocs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plugins/saveSubdocs.js b/lib/plugins/saveSubdocs.js index 22d8e2a8046..d4c899cba5e 100644 --- a/lib/plugins/saveSubdocs.js +++ b/lib/plugins/saveSubdocs.js @@ -18,7 +18,7 @@ module.exports = function saveSubdocs(schema) { return; } - await Promise.all(subdocs.map(async (subdoc) => { + 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);