From 6625a45801412bb10cd9556f5a3a7ee9e8562560 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 5 Jan 2023 19:29:07 +0100 Subject: [PATCH 1/9] feat(NODE-4034): make internal bulk result private --- src/bulk/common.ts | 17 ++++------------- test/tools/unified-spec-runner/operations.ts | 17 +---------------- 2 files changed, 5 insertions(+), 29 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 86ad050a9cd..2b53ff4cac8 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -123,11 +123,7 @@ export type AnyBulkWriteOperation = | { deleteOne: DeleteOneModel } | { deleteMany: DeleteManyModel }; -/** - * @public - * - * @deprecated Will be made internal in 5.0 - */ +/** @internal */ export interface BulkResult { ok: number; writeErrors: WriteError[]; @@ -172,8 +168,7 @@ export class Batch { * The result of a bulk write. */ export class BulkWriteResult { - /** @deprecated Will be removed in 5.0 */ - result: BulkResult; + private result: BulkResult; /** * Create a new BulkWriteResult instance @@ -181,6 +176,7 @@ export class BulkWriteResult { */ constructor(bulkResult: BulkResult) { this.result = bulkResult; + Object.defineProperty(this, 'result', { value: this.result, enumerable: false }); } /** Number of documents inserted. */ @@ -314,13 +310,8 @@ export class BulkWriteResult { } } - /* @deprecated Will be removed in 5.0 release */ - toJSON(): BulkResult { - return this.result; - } - toString(): string { - return `BulkWriteResult(${this.toJSON()})`; + return `BulkWriteResult(${this.result})`; } isOk(): boolean { diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index ec5f9681551..f587b615b5a 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -659,22 +659,7 @@ operations.set('rewrapManyDataKey', async ({ entities, operation }) => { const clientEncryption = entities.getEntity('clientEncryption', operation.object); const { filter, opts } = operation.arguments!; - const rewrapManyDataKeyResult = await clientEncryption.rewrapManyDataKey(filter, opts); - - if (rewrapManyDataKeyResult.bulkWriteResult != null) { - // TODO(NODE-4393): refactor BulkWriteResult to not have a 'result' property - // - // The unified spec runner match function will assert that documents have no extra - // keys. For `rewrapManyDataKey` operations, our unifed tests will fail because - // our BulkWriteResult class has an extra property - "result". We explicitly make it - // non-enumerable for the purposes of testing so that the tests can pass. - const { bulkWriteResult } = rewrapManyDataKeyResult; - Object.defineProperty(bulkWriteResult, 'result', { - value: bulkWriteResult.result, - enumerable: false - }); - } - return rewrapManyDataKeyResult; + return await clientEncryption.rewrapManyDataKey(filter, opts); }); operations.set('deleteKey', async ({ entities, operation }) => { From d592a0fb6671118960996adda8bdb02c8dccc9cb Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 5 Jan 2023 21:48:16 +0100 Subject: [PATCH 2/9] feat: add properties to bulk write result --- src/bulk/common.ts | 67 ++++++++----------- .../retryable_writes.spec.test.ts | 8 ++- 2 files changed, 34 insertions(+), 41 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 2b53ff4cac8..017265d520c 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -163,12 +163,33 @@ export class Batch { } } +function generateIdMap(ids: Document[]): { [key: number]: any } { + const idMap: { [index: number]: any } = {}; + for (const doc of ids ?? []) { + idMap[doc.index] = doc._id; + } + return idMap; +} /** * @public * The result of a bulk write. */ export class BulkWriteResult { private result: BulkResult; + /** Number of documents inserted. */ + insertedCount: number; + /** Number of documents matched for update. */ + matchedCount: number; + /** Number of documents modified. */ + modifiedCount: number; + /** Number of documents deleted. */ + deletedCount: number; + /** Number of documents upserted. */ + upsertedCount: number; + /** Upserted document generated Id's, hash key is the index of the originating operation */ + upsertedIds: { [key: number]: any }; + /** Inserted document generated Id's, hash key is the index of the originating operation */ + insertedIds: { [key: number]: any }; /** * Create a new BulkWriteResult instance @@ -176,48 +197,16 @@ export class BulkWriteResult { */ constructor(bulkResult: BulkResult) { this.result = bulkResult; + this.insertedCount = this.result.nInserted ?? 0; + this.matchedCount = this.result.nMatched ?? 0; + this.modifiedCount = this.result.nModified ?? 0; + this.deletedCount = this.result.nRemoved ?? 0; + this.upsertedCount = this.result.upserted.length ?? 0; + this.upsertedIds = generateIdMap(this.result.upserted); + this.insertedIds = generateIdMap(this.result.insertedIds); Object.defineProperty(this, 'result', { value: this.result, enumerable: false }); } - /** Number of documents inserted. */ - get insertedCount(): number { - return this.result.nInserted ?? 0; - } - /** Number of documents matched for update. */ - get matchedCount(): number { - return this.result.nMatched ?? 0; - } - /** Number of documents modified. */ - get modifiedCount(): number { - return this.result.nModified ?? 0; - } - /** Number of documents deleted. */ - get deletedCount(): number { - return this.result.nRemoved ?? 0; - } - /** Number of documents upserted. */ - get upsertedCount(): number { - return this.result.upserted.length ?? 0; - } - - /** Upserted document generated Id's, hash key is the index of the originating operation */ - get upsertedIds(): { [key: number]: any } { - const upserted: { [index: number]: any } = {}; - for (const doc of this.result.upserted ?? []) { - upserted[doc.index] = doc._id; - } - return upserted; - } - - /** Inserted document generated Id's, hash key is the index of the originating operation */ - get insertedIds(): { [key: number]: any } { - const inserted: { [index: number]: any } = {}; - for (const doc of this.result.insertedIds ?? []) { - inserted[doc.index] = doc._id; - } - return inserted; - } - /** Evaluates to true if the bulk operation correctly executes */ get ok(): number { return this.result.ok; diff --git a/test/integration/retryable-writes/retryable_writes.spec.test.ts b/test/integration/retryable-writes/retryable_writes.spec.test.ts index de506b79cf5..6fd14110ac1 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.test.ts @@ -150,8 +150,12 @@ async function executeScenarioTest(test, ctx: RetryableWriteTestContext) { const result = resultOrError; const expected = test.outcome.result; - // TODO(NODE-4034): Make CRUD results spec compliant - expect(result.value ?? result).to.deep.include(expected); + const actual = result.value ?? result; + // Some of our result classes contain the optional 'acknowledged' property which is + // not part of the test expectations. + for (const property in expected) { + expect(actual).to.have.deep.property(property, expected[property]); + } } if (test.outcome.collection) { From 8ea38e52a567156f309a0db251dca5d0028afab5 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 5 Jan 2023 22:44:07 +0100 Subject: [PATCH 3/9] test: fix missing insertedIds in spec tests --- src/bulk/common.ts | 3 +-- test/tools/unified-spec-runner/operations.ts | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 017265d520c..6a4f9b633bf 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -530,8 +530,8 @@ function executeCommands( } // Merge the results together - const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult); const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result); + const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult); if (mergeResult != null) { return callback(undefined, writeResult); } @@ -1226,7 +1226,6 @@ export abstract class BulkOperationBase { this.s.executed = true; const finalOptions = { ...this.s.options, ...options }; const operation = new BulkWriteShimOperation(this, finalOptions); - return executeOperation(this.s.collection.s.db.s.client, operation); }, callback); } diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index f587b615b5a..cf9d366e4f7 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -659,7 +659,21 @@ operations.set('rewrapManyDataKey', async ({ entities, operation }) => { const clientEncryption = entities.getEntity('clientEncryption', operation.object); const { filter, opts } = operation.arguments!; - return await clientEncryption.rewrapManyDataKey(filter, opts); + const rewrapManyDataKeyResult = await clientEncryption.rewrapManyDataKey(filter, opts); + if (rewrapManyDataKeyResult.bulkWriteResult != null) { + // TODO: Create DRIVERS ticket. + // + // The unified spec runner match function will assert that documents have no extra + // keys. For `rewrapManyDataKey` operations, our unifed tests will fail because + // our BulkWriteResult class has an insertedIds property, which is correct by the + // spec but not defined in the spec tests. + const { bulkWriteResult } = rewrapManyDataKeyResult; + Object.defineProperty(bulkWriteResult, 'insertedIds', { + value: bulkWriteResult.insertedIds, + enumerable: false + }); + } + return rewrapManyDataKeyResult; }); operations.set('deleteKey', async ({ entities, operation }) => { From 80bdaa378c8a9323b43a9582055798ff62512089 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 11 Jan 2023 11:44:39 +0100 Subject: [PATCH 4/9] chore: update migration guide --- etc/notes/CHANGES_5.0.0.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/etc/notes/CHANGES_5.0.0.md b/etc/notes/CHANGES_5.0.0.md index 048e2bbef02..950a0290eba 100644 --- a/etc/notes/CHANGES_5.0.0.md +++ b/etc/notes/CHANGES_5.0.0.md @@ -191,3 +191,21 @@ await collection.insertMany([{ name: 'fido' }, { name: 'luna' }]) The `keepGoing` option was a legacy name for setting `ordered` to `false` for bulk inserts. It was only supported by the legacy `collection.insert()` method which is now removed as noted above. + +### `BulkWriteResult` no longer contains a publicly enumerable `result` property. + +To access the raw result, please use `bulkWriteResult.getRawResponse()`. + +### `BulkWriteResult` now contains individual ressult properties. + +These can be accessed via: + +```ts + bulkWriteResult.insertedCount; + bulkWriteResult.matchedCount; + bulkWriteResult.modifiedCount; + bulkWriteResult.deletedCount; + bulkWriteResult.upsertedCount; + bulkWriteResult.upsertedIds; + bulkWriteResult.insertedIds; +``` From cb834ea4264f91e54c30157c72478b8ec0801ad6 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Wed, 11 Jan 2023 12:28:01 +0100 Subject: [PATCH 5/9] chore: link drivers ticket --- test/tools/unified-spec-runner/operations.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index cf9d366e4f7..a811bb3b8a9 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -661,7 +661,7 @@ operations.set('rewrapManyDataKey', async ({ entities, operation }) => { const rewrapManyDataKeyResult = await clientEncryption.rewrapManyDataKey(filter, opts); if (rewrapManyDataKeyResult.bulkWriteResult != null) { - // TODO: Create DRIVERS ticket. + // TODO: Can be removed with DRIVERS-2523 // // The unified spec runner match function will assert that documents have no extra // keys. For `rewrapManyDataKey` operations, our unifed tests will fail because From 4971fee40ffc5686fc44dcb1ff3947c640c2ab7b Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 12 Jan 2023 11:11:06 +0100 Subject: [PATCH 6/9] fix: add suggestions --- src/bulk/common.ts | 41 +++++++++---------- .../retryable_writes.spec.test.ts | 16 ++++++++ 2 files changed, 35 insertions(+), 22 deletions(-) diff --git a/src/bulk/common.ts b/src/bulk/common.ts index 6a4f9b633bf..1d1ade6e506 100644 --- a/src/bulk/common.ts +++ b/src/bulk/common.ts @@ -163,33 +163,34 @@ export class Batch { } } -function generateIdMap(ids: Document[]): { [key: number]: any } { - const idMap: { [index: number]: any } = {}; - for (const doc of ids ?? []) { - idMap[doc.index] = doc._id; - } - return idMap; -} /** * @public * The result of a bulk write. */ export class BulkWriteResult { - private result: BulkResult; + private readonly result: BulkResult; /** Number of documents inserted. */ - insertedCount: number; + readonly insertedCount: number; /** Number of documents matched for update. */ - matchedCount: number; + readonly matchedCount: number; /** Number of documents modified. */ - modifiedCount: number; + readonly modifiedCount: number; /** Number of documents deleted. */ - deletedCount: number; + readonly deletedCount: number; /** Number of documents upserted. */ - upsertedCount: number; + readonly upsertedCount: number; /** Upserted document generated Id's, hash key is the index of the originating operation */ - upsertedIds: { [key: number]: any }; + readonly upsertedIds: { [key: number]: any }; /** Inserted document generated Id's, hash key is the index of the originating operation */ - insertedIds: { [key: number]: any }; + readonly insertedIds: { [key: number]: any }; + + private static generateIdMap(ids: Document[]): { [key: number]: any } { + const idMap: { [index: number]: any } = {}; + for (const doc of ids) { + idMap[doc.index] = doc._id; + } + return idMap; + } /** * Create a new BulkWriteResult instance @@ -202,8 +203,8 @@ export class BulkWriteResult { this.modifiedCount = this.result.nModified ?? 0; this.deletedCount = this.result.nRemoved ?? 0; this.upsertedCount = this.result.upserted.length ?? 0; - this.upsertedIds = generateIdMap(this.result.upserted); - this.insertedIds = generateIdMap(this.result.insertedIds); + this.upsertedIds = BulkWriteResult.generateIdMap(this.result.upserted); + this.insertedIds = BulkWriteResult.generateIdMap(this.result.insertedIds); Object.defineProperty(this, 'result', { value: this.result, enumerable: false }); } @@ -530,12 +531,8 @@ function executeCommands( } // Merge the results together - const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result); + mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result); const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult); - if (mergeResult != null) { - return callback(undefined, writeResult); - } - if (bulkOperation.handleWriteError(callback, writeResult)) return; // Execute the next command in line diff --git a/test/integration/retryable-writes/retryable_writes.spec.test.ts b/test/integration/retryable-writes/retryable_writes.spec.test.ts index 6fd14110ac1..d895ac31710 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.test.ts @@ -14,6 +14,17 @@ interface RetryableWriteTestContext { failPointName?: any; } +const ALLOWED_BULK_PROPERTIES = [ + 'ackknowledged', + 'insertedCount', + 'matchedCount', + 'modifiedCount', + 'deletedCount', + 'upsertedCount', + 'upsertedIds', + 'insertedIds' +]; + describe('Legacy Retryable Writes Specs', function () { let ctx: RetryableWriteTestContext = {}; @@ -156,6 +167,11 @@ async function executeScenarioTest(test, ctx: RetryableWriteTestContext) { for (const property in expected) { expect(actual).to.have.deep.property(property, expected[property]); } + + // Check we don't have any extra properties on the bulk write result. + for (const property in actual) { + expect(ALLOWED_BULK_PROPERTIES).to.include(property); + } } if (test.outcome.collection) { From 4b6bea5443d109005721895ec2a7cc19ce50f4cb Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 12 Jan 2023 12:26:04 +0100 Subject: [PATCH 7/9] test: fix test --- test/integration/retryable-writes/retryable_writes.spec.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.test.ts b/test/integration/retryable-writes/retryable_writes.spec.test.ts index d895ac31710..1b36c07ac8d 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.test.ts @@ -15,7 +15,7 @@ interface RetryableWriteTestContext { } const ALLOWED_BULK_PROPERTIES = [ - 'ackknowledged', + 'acknowledged', 'insertedCount', 'matchedCount', 'modifiedCount', From 1badb1b87c84b20151ceba6918926a9184270580 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 12 Jan 2023 14:30:24 +0100 Subject: [PATCH 8/9] test: fix tests --- .../retryable_writes.spec.test.ts | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/test/integration/retryable-writes/retryable_writes.spec.test.ts b/test/integration/retryable-writes/retryable_writes.spec.test.ts index 1b36c07ac8d..6fd14110ac1 100644 --- a/test/integration/retryable-writes/retryable_writes.spec.test.ts +++ b/test/integration/retryable-writes/retryable_writes.spec.test.ts @@ -14,17 +14,6 @@ interface RetryableWriteTestContext { failPointName?: any; } -const ALLOWED_BULK_PROPERTIES = [ - 'acknowledged', - 'insertedCount', - 'matchedCount', - 'modifiedCount', - 'deletedCount', - 'upsertedCount', - 'upsertedIds', - 'insertedIds' -]; - describe('Legacy Retryable Writes Specs', function () { let ctx: RetryableWriteTestContext = {}; @@ -167,11 +156,6 @@ async function executeScenarioTest(test, ctx: RetryableWriteTestContext) { for (const property in expected) { expect(actual).to.have.deep.property(property, expected[property]); } - - // Check we don't have any extra properties on the bulk write result. - for (const property in actual) { - expect(ALLOWED_BULK_PROPERTIES).to.include(property); - } } if (test.outcome.collection) { From 318ffcbcd139fc524a2f29e48eedab116da00c01 Mon Sep 17 00:00:00 2001 From: Durran Jordan Date: Thu, 5 Jan 2023 23:06:07 +0100 Subject: [PATCH 9/9] test: add insertedIds to csfle bulk results --- .../tests/unified/rewrapManyDataKey.json | 30 +++++++++++++++---- .../tests/unified/rewrapManyDataKey.yml | 6 ++++ test/tools/unified-spec-runner/operations.ts | 16 +--------- 3 files changed, 31 insertions(+), 21 deletions(-) diff --git a/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.json b/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.json index 89860de0c07..6b3c9664a97 100644 --- a/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.json +++ b/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.json @@ -321,7 +321,10 @@ "modifiedCount": 4, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } } @@ -503,7 +506,10 @@ "modifiedCount": 4, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } } @@ -687,7 +693,10 @@ "modifiedCount": 4, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } } @@ -873,7 +882,10 @@ "modifiedCount": 4, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } } @@ -1055,7 +1067,10 @@ "modifiedCount": 4, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } } @@ -1218,7 +1233,10 @@ "modifiedCount": 5, "deletedCount": 0, "upsertedCount": 0, - "upsertedIds": {} + "upsertedIds": {}, + "insertedIds": { + "$$unsetOrMatches": {} + } } } }, diff --git a/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.yml b/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.yml index 51415586838..cc20e1b1710 100644 --- a/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.yml +++ b/test/spec/client-side-encryption/tests/unified/rewrapManyDataKey.yml @@ -130,6 +130,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } expectEvents: - client: *client0 events: @@ -182,6 +183,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } expectEvents: - client: *client0 events: @@ -236,6 +238,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } expectEvents: - client: *client0 events: @@ -285,6 +288,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } expectEvents: - client: *client0 events: @@ -334,6 +338,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } expectEvents: - client: *client0 events: @@ -381,6 +386,7 @@ tests: deletedCount: 0 upsertedCount: 0 upsertedIds: {} + insertedIds: { $$unsetOrMatches: {} } - name: find object: *collection0 arguments: diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index a811bb3b8a9..f587b615b5a 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -659,21 +659,7 @@ operations.set('rewrapManyDataKey', async ({ entities, operation }) => { const clientEncryption = entities.getEntity('clientEncryption', operation.object); const { filter, opts } = operation.arguments!; - const rewrapManyDataKeyResult = await clientEncryption.rewrapManyDataKey(filter, opts); - if (rewrapManyDataKeyResult.bulkWriteResult != null) { - // TODO: Can be removed with DRIVERS-2523 - // - // The unified spec runner match function will assert that documents have no extra - // keys. For `rewrapManyDataKey` operations, our unifed tests will fail because - // our BulkWriteResult class has an insertedIds property, which is correct by the - // spec but not defined in the spec tests. - const { bulkWriteResult } = rewrapManyDataKeyResult; - Object.defineProperty(bulkWriteResult, 'insertedIds', { - value: bulkWriteResult.insertedIds, - enumerable: false - }); - } - return rewrapManyDataKeyResult; + return await clientEncryption.rewrapManyDataKey(filter, opts); }); operations.set('deleteKey', async ({ entities, operation }) => {