From 58b4f94083f15e4aab95ab165013bed24bbab8cf Mon Sep 17 00:00:00 2001 From: Matt Broadstone Date: Tue, 26 May 2020 10:09:35 -0400 Subject: [PATCH] fix: always include `writeErrors` on a `BulkWriteError` instance We special case situations where there is only one error returned from a bulk operation, but that can be quite confusing for users of the bulk API. Promoting the single error to the top-level error message is reasonable, but in all cases we should use a consistent shape, and return `writeErrors` as a field on the object NODE-2625 --- lib/bulk/common.js | 15 ++++--------- lib/core/error.js | 11 ++++++++++ test/functional/bulk.test.js | 42 +++++++++++++++++++++++++----------- 3 files changed, 45 insertions(+), 23 deletions(-) diff --git a/lib/bulk/common.js b/lib/bulk/common.js index fea6c18e34..ac2fd5d2df 100644 --- a/lib/bulk/common.js +++ b/lib/bulk/common.js @@ -1200,19 +1200,10 @@ class BulkOperationBase { * @ignore * @param {function} callback * @param {BulkWriteResult} writeResult - * @param {class} self either OrderedBulkOperation or UnorderdBulkOperation + * @param {class} self either OrderedBulkOperation or UnorderedBulkOperation */ handleWriteError(callback, writeResult) { if (this.s.bulkResult.writeErrors.length > 0) { - if (this.s.bulkResult.writeErrors.length === 1) { - handleCallback( - callback, - new BulkWriteError(toError(this.s.bulkResult.writeErrors[0]), writeResult), - null - ); - return true; - } - const msg = this.s.bulkResult.writeErrors[0].errmsg ? this.s.bulkResult.writeErrors[0].errmsg : 'write operation failed'; @@ -1230,7 +1221,9 @@ class BulkOperationBase { null ); return true; - } else if (writeResult.getWriteConcernError()) { + } + + if (writeResult.getWriteConcernError()) { handleCallback( callback, new BulkWriteError(toError(writeResult.getWriteConcernError()), writeResult), diff --git a/lib/core/error.js b/lib/core/error.js index f2b4cf59fa..c50751da24 100644 --- a/lib/core/error.js +++ b/lib/core/error.js @@ -19,6 +19,10 @@ class MongoError extends Error { } else { super(message.message || message.errmsg || message.$err || 'n/a'); for (var name in message) { + if (name === 'errmsg') { + continue; + } + this[name] = message[name]; } } @@ -29,6 +33,13 @@ class MongoError extends Error { this.name = 'MongoError'; } + /** + * Legacy name for server error responses + */ + get errmsg() { + return this.message; + } + /** * Creates a new MongoError object * diff --git a/test/functional/bulk.test.js b/test/functional/bulk.test.js index 0524a67cb9..d215cff457 100644 --- a/test/functional/bulk.test.js +++ b/test/functional/bulk.test.js @@ -4,6 +4,7 @@ const test = require('./shared').assert, expect = require('chai').expect; const MongoError = require('../../index').MongoError; +const ignoreNsNotFound = require('./shared').ignoreNsNotFound; describe('Bulk', function() { before(function() { @@ -1616,16 +1617,41 @@ describe('Bulk', function() { .then(() => client.close()); }); + it('should promote a single error to the top-level message, and preserve writeErrors', function() { + const client = this.configuration.newClient(); + return client.connect().then(() => { + this.defer(() => client.close()); + + const coll = client.db().collection('single_bulk_write_error'); + return coll + .drop() + .catch(ignoreNsNotFound) + .then(() => coll.insert(Array.from({ length: 4 }, (_, i) => ({ _id: i, a: i })))) + .then(() => + coll.bulkWrite([{ insertOne: { _id: 5, a: 0 } }, { insertOne: { _id: 5, a: 0 } }]) + ) + .then( + () => { + throw new Error('expected a bulk error'); + }, + err => { + expect(err) + .property('message') + .to.match(/E11000/); + expect(err) + .to.have.property('writeErrors') + .with.length(1); + } + ); + }); + }); + it('should preserve order of operation index in unordered bulkWrite', function() { const client = this.configuration.newClient(); return client.connect().then(() => { this.defer(() => client.close()); const coll = client.db().collection('bulk_write_ordering_test'); - function ignoreNsNotFound(err) { - if (!err.message.match(/ns not found/)) throw err; - } - return coll .drop() .catch(ignoreNsNotFound) @@ -1667,10 +1693,6 @@ describe('Bulk', function() { this.defer(() => client.close()); const coll = client.db().collection('unordered_preserve_order'); - function ignoreNsNotFound(err) { - if (!err.message.match(/ns not found/)) throw err; - } - return coll .drop() .catch(ignoreNsNotFound) @@ -1704,10 +1726,6 @@ describe('Bulk', function() { this.defer(() => client.close()); const coll = client.db().collection('bulk_op_ordering_test'); - function ignoreNsNotFound(err) { - if (!err.message.match(/ns not found/)) throw err; - } - return coll .drop() .catch(ignoreNsNotFound)