chore: address comments

Author: durran

src/cursor/client_bulk_write_cursor.ts

@@ -1,5 +1,6 @@
 import type { Document } from '../bson';
 import { type ClientBulkWriteCursorResponse } from '../cmap/wire_protocol/responses';
+import { MongoBulkWriteCursorError } from '../error';
 import type { MongoClient } from '../mongo_client';
 import { ClientBulkWriteOperation } from '../operations/client_bulk_write/client_bulk_write';
 import { type ClientBulkWriteOptions } from '../operations/client_bulk_write/common';
@@ -43,7 +44,9 @@ export class ClientBulkWriteCursor extends AbstractCursor {
    */
   get response(): ClientBulkWriteCursorResponse {
     if (this.cursorResponse) return this.cursorResponse;
-    throw new Error('no cursor response');
+    throw new MongoBulkWriteCursorError(
+      'No client bulk write cursor response returned from the server.'
+    );
   }
 
   clone(): ClientBulkWriteCursor {

src/error.ts

@@ -616,6 +616,33 @@ export class MongoGCPError extends MongoOIDCError {
   }
 }
 
+/**
+ * An error generated when a ChangeStream operation fails to execute.
+ *
+ * @public
+ * @category Error
+ */
+export class MongoBulkWriteCursorError extends MongoRuntimeError {
+  /**
+   * **Do not use this constructor!**
+   *
+   * Meant for internal use only.
+   *
+   * @remarks
+   * This class is only meant to be constructed within the driver. This constructor is
+   * not subject to semantic versioning compatibility guarantees and may change at any time.
+   *
+   * @public
+   **/
+  constructor(message: string) {
+    super(message);
+  }
+
+  override get name(): string {
+    return 'MongoBulkWriteCursorError';
+  }
+}
+
 /**
  * An error generated when a ChangeStream operation fails to execute.
  *

src/operations/client_bulk_write/client_bulk_write.ts

@@ -9,7 +9,7 @@ import { Aspect, defineAspects } from '../operation';
 import { type ClientBulkWriteOptions } from './common';
 
 /**
- * Executes a single vlient bulk write operation within a potential batch.
+ * Executes a single client bulk write operation within a potential batch.
  * @internal
  */
 export class ClientBulkWriteOperation extends CommandOperation<ClientBulkWriteCursorResponse> {

src/operations/client_bulk_write/command_builder.ts

@@ -1,6 +1,8 @@
-import { type Document, ObjectId } from '../../bson';
+import { type Document } from '../../bson';
 import { DocumentSequence } from '../../cmap/commands';
+import { type PkFactory } from '../../mongo_client';
 import type { Filter, OptionalId, UpdateFilter, WithoutId } from '../../mongo_types';
+import { DEFAULT_PK_FACTORY } from '../../utils';
 import { type CollationOptions } from '../command';
 import { type Hint } from '../operation';
 import type {
@@ -30,14 +32,20 @@ export interface ClientBulkWriteCommand {
 export class ClientBulkWriteCommandBuilder {
   models: AnyClientBulkWriteModel[];
   options: ClientBulkWriteOptions;
+  pkFactory: PkFactory;
 
   /**
    * Create the command builder.
    * @param models - The client write models.
    */
-  constructor(models: AnyClientBulkWriteModel[], options: ClientBulkWriteOptions) {
+  constructor(
+    models: AnyClientBulkWriteModel[],
+    options: ClientBulkWriteOptions,
+    pkFactory?: PkFactory
+  ) {
     this.models = models;
     this.options = options;
+    this.pkFactory = pkFactory ?? DEFAULT_PK_FACTORY;
   }
 
   /**
@@ -63,10 +71,10 @@ export class ClientBulkWriteCommandBuilder {
       const ns = model.namespace;
       const index = namespaces.get(ns);
       if (index != null) {
-        operations.push(buildOperation(model, index));
+        operations.push(buildOperation(model, index, this.pkFactory));
       } else {
         namespaces.set(ns, currentNamespaceIndex);
-        operations.push(buildOperation(model, currentNamespaceIndex));
+        operations.push(buildOperation(model, currentNamespaceIndex, this.pkFactory));
         currentNamespaceIndex++;
       }
     }
@@ -90,7 +98,9 @@ export class ClientBulkWriteCommandBuilder {
       command.let = this.options.let;
     }
 
-    if (this.options.comment != null) {
+    // we check for undefined specifically here to allow falsy values
+    // eslint-disable-next-line no-restricted-syntax
+    if (this.options.comment !== undefined) {
       command.comment = this.options.comment;
     }
     return [command];
@@ -111,13 +121,14 @@ interface ClientInsertOperation {
  */
 export const buildInsertOneOperation = (
   model: ClientInsertOneModel,
-  index: number
+  index: number,
+  pkFactory: PkFactory
 ): ClientInsertOperation => {
   const document: ClientInsertOperation = {
     insert: index,
     document: model.document
   };
-  document.document._id = model.document._id ?? new ObjectId();
+  document.document._id = model.document._id ?? pkFactory.createPk();
   return document;
 };
 
@@ -279,10 +290,14 @@ export const buildReplaceOneOperation = (
 };
 
 /** @internal */
-export function buildOperation(model: AnyClientBulkWriteModel, index: number): Document {
+export function buildOperation(
+  model: AnyClientBulkWriteModel,
+  index: number,
+  pkFactory: PkFactory
+): Document {
   switch (model.name) {
     case 'insertOne':
-      return buildInsertOneOperation(model, index);
+      return buildInsertOneOperation(model, index, pkFactory);
     case 'deleteOne':
       return buildDeleteOneOperation(model, index);
     case 'deleteMany':

src/operations/client_bulk_write/common.ts

@@ -209,6 +209,11 @@ export interface ClientUpdateResult {
    * these two cases.
    */
   upsertedId?: any;
+
+  /**
+   * Determines if the upsert did include an _id, which includes the case of the _id being null.
+   */
+  didUpsert: boolean;
 }
 
 /** @public */

src/operations/client_bulk_write/executor.ts

@@ -5,7 +5,7 @@ import { type MongoClient } from '../../mongo_client';
 import { WriteConcern } from '../../write_concern';
 import { executeOperation } from '../execute_operation';
 import { ClientBulkWriteOperation } from './client_bulk_write';
-import { ClientBulkWriteCommandBuilder } from './command_builder';
+import { type ClientBulkWriteCommand, ClientBulkWriteCommandBuilder } from './command_builder';
 import {
   type AnyClientBulkWriteModel,
   type ClientBulkWriteOptions,
@@ -51,8 +51,13 @@ export class ClientBulkWriteExecutor {
   async execute(): Promise<ClientBulkWriteResult | { ok: 1 }> {
     // The command builder will take the user provided models and potential split the batch
     // into multiple commands due to size.
-    const commmandBuilder = new ClientBulkWriteCommandBuilder(this.operations, this.options);
-    const commands = commmandBuilder.buildCommands();
+    const pkFactory = this.client.s.options.pkFactory;
+    const commandBuilder = new ClientBulkWriteCommandBuilder(
+      this.operations,
+      this.options,
+      pkFactory
+    );
+    const commands = commandBuilder.buildCommands();
     if (this.options.writeConcern?.w === 0) {
       return await executeUnacknowledged(this.client, this.options, commands);
     }
@@ -66,7 +71,7 @@ export class ClientBulkWriteExecutor {
 async function executeAcknowledged(
   client: MongoClient,
   options: ClientBulkWriteOptions,
-  commands: Document[]
+  commands: ClientBulkWriteCommand[]
 ): Promise<ClientBulkWriteResult> {
   const resultsMerger = new ClientBulkWriteResultsMerger(options);
   // For each command will will create and exhaust a cursor for the results.

src/operations/client_bulk_write/results_merger.ts

@@ -22,23 +22,21 @@ export class ClientBulkWriteResultsMerger {
    */
   constructor(options: ClientBulkWriteOptions) {
     this.options = options;
-    const baseResult = {
+    this.result = {
       insertedCount: 0,
       upsertedCount: 0,
       matchedCount: 0,
       modifiedCount: 0,
-      deletedCount: 0
+      deletedCount: 0,
+      insertResults: undefined,
+      updateResults: undefined,
+      deleteResults: undefined
     };
 
     if (options.verboseResults) {
-      this.result = {
-        ...baseResult,
-        insertResults: new Map<number, ClientInsertOneResult>(),
-        updateResults: new Map<number, ClientUpdateResult>(),
-        deleteResults: new Map<number, ClientDeleteResult>()
-      };
-    } else {
-      this.result = baseResult;
+      this.result.insertResults = new Map<number, ClientInsertOneResult>();
+      this.result.updateResults = new Map<number, ClientUpdateResult>();
+      this.result.deleteResults = new Map<number, ClientDeleteResult>();
     }
   }
 
@@ -75,7 +73,10 @@ export class ClientBulkWriteResultsMerger {
           if ('update' in operation) {
             const result: ClientUpdateResult = {
               matchedCount: document.n,
-              modifiedCount: document.nModified || 0
+              modifiedCount: document.nModified ?? 0,
+              // We do specifically want to check undefined here since a null _id is valid.
+              // eslint-disable-next-line no-restricted-syntax
+              didUpsert: document.upserted?._id !== undefined
             };
             if (document.upserted) {
               result.upsertedId = document.upserted._id;

test/tools/unified-spec-runner/match.ts

@@ -217,7 +217,7 @@ export function resultCheck(
       // to convert the expected keys from the string numbers to actual numbers since the key
       // values in the maps are actual numbers.
       const isActualMap = actual instanceof Map;
-      const mapKey = !Number.isNaN(key) ? Number(key) : key;
+      const mapKey = !Number.isNaN(Number(key)) ? Number(key) : key;
       resultCheck(
         isActualMap ? actual.get(mapKey) : actual[key],
         value,

test/unit/operations/client_bulk_write/command_builder.test.ts

@@ -14,6 +14,7 @@ import {
   type ClientReplaceOneModel,
   type ClientUpdateManyModel,
   type ClientUpdateOneModel,
+  DEFAULT_PK_FACTORY,
   DocumentSequence,
   ObjectId
 } from '../../../mongodb';
@@ -232,7 +233,7 @@ describe('ClientBulkWriteCommandBuilder', function () {
         namespace: 'test.coll',
         document: { name: 1 }
       };
-      const operation = buildInsertOneOperation(model, 5);
+      const operation = buildInsertOneOperation(model, 5, DEFAULT_PK_FACTORY);
 
       it('generates the insert operation with an _id', function () {
         expect(operation.insert).to.equal(5);
@@ -248,7 +249,7 @@ describe('ClientBulkWriteCommandBuilder', function () {
         namespace: 'test.coll',
         document: { _id: id, name: 1 }
       };
-      const operation = buildInsertOneOperation(model, 5);
+      const operation = buildInsertOneOperation(model, 5, DEFAULT_PK_FACTORY);
 
       it('generates the insert operation with an _id', function () {
         expect(operation).to.deep.equal({ insert: 5, document: { _id: id, name: 1 } });

test/unit/operations/client_bulk_write/results_merger.test.ts

@@ -18,7 +18,10 @@ describe('ClientBulkWriteResultsMerger', function () {
         upsertedCount: 0,
         matchedCount: 0,
         modifiedCount: 0,
-        deletedCount: 0
+        deletedCount: 0,
+        deleteResults: undefined,
+        insertResults: undefined,
+        updateResults: undefined
       });
     });
   });
@@ -99,15 +102,17 @@ describe('ClientBulkWriteResultsMerger', function () {
           it('sets the update results', function () {
             expect(result.updateResults.get(1)).to.deep.equal({
               matchedCount: 1,
-              modifiedCount: 1
+              modifiedCount: 1,
+              didUpsert: false
             });
           });
 
           it('sets the upsert results', function () {
             expect(result.updateResults.get(2)).to.deep.equal({
               matchedCount: 0,
               modifiedCount: 0,
-              upsertedId: 1
+              upsertedId: 1,
+              didUpsert: true
             });
           });
 
@@ -172,7 +177,7 @@ describe('ClientBulkWriteResultsMerger', function () {
           });
 
           it('sets no insert results', function () {
-            expect(result).to.not.have.property('insertResults');
+            expect(result.insertResults).to.equal(undefined);
           });
 
           it('merges the upserted count', function () {
@@ -188,15 +193,15 @@ describe('ClientBulkWriteResultsMerger', function () {
           });
 
           it('sets no update results', function () {
-            expect(result).to.not.have.property('updateResults');
+            expect(result.updateResults).to.equal(undefined);
           });
 
           it('merges the deleted count', function () {
             expect(result.deletedCount).to.equal(1);
           });
 
           it('sets no delete results', function () {
-            expect(result).to.not.have.property('deleteResults');
+            expect(result.deleteResults).to.equal(undefined);
           });
         });
       });