feat(NODE-4034)!: make internal bulk result private (#3515)

Author: durran

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;
+```

src/bulk/common.ts

@@ -123,11 +123,7 @@ export type AnyBulkWriteOperation<TSchema extends Document = Document> =
   | { deleteOne: DeleteOneModel<TSchema> }
   | { deleteMany: DeleteManyModel<TSchema> };
 
-/**
- * @public
- *
- * @deprecated Will be made internal in 5.0
- */
+/** @internal */
 export interface BulkResult {
   ok: number;
   writeErrors: WriteError[];
@@ -172,54 +168,44 @@ export class Batch<T = Document> {
  * The result of a bulk write.
  */
 export class BulkWriteResult {
-  /** @deprecated Will be removed in 5.0 */
-  result: BulkResult;
-
-  /**
-   * Create a new BulkWriteResult instance
-   * @internal
-   */
-  constructor(bulkResult: BulkResult) {
-    this.result = bulkResult;
-  }
-
+  private readonly result: BulkResult;
   /** Number of documents inserted. */
-  get insertedCount(): number {
-    return this.result.nInserted ?? 0;
-  }
+  readonly insertedCount: number;
   /** Number of documents matched for update. */
-  get matchedCount(): number {
-    return this.result.nMatched ?? 0;
-  }
+  readonly matchedCount: number;
   /** Number of documents modified. */
-  get modifiedCount(): number {
-    return this.result.nModified ?? 0;
-  }
+  readonly modifiedCount: number;
   /** Number of documents deleted. */
-  get deletedCount(): number {
-    return this.result.nRemoved ?? 0;
-  }
+  readonly deletedCount: number;
   /** Number of documents upserted. */
-  get upsertedCount(): number {
-    return this.result.upserted.length ?? 0;
-  }
-
+  readonly upsertedCount: number;
   /** 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;
+  readonly upsertedIds: { [key: number]: any };
+  /** Inserted document generated Id's, hash key is the index of the originating operation */
+  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 upserted;
+    return idMap;
   }
 
-  /** 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;
+  /**
+   * Create a new BulkWriteResult instance
+   * @internal
+   */
+  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 = BulkWriteResult.generateIdMap(this.result.upserted);
+    this.insertedIds = BulkWriteResult.generateIdMap(this.result.insertedIds);
+    Object.defineProperty(this, 'result', { value: this.result, enumerable: false });
   }
 
   /** Evaluates to true if the bulk operation correctly executes */
@@ -314,13 +300,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 {
@@ -550,12 +531,8 @@ function executeCommands(
     }
 
     // Merge the results together
+    mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
     const writeResult = new BulkWriteResult(bulkOperation.s.bulkResult);
-    const mergeResult = mergeBatchResults(batch, bulkOperation.s.bulkResult, err, result);
-    if (mergeResult != null) {
-      return callback(undefined, writeResult);
-    }
-
     if (bulkOperation.handleWriteError(callback, writeResult)) return;
 
     // Execute the next command in line
@@ -1246,7 +1223,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);
   }

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) {

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": {}
+              }
             }
           }
         },

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:

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 }) => {