From bb9bc358b5362b4f825dc574b5d1d1023af5d353 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 17 May 2023 15:54:45 -0400 Subject: [PATCH 1/2] feat(model): add throwOnValidationError option for opting into getting MongooseBulkWriteError if all valid operations succeed in `bulkWrite()` and `insertMany()` Fix #13256 --- lib/error/bulkWriteError.js | 41 ++++++++++++++++++++++++ lib/model.js | 38 +++++++++++++++++++++-- test/model.test.js | 62 +++++++++++++++++++++++++++++++++++++ types/models.d.ts | 2 ++ 4 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 lib/error/bulkWriteError.js diff --git a/lib/error/bulkWriteError.js b/lib/error/bulkWriteError.js new file mode 100644 index 00000000000..1711b03b586 --- /dev/null +++ b/lib/error/bulkWriteError.js @@ -0,0 +1,41 @@ +/*! + * Module dependencies. + */ + +'use strict'; + +const MongooseError = require('./'); + + +/** + * If `bulkWrite()` or `insertMany()` has validation errors, but + * all valid operations succeed, and 'throwOnValidationError' is true, + * Mongoose will throw this error. + * + * @api private + */ + +class MongooseBulkWriteError extends MongooseError { + constructor(validationErrors, results, rawResult, operation) { + let preview = validationErrors.map(e => e.message).join(', '); + if (preview.length > 200) { + preview = preview.slice(0, 200) + '...'; + } + super(`${operation} failed with ${validationErrors.length} Mongoose validation errors: ${preview}`); + + this.validationErrors = validationErrors; + this.results = results; + this.rawResult = rawResult; + this.operation = operation; + } +} + +Object.defineProperty(MongooseBulkWriteError.prototype, 'name', { + value: 'MongooseBulkWriteError' +}); + +/*! + * exports + */ + +module.exports = MongooseBulkWriteError; diff --git a/lib/model.js b/lib/model.js index 361301bdb38..62a4b014837 100644 --- a/lib/model.js +++ b/lib/model.js @@ -64,6 +64,7 @@ const setDottedPath = require('./helpers/path/setDottedPath'); const STATES = require('./connectionstate'); const util = require('util'); const utils = require('./utils'); +const MongooseBulkWriteError = require('./error/bulkWriteError'); const VERSION_WHERE = 1; const VERSION_INC = 2; @@ -2962,6 +2963,7 @@ Model.startSession = function() { * @param {Boolean} [options.lean=false] if `true`, skips hydrating and validating the documents. This option is useful if you need the extra performance, but Mongoose won't validate the documents before inserting. * @param {Number} [options.limit=null] this limits the number of documents being processed (validation/casting) by mongoose in parallel, this does **NOT** send the documents in batches to MongoDB. Use this option if you're processing a large number of documents and your app is running out of memory. * @param {String|Object|Array} [options.populate=null] populates the result documents. This option is a no-op if `rawResult` is set. + * @param {Boolean} [options.throwOnValidationError=false] If true and `ordered: false`, throw an error if one of the operations failed validation, but all valid operations completed successfully. * @return {Promise} resolving to the raw result from the MongoDB driver if `options.rawResult` was `true`, or the documents that passed validation, otherwise * @api public */ @@ -3007,6 +3009,7 @@ Model.$__insertMany = function(arr, options, callback) { const limit = options.limit || 1000; const rawResult = !!options.rawResult; const ordered = typeof options.ordered === 'boolean' ? options.ordered : true; + const throwOnValidationError = typeof options.throwOnValidationError === 'boolean' ? options.throwOnValidationError : false; const lean = !!options.lean; if (!Array.isArray(arr)) { @@ -3113,6 +3116,20 @@ Model.$__insertMany = function(arr, options, callback) { _setIsNew(attribute, false); } + if (ordered === false && throwOnValidationError && validationErrors.length > 0) { + for (let i = 0; i < results.length; ++i) { + if (results[i] === void 0) { + results[i] = docs[i]; + } + } + return callback(new MongooseBulkWriteError( + validationErrors, + results, + res, + 'insertMany' + )); + } + if (rawResult) { if (ordered === false) { for (let i = 0; i < results.length; ++i) { @@ -3308,6 +3325,7 @@ function _setIsNew(doc, val) { * @param {Boolean} [options.j=true] If false, disable [journal acknowledgement](https://www.mongodb.com/docs/manual/reference/write-concern/#j-option) * @param {Boolean} [options.skipValidation=false] Set to true to skip Mongoose schema validation on bulk write operations. Mongoose currently runs validation on `insertOne` and `replaceOne` operations by default. * @param {Boolean} [options.bypassDocumentValidation=false] If true, disable [MongoDB server-side schema validation](https://www.mongodb.com/docs/manual/core/schema-validation/) for all writes in this bulk. + * @param {Boolean} [options.throwOnValidationError=false] If true and `ordered: false`, throw an error if one of the operations failed validation, but all valid operations completed successfully. * @param {Boolean} [options.strict=null] Overwrites the [`strict` option](https://mongoosejs.com/docs/guide.html#strict) on schema. If false, allows filtering and writing fields not defined in the schema for all writes in this bulk. * @return {Promise} resolves to a [`BulkWriteOpResult`](https://mongodb.github.io/node-mongodb-native/4.9/classes/BulkWriteResult.html) if the operation succeeds * @api public @@ -3355,12 +3373,14 @@ Model.bulkWrite = async function bulkWrite(ops, options) { let remaining = validations.length; let validOps = []; let validationErrors = []; + const results = []; for (let i = 0; i < validations.length; ++i) { validations[i]((err) => { if (err == null) { validOps.push(i); } else { validationErrors.push({ index: i, error: err }); + results[i] = err; } if (--remaining <= 0) { completeUnorderedValidation.call(this); @@ -3373,6 +3393,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { map(v => v.error); function completeUnorderedValidation() { + const validOpIndexes = validOps; validOps = validOps.sort().map(index => ops[index]); this.$__collection.bulkWrite(validOps, options, (error, res) => { @@ -3385,9 +3406,22 @@ Model.bulkWrite = async function bulkWrite(ops, options) { return reject(error); } + for (let i = 0; i < validOpIndexes.length; ++i) { + results[validOpIndexes[i]] = null; + } if (validationErrors.length > 0) { - res.mongoose = res.mongoose || {}; - res.mongoose.validationErrors = validationErrors; + if ('throwOnValidationError' in options && options.throwOnValidationError) { + return reject(new MongooseBulkWriteError( + validationErrors, + results, + res, + 'bulkWrite' + )); + } else { + res.mongoose = res.mongoose || {}; + res.mongoose.validationErrors = validationErrors; + res.mongoose.results = results; + } } resolve(res); diff --git a/test/model.test.js b/test/model.test.js index c6e0648c9a0..32aa31b027e 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -4189,6 +4189,45 @@ describe('Model', function() { const { num } = await Test.findById(_id); assert.equal(num, 99); }); + + it('bulkWrite should throw an error if there were operations that failed validation, ' + + 'but all operations that passed validation succeeded (gh-13256)', async function() { + const userSchema = new Schema({ age: { type: Number } }); + const User = db.model('User', userSchema); + + const createdUser = await User.create({ name: 'Test' }); + + const err = await User.bulkWrite([ + { + updateOne: { + filter: { _id: createdUser._id }, + update: { $set: { age: 'NaN' } }, + upsert: true + } + }, + { + updateOne: { + filter: { _id: createdUser._id }, + update: { $set: { age: 13 } }, + upsert: true + } + }, + { + updateOne: { + filter: { _id: createdUser._id }, + update: { $set: { age: 12 } }, + upsert: true + } + } + ], { ordered: false, throwOnValidationError: true }) + .then(() => null) + .catch(err => err); + + assert.ok(err); + assert.equal(err.name, 'MongooseBulkWriteError'); + assert.equal(err.validationErrors[0].path, 'age'); + assert.equal(err.results[0].path, 'age'); + }); }); it('deleteOne with cast error (gh-5323)', async function() { @@ -6177,6 +6216,29 @@ describe('Model', function() { }); + it('insertMany should throw an error if there were operations that failed validation, ' + + 'but all operations that passed validation succeeded (gh-13256)', async function() { + const userSchema = new Schema({ + age: { type: Number } + }); + + const User = db.model('User', userSchema); + + const err = await User.insertMany([ + new User({ age: 12 }), + new User({ age: 12 }), + new User({ age: 'NaN' }) + ], { ordered: false, throwOnValidationError: true }) + .then(() => null) + .catch(err => err); + + assert.ok(err); + assert.equal(err.name, 'MongooseBulkWriteError'); + assert.equal(err.validationErrors[0].errors['age'].name, 'CastError'); + assert.ok(err.results[2] instanceof Error); + assert.equal(err.results[2].errors['age'].name, 'CastError'); + }); + it('returns writeResult on success', async() => { const userSchema = new Schema({ diff --git a/types/models.d.ts b/types/models.d.ts index 1efcd565063..f99492dca31 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -25,6 +25,7 @@ declare module 'mongoose' { interface MongooseBulkWriteOptions { skipValidation?: boolean; + throwOnValidationError?: boolean; } interface InsertManyOptions extends @@ -34,6 +35,7 @@ declare module 'mongoose' { rawResult?: boolean; ordered?: boolean; lean?: boolean; + throwOnValidationError?: boolean; } type InsertManyResult = mongodb.InsertManyResult & { From cc2adecfc7424f43c59cac259998a3cd2f6fb7b2 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Thu, 18 May 2023 18:24:11 -0400 Subject: [PATCH 2/2] test: fix some code review comments --- lib/model.js | 2 +- test/model.test.js | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/model.js b/lib/model.js index 62a4b014837..b322ed23653 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3410,7 +3410,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { results[validOpIndexes[i]] = null; } if (validationErrors.length > 0) { - if ('throwOnValidationError' in options && options.throwOnValidationError) { + if (options.throwOnValidationError) { return reject(new MongooseBulkWriteError( validationErrors, results, diff --git a/test/model.test.js b/test/model.test.js index 32aa31b027e..d448edae8fc 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -6237,6 +6237,9 @@ describe('Model', function() { assert.equal(err.validationErrors[0].errors['age'].name, 'CastError'); assert.ok(err.results[2] instanceof Error); assert.equal(err.results[2].errors['age'].name, 'CastError'); + + const docs = await User.find(); + assert.deepStrictEqual(docs.map(doc => doc.age), [12, 12]); }); it('returns writeResult on success', async() => {