Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(model): add throwOnValidationError option for opting into getting MongooseBulkWriteError if all valid operations succeed in bulkWrite() and insertMany() #13410

Merged
merged 2 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 41 additions & 0 deletions lib/error/bulkWriteError.js
Original file line number Diff line number Diff line change
@@ -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;
38 changes: 36 additions & 2 deletions lib/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that this should default to true, since this is the intuitive behavior anyway, we can consider this as a bug rather than a missing feature. People should opt-out if they want backward-compatible behavior for some reason, then on v8.0 we can remove the option altogether.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we have tests that assert on the counterintuitive behavior, and for stability's sake I like to avoid changing existing tests outside of major releases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think it's good to merge as is, and remove the option in v8.0.

What do you think? Or would you rather to keep it as an option in v8, and maybe just change the default?

* @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
*/
Expand Down Expand Up @@ -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)) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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) => {
Expand All @@ -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 (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);
Expand Down
65 changes: 65 additions & 0 deletions test/model.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -6177,6 +6216,32 @@ 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);
vkarpov15 marked this conversation as resolved.
Show resolved Hide resolved
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');

const docs = await User.find();
assert.deepStrictEqual(docs.map(doc => doc.age), [12, 12]);
});

it('returns writeResult on success', async() => {

const userSchema = new Schema({
Expand Down
2 changes: 2 additions & 0 deletions types/models.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ declare module 'mongoose' {

interface MongooseBulkWriteOptions {
skipValidation?: boolean;
throwOnValidationError?: boolean;
}

interface InsertManyOptions extends
Expand All @@ -34,6 +35,7 @@ declare module 'mongoose' {
rawResult?: boolean;
ordered?: boolean;
lean?: boolean;
throwOnValidationError?: boolean;
}

type InsertManyResult<T> = mongodb.InsertManyResult<T> & {
Expand Down