From 8ce32c05ae2ed88266a48094ddb120c3fd56cb60 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 28 Aug 2024 12:36:08 -0400 Subject: [PATCH 1/3] fix(model): throw error if `bulkSave()` did not insert or update any documents Re: #14763 --- lib/model.js | 48 ++++++++++++++++++++++++++++++++-------------- test/model.test.js | 25 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/lib/model.js b/lib/model.js index e027087215..b3c79564b8 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3364,11 +3364,18 @@ Model.bulkWrite = async function bulkWrite(ops, options) { }; /** - * takes an array of documents, gets the changes and inserts/updates documents in the database - * according to whether or not the document is new, or whether it has changes or not. + * Takes an array of documents, gets the changes and inserts/updates documents in the database + * according to whether or not the document is new, or whether it has changes or not. * * `bulkSave` uses `bulkWrite` under the hood, so it's mostly useful when dealing with many documents (10K+) * + * `bulkSave()` throws errors under the following conditions: + * + * - `bulkWrite()` fails (for example, due to being unable to connect to MongoDB or due to duplicate key error) + * - `bulkWrite()` did not insert or update any documents + * + * Note that `bulkSave()` will **not** throw an error if only some of the `save()` calls succeeded. + * * @param {Array} documents * @param {Object} [options] options passed to the underlying `bulkWrite()` * @param {Boolean} [options.timestamps] defaults to `null`, when set to false, mongoose will not add/update timestamps to the documents. @@ -3376,7 +3383,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { * @param {String|number} [options.w=1] The [write concern](https://www.mongodb.com/docs/manual/reference/write-concern/). See [`Query#w()`](https://mongoosejs.com/docs/api/query.html#Query.prototype.w()) for more information. * @param {number} [options.wtimeout=null] The [write concern timeout](https://www.mongodb.com/docs/manual/reference/write-concern/#wtimeout). * @param {Boolean} [options.j=true] If false, disable [journal acknowledgement](https://www.mongodb.com/docs/manual/reference/write-concern/#j-option) - * + * @return {BulkWriteResult} the return value from `bulkWrite()` */ Model.bulkSave = async function bulkSave(documents, options) { options = options || {}; @@ -3404,18 +3411,31 @@ Model.bulkSave = async function bulkSave(documents, options) { (err) => ({ bulkWriteResult: null, bulkWriteError: err }) ); - await Promise.all( - documents.map(async(document) => { - const documentError = bulkWriteError && bulkWriteError.writeErrors.find(writeError => { - const writeErrorDocumentId = writeError.err.op._id || writeError.err.op.q._id; - return writeErrorDocumentId.toString() === document._doc._id.toString(); - }); + const matchedCount = bulkWriteResult?.matchedCount ?? 0; + const insertedCount = bulkWriteResult?.insertedCount ?? 0; + if (writeOperations.length > 0 && matchedCount + insertedCount === 0 && !bulkWriteError) { + throw new DocumentNotFoundError( + writeOperations.filter(op => op.updateOne).map(op => op.updateOne.filter), + this.modelName, + writeOperations.length, + bulkWriteResult + ); + } - if (documentError == null) { - await handleSuccessfulWrite(document); - } - }) - ); + const successfulDocuments = []; + for (let i = 0; i < documents.length; i++) { + const document = documents[i]; + const documentError = bulkWriteError && bulkWriteError.writeErrors.find(writeError => { + const writeErrorDocumentId = writeError.err.op._id || writeError.err.op.q._id; + return writeErrorDocumentId.toString() === document._doc._id.toString(); + }); + + if (documentError == null) { + successfulDocuments.push(document); + } + } + + await Promise.all(successfulDocuments.map(document => handleSuccessfulWrite(document))); if (bulkWriteError && bulkWriteError.writeErrors && bulkWriteError.writeErrors.length) { throw bulkWriteError; diff --git a/test/model.test.js b/test/model.test.js index 855f1eecb8..6abf954885 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -6939,6 +6939,31 @@ describe('Model', function() { assert.ok(err == null); }); + it('should error if no documents were inserted (gh-14763)', async function() { + const fooSchema = new mongoose.Schema({ + bar: { type: Number } + }, { optimisticConcurrency: true }); + const TestModel = db.model('Test', fooSchema); + + const foo = await TestModel.create({ + bar: 0 + }); + + // update 1 + foo.bar = 1; + await foo.save(); + + // parallel update + const fooCopy = await TestModel.findById(foo._id); + fooCopy.bar = 99; + await fooCopy.save(); + + foo.bar = 2; + const err = await TestModel.bulkSave([foo]).then(() => null, err => err); + assert.equal(err.name, 'DocumentNotFoundError'); + assert.equal(err.numAffected, 1); + assert.ok(Array.isArray(err.filter)); + }); it('Using bulkSave should not trigger an error (gh-11071)', async function() { const pairSchema = mongoose.Schema({ From b42be657de04b9ad2eb1b13fe74c67aee9a71448 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Wed, 28 Aug 2024 12:52:31 -0400 Subject: [PATCH 2/3] add test coverage and docs re: bulkSave() handling validation errors --- lib/model.js | 3 ++- test/model.test.js | 19 ++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/model.js b/lib/model.js index b3c79564b8..3891bef8a1 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3371,8 +3371,9 @@ Model.bulkWrite = async function bulkWrite(ops, options) { * * `bulkSave()` throws errors under the following conditions: * + * - one of the provided documents fails validation. In this case, `bulkSave()` does not send a `bulkWrite()`, and throws the first validation error. * - `bulkWrite()` fails (for example, due to being unable to connect to MongoDB or due to duplicate key error) - * - `bulkWrite()` did not insert or update any documents + * - `bulkWrite()` did not insert or update any documents. In this case, `bulkSave()` will throw a DocumentNotFound error. * * Note that `bulkSave()` will **not** throw an error if only some of the `save()` calls succeeded. * diff --git a/test/model.test.js b/test/model.test.js index 6abf954885..b9d01e387f 100644 --- a/test/model.test.js +++ b/test/model.test.js @@ -6939,7 +6939,7 @@ describe('Model', function() { assert.ok(err == null); }); - it('should error if no documents were inserted (gh-14763)', async function() { + it('should error if no documents were inserted or updated (gh-14763)', async function() { const fooSchema = new mongoose.Schema({ bar: { type: Number } }, { optimisticConcurrency: true }); @@ -6964,6 +6964,23 @@ describe('Model', function() { assert.equal(err.numAffected, 1); assert.ok(Array.isArray(err.filter)); }); + it('should error if there is a validation error', async function() { + const fooSchema = new mongoose.Schema({ + bar: { type: Number } + }, { optimisticConcurrency: true }); + const TestModel = db.model('Test', fooSchema); + + const docs = [ + new TestModel({ bar: 42 }), + new TestModel({ bar: 'taco' }) + ]; + const err = await TestModel.bulkSave(docs).then(() => null, err => err); + assert.equal(err.name, 'ValidationError'); + + // bulkSave() does not save any documents if any documents fail validation + const fromDb = await TestModel.find(); + assert.equal(fromDb.length, 0); + }); it('Using bulkSave should not trigger an error (gh-11071)', async function() { const pairSchema = mongoose.Schema({ From 5b40f1b413fa9754163da83c9fcfb0bd79245b33 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Tue, 3 Sep 2024 09:40:13 -0400 Subject: [PATCH 3/3] Update lib/model.js Co-authored-by: Hafez --- lib/model.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/model.js b/lib/model.js index 3891bef8a1..e567ea5d1c 100644 --- a/lib/model.js +++ b/lib/model.js @@ -3373,7 +3373,7 @@ Model.bulkWrite = async function bulkWrite(ops, options) { * * - one of the provided documents fails validation. In this case, `bulkSave()` does not send a `bulkWrite()`, and throws the first validation error. * - `bulkWrite()` fails (for example, due to being unable to connect to MongoDB or due to duplicate key error) - * - `bulkWrite()` did not insert or update any documents. In this case, `bulkSave()` will throw a DocumentNotFound error. + * - `bulkWrite()` did not insert or update **any** documents. In this case, `bulkSave()` will throw a DocumentNotFound error. * * Note that `bulkSave()` will **not** throw an error if only some of the `save()` calls succeeded. *