refactor: Move commit logic and add tests that prepare for transaction function changes

.github/.OwlBot.lock.yaml

@@ -13,5 +13,5 @@
 # limitations under the License.
 docker:
   image: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest
-  digest: sha256:abc68a9bbf4fa808b25fa16d3b11141059dc757dbc34f024744bba36c200b40f
-# created: 2023-10-04T20:56:40.710775365Z
+  digest: sha256:e92044720ab3cb6984a70b0c6001081204375959ba3599ef6c42dd99a7783a67
+# created: 2023-11-10T00:24:05.581078808Z

package.json

@@ -53,12 +53,12 @@
   "devDependencies": {
     "@google-cloud/storage": "^7.0.1",
     "@types/extend": "^3.0.1",
-    "@types/is": "0.0.24",
+    "@types/is": "0.0.25",
     "@types/js-yaml": "^4.0.0",
     "@types/mocha": "^9.0.0",
     "@types/node": "^20.4.9",
     "@types/proxyquire": "^1.3.28",
-    "@types/sinon": "^10.0.0",
+    "@types/sinon": "^17.0.0",
     "async": "^3.2.4",
     "c8": "^8.0.1",
     "gapic-tools": "^0.2.0",

src/transaction.ts

@@ -161,117 +161,7 @@ class Transaction extends DatastoreRequest {
         : () => {};
     const gaxOptions =
       typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {};
-
-    if (this.skipCommit) {
-      setImmediate(callback);
-      return;
-    }
-
-    const keys: Entities = {};
-
-    this.modifiedEntities_
-      // Reverse the order of the queue to respect the "last queued request
-      // wins" behavior.
-      .reverse()
-      // Limit the operations we're going to send through to only the most
-      // recently queued operations. E.g., if a user tries to save with the
-      // same key they just asked to be deleted, the delete request will be
-      // ignored, giving preference to the save operation.
-      .filter((modifiedEntity: Entity) => {
-        const key = modifiedEntity.entity.key;
-
-        if (!entity.isKeyComplete(key)) return true;
-
-        const stringifiedKey = JSON.stringify(modifiedEntity.entity.key);
-
-        if (!keys[stringifiedKey]) {
-          keys[stringifiedKey] = true;
-          return true;
-        }
-
-        return false;
-      })
-      // Group entities together by method: `save` mutations, then `delete`.
-      // Note: `save` mutations being first is required to maintain order when
-      // assigning IDs to incomplete keys.
-      .sort((a, b) => {
-        return a.method < b.method ? 1 : a.method > b.method ? -1 : 0;
-      })
-      // Group arguments together so that we only make one call to each
-      // method. This is important for `DatastoreRequest.save`, especially, as
-      // that method handles assigning auto-generated IDs to the original keys
-      // passed in. When we eventually execute the `save` method's API
-      // callback, having all the keys together is necessary to maintain
-      // order.
-      .reduce((acc: Entities, entityObject: Entity) => {
-        const lastEntityObject = acc[acc.length - 1];
-        const sameMethod =
-          lastEntityObject && entityObject.method === lastEntityObject.method;
-
-        if (!lastEntityObject || !sameMethod) {
-          acc.push(entityObject);
-        } else {
-          lastEntityObject.args = lastEntityObject.args.concat(
-            entityObject.args
-          );
-        }
-
-        return acc;
-      }, [])
-      // Call each of the mutational methods (DatastoreRequest[save,delete])
-      // to build up a `req` array on this instance. This will also build up a
-      // `callbacks` array, that is the same callback that would run if we
-      // were using `save` and `delete` outside of a transaction, to process
-      // the response from the API.
-      .forEach(
-        (modifiedEntity: {method: string; args: {reverse: () => void}}) => {
-          const method = modifiedEntity.method;
-          const args = modifiedEntity.args.reverse();
-          Datastore.prototype[method].call(this, args, () => {});
-        }
-      );
-
-    // Take the `req` array built previously, and merge them into one request to
-    // send as the final transactional commit.
-    const reqOpts = {
-      mutations: this.requests_
-        .map((x: {mutations: google.datastore.v1.Mutation}) => x.mutations)
-        .reduce(
-          (a: {concat: (arg0: Entity) => void}, b: Entity) => a.concat(b),
-          []
-        ),
-    };
-
-    this.request_(
-      {
-        client: 'DatastoreClient',
-        method: 'commit',
-        reqOpts,
-        gaxOpts: gaxOptions || {},
-      },
-      (err, resp) => {
-        if (err) {
-          // Rollback automatically for the user.
-          this.rollback(() => {
-            // Provide the error & API response from the failed commit to the
-            // user. Even a failed rollback should be transparent. RE:
-            // https://github.com/GoogleCloudPlatform/google-cloud-node/pull/1369#discussion_r66833976
-            callback(err, resp);
-          });
-          return;
-        }
-
-        // The `callbacks` array was built previously. These are the callbacks
-        // that handle the API response normally when using the
-        // DatastoreRequest.save and .delete methods.
-        this.requestCallbacks_.forEach(
-          (cb: (arg0: null, arg1: Entity) => void) => {
-            cb(null, resp);
-          }
-        );
-        callback(null, resp);
-      }
-    );
+    this.#runCommit(gaxOptions, callback);
   }
 
   /**
@@ -561,6 +451,131 @@ class Transaction extends DatastoreRequest {
     });
   }
 
+  /**
+   * This function is a pass-through for the transaction.commit method
+   * It contains the business logic used for committing a transaction
+   *
+   * @param {object} [gaxOptions] Request configuration options, outlined here:
+   *     https://googleapis.github.io/gax-nodejs/global.html#CallOptions.
+   * @param {function} callback The callback function.
+   * @private
+   */
+  #runCommit(
+    gaxOptions: CallOptions,
+    callback: CommitCallback
+  ): void | Promise<CommitResponse> {
+    if (this.skipCommit) {
+      setImmediate(callback);
+      return;
+    }
+
+    const keys: Entities = {};
+
+    this.modifiedEntities_
+      // Reverse the order of the queue to respect the "last queued request
+      // wins" behavior.
+      .reverse()
+      // Limit the operations we're going to send through to only the most
+      // recently queued operations. E.g., if a user tries to save with the
+      // same key they just asked to be deleted, the delete request will be
+      // ignored, giving preference to the save operation.
+      .filter((modifiedEntity: Entity) => {
+        const key = modifiedEntity.entity.key;
+
+        if (!entity.isKeyComplete(key)) return true;
+
+        const stringifiedKey = JSON.stringify(modifiedEntity.entity.key);
+
+        if (!keys[stringifiedKey]) {
+          keys[stringifiedKey] = true;
+          return true;
+        }
+
+        return false;
+      })
+      // Group entities together by method: `save` mutations, then `delete`.
+      // Note: `save` mutations being first is required to maintain order when
+      // assigning IDs to incomplete keys.
+      .sort((a, b) => {
+        return a.method < b.method ? 1 : a.method > b.method ? -1 : 0;
+      })
+      // Group arguments together so that we only make one call to each
+      // method. This is important for `DatastoreRequest.save`, especially, as
+      // that method handles assigning auto-generated IDs to the original keys
+      // passed in. When we eventually execute the `save` method's API
+      // callback, having all the keys together is necessary to maintain
+      // order.
+      .reduce((acc: Entities, entityObject: Entity) => {
+        const lastEntityObject = acc[acc.length - 1];
+        const sameMethod =
+          lastEntityObject && entityObject.method === lastEntityObject.method;
+
+        if (!lastEntityObject || !sameMethod) {
+          acc.push(entityObject);
+        } else {
+          lastEntityObject.args = lastEntityObject.args.concat(
+            entityObject.args
+          );
+        }
+
+        return acc;
+      }, [])
+      // Call each of the mutational methods (DatastoreRequest[save,delete])
+      // to build up a `req` array on this instance. This will also build up a
+      // `callbacks` array, that is the same callback that would run if we
+      // were using `save` and `delete` outside of a transaction, to process
+      // the response from the API.
+      .forEach(
+        (modifiedEntity: {method: string; args: {reverse: () => void}}) => {
+          const method = modifiedEntity.method;
+          const args = modifiedEntity.args.reverse();
+          Datastore.prototype[method].call(this, args, () => {});
+        }
+      );
+
+    // Take the `req` array built previously, and merge them into one request to
+    // send as the final transactional commit.
+    const reqOpts = {
+      mutations: this.requests_
+        .map((x: {mutations: google.datastore.v1.Mutation}) => x.mutations)
+        .reduce(
+          (a: {concat: (arg0: Entity) => void}, b: Entity) => a.concat(b),
+          []
+        ),
+    };
+
+    this.request_(
+      {
+        client: 'DatastoreClient',
+        method: 'commit',
+        reqOpts,
+        gaxOpts: gaxOptions || {},
+      },
+      (err, resp) => {
+        if (err) {
+          // Rollback automatically for the user.
+          this.rollback(() => {
+            // Provide the error & API response from the failed commit to the
+            // user. Even a failed rollback should be transparent. RE:
+            // https://github.com/GoogleCloudPlatform/google-cloud-node/pull/1369#discussion_r66833976
+            callback(err, resp);
+          });
+          return;
+        }
+
+        // The `callbacks` array was built previously. These are the callbacks
+        // that handle the API response normally when using the
+        // DatastoreRequest.save and .delete methods.
+        this.requestCallbacks_.forEach(
+          (cb: (arg0: null, arg1: Entity) => void) => {
+            cb(null, resp);
+          }
+        );
+        callback(null, resp);
+      }
+    );
+  }
+
   /**
    * This function parses results from a beginTransaction call
    *