refactor: Break transaction.run into smaller pieces for use with asyn…

…c functions and other read/write calls (#1196) * Add a unit test for commit as a non-tx The fact that using a transaction object to do a commit results in a non-transaction should be documented so that if we decide to introduce a change later where this behaves differently then it is well documented. # Conflicts: # test/transaction.ts * use linter * Write tests to capture current behavior of error When begin transaction sends back an error, we want some tests to capture what the behavior is so that when we make changes to the run function then behavior is preserved. * Add tests for passing a response A response should reach the user the right way. Add tests to make sure behavior is preserved. * run async close to working In the run function delegate calls to runAsync and use run async to make promise calls * Add runAsync to promise excludes This allows this function to return a promise instead of a promise wrapped in a promise. This makes the tests pass and behave the way they should. * Remove space * Change to use this instead of self Do not call request from self * Eliminate unused comments * Add two comments Comments should actually explain what is being done * Remove the commit test for this PR The commit test for this PR should be removed because it is not really relevant for the async run functionality. * Clarify types throughout the function The types used should be very specific so that reading the code isn’t confusing. * Add a bit more typing for clarity Add a type to the resolve function just to introduce more clarity * Change types used in the data client callback Make the types more specific in the data client callback so that it is easier to track down the signature and match against the begin transaction function. * run the linter * Add comments to clarify PR * Refactor the parsing logic out of run The parsing logic is going to be needed elsewhere so taking it apart now. * Change interface of request promise callback The interface name should be changed so that it matches what it is. It is the callback used to form a promise. * Hide data completely Change accessors to hide data completely instead of using the private modifier * PR use if/else block Eliminate the early return as suggested in the PR * Add comments to document the new functions The comments capture the parameters and return type. * Update return type in docs * Update the tests to include runAsync runAsync should be in promisfy excludes * refactor: Break transaction.run into smaller pieces for use with async functions and other read/write calls * Rename a function to be more descriptive Make sure it is explicit that we are parsing begin results. * Modify comment Modify comment so that it doesn’t reference the way the code was before.
googleapis · danieljbruce · Feb 26, 2024 · Nov 17, 2023 · Nov 22, 2023 · Feb 12, 2024
commit 7284d99c22de1acb53e70273e8e98bcecf69de3e
@@ -32,6 +32,18 @@ import {
 } from './request';
 import {AggregateQuery} from './aggregate';
 
+// RequestPromiseReturnType should line up with the types in RequestCallback
+interface RequestPromiseReturnType {
+ err?: Error | null;
+ resp: any; // TODO: Replace with google.datastore.v1.IBeginTransactionResponse and address downstream issues
+}
+interface RequestResolveFunction {
+ (callbackData: RequestPromiseReturnType): void;
+}
+interface RequestAsPromiseCallback {
+ (resolve: RequestResolveFunction): void;
+}
+
 /**
  * A transaction is a set of Datastore operations on one or more entities. Each
  * transaction is guaranteed to be atomic, which means that transactions are
@@ -544,10 +556,47 @@ class Transaction extends DatastoreRequest {
  typeof optionsOrCallback === 'object' ? optionsOrCallback : {};
  const callback =
  typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;
+ this.#runAsync(options).then((response: RequestPromiseReturnType) => {
+ this.#processBeginResults(response, callback);
+ });
+ }
 
- const reqOpts = {
+ /**
+ * This function parses results from a beginTransaction call
+ *
+ * @param {RequestPromiseReturnType} response The response from a call to
+ * begin a transaction.
+ * @param {RunCallback} callback A callback that accepts an error and a
+ * response as arguments.
+ *
+ **/
+ #processBeginResults(
+ response: RequestPromiseReturnType,
+ callback: RunCallback
+ ): void {
+ const err = response.err;
+ const resp = response.resp;
+ if (err) {
+ callback(err, null, resp);
+ } else {
+ this.id = resp!.transaction;
+ callback(null, this, resp);
+ }
+ }
+
+ /**
+ * This async function makes a beginTransaction call and returns a promise with
+ * the information returned from the call that was made.
+ *
+ * @param {RunOptions} options The options used for a beginTransaction call.
+ * @returns {Promise<RequestPromiseReturnType>}
+ *
+ *
+ **/
+ async #runAsync(options: RunOptions): Promise<RequestPromiseReturnType> {
+ const reqOpts: RequestOptions = {
  transactionOptions: {},
- } as RequestOptions;
+ };
 
  if (options.readOnly || this.readOnly) {
  reqOpts.transactionOptions!.readOnly = {};
@@ -562,23 +611,26 @@ class Transaction extends DatastoreRequest {
  if (options.transactionOptions) {
  reqOpts.transactionOptions = options.transactionOptions;
  }
-
- this.request_(
- {
- client: 'DatastoreClient',
- method: 'beginTransaction',
- reqOpts,
- gaxOpts: options.gaxOptions,
- },
- (err, resp) => {
- if (err) {
- callback(err, null, resp);
- return;
+ const promiseFunction: RequestAsPromiseCallback = (
+ resolve: RequestResolveFunction
+ ) => {
+ this.request_(
+ {
+ client: 'DatastoreClient',
+ method: 'beginTransaction',
+ reqOpts,
+ gaxOpts: options.gaxOptions,
+ },
+ // Always use resolve because then this function can return both the error and the response
+ (err, resp) => {
+ resolve({
+ err,
+ resp,
+ });
  }
- this.id = resp!.transaction;
- callback(null, this, resp);
- }
- );
+ );
+ };
+ return new Promise(promiseFunction);
  }
 
  /**
@@ -810,6 +862,7 @@ promisifyAll(Transaction, {
  'createQuery',
  'delete',
  'insert',
+ '#runAsync',
  'save',
  'update',
  'upsert',

@@ -21,15 +21,21 @@ import * as proxyquire from 'proxyquire';
 import {
  Datastore,
  DatastoreOptions,
+ DatastoreClient,
  DatastoreRequest,
  Query,
  TransactionOptions,
+ Transaction,
 } from '../src';
 import {Entity} from '../src/entity';
 import * as tsTypes from '../src/transaction';
 import * as sinon from 'sinon';
+import {Callback, CallOptions, ClientStub} from 'google-gax';
 import {RequestConfig} from '../src/request';
 import {SECOND_DATABASE_ID} from './index';
+import {google} from '../protos/protos';
+import {RunCallback} from '../src/transaction';
+import * as protos from '../protos/protos';
 const async = require('async');
 
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -51,6 +57,7 @@ const fakePfy = Object.assign({}, pfy, {
  'createQuery',
  'delete',
  'insert',
+ '#runAsync',
  'save',
  'update',
  'upsert',
@@ -147,6 +154,137 @@ async.each(
  });
  });
 
+ describe('run without setting up transaction id', () => {
+ // These tests were created so that when transaction.run is restructured we
+ // can be confident that it works the same way as before.
+ const testResp = {
+ transaction: Buffer.from(Array.from(Array(100).keys())),
+ };
+ const namespace = 'run-without-mock';
+ const projectId = 'project-id';
+ const testErrorMessage = 'test-error';
+ const options = {
+ projectId,
+ namespace,
+ };
+ const datastore = new Datastore(options);
+ const transactionWithoutMock = datastore.transaction();
+ const dataClientName = 'DatastoreClient';
+ let dataClient: ClientStub | undefined;
+ let originalBeginTransactionMethod: Function;
+
+ beforeEach(async () => {
+ // In this before hook, save the original beginTransaction method in a variable.
+ // After tests are finished, reassign beginTransaction to the variable.
+ // This way, mocking beginTransaction in this block doesn't affect other tests.
+ const gapic = Object.freeze({
+ v1: require('../src/v1'),
+ });
+ // Datastore Gapic clients haven't been initialized yet so we initialize them here.
+ datastore.clients_.set(
+ dataClientName,
+ new gapic.v1[dataClientName](options)
+ );
+ dataClient = datastore.clients_.get(dataClientName);
+ if (dataClient && dataClient.beginTransaction) {
+ originalBeginTransactionMethod = dataClient.beginTransaction;
+ }
+ });
+
+ afterEach(() => {
+ // beginTransaction has likely been mocked out in these tests.
+ // We should reassign beginTransaction back to its original value for tests outside this block.
+ if (dataClient && originalBeginTransactionMethod) {
+ dataClient.beginTransaction = originalBeginTransactionMethod;
+ }
+ });
+
+ describe('should pass error back to the user', async () => {
+ beforeEach(() => {
+ // Mock out begin transaction and send error back to the user
+ // from the Gapic layer.
+ if (dataClient) {
+ dataClient.beginTransaction = (
+ request: protos.google.datastore.v1.IBeginTransactionRequest,
+ options: CallOptions,
+ callback: Callback<
+ protos.google.datastore.v1.IBeginTransactionResponse,
+ | protos.google.datastore.v1.IBeginTransactionRequest
+ | null
+ | undefined,
+ {} | null | undefined
+ >
+ ) => {
+ callback(new Error(testErrorMessage), testResp);
+ };
+ }
+ });
+
+ it('should send back the error when awaiting a promise', async () => {
+ try {
+ await transactionWithoutMock.run();
+ assert.fail('The run call should have failed.');
+ } catch (error: any) {
+ // TODO: Substitute type any
+ assert.strictEqual(error['message'], testErrorMessage);
+ }
+ });
+ it('should send back the error when using a callback', done => {
+ const runCallback: RunCallback = (
+ error: Error | null,
+ transaction: Transaction | null,
+ response?: google.datastore.v1.IBeginTransactionResponse
+ ) => {
+ assert(error);
+ assert.strictEqual(error.message, testErrorMessage);
+ assert.strictEqual(transaction, null);
+ assert.strictEqual(response, testResp);
+ done();
+ };
+ transactionWithoutMock.run({}, runCallback);
+ });
+ });
+ describe('should pass response back to the user', async () => {
+ beforeEach(() => {
+ // Mock out begin transaction and send a response
+ // back to the user from the Gapic layer.
+ if (dataClient) {
+ dataClient.beginTransaction = (
+ request: protos.google.datastore.v1.IBeginTransactionRequest,
+ options: CallOptions,
+ callback: Callback<
+ protos.google.datastore.v1.IBeginTransactionResponse,
+ | protos.google.datastore.v1.IBeginTransactionRequest
+ | null
+ | undefined,
+ {} | null | undefined
+ >
+ ) => {
+ callback(null, testResp);
+ };
+ }
+ });
+ it('should send back the response when awaiting a promise', async () => {
+ const [transaction, resp] = await transactionWithoutMock.run();
+ assert.strictEqual(transaction, transactionWithoutMock);
+ assert.strictEqual(resp, testResp);
+ });
+ it('should send back the response when using a callback', done => {
+ const runCallback: RunCallback = (
+ error: Error | null,
+ transaction: Transaction | null,
+ response?: google.datastore.v1.IBeginTransactionResponse
+ ) => {
+ assert.strictEqual(error, null);
+ assert.strictEqual(response, testResp);
+ assert.strictEqual(transaction, transactionWithoutMock);
+ done();
+ };
+ transactionWithoutMock.run({}, runCallback);
+ });
+ });
+ });
+
  describe('commit', () => {
  beforeEach(() => {
  transaction.id = TRANSACTION_ID;