From 04e69797652f338edd9d75f8530eb784a72d0a37 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 27 Mar 2023 12:58:38 -0400 Subject: [PATCH 1/2] fix(model): execute valid write operations if calling `bulkWrite()` with ordered: false Fix #13176 --- lib/model.js | 82 ++++++++++++++++++++++++++++++++++++---------- test/model.test.js | 63 +++++++++++++++++++++++++++++++++++ types/models.d.ts | 11 +++++-- 3 files changed, 136 insertions(+), 20 deletions(-) diff --git a/lib/model.js b/lib/model.js index f26c18babbb..aa00931a6de 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3736,33 +3736,81 @@ Model.bulkWrite = function(ops, options, callback) { options = null; } options = options || {}; + const ordered = options.ordered == null ? true : options.ordered; const validations = ops.map(op => castBulkWrite(this, op, options)); callback = this.$handleCallbackError(callback); return this.db.base._promiseOrCallback(callback, cb => { cb = this.$wrapCallback(cb); - each(validations, (fn, cb) => fn(cb), error => { - if (error) { - return cb(error); - } + if (ordered) { + each(validations, (fn, cb) => fn(cb), error => { + if (error) { + return cb(error); + } - if (ops.length === 0) { - return cb(null, getDefaultBulkwriteResult()); - } + if (ops.length === 0) { + return cb(null, getDefaultBulkwriteResult()); + } - try { - this.$__collection.bulkWrite(ops, options, (error, res) => { - if (error) { - return cb(error); + try { + this.$__collection.bulkWrite(ops, options, (error, res) => { + if (error) { + return cb(error); + } + + cb(null, res); + }); + } catch (err) { + return cb(err); + } + }); + + return; + } + + let remaining = validations.length; + let validOps = []; + let validationErrors = []; + for (let i = 0; i < validations.length; ++i) { + validations[i]((err) => { + if (err == null) { + validOps.push(i); + } else { + validationErrors.push({ index: i, error: err }); + } + if (--remaining <= 0) { + completeUnorderedValidation.call(this); + } + }); + } + + validationErrors = validationErrors. + sort((v1, v2) => v1.index - v2.index). + map(v => v.error); + + function completeUnorderedValidation() { + validOps.sort(); + validOps = validOps.map(index => ops[index]); + + this.$__collection.bulkWrite(validOps, options, (error, res) => { + if (error) { + if (validationErrors.length > 0) { + error.mongoose = error.mongoose || {}; + error.mongoose.validationErrors = validationErrors; } - cb(null, res); - }); - } catch (err) { - return cb(err); - } - }); + return cb(error); + } + + if (validationErrors.length > 0) { + res.mongoose = res.mongoose || {}; + res.mongoose.validationErrors = validationErrors; + } + + cb(null, res); + }); + } }, this.events); }; diff --git a/test/model.test.js b/test/model.test.js index 7e1b903073e..848a1d6455b 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -6010,6 +6010,69 @@ describe('Model', function() { } }]); }); + + it('sends valid ops if ordered = false (gh-13176)', async function() { + const testSchema = new mongoose.Schema({ + num: Number + }); + const Test = db.model('Test', testSchema); + + const res = await Test.bulkWrite([ + { + updateOne: { + filter: {}, + update: { $set: { num: 'not a number' } }, + upsert: true + } + }, + { + updateOne: { + filter: {}, + update: { $set: { num: 42 } } + } + } + ], { ordered: false }); + assert.ok(res.mongoose); + assert.equal(res.mongoose.validationErrors.length, 1); + assert.strictEqual(res.result.nUpserted, 0); + }); + + it('decorates write error with validation errors if unordered fails (gh-13176)', async function() { + const testSchema = new mongoose.Schema({ + num: Number + }); + const Test = db.model('Test', testSchema); + + await Test.deleteMany({}); + const { _id } = await Test.create({ num: 42 }); + + const err = await Test.bulkWrite([ + { + updateOne: { + filter: {}, + update: { $set: { num: 'not a number' } }, + upsert: true + } + }, + { + updateOne: { + filter: {}, + update: { $push: { num: 42 } } + } + }, + { + updateOne: { + filter: {}, + update: { $inc: { num: 57 } } + } + } + ], { ordered: false }).then(() => null, err => err); + assert.ok(err); + assert.equal(err.mongoose.validationErrors.length, 1); + + const { num } = await Test.findById(_id); + assert.equal(num, 99); + }); }); it('insertMany with Decimal (gh-5190)', async function() { diff --git a/types/models.d.ts b/types/models.d.ts index bcfe680e532..d8919be0dad 100644 --- a/types/models.d.ts +++ b/types/models.d.ts @@ -162,9 +162,14 @@ declare module 'mongoose' { * if you use `create()`) because with `bulkWrite()` there is only one network * round trip to the MongoDB server. */ - bulkWrite(writes: Array>, options: mongodb.BulkWriteOptions & MongooseBulkWriteOptions, callback: Callback): void; - bulkWrite(writes: Array>, callback: Callback): void; - bulkWrite(writes: Array>, options?: mongodb.BulkWriteOptions & MongooseBulkWriteOptions): Promise; + bulkWrite( + writes: Array>, + options: mongodb.BulkWriteOptions & MongooseBulkWriteOptions & { ordered: false } + ): Promise; + bulkWrite( + writes: Array>, + options?: mongodb.BulkWriteOptions & MongooseBulkWriteOptions + ): Promise; /** * Sends multiple `save()` calls in a single `bulkWrite()`. This is faster than From 36df3a09842fe3b1c84e2241958df0fe57eae254 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 5 Apr 2023 17:31:01 -0400 Subject: [PATCH 2/2] refactor: quick fix re: code review comments --- lib/model.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/model.js b/lib/model.js index aa00931a6de..07bd9ef43b0 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3790,8 +3790,7 @@ Model.bulkWrite = function(ops, options, callback) { map(v => v.error); function completeUnorderedValidation() { - validOps.sort(); - validOps = validOps.map(index => ops[index]); + validOps = validOps.sort().map(index => ops[index]); this.$__collection.bulkWrite(validOps, options, (error, res) => { if (error) {