From 09f2a6796b4f1ecec4880678e19375a07e446949 Mon Sep 17 00:00:00 2001 From: aditi-khare-mongoDB <106987683+aditi-khare-mongoDB@users.noreply.github.com> Date: Wed, 27 Sep 2023 05:42:04 -0400 Subject: [PATCH] fix(NODE-5628): bulkWriteResult.insertedIds does not filter out _ids that are not actually inserted (#3867) Co-authored-by: Durran Jordan --- src/bulk/common.ts | 45 +++++++++-- test/integration/crud/bulk.test.ts | 118 ++++++++++++++++++++++++++++- 2 files changed, 155 insertions(+), 8 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 054a2e9e3b..f7425c691b 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -197,7 +197,7 @@ export class BulkWriteResult { * Create a new BulkWriteResult instance * @internal */ - constructor(bulkResult: BulkResult) { + constructor(bulkResult: BulkResult, isOrdered: boolean) { this.result = bulkResult; this.insertedCount = this.result.nInserted ?? 0; this.matchedCount = this.result.nMatched ?? 0; @@ -205,7 +205,9 @@ export class BulkWriteResult { this.deletedCount = this.result.nRemoved ?? 0; this.upsertedCount = this.result.upserted.length ?? 0; this.upsertedIds = BulkWriteResult.generateIdMap(this.result.upserted); - this.insertedIds = BulkWriteResult.generateIdMap(this.result.insertedIds); + this.insertedIds = BulkWriteResult.generateIdMap( + this.getSuccessfullyInsertedIds(bulkResult, isOrdered) + ); Object.defineProperty(this, 'result', { value: this.result, enumerable: false }); } @@ -214,6 +216,22 @@ export class BulkWriteResult { return this.result.ok; } + /** + * Returns document_ids that were actually inserted + * @internal + */ + private getSuccessfullyInsertedIds(bulkResult: BulkResult, isOrdered: boolean): Document[] { + if (bulkResult.writeErrors.length === 0) return bulkResult.insertedIds; + + if (isOrdered) { + return bulkResult.insertedIds.slice(0, bulkResult.writeErrors[0].index); + } + + return bulkResult.insertedIds.filter( + ({ index }) => !bulkResult.writeErrors.some(writeError => index === writeError.index) + ); + } + /** Returns the upserted id at the given index */ getUpsertedIdAt(index: number): Document | undefined { return this.result.upserted[index]; @@ -479,7 +497,10 @@ function executeCommands( callback: Callback ) { if (bulkOperation.s.batches.length === 0) { - return callback(undefined, new BulkWriteResult(bulkOperation.s.bulkResult)); + return callback( + undefined, + new BulkWriteResult(bulkOperation.s.bulkResult, bulkOperation.isOrdered) + ); } const batch = bulkOperation.s.batches.shift() as Batch; @@ -488,17 +509,26 @@ function executeCommands( // Error is a driver related error not a bulk op error, return early if (err && 'message' in err && !(err instanceof MongoWriteConcernError)) { return callback( - new MongoBulkWriteError(err, new BulkWriteResult(bulkOperation.s.bulkResult)) + new MongoBulkWriteError( + err, + new BulkWriteResult(bulkOperation.s.bulkResult, bulkOperation.isOrdered) + ) ); } if (err instanceof MongoWriteConcernError) { - return handleMongoWriteConcernError(batch, bulkOperation.s.bulkResult, err, callback); + return handleMongoWriteConcernError( + batch, + bulkOperation.s.bulkResult, + bulkOperation.isOrdered, + err, + callback + ); } // Merge the results together mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result); - const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult); + const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult, bulkOperation.isOrdered); if (bulkOperation.handleWriteError(callback, writeResult)) return; // Execute the next command in line @@ -572,6 +602,7 @@ function executeCommands( function handleMongoWriteConcernError( batch: Batch, bulkResult: BulkResult, + isOrdered: boolean, err: MongoWriteConcernError, callback: Callback ) { @@ -583,7 +614,7 @@ function handleMongoWriteConcernError( message: err.result?.writeConcernError.errmsg, code: err.result?.writeConcernError.result }, - new BulkWriteResult(bulkResult) + new BulkWriteResult(bulkResult, isOrdered) ) ); } diff --git a/test/integration/crud/bulk.test.ts b/test/integration/crud/bulk.test.ts index e54b6fe86f..a5f3f9c468 100644 --- a/test/integration/crud/bulk.test.ts +++ b/test/integration/crud/bulk.test.ts @@ -5,6 +5,7 @@ import { type Collection, Long, MongoBatchReExecutionError, + MongoBulkWriteError, type MongoClient, MongoDriverError, MongoInvalidArgumentError @@ -31,7 +32,6 @@ describe('Bulk', function () { .createCollection('test') .catch(() => null); // make ns exist }); - afterEach(async function () { const cleanup = this.configuration.newClient(); await cleanup @@ -91,6 +91,7 @@ describe('Bulk', function () { } }); }); + context('when passed a valid document list', function () { it('insertMany should not throw a MongoInvalidArgument error when called with a valid operation', async function () { try { @@ -104,6 +105,121 @@ describe('Bulk', function () { } }); }); + + context('when inserting duplicate values', function () { + let col; + + beforeEach(async function () { + const db = client.db(); + col = db.collection('test'); + await col.createIndex([{ a: 1 }], { unique: true, sparse: false }); + }); + + async function assertFailsWithDuplicateFields(input, isOrdered, expectedInsertedIds) { + const error = await col.insertMany(input, { ordered: isOrdered }).catch(error => error); + expect(error).to.be.instanceOf(MongoBulkWriteError); + expect(error.result.insertedCount).to.equal(Object.keys(error.result.insertedIds).length); + expect(error.result.insertedIds).to.deep.equal(expectedInsertedIds); + } + + context('when the insert is ordered', function () { + it('contains the correct insertedIds on one duplicate insert', async function () { + await assertFailsWithDuplicateFields( + [ + { _id: 0, a: 1 }, + { _id: 1, a: 1 } + ], + true, + { 0: 0 } + ); + }); + + it('contains the correct insertedIds on multiple duplicate inserts', async function () { + await assertFailsWithDuplicateFields( + [ + { _id: 0, a: 1 }, + { _id: 1, a: 1 }, + { _id: 2, a: 1 }, + { _id: 3, b: 2 } + ], + true, + { 0: 0 } + ); + }); + }); + + context('when the insert is unordered', function () { + it('contains the correct insertedIds on multiple duplicate inserts', async function () { + await assertFailsWithDuplicateFields( + [ + { _id: 0, a: 1 }, + { _id: 1, a: 1 }, + { _id: 2, a: 1 }, + { _id: 3, b: 2 } + ], + false, + { 0: 0, 3: 3 } + ); + }); + }); + }); + }); + + describe('#bulkWrite()', function () { + context('when inserting duplicate values', function () { + let col; + + beforeEach(async function () { + const db = client.db(); + col = db.collection('test'); + await col.createIndex([{ a: 1 }], { unique: true, sparse: false }); + }); + + async function assertFailsWithDuplicateFields(input, isOrdered, expectedInsertedIds) { + const error = await col.bulkWrite(input, { ordered: isOrdered }).catch(error => error); + expect(error).to.be.instanceOf(MongoBulkWriteError); + expect(error.result.insertedCount).to.equal(Object.keys(error.result.insertedIds).length); + expect(error.result.insertedIds).to.deep.equal(expectedInsertedIds); + } + + context('when the insert is ordered', function () { + it('contains the correct insertedIds on one duplicate insert', async function () { + await assertFailsWithDuplicateFields( + [{ insertOne: { _id: 0, a: 1 } }, { insertOne: { _id: 1, a: 1 } }], + true, + { 0: 0 } + ); + }); + + it('contains the correct insertedIds on multiple duplicate inserts', async function () { + await assertFailsWithDuplicateFields( + [ + { insertOne: { _id: 0, a: 1 } }, + { insertOne: { _id: 1, a: 1 } }, + { insertOne: { _id: 2, a: 1 } }, + { insertOne: { _id: 3, b: 2 } } + ], + true, + { 0: 0 } + ); + }); + }); + + context('when the insert is unordered', function () { + it('contains the correct insertedIds on multiple duplicate inserts', async function () { + await assertFailsWithDuplicateFields( + [ + { insertOne: { _id: 0, a: 1 } }, + { insertOne: { _id: 1, a: 1 } }, + { insertOne: { _id: 2, a: 1 } }, + { insertOne: { _id: 3, b: 2 } } + ], + false, + { 0: 0, 3: 3 } + ); + }); + }); + }); }); });