From 5de45453fe4cf4c8cf977a8df196ea7d64416cad Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 2 Nov 2023 17:39:10 -0400 Subject: [PATCH 01/50] Add the mutex and transaction state etc. Still an issue with system tests. commit still needs tweaks to work with async. --- package.json | 1 + src/request.ts | 21 +++++++ src/transaction.ts | 92 +++++++++++++++++++++++++++-- test/transaction.ts | 141 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 249 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 95c436c5b..5956f1969 100644 --- a/package.json +++ b/package.json @@ -43,6 +43,7 @@ "dependencies": { "@google-cloud/promisify": "^4.0.0", "arrify": "^2.0.1", + "async-mutex": "^0.4.0", "concat-stream": "^2.0.0", "extend": "^3.0.2", "google-gax": "^4.0.5", diff --git a/src/request.ts b/src/request.ts index d39f572ef..6ba6993dc 100644 --- a/src/request.ts +++ b/src/request.ts @@ -57,6 +57,7 @@ import { import {Datastore} from '.'; import ITimestamp = google.protobuf.ITimestamp; import {AggregateQuery} from './aggregate'; +import {Mutex} from 'async-mutex'; /** * A map of read consistency values to proto codes. @@ -69,6 +70,23 @@ const CONSISTENCY_PROTO_CODE: ConsistencyProtoCode = { strong: 1, }; +// TODO: Typescript had difficulty working with enums before. +// TODO: Try to get enums working instead of using static properties. + +export class TransactionState { + static NOT_TRANSACTION = Symbol('NON_TRANSACTION'); + static NOT_STARTED = Symbol('NOT_STARTED'); + // IN_PROGRESS currently tracks the expired state as well + static IN_PROGRESS = Symbol('IN_PROGRESS'); +} + +/* +export enum TransactionState { + NOT_TRANSACTION, + NOT_STARTED, + IN_PROGRESS +} + */ /** * Handle logic for Datastore API operations. Handles request logic for * Datastore. @@ -89,6 +107,9 @@ class DatastoreRequest { | Array<(err: Error | null, resp: Entity | null) => void> | Entity; datastore!: Datastore; + // TODO: Replace protected with a symbol for better data hiding. + protected mutex = new Mutex(); + protected state: Symbol = TransactionState.NOT_TRANSACTION; [key: string]: Entity; /** diff --git a/src/transaction.ts b/src/transaction.ts index 8dcdca1ec..4662cf097 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -29,6 +29,7 @@ import { DatastoreRequest, RequestOptions, PrepareEntityObjectResponse, + TransactionState, } from './request'; import {AggregateQuery} from './aggregate'; @@ -44,6 +45,12 @@ interface RequestAsPromiseCallback { (resolve: RequestResolveFunction): void; } +// Data types in CommitPromiseReturnType should line up with CommitCallback +interface CommitPromiseReturnType { + err?: Error | null; + resp?: google.datastore.v1.ICommitResponse; +} + /** * 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 @@ -99,6 +106,7 @@ class Transaction extends DatastoreRequest { // Queue the requests to make when we send the transactional commit. this.requests_ = []; + this.state = TransactionState.NOT_STARTED; } /*! Developer Documentation @@ -162,6 +170,47 @@ class Transaction extends DatastoreRequest { const gaxOptions = typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; this.runCommit(gaxOptions, callback); + // TODO: Add call to commitAsync here and handle result in the .then hook + /* + this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { + callback(response.err, response.resp); + }); + */ + } + + // The promise that commitAsync uses should always resolve and never reject. + // The data it resolves with will contain response and error information. + private async commitAsync( + gaxOptions: CallOptions + ): Promise { + if (this.state === TransactionState.NOT_STARTED) { + const release = await this.mutex.acquire(); + try { + try { + if (this.state === TransactionState.NOT_STARTED) { + const runResults = await this.runAsync({gaxOptions}); + this.parseRunSuccess(runResults); + } + } finally { + // TODO: Check that error actually reaches user + release(); // TODO: Be sure to release the mutex in the error state + } + } catch (err: any) { + return await new Promise(resolve => { + // resp from the beginTransaction call never reaches the user + // if we allowed it to then the return type of commit would need to change + resolve({err}); + }); + } + } + return await new Promise(resolve => { + this.runCommit( + gaxOptions, + (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { + resolve({err, resp}); + } + ); + }); } /** @@ -446,15 +495,37 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'object' ? optionsOrCallback : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; - this.runAsync(options).then((response: RequestPromiseReturnType) => { - this.parseRunAsync(response, callback); - }); + if (this.state === TransactionState.NOT_STARTED) { + this.mutex.acquire().then(release => { + this.runAsync(options).then((response: RequestPromiseReturnType) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.parseRunAsync(response, callback); + }); + }); + } else { + process.emitWarning( + 'run has already been called and should not be called again.' + ); + } } + private runCommit(gaxOptions?: CallOptions): Promise; + private runCommit(callback: CommitCallback): void; + private runCommit(gaxOptions: CallOptions, callback: CommitCallback): void; private runCommit( - gaxOptions: CallOptions, - callback: CommitCallback + gaxOptionsOrCallback?: CallOptions | CommitCallback, + cb?: CommitCallback ): void | Promise { + const callback = + typeof gaxOptionsOrCallback === 'function' + ? gaxOptionsOrCallback + : typeof cb === 'function' + ? cb + : () => {}; + const gaxOptions = + typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; + if (this.skipCommit) { setImmediate(callback); return; @@ -578,9 +649,16 @@ class Transaction extends DatastoreRequest { callback(err, null, resp); return; } - this.id = resp!.transaction; + this.parseRunSuccess(response); callback(null, this, resp); } + + private parseRunSuccess(response: RequestPromiseReturnType) { + const resp = response.resp; + this.id = resp!.transaction; + this.state = TransactionState.IN_PROGRESS; + } + // TODO: Replace with #runAsync when pack and play error is gone private async runAsync( options: RunOptions @@ -855,6 +933,8 @@ promisifyAll(Transaction, { 'delete', 'insert', 'runAsync', + 'parseRunAsync', + 'parseTransactionResponse', 'save', 'update', 'upsert', diff --git a/test/transaction.ts b/test/transaction.ts index f1a942b05..7c14563bd 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -154,6 +154,147 @@ async.each( }); }); + /* + describe('commit 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 = { + mutationResults: [ + { + key: { + path: [ + { + kind: 'some-kind', + }, + ], + }, + }, + ], + }; + 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 originalCommitMethod: 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.commit) { + originalCommitMethod = dataClient.commit; + } + }); + + 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 && originalCommitMethod) { + dataClient.commit = originalCommitMethod; + } + }); + + 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.commit = ( + request: protos.google.datastore.v1.ICommitRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.ICommitResponse, + protos.google.datastore.v1.ICommitRequest | null | undefined, + {} | null | undefined + > + ) => { + callback(new Error(testErrorMessage), testResp); + }; + } + }); + + it('should send back the error when awaiting a promise',async () => { + try { + await transactionWithoutMock.run(); + await transactionWithoutMock.commit(); + 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(); + }); + }); + 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('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. From 1ccacec8bf90130adcbe27cec29ce867133d2e6e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 3 Nov 2023 10:18:59 -0400 Subject: [PATCH 02/50] Remove no-op, get commit tests working Remove the no-op, get commit tests in place. --- src/transaction.ts | 21 ++-- test/transaction.ts | 301 +++++++++++++++++++++++--------------------- 2 files changed, 170 insertions(+), 152 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 4662cf097..ab3ec360f 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -495,19 +495,20 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'object' ? optionsOrCallback : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; - if (this.state === TransactionState.NOT_STARTED) { - this.mutex.acquire().then(release => { - this.runAsync(options).then((response: RequestPromiseReturnType) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.parseRunAsync(response, callback); - }); - }); - } else { + // TODO: Whenever run is called a second time and a warning is emitted then do nothing. + // TODO: This means rewriting many tests so that they don't use the same transaction object. + if (this.state !== TransactionState.NOT_STARTED) { process.emitWarning( 'run has already been called and should not be called again.' ); } + this.mutex.acquire().then(release => { + this.runAsync(options).then((response: RequestPromiseReturnType) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.parseRunAsync(response, callback); + }); + }); } private runCommit(gaxOptions?: CallOptions): Promise; @@ -932,9 +933,9 @@ promisifyAll(Transaction, { 'createQuery', 'delete', 'insert', - 'runAsync', 'parseRunAsync', 'parseTransactionResponse', + 'runAsync', 'save', 'update', 'upsert', diff --git a/test/transaction.ts b/test/transaction.ts index 7c14563bd..dcdd95b1b 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -31,7 +31,7 @@ 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 {CommitCallback, RequestConfig} from '../src/request'; import {SECOND_DATABASE_ID} from './index'; import {google} from '../protos/protos'; import {RunCallback} from '../src/transaction'; @@ -57,6 +57,8 @@ const fakePfy = Object.assign({}, pfy, { 'createQuery', 'delete', 'insert', + 'parseRunAsync', + 'parseTransactionResponse', 'runAsync', 'save', 'update', @@ -154,147 +156,6 @@ async.each( }); }); - /* - describe('commit 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 = { - mutationResults: [ - { - key: { - path: [ - { - kind: 'some-kind', - }, - ], - }, - }, - ], - }; - 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 originalCommitMethod: 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.commit) { - originalCommitMethod = dataClient.commit; - } - }); - - 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 && originalCommitMethod) { - dataClient.commit = originalCommitMethod; - } - }); - - 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.commit = ( - request: protos.google.datastore.v1.ICommitRequest, - options: CallOptions, - callback: Callback< - protos.google.datastore.v1.ICommitResponse, - protos.google.datastore.v1.ICommitRequest | null | undefined, - {} | null | undefined - > - ) => { - callback(new Error(testErrorMessage), testResp); - }; - } - }); - - it('should send back the error when awaiting a promise',async () => { - try { - await transactionWithoutMock.run(); - await transactionWithoutMock.commit(); - 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(); - }); - }); - 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('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. @@ -423,6 +284,162 @@ async.each( }; transactionWithoutMock.run({}, runCallback); }); + // TODO: Add a test here for calling commit + describe('commit without setting up transaction id when run returns a response', () => { + // These tests were created so that when transaction.commit is restructured we + // can be confident that it works the same way as before. + const testCommitResp = { + mutationResults: [ + { + key: { + path: [ + { + kind: 'some-kind', + }, + ], + }, + }, + ], + }; + const namespace = 'run-without-mock'; + const projectId = 'project-id'; + const testErrorMessage = 'test-commit-error'; + const options = { + projectId, + namespace, + }; + const datastore = new Datastore(options); + const transactionWithoutMock = datastore.transaction(); + const dataClientName = 'DatastoreClient'; + let dataClient: ClientStub | undefined; + let originalCommitMethod: 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.commit) { + originalCommitMethod = dataClient.commit; + } + }); + + 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 && originalCommitMethod) { + dataClient.commit = originalCommitMethod; + } + }); + + 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.commit = ( + request: protos.google.datastore.v1.ICommitRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.ICommitResponse, + | protos.google.datastore.v1.ICommitRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback(new Error(testErrorMessage), testCommitResp); + }; + } + }); + + it('should send back the error when awaiting a promise', async () => { + try { + await transactionWithoutMock.run(); + await transactionWithoutMock.commit(); + 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 commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + assert(error); + assert.strictEqual(error.message, testErrorMessage); + assert.strictEqual(transaction, null); + assert.strictEqual(response, testCommitResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.commit(commitCallback); + } + ); + }); + }); + 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.commit = ( + request: protos.google.datastore.v1.ICommitRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.ICommitResponse, + | protos.google.datastore.v1.ICommitRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback(null, testCommitResp); + }; + } + }); + it('should send back the response when awaiting a promise', async () => { + await transactionWithoutMock.run(); + const commitResults = await transactionWithoutMock.commit(); + assert.strictEqual(commitResults, testCommitResp); + }); + it('should send back the response when using a callback', done => { + const commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + assert(error); + assert.strictEqual(error, null); + assert.strictEqual(response, testCommitResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.commit(commitCallback); + } + ); + }); + }); + }); }); }); From 37918d76e3a8f502cc23a1b3f6602e02e87be759 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 3 Nov 2023 10:49:26 -0400 Subject: [PATCH 03/50] Add mocks and additional debugging Mocks and additional debugging hooks to introspect what is going on. --- src/transaction.ts | 15 ++++++++++----- test/transaction.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index ab3ec360f..d7b561357 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -503,11 +503,16 @@ class Transaction extends DatastoreRequest { ); } this.mutex.acquire().then(release => { - this.runAsync(options).then((response: RequestPromiseReturnType) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.parseRunAsync(response, callback); - }); + this.runAsync(options) + .then((response: RequestPromiseReturnType) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.parseRunAsync(response, callback); + }) + .catch((err: any) => { + // TODO: Remove this catch block + callback(Error('The error should always be caught by then'), this); + }); }); } diff --git a/test/transaction.ts b/test/transaction.ts index dcdd95b1b..1994f8c0c 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -358,6 +358,19 @@ async.each( ) => { callback(new Error(testErrorMessage), testCommitResp); }; + 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); + }; } }); @@ -411,6 +424,20 @@ async.each( ) => { callback(null, testCommitResp); }; + // TODO: See if eliminating this mock will fix the problem + 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 () => { From a02d293976cb25ea4d599d29f8836835b619ba6a Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 3 Nov 2023 15:11:29 -0400 Subject: [PATCH 04/50] Add tests for commit Make sure commit behaves the same way as before. --- test/transaction.ts | 187 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 185 insertions(+), 2 deletions(-) diff --git a/test/transaction.ts b/test/transaction.ts index f1a942b05..bd8a85f03 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -31,7 +31,7 @@ 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 {CommitCallback, RequestConfig} from '../src/request'; import {SECOND_DATABASE_ID} from './index'; import {google} from '../protos/protos'; import {RunCallback} from '../src/transaction'; @@ -154,7 +154,7 @@ async.each( }); }); - describe('run without setting up transaction id', () => { + describe.only('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 = { @@ -282,6 +282,189 @@ async.each( }; transactionWithoutMock.run({}, runCallback); }); + // TODO: Add a test here for calling commit + describe('commit without setting up transaction id when run returns a response', () => { + // These tests were created so that when transaction.commit is restructured we + // can be confident that it works the same way as before. + const testCommitResp = { + mutationResults: [ + { + key: { + path: [ + { + kind: 'some-kind', + }, + ], + }, + }, + ], + }; + const namespace = 'run-without-mock'; + const projectId = 'project-id'; + const testErrorMessage = 'test-commit-error'; + const options = { + projectId, + namespace, + }; + const datastore = new Datastore(options); + const transactionWithoutMock = datastore.transaction(); + const dataClientName = 'DatastoreClient'; + let dataClient: ClientStub | undefined; + let originalCommitMethod: 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.commit) { + originalCommitMethod = dataClient.commit; + } + }); + + 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 && originalCommitMethod) { + dataClient.commit = originalCommitMethod; + } + }); + + 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.commit = ( + request: protos.google.datastore.v1.ICommitRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.ICommitResponse, + | protos.google.datastore.v1.ICommitRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback(new Error(testErrorMessage), testCommitResp); + }; + 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 error when awaiting a promise', async () => { + try { + await transactionWithoutMock.run(); + await transactionWithoutMock.commit(); + 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 commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + assert(error); + assert.strictEqual(error.message, testErrorMessage); + assert.strictEqual(transaction, null); + assert.strictEqual(response, testCommitResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.commit(commitCallback); + } + ); + }); + }); + 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.commit = ( + request: protos.google.datastore.v1.ICommitRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.ICommitResponse, + | protos.google.datastore.v1.ICommitRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback(null, testCommitResp); + }; + // TODO: See if eliminating this mock will fix the problem + 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 () => { + await transactionWithoutMock.run(); + const commitResults = await transactionWithoutMock.commit(); + assert.strictEqual(commitResults, testCommitResp); + }); + it('should send back the response when using a callback', done => { + const commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + assert(error); + assert.strictEqual(error, null); + assert.strictEqual(response, testCommitResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.commit(commitCallback); + } + ); + }); + }); + }); }); }); From 81f2ec0aed40e79d13f8f79520b94b11e887cc09 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 3 Nov 2023 16:50:50 -0400 Subject: [PATCH 05/50] reverting changes to add new test on transaction --- test/transaction.ts | 325 +------------------------------------------- 1 file changed, 1 insertion(+), 324 deletions(-) diff --git a/test/transaction.ts b/test/transaction.ts index 1994f8c0c..06faf3421 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -21,21 +21,15 @@ 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 {CommitCallback, RequestConfig} from '../src/request'; +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 @@ -57,9 +51,6 @@ const fakePfy = Object.assign({}, pfy, { 'createQuery', 'delete', 'insert', - 'parseRunAsync', - 'parseTransactionResponse', - 'runAsync', 'save', 'update', 'upsert', @@ -156,320 +147,6 @@ 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); - }); - // TODO: Add a test here for calling commit - describe('commit without setting up transaction id when run returns a response', () => { - // These tests were created so that when transaction.commit is restructured we - // can be confident that it works the same way as before. - const testCommitResp = { - mutationResults: [ - { - key: { - path: [ - { - kind: 'some-kind', - }, - ], - }, - }, - ], - }; - const namespace = 'run-without-mock'; - const projectId = 'project-id'; - const testErrorMessage = 'test-commit-error'; - const options = { - projectId, - namespace, - }; - const datastore = new Datastore(options); - const transactionWithoutMock = datastore.transaction(); - const dataClientName = 'DatastoreClient'; - let dataClient: ClientStub | undefined; - let originalCommitMethod: 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.commit) { - originalCommitMethod = dataClient.commit; - } - }); - - 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 && originalCommitMethod) { - dataClient.commit = originalCommitMethod; - } - }); - - 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.commit = ( - request: protos.google.datastore.v1.ICommitRequest, - options: CallOptions, - callback: Callback< - protos.google.datastore.v1.ICommitResponse, - | protos.google.datastore.v1.ICommitRequest - | null - | undefined, - {} | null | undefined - > - ) => { - callback(new Error(testErrorMessage), testCommitResp); - }; - 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 error when awaiting a promise', async () => { - try { - await transactionWithoutMock.run(); - await transactionWithoutMock.commit(); - 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 commitCallback: CommitCallback = ( - error: Error | null | undefined, - response?: google.datastore.v1.ICommitResponse - ) => { - assert(error); - assert.strictEqual(error.message, testErrorMessage); - assert.strictEqual(transaction, null); - assert.strictEqual(response, testCommitResp); - done(); - }; - transactionWithoutMock.run( - ( - error: Error | null, - transaction: Transaction | null, - response?: google.datastore.v1.IBeginTransactionResponse - ) => { - transactionWithoutMock.commit(commitCallback); - } - ); - }); - }); - 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.commit = ( - request: protos.google.datastore.v1.ICommitRequest, - options: CallOptions, - callback: Callback< - protos.google.datastore.v1.ICommitResponse, - | protos.google.datastore.v1.ICommitRequest - | null - | undefined, - {} | null | undefined - > - ) => { - callback(null, testCommitResp); - }; - // TODO: See if eliminating this mock will fix the problem - 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 () => { - await transactionWithoutMock.run(); - const commitResults = await transactionWithoutMock.commit(); - assert.strictEqual(commitResults, testCommitResp); - }); - it('should send back the response when using a callback', done => { - const commitCallback: CommitCallback = ( - error: Error | null | undefined, - response?: google.datastore.v1.ICommitResponse - ) => { - assert(error); - assert.strictEqual(error, null); - assert.strictEqual(response, testCommitResp); - done(); - }; - transactionWithoutMock.run( - ( - error: Error | null, - transaction: Transaction | null, - response?: google.datastore.v1.IBeginTransactionResponse - ) => { - transactionWithoutMock.commit(commitCallback); - } - ); - }); - }); - }); - }); - }); - describe('commit', () => { beforeEach(() => { transaction.id = TRANSACTION_ID; From d5f5f4bffaf0ad16103988add984add5614adc23 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 3 Nov 2023 17:11:46 -0400 Subject: [PATCH 06/50] Change the promise Make the promise simpler. Change the tests to exclude functions with promisify. --- src/transaction.ts | 8 +------- test/transaction.ts | 2 ++ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index d7b561357..1a1b0c0bd 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -171,11 +171,9 @@ class Transaction extends DatastoreRequest { typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; this.runCommit(gaxOptions, callback); // TODO: Add call to commitAsync here and handle result in the .then hook - /* this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { callback(response.err, response.resp); }); - */ } // The promise that commitAsync uses should always resolve and never reject. @@ -196,11 +194,7 @@ class Transaction extends DatastoreRequest { release(); // TODO: Be sure to release the mutex in the error state } } catch (err: any) { - return await new Promise(resolve => { - // resp from the beginTransaction call never reaches the user - // if we allowed it to then the return type of commit would need to change - resolve({err}); - }); + return {err}; } } return await new Promise(resolve => { diff --git a/test/transaction.ts b/test/transaction.ts index 7b5ed19d6..1b0c248a7 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -57,6 +57,8 @@ const fakePfy = Object.assign({}, pfy, { 'createQuery', 'delete', 'insert', + 'parseRunAsync', + 'parseTransactionResponse', 'runAsync', 'save', 'update', From c9e8e9509ce32ca62c6435d989caf60a39c941c8 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 14:24:22 -0500 Subject: [PATCH 07/50] Move the mutex and the state down to derived class The mutex and state should be moved down to the derived class. We are going to override get/runQuery/runAggregateQuery there. --- src/request.ts | 18 +------------- src/transaction.ts | 62 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/src/request.ts b/src/request.ts index 6ba6993dc..cad0751b3 100644 --- a/src/request.ts +++ b/src/request.ts @@ -57,7 +57,7 @@ import { import {Datastore} from '.'; import ITimestamp = google.protobuf.ITimestamp; import {AggregateQuery} from './aggregate'; -import {Mutex} from 'async-mutex'; + /** * A map of read consistency values to proto codes. @@ -73,20 +73,6 @@ const CONSISTENCY_PROTO_CODE: ConsistencyProtoCode = { // TODO: Typescript had difficulty working with enums before. // TODO: Try to get enums working instead of using static properties. -export class TransactionState { - static NOT_TRANSACTION = Symbol('NON_TRANSACTION'); - static NOT_STARTED = Symbol('NOT_STARTED'); - // IN_PROGRESS currently tracks the expired state as well - static IN_PROGRESS = Symbol('IN_PROGRESS'); -} - -/* -export enum TransactionState { - NOT_TRANSACTION, - NOT_STARTED, - IN_PROGRESS -} - */ /** * Handle logic for Datastore API operations. Handles request logic for * Datastore. @@ -108,8 +94,6 @@ class DatastoreRequest { | Entity; datastore!: Datastore; // TODO: Replace protected with a symbol for better data hiding. - protected mutex = new Mutex(); - protected state: Symbol = TransactionState.NOT_TRANSACTION; [key: string]: Entity; /** diff --git a/src/transaction.ts b/src/transaction.ts index 1a1b0c0bd..a7fd035cc 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -29,9 +29,12 @@ import { DatastoreRequest, RequestOptions, PrepareEntityObjectResponse, - TransactionState, + CreateReadStreamOptions, + GetResponse, + GetCallback, } from './request'; import {AggregateQuery} from './aggregate'; +import {Mutex} from 'async-mutex'; // RequestPromiseReturnType should line up with the types in RequestCallback interface RequestPromiseReturnType { @@ -51,6 +54,13 @@ interface CommitPromiseReturnType { resp?: google.datastore.v1.ICommitResponse; } +class TransactionState { + static NOT_TRANSACTION = Symbol('NON_TRANSACTION'); + static NOT_STARTED = Symbol('NOT_STARTED'); + // IN_PROGRESS currently tracks the expired state as well + static IN_PROGRESS = Symbol('IN_PROGRESS'); +} + /** * 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 @@ -77,6 +87,8 @@ class Transaction extends DatastoreRequest { request: Function; modifiedEntities_: ModifiedEntities; skipCommit?: boolean; + #mutex = new Mutex(); + #state: Symbol = TransactionState.NOT_TRANSACTION; constructor(datastore: Datastore, options?: TransactionOptions) { super(); /** @@ -106,7 +118,7 @@ class Transaction extends DatastoreRequest { // Queue the requests to make when we send the transactional commit. this.requests_ = []; - this.state = TransactionState.NOT_STARTED; + this.#state = TransactionState.NOT_STARTED; } /*! Developer Documentation @@ -181,13 +193,13 @@ class Transaction extends DatastoreRequest { private async commitAsync( gaxOptions: CallOptions ): Promise { - if (this.state === TransactionState.NOT_STARTED) { - const release = await this.mutex.acquire(); + if (this.#state === TransactionState.NOT_STARTED) { + const release = await this.#mutex.acquire(); try { try { - if (this.state === TransactionState.NOT_STARTED) { + if (this.#state === TransactionState.NOT_STARTED) { const runResults = await this.runAsync({gaxOptions}); - this.parseRunSuccess(runResults); + this.#parseRunSuccess(runResults); } } finally { // TODO: Check that error actually reaches user @@ -343,6 +355,30 @@ class Transaction extends DatastoreRequest { }); } + get( + keys: entity.Key | entity.Key[], + options?: CreateReadStreamOptions + ): Promise; + get(keys: entity.Key | entity.Key[], callback: GetCallback): void; + get( + keys: entity.Key | entity.Key[], + options: CreateReadStreamOptions, + callback: GetCallback + ): void; + get( + keys: entity.Key | entity.Key[], + optionsOrCallback?: CreateReadStreamOptions | GetCallback, + cb?: GetCallback + ): void | Promise { + const options = + typeof optionsOrCallback === 'object' && optionsOrCallback + ? optionsOrCallback + : {}; + const callback = + typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; + super.get(keys, options, callback); + } + /** * Maps to {@link https://cloud.google.com/nodejs/docs/reference/datastore/latest/datastore/transaction#_google_cloud_datastore_Transaction_save_member_1_|Datastore#save}, forcing the method to be `insert`. * @@ -491,17 +527,17 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; // TODO: Whenever run is called a second time and a warning is emitted then do nothing. // TODO: This means rewriting many tests so that they don't use the same transaction object. - if (this.state !== TransactionState.NOT_STARTED) { + if (this.#state !== TransactionState.NOT_STARTED) { process.emitWarning( 'run has already been called and should not be called again.' ); } - this.mutex.acquire().then(release => { + this.#mutex.acquire().then(release => { this.runAsync(options) .then((response: RequestPromiseReturnType) => { // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. release(); - this.parseRunAsync(response, callback); + this.#parseRunAsync(response, callback); }) .catch((err: any) => { // TODO: Remove this catch block @@ -639,7 +675,7 @@ class Transaction extends DatastoreRequest { } // TODO: Replace with #parseRunAsync when pack and play error is gone - private parseRunAsync( + #parseRunAsync( response: RequestPromiseReturnType, callback: RunCallback ): void { @@ -649,14 +685,14 @@ class Transaction extends DatastoreRequest { callback(err, null, resp); return; } - this.parseRunSuccess(response); + this.#parseRunSuccess(response); callback(null, this, resp); } - private parseRunSuccess(response: RequestPromiseReturnType) { + #parseRunSuccess(response: RequestPromiseReturnType) { const resp = response.resp; this.id = resp!.transaction; - this.state = TransactionState.IN_PROGRESS; + this.#state = TransactionState.IN_PROGRESS; } // TODO: Replace with #runAsync when pack and play error is gone From 071d04b94b92802fb9ca2a6dfdc9ffa11e9b3f5d Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 14:45:34 -0500 Subject: [PATCH 08/50] Add hook to call run before commit Add the hook to call run before calling commit in existing tests. --- src/transaction.ts | 2 +- test/transaction.ts | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index a7fd035cc..f86cb9d09 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -181,7 +181,7 @@ class Transaction extends DatastoreRequest { : () => {}; const gaxOptions = typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; - this.runCommit(gaxOptions, callback); + // this.runCommit(gaxOptions, callback); // TODO: Add call to commitAsync here and handle result in the .then hook this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { callback(response.err, response.resp); diff --git a/test/transaction.ts b/test/transaction.ts index 1b0c248a7..12aa463f7 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -459,8 +459,15 @@ async.each( }); describe('commit', () => { - beforeEach(() => { + beforeEach(done => { transaction.id = TRANSACTION_ID; + transaction.request_ = (config, callback) => { + done(); + callback(null, { + transaction: Buffer.from(Array.from(Array(100).keys())), + }); + }; + transaction.run(); }); afterEach(() => { From 3ff33ccaa87a0363f39a2cc047236e73cddb0bd7 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 15:11:25 -0500 Subject: [PATCH 09/50] Add commitAsync to promisify excludes commitAsync should be resolved and then() function should be called as it was not being called before --- src/transaction.ts | 7 +++++++ test/transaction.ts | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/transaction.ts b/src/transaction.ts index f86cb9d09..2387d7de6 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -184,6 +184,8 @@ class Transaction extends DatastoreRequest { // this.runCommit(gaxOptions, callback); // TODO: Add call to commitAsync here and handle result in the .then hook this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { + console.log(response.err); + console.log(response.resp); callback(response.err, response.resp); }); } @@ -193,6 +195,7 @@ class Transaction extends DatastoreRequest { private async commitAsync( gaxOptions: CallOptions ): Promise { + console.log('test'); if (this.#state === TransactionState.NOT_STARTED) { const release = await this.#mutex.acquire(); try { @@ -213,6 +216,7 @@ class Transaction extends DatastoreRequest { this.runCommit( gaxOptions, (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { + console.log('resolving'); resolve({err, resp}); } ); @@ -652,10 +656,12 @@ class Transaction extends DatastoreRequest { (err, resp) => { if (err) { // Rollback automatically for the user. + console.log('rolling back'); 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 + console.log('rolled back'); callback(err, resp); }); return; @@ -965,6 +971,7 @@ export interface RunOptions { promisifyAll(Transaction, { exclude: [ 'createAggregationQuery', + 'commitAsync', 'createQuery', 'delete', 'insert', diff --git a/test/transaction.ts b/test/transaction.ts index 12aa463f7..07246b7c6 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -54,6 +54,7 @@ const fakePfy = Object.assign({}, pfy, { promisified = true; assert.deepStrictEqual(options.exclude, [ 'createAggregationQuery', + 'commitAsync', 'createQuery', 'delete', 'insert', @@ -156,7 +157,7 @@ async.each( }); }); - describe('run without setting up transaction id', () => { + describe.only('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 = { From bddc35537ecf2c52df93b1baed540920b5f5a78e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 15:14:54 -0500 Subject: [PATCH 10/50] remove the console logs --- src/transaction.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 2387d7de6..038ac947f 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -184,8 +184,6 @@ class Transaction extends DatastoreRequest { // this.runCommit(gaxOptions, callback); // TODO: Add call to commitAsync here and handle result in the .then hook this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { - console.log(response.err); - console.log(response.resp); callback(response.err, response.resp); }); } @@ -195,7 +193,6 @@ class Transaction extends DatastoreRequest { private async commitAsync( gaxOptions: CallOptions ): Promise { - console.log('test'); if (this.#state === TransactionState.NOT_STARTED) { const release = await this.#mutex.acquire(); try { @@ -216,7 +213,6 @@ class Transaction extends DatastoreRequest { this.runCommit( gaxOptions, (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { - console.log('resolving'); resolve({err, resp}); } ); @@ -656,12 +652,10 @@ class Transaction extends DatastoreRequest { (err, resp) => { if (err) { // Rollback automatically for the user. - console.log('rolling back'); 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 - console.log('rolled back'); callback(err, resp); }); return; From b33d59380518ccaca960e63c32d87ef0032101a3 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 15:37:01 -0500 Subject: [PATCH 11/50] Delete run commit --- src/transaction.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 038ac947f..d7d6af1b4 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -181,8 +181,6 @@ class Transaction extends DatastoreRequest { : () => {}; const gaxOptions = typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; - // this.runCommit(gaxOptions, callback); - // TODO: Add call to commitAsync here and handle result in the .then hook this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { callback(response.err, response.resp); }); From b055e3ddbfdfd168ddbb7f40044412fcae4437f9 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 15:42:23 -0500 Subject: [PATCH 12/50] Remove the private identifier Use the private modifier instead to hide data --- src/transaction.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index d7d6af1b4..bc5122399 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -181,14 +181,14 @@ class Transaction extends DatastoreRequest { : () => {}; const gaxOptions = typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; - this.commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { + this.#commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { callback(response.err, response.resp); }); } // The promise that commitAsync uses should always resolve and never reject. // The data it resolves with will contain response and error information. - private async commitAsync( + async #commitAsync( gaxOptions: CallOptions ): Promise { if (this.#state === TransactionState.NOT_STARTED) { @@ -208,7 +208,7 @@ class Transaction extends DatastoreRequest { } } return await new Promise(resolve => { - this.runCommit( + this.#runCommit( gaxOptions, (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { resolve({err, resp}); @@ -544,10 +544,10 @@ class Transaction extends DatastoreRequest { }); } - private runCommit(gaxOptions?: CallOptions): Promise; - private runCommit(callback: CommitCallback): void; - private runCommit(gaxOptions: CallOptions, callback: CommitCallback): void; - private runCommit( + #runCommit(gaxOptions?: CallOptions): Promise; + #runCommit(callback: CommitCallback): void; + #runCommit(gaxOptions: CallOptions, callback: CommitCallback): void; + #runCommit( gaxOptionsOrCallback?: CallOptions | CommitCallback, cb?: CommitCallback ): void | Promise { @@ -963,7 +963,7 @@ export interface RunOptions { promisifyAll(Transaction, { exclude: [ 'createAggregationQuery', - 'commitAsync', + '#commitAsync', 'createQuery', 'delete', 'insert', From f3c0e1daad51f064f3cc35b5df9f05addd94f504 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 16:17:03 -0500 Subject: [PATCH 13/50] Add withBeginTransaction withBeginTransaction will be used with all calls that begin transactions and then intend to use the mutex for locking when the begin transaction call is made. --- src/transaction.ts | 48 +++++++++++++++++++++++++++++++++++++++++++-- test/transaction.ts | 2 +- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index bc5122399..22dda1d66 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -53,6 +53,10 @@ interface CommitPromiseReturnType { err?: Error | null; resp?: google.datastore.v1.ICommitResponse; } +interface PassThroughReturnType { + err?: Error | null; + resp?: T; +} class TransactionState { static NOT_TRANSACTION = Symbol('NON_TRANSACTION'); @@ -181,9 +185,21 @@ class Transaction extends DatastoreRequest { : () => {}; const gaxOptions = typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; - this.#commitAsync(gaxOptions).then((response: CommitPromiseReturnType) => { - callback(response.err, response.resp); + type commitPromiseType = + PassThroughReturnType; + const promise: Promise = new Promise(resolve => { + this.#runCommit( + gaxOptions, + (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { + resolve({err, resp}); + } + ); }); + this.#withBeginTransaction(gaxOptions, promise).then( + (response: commitPromiseType) => { + callback(response.err, response.resp); + } + ); } // The promise that commitAsync uses should always resolve and never reject. @@ -217,6 +233,29 @@ class Transaction extends DatastoreRequest { }); } + async #withBeginTransaction( + gaxOptions: CallOptions, + promise: Promise> + ): Promise> { + if (this.#state === TransactionState.NOT_STARTED) { + const release = await this.#mutex.acquire(); + try { + try { + if (this.#state === TransactionState.NOT_STARTED) { + const runResults = await this.runAsync({gaxOptions}); + this.#parseRunSuccess(runResults); + } + } finally { + // TODO: Check that error actually reaches user + release(); // TODO: Be sure to release the mutex in the error state + } + } catch (err: any) { + return {err}; + } + } + return await promise; + } + /** * Create a query for the specified kind. See {module:datastore/query} for all * of the available methods. @@ -353,6 +392,7 @@ class Transaction extends DatastoreRequest { }); } + /* get( keys: entity.Key | entity.Key[], options?: CreateReadStreamOptions @@ -374,8 +414,12 @@ class Transaction extends DatastoreRequest { : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; + const promise = new Promise(resolve => { + super.get(keys, options, callback); + }); super.get(keys, options, callback); } + */ /** * Maps to {@link https://cloud.google.com/nodejs/docs/reference/datastore/latest/datastore/transaction#_google_cloud_datastore_Transaction_save_member_1_|Datastore#save}, forcing the method to be `insert`. diff --git a/test/transaction.ts b/test/transaction.ts index 07246b7c6..9b72140c3 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -54,7 +54,7 @@ const fakePfy = Object.assign({}, pfy, { promisified = true; assert.deepStrictEqual(options.exclude, [ 'createAggregationQuery', - 'commitAsync', + '#commitAsync', 'createQuery', 'delete', 'insert', From cebc155cb9fcaaf20f42991e80684b36bd1f1ba9 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 16:37:26 -0500 Subject: [PATCH 14/50] Add another level of abstraction Add #beginWithCallback so that all the read calls can be made with one line of code. --- src/transaction.ts | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 22dda1d66..5b136ffe2 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -135,6 +135,18 @@ class Transaction extends DatastoreRequest { * the final commit request with. */ + #beginWithCallback( + gaxOptions: CallOptions, + promise: Promise>, + callback: (err?: Error | null, resp?: T) => void + ) { + this.#withBeginTransaction(gaxOptions, promise).then( + (response: PassThroughReturnType) => { + callback(response.err, response.resp); + } + ); + } + /** * Commit the remote transaction and finalize the current transaction * instance. @@ -195,11 +207,7 @@ class Transaction extends DatastoreRequest { } ); }); - this.#withBeginTransaction(gaxOptions, promise).then( - (response: commitPromiseType) => { - callback(response.err, response.resp); - } - ); + this.#beginWithCallback(gaxOptions, promise, callback); } // The promise that commitAsync uses should always resolve and never reject. From fe78cc6e0c8d9edc8490733181f134980f597b95 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 17:01:53 -0500 Subject: [PATCH 15/50] commit async is not needed anymore --- src/transaction.ts | 37 ++----------------------------------- 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 5b136ffe2..17b087e95 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -210,37 +210,6 @@ class Transaction extends DatastoreRequest { this.#beginWithCallback(gaxOptions, promise, callback); } - // The promise that commitAsync uses should always resolve and never reject. - // The data it resolves with will contain response and error information. - async #commitAsync( - gaxOptions: CallOptions - ): Promise { - if (this.#state === TransactionState.NOT_STARTED) { - const release = await this.#mutex.acquire(); - try { - try { - if (this.#state === TransactionState.NOT_STARTED) { - const runResults = await this.runAsync({gaxOptions}); - this.#parseRunSuccess(runResults); - } - } finally { - // TODO: Check that error actually reaches user - release(); // TODO: Be sure to release the mutex in the error state - } - } catch (err: any) { - return {err}; - } - } - return await new Promise(resolve => { - this.#runCommit( - gaxOptions, - (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { - resolve({err, resp}); - } - ); - }); - } - async #withBeginTransaction( gaxOptions: CallOptions, promise: Promise> @@ -400,7 +369,6 @@ class Transaction extends DatastoreRequest { }); } - /* get( keys: entity.Key | entity.Key[], options?: CreateReadStreamOptions @@ -423,11 +391,10 @@ class Transaction extends DatastoreRequest { const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; const promise = new Promise(resolve => { - super.get(keys, options, callback); + super.get(keys, options, ()); }); - super.get(keys, options, callback); + } - */ /** * Maps to {@link https://cloud.google.com/nodejs/docs/reference/datastore/latest/datastore/transaction#_google_cloud_datastore_Transaction_save_member_1_|Datastore#save}, forcing the method to be `insert`. From 36360a7f72bf57c7092d11b8720535e0986b192f Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 17:02:28 -0500 Subject: [PATCH 16/50] This data structure is not needed anymore --- src/transaction.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 17b087e95..ea342870a 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -48,11 +48,6 @@ interface RequestAsPromiseCallback { (resolve: RequestResolveFunction): void; } -// Data types in CommitPromiseReturnType should line up with CommitCallback -interface CommitPromiseReturnType { - err?: Error | null; - resp?: google.datastore.v1.ICommitResponse; -} interface PassThroughReturnType { err?: Error | null; resp?: T; From 33e09ac43c53365bdd0d6b3c9cdfa4c95665bd93 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 17:33:29 -0500 Subject: [PATCH 17/50] Replace types associated with run to use generics The generic parameter should be used for types with run. --- src/transaction.ts | 51 ++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index ea342870a..9c905a81a 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -36,16 +36,11 @@ import { import {AggregateQuery} from './aggregate'; import {Mutex} from 'async-mutex'; -// 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 RequestResolveFunction { + (callbackData: PassThroughReturnType): void; } -interface RequestAsPromiseCallback { - (resolve: RequestResolveFunction): void; +interface RequestAsPromiseCallback { + (resolve: RequestResolveFunction): void; } interface PassThroughReturnType { @@ -131,7 +126,7 @@ class Transaction extends DatastoreRequest { */ #beginWithCallback( - gaxOptions: CallOptions, + gaxOptions: CallOptions | undefined, promise: Promise>, callback: (err?: Error | null, resp?: T) => void ) { @@ -206,7 +201,7 @@ class Transaction extends DatastoreRequest { } async #withBeginTransaction( - gaxOptions: CallOptions, + gaxOptions: CallOptions | undefined, promise: Promise> ): Promise> { if (this.#state === TransactionState.NOT_STARTED) { @@ -385,10 +380,13 @@ class Transaction extends DatastoreRequest { : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; - const promise = new Promise(resolve => { - super.get(keys, options, ()); + type getPromiseType = PassThroughReturnType; + const promise: Promise = new Promise(resolve => { + super.get(keys, options, (err?: Error | null, resp?: GetResponse) => { + resolve({err, resp}); + }); }); - + this.#beginWithCallback(options.gaxOptions, promise, callback); } /** @@ -546,11 +544,16 @@ class Transaction extends DatastoreRequest { } this.#mutex.acquire().then(release => { this.runAsync(options) - .then((response: RequestPromiseReturnType) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.#parseRunAsync(response, callback); - }) + // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues + .then( + ( + response: PassThroughReturnType + ) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.#parseRunAsync(response, callback); + } + ) .catch((err: any) => { // TODO: Remove this catch block callback(Error('The error should always be caught by then'), this); @@ -688,7 +691,7 @@ class Transaction extends DatastoreRequest { // TODO: Replace with #parseRunAsync when pack and play error is gone #parseRunAsync( - response: RequestPromiseReturnType, + response: PassThroughReturnType, callback: RunCallback ): void { const err = response.err; @@ -701,7 +704,7 @@ class Transaction extends DatastoreRequest { callback(null, this, resp); } - #parseRunSuccess(response: RequestPromiseReturnType) { + #parseRunSuccess(response: PassThroughReturnType) { const resp = response.resp; this.id = resp!.transaction; this.#state = TransactionState.IN_PROGRESS; @@ -710,7 +713,7 @@ class Transaction extends DatastoreRequest { // TODO: Replace with #runAsync when pack and play error is gone private async runAsync( options: RunOptions - ): Promise { + ): Promise> { const reqOpts: RequestOptions = { transactionOptions: {}, }; @@ -728,8 +731,8 @@ class Transaction extends DatastoreRequest { if (options.transactionOptions) { reqOpts.transactionOptions = options.transactionOptions; } - const promiseFunction: RequestAsPromiseCallback = ( - resolve: RequestResolveFunction + const promiseFunction: RequestAsPromiseCallback = ( + resolve: RequestResolveFunction ) => { this.request_( { From 8e8f5456cba623f834d6daf3f83090b437f728ea Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Mon, 6 Nov 2023 17:36:00 -0500 Subject: [PATCH 18/50] Make response type more specific for parseRunAsync More specific types are better and it make code easier to read --- src/transaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/transaction.ts b/src/transaction.ts index 9c905a81a..e046652e8 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -691,7 +691,7 @@ class Transaction extends DatastoreRequest { // TODO: Replace with #parseRunAsync when pack and play error is gone #parseRunAsync( - response: PassThroughReturnType, + response: PassThroughReturnType, callback: RunCallback ): void { const err = response.err; From d4b40e86712e77f0eca13a7ba7bc45e6970e5353 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 09:57:02 -0500 Subject: [PATCH 19/50] Add implementation for runQuery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runQuery should make the call to begin the transaction first if that hasn’t already happened yet. --- src/transaction.ts | 52 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index e046652e8..2d4befce8 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -22,7 +22,13 @@ import {google} from '../protos/protos'; import {Datastore, TransactionOptions} from '.'; import {entity, Entity, Entities} from './entity'; -import {Query} from './query'; +import { + Query, + RunQueryCallback, + RunQueryInfo, + RunQueryOptions, + RunQueryResponse, +} from './query'; import { CommitCallback, CommitResponse, @@ -36,6 +42,10 @@ import { import {AggregateQuery} from './aggregate'; import {Mutex} from 'async-mutex'; +type RunQueryResponseOptional = [ + Entity[] | undefined, + RunQueryInfo | undefined, +]; interface RequestResolveFunction { (callbackData: PassThroughReturnType): void; } @@ -380,8 +390,8 @@ class Transaction extends DatastoreRequest { : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; - type getPromiseType = PassThroughReturnType; - const promise: Promise = new Promise(resolve => { + type promiseType = PassThroughReturnType; + const promise: Promise = new Promise(resolve => { super.get(keys, options, (err?: Error | null, resp?: GetResponse) => { resolve({err, resp}); }); @@ -754,6 +764,42 @@ class Transaction extends DatastoreRequest { return new Promise(promiseFunction); } + runQuery(query: Query, options?: RunQueryOptions): Promise; + runQuery( + query: Query, + options: RunQueryOptions, + callback: RunQueryCallback + ): void; + runQuery(query: Query, callback: RunQueryCallback): void; + runQuery( + query: Query, + optionsOrCallback?: RunQueryOptions | RunQueryCallback, + cb?: RunQueryCallback + ): void | Promise { + const options = + typeof optionsOrCallback === 'object' && optionsOrCallback + ? optionsOrCallback + : {}; + const callback = + typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; + type promiseType = PassThroughReturnType; + const promise: Promise = new Promise(resolve => { + super.runQuery( + query, + options, + (err: Error | null, entities?: Entity[], info?: RunQueryInfo) => { + resolve({err, resp: [entities, info]}); + } + ); + }); + this.#withBeginTransaction(options.gaxOptions, promise).then( + (response: PassThroughReturnType) => { + const error = response.err ? response.err : null; + callback(error, response.resp); + } + ); + } + /** * Insert or update the specified object(s) in the current transaction. If a * key is incomplete, its associated object is inserted and the original Key From 6c7a147a252a7ba14e689804101ee9c4f6ab25b8 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 14:39:41 -0500 Subject: [PATCH 20/50] Making fixes for runAggregationQuery Fixes for run aggregation query. Still getting undefined results. --- src/request.ts | 6 ++- src/transaction.ts | 106 ++++++++++++++++++++++++++++++++++++++- system-test/datastore.ts | 20 +++----- test/transaction.ts | 5 +- 4 files changed, 121 insertions(+), 16 deletions(-) diff --git a/src/request.ts b/src/request.ts index cad0751b3..522c8f97a 100644 --- a/src/request.ts +++ b/src/request.ts @@ -58,7 +58,6 @@ import {Datastore} from '.'; import ITimestamp = google.protobuf.ITimestamp; import {AggregateQuery} from './aggregate'; - /** * A map of read consistency values to proto codes. * @@ -589,6 +588,7 @@ class DatastoreRequest { const reqOpts: RunAggregationQueryRequest = Object.assign(sharedQueryOpts, { aggregationQuery: aggregationQueryOptions, }); + console.log('Making run aggregation query call'); this.request_( { client: 'DatastoreClient', @@ -597,6 +597,9 @@ class DatastoreRequest { gaxOpts: options.gaxOptions, }, (err, res) => { + console.log('Run aggregation response'); + console.log(err); + console.log(res); if (res && res.batch) { const results = res.batch.aggregationResults; const finalResults = results @@ -613,6 +616,7 @@ class DatastoreRequest { ) ) ); + console.log('calling callback'); callback(err, finalResults); } else { callback(err, res); diff --git a/src/transaction.ts b/src/transaction.ts index 2d4befce8..df5228d77 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -38,6 +38,7 @@ import { CreateReadStreamOptions, GetResponse, GetCallback, + RequestCallback, } from './request'; import {AggregateQuery} from './aggregate'; import {Mutex} from 'async-mutex'; @@ -233,6 +234,35 @@ class Transaction extends DatastoreRequest { return await promise; } + // TODO: Simplify the generics here: remove PassThroughReturnType + async #someFunction( + gaxOptions: CallOptions | undefined, + resolver: ( + resolve: ( + value: PassThroughReturnType | PromiseLike> + ) => void + ) => void + ): Promise> { + if (this.#state === TransactionState.NOT_STARTED) { + const release = await this.#mutex.acquire(); + try { + try { + if (this.#state === TransactionState.NOT_STARTED) { + const runResults = await this.runAsync({gaxOptions}); + this.#parseRunSuccess(runResults); + } + } finally { + // TODO: Check that error actually reaches user + release(); // TODO: Be sure to release the mutex in the error state + } + } catch (err: any) { + return {err}; + } + } + const promiseResults = await new Promise(resolver); + return promiseResults; + } + /** * Create a query for the specified kind. See {module:datastore/query} for all * of the available methods. @@ -764,6 +794,76 @@ class Transaction extends DatastoreRequest { return new Promise(promiseFunction); } + runAggregationQuery( + query: AggregateQuery, + options?: RunQueryOptions + ): Promise; + runAggregationQuery( + query: AggregateQuery, + options: RunQueryOptions, + callback: RequestCallback + ): void; + runAggregationQuery(query: AggregateQuery, callback: RequestCallback): void; + runAggregationQuery( + query: AggregateQuery, + optionsOrCallback?: RunQueryOptions | RequestCallback, + cb?: RequestCallback + ): void | Promise { + const options = + typeof optionsOrCallback === 'object' && optionsOrCallback + ? optionsOrCallback + : {}; + const a = 7; + const callback = + typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; + type promiseType = PassThroughReturnType>; + const someOtherPromise = new Promise(resolve => { + console.log('some other'); + resolve('something'); + }); + type resolverType = ( + resolve: ( + value: + | PassThroughReturnType + | PromiseLike> + ) => void + ) => void; + console.log('call runAggregationQuery'); + const resolver: resolverType = resolve => { + console.log('resolving'); + super.runAggregationQuery( + query, + options, + (err?: Error | null, resp?: any) => { + console.log('resolved'); + resolve({err, resp}); + } + ); + }; + console.log('some-function'); + this.#someFunction(options.gaxOptions, resolver).then( + (response: promiseType) => { + console.log('passing into callback'); + const error = response.err ? response.err : null; + console.log('passing into callback 2'); + console.log(error); + console.log(response.resp); + // callback(error, response.resp); + callback(null, 'some-response'); + } + ); + //const promise: Promise = new Promise(); + // console.log('starting withBeginTransaction'); + /* + this.#withBeginTransaction(options.gaxOptions, promise).then( + (response: promiseType) => { + const error = response.err ? response.err : null; + callback(error, response.resp); + } + ); + */ + } + runQuery(query: Query, options?: RunQueryOptions): Promise; runQuery( query: Query, @@ -793,7 +893,7 @@ class Transaction extends DatastoreRequest { ); }); this.#withBeginTransaction(options.gaxOptions, promise).then( - (response: PassThroughReturnType) => { + (response: promiseType) => { const error = response.err ? response.err : null; callback(error, response.resp); } @@ -1029,11 +1129,15 @@ promisifyAll(Transaction, { '#commitAsync', 'createQuery', 'delete', + 'get', 'insert', 'parseRunAsync', 'parseTransactionResponse', + 'runAggregationQuery', 'runAsync', + 'runQuery', 'save', + '#someFunction', 'update', 'upsert', ], diff --git a/system-test/datastore.ts b/system-test/datastore.ts index 2d1e6bdc1..1fa89cdfc 100644 --- a/system-test/datastore.ts +++ b/system-test/datastore.ts @@ -1749,7 +1749,7 @@ async.each( assert.deepStrictEqual(results, [{property_1: 4}]); }); }); - describe('transactions', () => { + describe.only('transactions', () => { it('should run in a transaction', async () => { const key = datastore.key(['Company', 'Google']); const obj = { @@ -1862,6 +1862,7 @@ async.each( ); }); it('should aggregate query within a count transaction', async () => { + console.log('running test of interest'); const transaction = datastore.transaction(); await transaction.run(); const query = transaction.createQuery('Company'); @@ -1870,12 +1871,11 @@ async.each( .count('total'); let result; try { + const allResults = await aggregateQuery.run(); [result] = await aggregateQuery.run(); } catch (e) { await transaction.rollback(); - assert.fail( - 'The aggregation query run should have been successful' - ); + throw e; } assert.deepStrictEqual(result, [{total: 2}]); await transaction.commit(); @@ -1892,9 +1892,7 @@ async.each( [result] = await aggregateQuery.run(); } catch (e) { await transaction.rollback(); - assert.fail( - 'The aggregation query run should have been successful' - ); + throw e; } assert.deepStrictEqual(result, [{'total rating': 200}]); await transaction.commit(); @@ -1911,9 +1909,7 @@ async.each( [result] = await aggregateQuery.run(); } catch (e) { await transaction.rollback(); - assert.fail( - 'The aggregation query run should have been successful' - ); + throw e; } assert.deepStrictEqual(result, [{'average rating': 100}]); await transaction.commit(); @@ -1929,9 +1925,7 @@ async.each( [result] = await aggregateQuery.run(); } catch (e) { await transaction.rollback(); - assert.fail( - 'The aggregation query run should have been successful' - ); + throw e; } return result; } diff --git a/test/transaction.ts b/test/transaction.ts index 9b72140c3..2f0313993 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -57,10 +57,13 @@ const fakePfy = Object.assign({}, pfy, { '#commitAsync', 'createQuery', 'delete', + 'get', 'insert', 'parseRunAsync', 'parseTransactionResponse', + 'runAggregationQuery', 'runAsync', + 'runQuery', 'save', 'update', 'upsert', @@ -157,7 +160,7 @@ async.each( }); }); - describe.only('run without setting up transaction id', () => { + 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 = { From 4c3cb5c0ba9fd9ffe10717ed2a87752b484135a3 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 16:38:35 -0500 Subject: [PATCH 21/50] Write tests for runAggregateQuery Make sure that runAggregateQuery return results make it back to the user. --- src/request.ts | 1 + test/transaction.ts | 218 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 211 insertions(+), 8 deletions(-) diff --git a/src/request.ts b/src/request.ts index 522c8f97a..4a5a4abe0 100644 --- a/src/request.ts +++ b/src/request.ts @@ -600,6 +600,7 @@ class DatastoreRequest { console.log('Run aggregation response'); console.log(err); console.log(res); + console.log(JSON.stringify(res)); if (res && res.batch) { const results = res.batch.aggregationResults; const finalResults = results diff --git a/test/transaction.ts b/test/transaction.ts index 2f0313993..7e68b6687 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -26,12 +26,13 @@ import { Query, TransactionOptions, Transaction, + AggregateField, } 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 {CommitCallback, RequestConfig} from '../src/request'; +import {CommitCallback, RequestCallback, RequestConfig} from '../src/request'; import {SECOND_DATABASE_ID} from './index'; import {google} from '../protos/protos'; import {RunCallback} from '../src/transaction'; @@ -163,7 +164,7 @@ 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 = { + const testRunResp = { transaction: Buffer.from(Array.from(Array(100).keys())), }; const namespace = 'run-without-mock'; @@ -221,7 +222,7 @@ async.each( {} | null | undefined > ) => { - callback(new Error(testErrorMessage), testResp); + callback(new Error(testErrorMessage), testRunResp); }; } }); @@ -244,7 +245,7 @@ async.each( assert(error); assert.strictEqual(error.message, testErrorMessage); assert.strictEqual(transaction, null); - assert.strictEqual(response, testResp); + assert.strictEqual(response, testRunResp); done(); }; transactionWithoutMock.run({}, runCallback); @@ -266,14 +267,14 @@ async.each( {} | null | undefined > ) => { - callback(null, testResp); + callback(null, testRunResp); }; } }); 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); + assert.strictEqual(resp, testRunResp); }); it('should send back the response when using a callback', done => { const runCallback: RunCallback = ( @@ -282,7 +283,7 @@ async.each( response?: google.datastore.v1.IBeginTransactionResponse ) => { assert.strictEqual(error, null); - assert.strictEqual(response, testResp); + assert.strictEqual(response, testRunResp); assert.strictEqual(transaction, transactionWithoutMock); done(); }; @@ -348,7 +349,7 @@ async.each( {} | null | undefined > ) => { - callback(null, testResp); + callback(null, testRunResp); }; } }); @@ -459,6 +460,207 @@ async.each( }); }); }); + + describe('runAggregationQuery without setting up transaction id when run returns a response', () => { + // These tests were created so that when transaction.runAggregateQuery is restructured we + // can be confident that it works the same way as before. + + const runAggregationQueryResp = { + batch: { + aggregationResults: [ + { + aggregateProperties: { + 'average rating': { + meaning: 0, + excludeFromIndexes: false, + doubleValue: 100, + valueType: 'doubleValue', + }, + }, + }, + ], + moreResults: + google.datastore.v1.QueryResultBatch.MoreResultsType + .NO_MORE_RESULTS, + readTime: {seconds: '1699390681', nanos: 961667000}, + }, + query: null, + transaction: testRunResp.transaction, + }; + const namespace = 'run-without-mock'; + const projectId = 'project-id'; + const testErrorMessage = 'test-run-Aggregate-Query-error'; + const options = { + projectId, + namespace, + }; + const datastore = new Datastore(options); + const q = datastore.createQuery('Character'); + const aggregate = datastore + .createAggregationQuery(q) + .addAggregation(AggregateField.average('appearances')); + let transactionWithoutMock: Transaction; + const dataClientName = 'DatastoreClient'; + let dataClient: ClientStub | undefined; + let originalRunAggregateQueryMethod: Function; + + beforeEach(async () => { + // Create a fresh transaction for each test because transaction state changes after a commit. + transactionWithoutMock = datastore.transaction(); + // 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.runAggregationQuery) { + originalRunAggregateQueryMethod = + dataClient.runAggregationQuery; + } + if (dataClient && dataClient.beginTransaction) { + 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, testRunResp); + }; + } + }); + + 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 && originalRunAggregateQueryMethod) { + dataClient.runAggregationQuery = + originalRunAggregateQueryMethod; + } + }); + + 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.runAggregationQuery = ( + request: protos.google.datastore.v1.IRunAggregationQueryRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.IRunAggregationQueryResponse, + | protos.google.datastore.v1.IRunAggregationQueryRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback( + new Error(testErrorMessage), + runAggregationQueryResp + ); + }; + } + }); + + it('should send back the error when awaiting a promise', async () => { + try { + await transactionWithoutMock.run(); + await transactionWithoutMock.runAggregationQuery(aggregate); + 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 runAggregateQueryCallback: RequestCallback = ( + error: Error | null | undefined, + response?: any + ) => { + assert(error); + assert.strictEqual(error.message, testErrorMessage); + assert.strictEqual(response, runAggregationQueryResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.runAggregationQuery( + aggregate, + runAggregateQueryCallback + ); + } + ); + }); + }); + 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.runAggregationQuery = ( + request: protos.google.datastore.v1.IRunAggregationQueryRequest, + options: CallOptions, + callback: Callback< + protos.google.datastore.v1.IRunAggregationQueryResponse, + | protos.google.datastore.v1.IRunAggregationQueryRequest + | null + | undefined, + {} | null | undefined + > + ) => { + callback(null, runAggregationQueryResp); + }; + } + }); + it('should send back the response when awaiting a promise', async () => { + await transactionWithoutMock.run(); + const allResults = + await transactionWithoutMock.runAggregationQuery(aggregate); + const [runAggregateQueryResults] = allResults; + assert.strictEqual( + runAggregateQueryResults, + runAggregationQueryResp + ); + }); + it('should send back the response when using a callback', done => { + const runAggregateQueryCallback: RequestCallback = ( + error: Error | null | undefined, + response?: any + ) => { + assert.strictEqual(error, null); + assert.strictEqual(response, runAggregationQueryResp); + done(); + }; + transactionWithoutMock.run( + ( + error: Error | null, + transaction: Transaction | null, + response?: google.datastore.v1.IBeginTransactionResponse + ) => { + transactionWithoutMock.runAggregationQuery( + aggregate, + runAggregateQueryCallback + ); + } + ); + }); + }); + }); }); }); From d5072295d2f174c6c78721efe46f6d98b2072b13 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 17:01:45 -0500 Subject: [PATCH 22/50] Get one test case passing for runAggregateQuery runAggregateQuery should not be excluded by promisify. Otherwise it will not return a promise, but we want it to return a promise. --- src/transaction.ts | 8 +++----- test/transaction.ts | 7 +++---- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index df5228d77..65d141320 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -260,6 +260,8 @@ class Transaction extends DatastoreRequest { } } const promiseResults = await new Promise(resolver); + console.log('Promise results'); + console.log(promiseResults); return promiseResults; } @@ -848,8 +850,7 @@ class Transaction extends DatastoreRequest { console.log('passing into callback 2'); console.log(error); console.log(response.resp); - // callback(error, response.resp); - callback(null, 'some-response'); + callback(error, response.resp); } ); //const promise: Promise = new Promise(); @@ -1129,13 +1130,10 @@ promisifyAll(Transaction, { '#commitAsync', 'createQuery', 'delete', - 'get', 'insert', 'parseRunAsync', 'parseTransactionResponse', - 'runAggregationQuery', 'runAsync', - 'runQuery', 'save', '#someFunction', 'update', diff --git a/test/transaction.ts b/test/transaction.ts index 7e68b6687..17b90f3ae 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -58,14 +58,12 @@ const fakePfy = Object.assign({}, pfy, { '#commitAsync', 'createQuery', 'delete', - 'get', 'insert', 'parseRunAsync', 'parseTransactionResponse', - 'runAggregationQuery', 'runAsync', - 'runQuery', 'save', + '#someFunction', 'update', 'upsert', ]); @@ -576,7 +574,8 @@ async.each( it('should send back the error when awaiting a promise', async () => { try { await transactionWithoutMock.run(); - await transactionWithoutMock.runAggregationQuery(aggregate); + const results = + await transactionWithoutMock.runAggregationQuery(aggregate); assert.fail('The run call should have failed.'); } catch (error: any) { // TODO: Substitute type any From 8afa968f193d164e07794473ef2a7971d8b068a2 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 17:03:52 -0500 Subject: [PATCH 23/50] remove console log clutter --- src/request.ts | 5 ----- src/transaction.ts | 14 -------------- 2 files changed, 19 deletions(-) diff --git a/src/request.ts b/src/request.ts index 4a5a4abe0..6c3833b38 100644 --- a/src/request.ts +++ b/src/request.ts @@ -597,10 +597,6 @@ class DatastoreRequest { gaxOpts: options.gaxOptions, }, (err, res) => { - console.log('Run aggregation response'); - console.log(err); - console.log(res); - console.log(JSON.stringify(res)); if (res && res.batch) { const results = res.batch.aggregationResults; const finalResults = results @@ -617,7 +613,6 @@ class DatastoreRequest { ) ) ); - console.log('calling callback'); callback(err, finalResults); } else { callback(err, res); diff --git a/src/transaction.ts b/src/transaction.ts index 65d141320..72930ee05 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -260,8 +260,6 @@ class Transaction extends DatastoreRequest { } } const promiseResults = await new Promise(resolver); - console.log('Promise results'); - console.log(promiseResults); return promiseResults; } @@ -819,10 +817,6 @@ class Transaction extends DatastoreRequest { const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; type promiseType = PassThroughReturnType>; - const someOtherPromise = new Promise(resolve => { - console.log('some other'); - resolve('something'); - }); type resolverType = ( resolve: ( value: @@ -830,26 +824,18 @@ class Transaction extends DatastoreRequest { | PromiseLike> ) => void ) => void; - console.log('call runAggregationQuery'); const resolver: resolverType = resolve => { - console.log('resolving'); super.runAggregationQuery( query, options, (err?: Error | null, resp?: any) => { - console.log('resolved'); resolve({err, resp}); } ); }; - console.log('some-function'); this.#someFunction(options.gaxOptions, resolver).then( (response: promiseType) => { - console.log('passing into callback'); const error = response.err ? response.err : null; - console.log('passing into callback 2'); - console.log(error); - console.log(response.resp); callback(error, response.resp); } ); From db48cb30e497b13d1131f08f3b9715439e0dea1e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 17:18:36 -0500 Subject: [PATCH 24/50] Change tests for runAggregationQuery Change the test to use deep strict equal since the objects being compared will not be reference equal. --- src/request.ts | 1 - test/transaction.ts | 9 +++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/request.ts b/src/request.ts index 6c3833b38..1397a09f1 100644 --- a/src/request.ts +++ b/src/request.ts @@ -588,7 +588,6 @@ class DatastoreRequest { const reqOpts: RunAggregationQueryRequest = Object.assign(sharedQueryOpts, { aggregationQuery: aggregationQueryOptions, }); - console.log('Making run aggregation query call'); this.request_( { client: 'DatastoreClient', diff --git a/test/transaction.ts b/test/transaction.ts index 17b90f3ae..dd9a30e78 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -463,6 +463,7 @@ async.each( // These tests were created so that when transaction.runAggregateQuery is restructured we // can be confident that it works the same way as before. + const runAggregationQueryUserResp = [{'average rating': 100}]; const runAggregationQueryResp = { batch: { aggregationResults: [ @@ -589,7 +590,7 @@ async.each( ) => { assert(error); assert.strictEqual(error.message, testErrorMessage); - assert.strictEqual(response, runAggregationQueryResp); + assert.deepStrictEqual(response, runAggregationQueryUserResp); done(); }; transactionWithoutMock.run( @@ -631,9 +632,9 @@ async.each( const allResults = await transactionWithoutMock.runAggregationQuery(aggregate); const [runAggregateQueryResults] = allResults; - assert.strictEqual( + assert.deepStrictEqual( runAggregateQueryResults, - runAggregationQueryResp + runAggregationQueryUserResp ); }); it('should send back the response when using a callback', done => { @@ -642,7 +643,7 @@ async.each( response?: any ) => { assert.strictEqual(error, null); - assert.strictEqual(response, runAggregationQueryResp); + assert.deepStrictEqual(response, runAggregationQueryUserResp); done(); }; transactionWithoutMock.run( From 69cd491f80a6369213b0e889a6af71b57475ef68 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Tue, 7 Nov 2023 17:33:01 -0500 Subject: [PATCH 25/50] Add resolver type Eliminate some unused code --- src/transaction.ts | 34 +++++++++++----------------------- 1 file changed, 11 insertions(+), 23 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 72930ee05..7da98be37 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -47,16 +47,22 @@ type RunQueryResponseOptional = [ Entity[] | undefined, RunQueryInfo | undefined, ]; +interface PassThroughReturnType { + err?: Error | null; + resp?: T; +} interface RequestResolveFunction { (callbackData: PassThroughReturnType): void; } interface RequestAsPromiseCallback { (resolve: RequestResolveFunction): void; } - -interface PassThroughReturnType { - err?: Error | null; - resp?: T; +interface ResolverType { + ( + resolve: ( + value: PassThroughReturnType | PromiseLike> + ) => void + ): void; } class TransactionState { @@ -813,18 +819,10 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'object' && optionsOrCallback ? optionsOrCallback : {}; - const a = 7; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; type promiseType = PassThroughReturnType>; - type resolverType = ( - resolve: ( - value: - | PassThroughReturnType - | PromiseLike> - ) => void - ) => void; - const resolver: resolverType = resolve => { + const resolver: ResolverType = resolve => { super.runAggregationQuery( query, options, @@ -839,16 +837,6 @@ class Transaction extends DatastoreRequest { callback(error, response.resp); } ); - //const promise: Promise = new Promise(); - // console.log('starting withBeginTransaction'); - /* - this.#withBeginTransaction(options.gaxOptions, promise).then( - (response: promiseType) => { - const error = response.err ? response.err : null; - callback(error, response.resp); - } - ); - */ } runQuery(query: Query, options?: RunQueryOptions): Promise; From 2391ca84f099c127bcddc418cc452ff2e886bf8e Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 15:20:01 -0500 Subject: [PATCH 26/50] remove only and console log --- system-test/datastore.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/system-test/datastore.ts b/system-test/datastore.ts index 1fa89cdfc..cbda61361 100644 --- a/system-test/datastore.ts +++ b/system-test/datastore.ts @@ -1749,7 +1749,7 @@ async.each( assert.deepStrictEqual(results, [{property_1: 4}]); }); }); - describe.only('transactions', () => { + describe('transactions', () => { it('should run in a transaction', async () => { const key = datastore.key(['Company', 'Google']); const obj = { @@ -1862,7 +1862,6 @@ async.each( ); }); it('should aggregate query within a count transaction', async () => { - console.log('running test of interest'); const transaction = datastore.transaction(); await transaction.run(); const query = transaction.createQuery('Company'); From 905c7daf90cb652e3faa4b19af69ec154df63293 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 15:29:05 -0500 Subject: [PATCH 27/50] Try modifications to runQuery Add modifications to runQuery to use some function to return values --- src/transaction.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index b3babd286..0c4b5b7de 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -839,7 +839,6 @@ class Transaction extends DatastoreRequest { ); } - /* runQuery(query: Query, options?: RunQueryOptions): Promise; runQuery( query: Query, @@ -859,7 +858,7 @@ class Transaction extends DatastoreRequest { const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; type promiseType = PassThroughReturnType; - const promise: Promise = new Promise(resolve => { + const resolver: ResolverType = resolve => { super.runQuery( query, options, @@ -867,15 +866,14 @@ class Transaction extends DatastoreRequest { resolve({err, resp: [entities, info]}); } ); - }); - this.#withBeginTransaction(options.gaxOptions, promise).then( + }; + this.#someFunction(options.gaxOptions, resolver).then( (response: promiseType) => { const error = response.err ? response.err : null; callback(error, response.resp); } ); } - */ /** * Insert or update the specified object(s) in the current transaction. If a From d78210a49b544bcf9cae206edced27fb11ca3f94 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:32:13 -0500 Subject: [PATCH 28/50] =?UTF-8?q?Modify=20commit=20so=20it=20doesn?= =?UTF-8?q?=E2=80=99t=20run=20early?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don’t start commit with a promise or the promise will run early. --- src/transaction.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 0c4b5b7de..a9bd051fb 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -206,15 +206,21 @@ class Transaction extends DatastoreRequest { typeof gaxOptionsOrCallback === 'object' ? gaxOptionsOrCallback : {}; type commitPromiseType = PassThroughReturnType; - const promise: Promise = new Promise(resolve => { + const resolver: ResolverType< + google.datastore.v1.ICommitResponse + > = resolve => { this.#runCommit( gaxOptions, (err?: Error | null, resp?: google.datastore.v1.ICommitResponse) => { resolve({err, resp}); } ); - }); - this.#beginWithCallback(gaxOptions, promise, callback); + }; + this.#someFunction(gaxOptions, resolver).then( + (response: commitPromiseType) => { + callback(response.err, response.resp); + } + ); } async #withBeginTransaction( @@ -870,7 +876,9 @@ class Transaction extends DatastoreRequest { this.#someFunction(options.gaxOptions, resolver).then( (response: promiseType) => { const error = response.err ? response.err : null; - callback(error, response.resp); + const entities = response.resp ? response.resp[0] : undefined; + const info = response.resp ? response.resp[1] : undefined; + callback(error, entities, info); } ); } From 13f9b0937ff505764c3fff05a8ac1a170e35a06b Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:40:45 -0500 Subject: [PATCH 29/50] Update get with resolver get should use the same pattern as runQuery and runAggregationQuery to use a resolver for the mutex business logic --- src/transaction.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index a9bd051fb..a24c7271d 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -433,12 +433,16 @@ class Transaction extends DatastoreRequest { const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; type promiseType = PassThroughReturnType; - const promise: Promise = new Promise(resolve => { + const resolver: ResolverType = resolve => { super.get(keys, options, (err?: Error | null, resp?: GetResponse) => { resolve({err, resp}); }); - }); - this.#beginWithCallback(options.gaxOptions, promise, callback); + }; + this.#someFunction(options.gaxOptions, resolver).then( + (response: promiseType) => { + callback(response.err, response.resp); + } + ); } /** From 14dc7430fe4bc1604cdbeda840679e40aae6f9c5 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:41:37 -0500 Subject: [PATCH 30/50] remove #beginWithCallback #beginWithCallback is no longer used so remove it --- src/transaction.ts | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index a24c7271d..f09bdf43c 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -142,18 +142,6 @@ class Transaction extends DatastoreRequest { * the final commit request with. */ - #beginWithCallback( - gaxOptions: CallOptions | undefined, - promise: Promise>, - callback: (err?: Error | null, resp?: T) => void - ) { - this.#withBeginTransaction(gaxOptions, promise).then( - (response: PassThroughReturnType) => { - callback(response.err, response.resp); - } - ); - } - /** * Commit the remote transaction and finalize the current transaction * instance. From 898df4261ab76990a9de74a6d9ff39cd5930e745 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:42:48 -0500 Subject: [PATCH 31/50] Remove #withBeginTransaction This function is no longer used --- test/transaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/transaction.ts b/test/transaction.ts index ee2ccf4b9..a03cd56e5 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -670,7 +670,7 @@ async.each( }); // TODO: Add a test here for calling commit - describe('various functions without setting up transaction id when run returns a response', () => { + describe.only('various functions without setting up transaction id when run returns a response', () => { // These tests were created so that when transaction.run is restructured we // can be confident that it works the same way as before. const testRunResp = { From c20974e2ed2d56f3f306f3edea0add0a0dcee195 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:45:14 -0500 Subject: [PATCH 32/50] Rename #someFunction with begin transaction #someFunction should be named differently --- src/transaction.ts | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index f09bdf43c..3c7f1c9ce 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -204,38 +204,15 @@ class Transaction extends DatastoreRequest { } ); }; - this.#someFunction(gaxOptions, resolver).then( + this.#withBeginTransaction(gaxOptions, resolver).then( (response: commitPromiseType) => { callback(response.err, response.resp); } ); } - async #withBeginTransaction( - gaxOptions: CallOptions | undefined, - promise: Promise> - ): Promise> { - if (this.#state === TransactionState.NOT_STARTED) { - const release = await this.#mutex.acquire(); - try { - try { - if (this.#state === TransactionState.NOT_STARTED) { - const runResults = await this.runAsync({gaxOptions}); - this.#parseRunSuccess(runResults); - } - } finally { - // TODO: Check that error actually reaches user - release(); // TODO: Be sure to release the mutex in the error state - } - } catch (err: any) { - return {err}; - } - } - return await promise; - } - // TODO: Simplify the generics here: remove PassThroughReturnType - async #someFunction( + async #withBeginTransaction( gaxOptions: CallOptions | undefined, resolver: ( resolve: ( @@ -426,7 +403,7 @@ class Transaction extends DatastoreRequest { resolve({err, resp}); }); }; - this.#someFunction(options.gaxOptions, resolver).then( + this.#withBeginTransaction(options.gaxOptions, resolver).then( (response: promiseType) => { callback(response.err, response.resp); } @@ -829,7 +806,7 @@ class Transaction extends DatastoreRequest { } ); }; - this.#someFunction(options.gaxOptions, resolver).then( + this.#withBeginTransaction(options.gaxOptions, resolver).then( (response: promiseType) => { const error = response.err ? response.err : null; callback(error, response.resp); @@ -865,7 +842,7 @@ class Transaction extends DatastoreRequest { } ); }; - this.#someFunction(options.gaxOptions, resolver).then( + this.#withBeginTransaction(options.gaxOptions, resolver).then( (response: promiseType) => { const error = response.err ? response.err : null; const entities = response.resp ? response.resp[0] : undefined; @@ -1109,7 +1086,7 @@ promisifyAll(Transaction, { 'parseTransactionResponse', 'runAsync', 'save', - '#someFunction', + '#withBeginTransaction', 'update', 'upsert', ], From ce852d133e2161108c9721d5fc6ecad94712b47a Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Wed, 8 Nov 2023 16:58:54 -0500 Subject: [PATCH 33/50] Fix promisify. Change to a deepStrictEqual check. A deepStrictEqual check is all that is needed in this test that currently reuses a transaction. --- src/transaction.ts | 36 +++++++++++++++++++----------------- test/transaction.ts | 6 +++--- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 3c7f1c9ce..cdad7c238 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -562,24 +562,26 @@ class Transaction extends DatastoreRequest { process.emitWarning( 'run has already been called and should not be called again.' ); + callback(null, this, {transaction: this.id}); + } else { + this.#mutex.acquire().then(release => { + this.runAsync(options) + // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues + .then( + ( + response: PassThroughReturnType + ) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.#parseRunAsync(response, callback); + } + ) + .catch((err: any) => { + // TODO: Remove this catch block + callback(Error('The error should always be caught by then'), this); + }); + }); } - this.#mutex.acquire().then(release => { - this.runAsync(options) - // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues - .then( - ( - response: PassThroughReturnType - ) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.#parseRunAsync(response, callback); - } - ) - .catch((err: any) => { - // TODO: Remove this catch block - callback(Error('The error should always be caught by then'), this); - }); - }); } #runCommit(gaxOptions?: CallOptions): Promise; diff --git a/test/transaction.ts b/test/transaction.ts index a03cd56e5..0e2243b69 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -70,7 +70,7 @@ const fakePfy = Object.assign({}, pfy, { 'parseTransactionResponse', 'runAsync', 'save', - '#someFunction', + '#withBeginTransaction', 'update', 'upsert', ]); @@ -288,7 +288,7 @@ async.each( response?: google.datastore.v1.IBeginTransactionResponse ) => { assert.strictEqual(error, null); - assert.strictEqual(response, testRunResp); + assert.deepStrictEqual(response, testRunResp); assert.strictEqual(transaction, transactionWithoutMock); done(); }; @@ -670,7 +670,7 @@ async.each( }); // TODO: Add a test here for calling commit - describe.only('various functions without setting up transaction id when run returns a response', () => { + describe('various functions without setting up transaction id when run returns a response', () => { // These tests were created so that when transaction.run is restructured we // can be confident that it works the same way as before. const testRunResp = { From b0fe8f45bd54c10225d0fe7a2f3924b1bbb180e6 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 9 Nov 2023 11:49:11 -0500 Subject: [PATCH 34/50] Add setImmediate to the tests If we add a delay to the tests then the mutex has the opportunity to unlock before running the tests. --- test/transaction.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/transaction.ts b/test/transaction.ts index 0e2243b69..bfbeb5edb 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -1231,7 +1231,10 @@ async.each( callback(null, { transaction: Buffer.from(Array.from(Array(100).keys())), }); - done(); + // Delay to give the transaction mutex the opportunity to unlock before running tests. + setImmediate(() => { + done(); + }); }; transaction.run(); }); From 3c528425e9381ac05b9db55e61a1528ed7ba0cf8 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 9 Nov 2023 14:48:43 -0500 Subject: [PATCH 35/50] Added some tests for call ordering We want to make sure that the calls get delivered and received in order --- src/transaction.ts | 6 ++++ test/transaction.ts | 83 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 1 deletion(-) diff --git a/src/transaction.ts b/src/transaction.ts index cdad7c238..18579dd5d 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -221,7 +221,9 @@ class Transaction extends DatastoreRequest { ) => void ): Promise> { if (this.#state === TransactionState.NOT_STARTED) { + console.log('acquiring mutex'); const release = await this.#mutex.acquire(); + console.log('acquired mutex'); try { try { if (this.#state === TransactionState.NOT_STARTED) { @@ -565,6 +567,7 @@ class Transaction extends DatastoreRequest { callback(null, this, {transaction: this.id}); } else { this.#mutex.acquire().then(release => { + // TODO: Check for not-started here? this.runAsync(options) // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues .then( @@ -573,6 +576,8 @@ class Transaction extends DatastoreRequest { ) => { // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. release(); + console.log('Locked'); + console.log(this.#mutex.isLocked()); this.#parseRunAsync(response, callback); } ) @@ -728,6 +733,7 @@ class Transaction extends DatastoreRequest { } #parseRunSuccess(response: PassThroughReturnType) { + console.log('saving id'); const resp = response.resp; this.id = resp!.transaction; this.#state = TransactionState.IN_PROGRESS; diff --git a/test/transaction.ts b/test/transaction.ts index bfbeb5edb..51124f156 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -684,8 +684,11 @@ async.each( mockedBeginTransaction: any; mockedFunction: any; // TODO: replace with type functionsMocked: {name: string; mockedFunction: any}[]; + callBackSignaler: (callbackReached: string) => void; - constructor() { + constructor(callBackSignaler?: (callbackReached: string) => void) { + const defaultCallback = () => {}; + this.callBackSignaler = callBackSignaler ?? defaultCallback; const namespace = 'run-without-mock'; const projectId = 'project-id'; const options = { @@ -724,6 +727,7 @@ async.each( {} | null | undefined > ) => { + this.callBackSignaler('beginTransaction called'); callback(null, testRunResp); }; } @@ -757,6 +761,7 @@ async.each( {} | null | undefined > ) => { + this.callBackSignaler(`${functionName} called`); callback(error, response); }; } @@ -1223,6 +1228,82 @@ async.each( }); }); }); + describe('concurrency', async () => { + const testCommitResp = { + mutationResults: [ + { + key: { + path: [ + { + kind: 'some-kind', + }, + ], + }, + }, + ], + }; + let transactionWrapper: MockedTransactionWrapper; + + beforeEach(async () => { + transactionWrapper = new MockedTransactionWrapper(); + }); + + afterEach(() => { + transactionWrapper.resetBeginTransaction(); + transactionWrapper.resetGapicFunctions(); + }); + + describe('should pass response back to the user', async () => { + beforeEach(() => { + transactionWrapper.mockGapicFunction( + 'commit', + testCommitResp, + null + ); + }); + + it('should call the callbacks in the proper order', done => { + const transaction = transactionWrapper.transaction; + const callbackOrder: string[] = []; + function checkForCompletion() { + if (callbackOrder.length >= 5) { + // TODO: assertion check here + assert.deepStrictEqual(callbackOrder, [ + 'functions called', + 'beginTransaction called', + 'run callback', + 'commit called', + 'commit callback', + ]); + done(); + } + } + function gapicCallHandler(call: string): void { + callbackOrder.push(call); + checkForCompletion(); + } + transactionWrapper.callBackSignaler = gapicCallHandler; + const runCallback: RunCallback = ( + error: Error | null | undefined, + response?: any + ) => { + callbackOrder.push('run callback'); + checkForCompletion(); + }; + const commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + callbackOrder.push('commit callback'); + checkForCompletion(); + }; + transaction.run(runCallback); + transaction.commit(commitCallback); + callbackOrder.push('functions called'); + checkForCompletion(); + }); + }); + }); }); describe('commit', () => { beforeEach(done => { From 816ab0cecfc50af7a21ef31095a073fffb3346cb Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 9 Nov 2023 15:35:19 -0500 Subject: [PATCH 36/50] Pack testing tool into an object Put the testing tool in an object so that we can explore more orderings and possibilities. --- src/transaction.ts | 2 + test/transaction.ts | 119 +++++++++++++++++++++++++++++--------------- 2 files changed, 82 insertions(+), 39 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 18579dd5d..c947802db 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -566,7 +566,9 @@ class Transaction extends DatastoreRequest { ); callback(null, this, {transaction: this.id}); } else { + console.log('acquiring run mutex'); this.#mutex.acquire().then(release => { + console.log('run mutex acquired'); // TODO: Check for not-started here? this.runAsync(options) // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues diff --git a/test/transaction.ts b/test/transaction.ts index 51124f156..838fd6f1d 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -727,7 +727,9 @@ async.each( {} | null | undefined > ) => { + console.log('beginTransaction called'); this.callBackSignaler('beginTransaction called'); + console.log('callback signaller called'); callback(null, testRunResp); }; } @@ -1228,7 +1230,7 @@ async.each( }); }); }); - describe('concurrency', async () => { + describe.only('concurrency', async () => { const testCommitResp = { mutationResults: [ { @@ -1253,54 +1255,93 @@ async.each( transactionWrapper.resetGapicFunctions(); }); - describe('should pass response back to the user', async () => { - beforeEach(() => { - transactionWrapper.mockGapicFunction( - 'commit', - testCommitResp, - null - ); - }); - - it('should call the callbacks in the proper order', done => { - const transaction = transactionWrapper.transaction; - const callbackOrder: string[] = []; - function checkForCompletion() { - if (callbackOrder.length >= 5) { + class TransactionOrderTester { + callbackOrder: string[] = []; + transactionWrapper: MockedTransactionWrapper; + done: (err?: any) => void; + checkForCompletion() { + if (this.callbackOrder.length >= 5) { + try { // TODO: assertion check here - assert.deepStrictEqual(callbackOrder, [ + assert.deepStrictEqual(this.callbackOrder, [ 'functions called', 'beginTransaction called', 'run callback', 'commit called', 'commit callback', ]); - done(); + this.done(); + } catch (e) { + this.done(e); } } - function gapicCallHandler(call: string): void { - callbackOrder.push(call); - checkForCompletion(); - } - transactionWrapper.callBackSignaler = gapicCallHandler; - const runCallback: RunCallback = ( - error: Error | null | undefined, - response?: any - ) => { - callbackOrder.push('run callback'); - checkForCompletion(); - }; - const commitCallback: CommitCallback = ( - error: Error | null | undefined, - response?: google.datastore.v1.ICommitResponse - ) => { - callbackOrder.push('commit callback'); - checkForCompletion(); + } + + runCallback: RunCallback = ( + error: Error | null | undefined, + response?: any + ) => { + console.log('calling run callback'); + this.callbackOrder.push('run callback'); + this.checkForCompletion(); + }; + commitCallback: CommitCallback = ( + error: Error | null | undefined, + response?: google.datastore.v1.ICommitResponse + ) => { + console.log('calling commit callback'); + this.callbackOrder.push('commit callback'); + this.checkForCompletion(); + }; + + constructor( + transactionWrapper: MockedTransactionWrapper, + done: (err?: any) => void + ) { + const gapicCallHandler = (call: string) => { + try { + this.callbackOrder.push(call); + this.checkForCompletion(); + } catch (e) { + this.done(e); + } }; - transaction.run(runCallback); - transaction.commit(commitCallback); - callbackOrder.push('functions called'); - checkForCompletion(); + this.done = done; + transactionWrapper.callBackSignaler = gapicCallHandler; + this.transactionWrapper = transactionWrapper; + } + + callRun() { + this.transactionWrapper.transaction.run(this.runCallback); + } + + callCommit() { + this.transactionWrapper.transaction.commit(this.commitCallback); + } + + pushString(callbackPushed: string) { + this.callbackOrder.push(callbackPushed); + this.checkForCompletion(); + } + } + + describe('should pass response back to the user', async () => { + beforeEach(() => { + transactionWrapper.mockGapicFunction( + 'commit', + testCommitResp, + null + ); + }); + + it('should call the callbacks in the proper order', done => { + const transactionOrderTester = new TransactionOrderTester( + transactionWrapper, + done + ); + transactionOrderTester.callRun(); + transactionOrderTester.callCommit(); + transactionOrderTester.pushString('functions called'); }); }); }); From 21f3afdbc72f1345cfd287d1d420dbe386e704ab Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 9 Nov 2023 15:42:57 -0500 Subject: [PATCH 37/50] Eliminate console logs, use expected order Allow expected order to be passed into the tester. This will make the object reusable. --- src/transaction.ts | 7 ------- test/transaction.ts | 30 ++++++++++++++++-------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index c947802db..c743f73d9 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -221,9 +221,7 @@ class Transaction extends DatastoreRequest { ) => void ): Promise> { if (this.#state === TransactionState.NOT_STARTED) { - console.log('acquiring mutex'); const release = await this.#mutex.acquire(); - console.log('acquired mutex'); try { try { if (this.#state === TransactionState.NOT_STARTED) { @@ -566,9 +564,7 @@ class Transaction extends DatastoreRequest { ); callback(null, this, {transaction: this.id}); } else { - console.log('acquiring run mutex'); this.#mutex.acquire().then(release => { - console.log('run mutex acquired'); // TODO: Check for not-started here? this.runAsync(options) // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues @@ -578,8 +574,6 @@ class Transaction extends DatastoreRequest { ) => { // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. release(); - console.log('Locked'); - console.log(this.#mutex.isLocked()); this.#parseRunAsync(response, callback); } ) @@ -735,7 +729,6 @@ class Transaction extends DatastoreRequest { } #parseRunSuccess(response: PassThroughReturnType) { - console.log('saving id'); const resp = response.resp; this.id = resp!.transaction; this.#state = TransactionState.IN_PROGRESS; diff --git a/test/transaction.ts b/test/transaction.ts index 838fd6f1d..b258b2b88 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -727,9 +727,7 @@ async.each( {} | null | undefined > ) => { - console.log('beginTransaction called'); this.callBackSignaler('beginTransaction called'); - console.log('callback signaller called'); callback(null, testRunResp); }; } @@ -1255,7 +1253,10 @@ async.each( transactionWrapper.resetGapicFunctions(); }); + // This object is used for testing the order that different events occur + // The events can include class TransactionOrderTester { + expectedOrder: string[] = []; callbackOrder: string[] = []; transactionWrapper: MockedTransactionWrapper; done: (err?: any) => void; @@ -1263,13 +1264,7 @@ async.each( if (this.callbackOrder.length >= 5) { try { // TODO: assertion check here - assert.deepStrictEqual(this.callbackOrder, [ - 'functions called', - 'beginTransaction called', - 'run callback', - 'commit called', - 'commit callback', - ]); + assert.deepStrictEqual(this.callbackOrder, this.expectedOrder); this.done(); } catch (e) { this.done(e); @@ -1281,7 +1276,6 @@ async.each( error: Error | null | undefined, response?: any ) => { - console.log('calling run callback'); this.callbackOrder.push('run callback'); this.checkForCompletion(); }; @@ -1289,21 +1283,22 @@ async.each( error: Error | null | undefined, response?: google.datastore.v1.ICommitResponse ) => { - console.log('calling commit callback'); this.callbackOrder.push('commit callback'); this.checkForCompletion(); }; constructor( transactionWrapper: MockedTransactionWrapper, - done: (err?: any) => void + done: (err?: any) => void, + expectedOrder: string[] ) { + this.expectedOrder = expectedOrder; const gapicCallHandler = (call: string) => { try { this.callbackOrder.push(call); this.checkForCompletion(); } catch (e) { - this.done(e); + done(e); } }; this.done = done; @@ -1337,7 +1332,14 @@ async.each( it('should call the callbacks in the proper order', done => { const transactionOrderTester = new TransactionOrderTester( transactionWrapper, - done + done, + [ + 'functions called', + 'beginTransaction called', + 'run callback', + 'commit called', + 'commit callback', + ] ); transactionOrderTester.callRun(); transactionOrderTester.callCommit(); From 7677400aeb874a02a5f09725a6281d08a39a9ef5 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Thu, 9 Nov 2023 16:23:21 -0500 Subject: [PATCH 38/50] Add a check for transaction not started yet Need a check to be sure that the transaction is not in progress if making another beginTransaction call. --- src/transaction.ts | 46 ++++++++++++++++++++++++++------------------- test/transaction.ts | 38 ++++++++++++++++++++++++++++++++++--- 2 files changed, 62 insertions(+), 22 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index c743f73d9..b0ef5bb66 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -558,29 +558,37 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; // TODO: Whenever run is called a second time and a warning is emitted then do nothing. // TODO: This means rewriting many tests so that they don't use the same transaction object. + const warningMessage = + 'run has already been called and should not be called again.'; if (this.#state !== TransactionState.NOT_STARTED) { - process.emitWarning( - 'run has already been called and should not be called again.' - ); + process.emitWarning(warningMessage); callback(null, this, {transaction: this.id}); } else { this.#mutex.acquire().then(release => { - // TODO: Check for not-started here? - this.runAsync(options) - // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues - .then( - ( - response: PassThroughReturnType - ) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.#parseRunAsync(response, callback); - } - ) - .catch((err: any) => { - // TODO: Remove this catch block - callback(Error('The error should always be caught by then'), this); - }); + if (this.#state === TransactionState.NOT_STARTED) { + this.runAsync(options) + // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues + .then( + ( + response: PassThroughReturnType + ) => { + // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. + release(); + this.#parseRunAsync(response, callback); + } + ) + .catch((err: any) => { + // TODO: Remove this catch block + callback( + Error('The error should always be caught by then'), + this + ); + }); + } else { + release(); + process.emitWarning(warningMessage); + callback(null, this, {transaction: this.id}); + } }); } } diff --git a/test/transaction.ts b/test/transaction.ts index b258b2b88..75b0a23f4 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -1261,10 +1261,13 @@ async.each( transactionWrapper: MockedTransactionWrapper; done: (err?: any) => void; checkForCompletion() { - if (this.callbackOrder.length >= 5) { + if (this.callbackOrder.length >= this.expectedOrder.length) { try { // TODO: assertion check here - assert.deepStrictEqual(this.callbackOrder, this.expectedOrder); + assert.deepStrictEqual( + this.callbackOrder, + this.expectedOrder + ); this.done(); } catch (e) { this.done(e); @@ -1329,7 +1332,7 @@ async.each( ); }); - it('should call the callbacks in the proper order', done => { + it('should call the callbacks in the proper order with run and commit', done => { const transactionOrderTester = new TransactionOrderTester( transactionWrapper, done, @@ -1345,6 +1348,35 @@ async.each( transactionOrderTester.callCommit(); transactionOrderTester.pushString('functions called'); }); + it('should call the callbacks in the proper order with commit', done => { + const transactionOrderTester = new TransactionOrderTester( + transactionWrapper, + done, + [ + 'functions called', + 'beginTransaction called', + 'commit called', + 'commit callback', + ] + ); + transactionOrderTester.callCommit(); + transactionOrderTester.pushString('functions called'); + }); + it('should call the callbacks in the proper order with two run calls', done => { + const transactionOrderTester = new TransactionOrderTester( + transactionWrapper, + done, + [ + 'functions called', + 'beginTransaction called', + 'run callback', + 'run callback', + ] + ); + transactionOrderTester.callRun(); + transactionOrderTester.callRun(); + transactionOrderTester.pushString('functions called'); + }); }); }); }); From 34903bd8798bd3ae9da224160618dc49d3b81af1 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 10:15:05 -0500 Subject: [PATCH 39/50] Remove NOT_TRANSACTION Remove the NOT_TRANSACTION and default to NOT_STARTED --- src/transaction.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index b0ef5bb66..016551aa9 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -66,7 +66,6 @@ interface ResolverType { } class TransactionState { - static NOT_TRANSACTION = Symbol('NON_TRANSACTION'); static NOT_STARTED = Symbol('NOT_STARTED'); // IN_PROGRESS currently tracks the expired state as well static IN_PROGRESS = Symbol('IN_PROGRESS'); @@ -99,7 +98,7 @@ class Transaction extends DatastoreRequest { modifiedEntities_: ModifiedEntities; skipCommit?: boolean; #mutex = new Mutex(); - #state: Symbol = TransactionState.NOT_TRANSACTION; + #state = TransactionState.NOT_STARTED; constructor(datastore: Datastore, options?: TransactionOptions) { super(); /** @@ -129,7 +128,6 @@ class Transaction extends DatastoreRequest { // Queue the requests to make when we send the transactional commit. this.requests_ = []; - this.#state = TransactionState.NOT_STARTED; } /*! Developer Documentation From 390d5ee8502f9632d376e628b9c05ee99998a83f Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 10:16:22 -0500 Subject: [PATCH 40/50] Remove only Remove only and let all unit tests run --- test/transaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/transaction.ts b/test/transaction.ts index 75b0a23f4..6e9b13a84 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -1228,7 +1228,7 @@ async.each( }); }); }); - describe.only('concurrency', async () => { + describe('concurrency', async () => { const testCommitResp = { mutationResults: [ { From c8b5c49a6b18634674f85ace71dc16050773523c Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 10:21:44 -0500 Subject: [PATCH 41/50] Use an enum instead of static class members Use an enum to track transaction state. --- src/transaction.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 016551aa9..fb257ce77 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -65,10 +65,9 @@ interface ResolverType { ): void; } -class TransactionState { - static NOT_STARTED = Symbol('NOT_STARTED'); - // IN_PROGRESS currently tracks the expired state as well - static IN_PROGRESS = Symbol('IN_PROGRESS'); +enum TransactionState { + NOT_STARTED, + IN_PROGRESS, // IN_PROGRESS currently tracks the expired state as well } /** @@ -227,8 +226,7 @@ class Transaction extends DatastoreRequest { this.#parseRunSuccess(runResults); } } finally { - // TODO: Check that error actually reaches user - release(); // TODO: Be sure to release the mutex in the error state + release(); } } catch (err: any) { return {err}; From 0bdfa0b1ed6ab3d9968432d9989a9e6c682f1bca Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 10:26:39 -0500 Subject: [PATCH 42/50] TODOs are done --- src/request.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/request.ts b/src/request.ts index 1397a09f1..d39f572ef 100644 --- a/src/request.ts +++ b/src/request.ts @@ -69,9 +69,6 @@ const CONSISTENCY_PROTO_CODE: ConsistencyProtoCode = { strong: 1, }; -// TODO: Typescript had difficulty working with enums before. -// TODO: Try to get enums working instead of using static properties. - /** * Handle logic for Datastore API operations. Handles request logic for * Datastore. @@ -92,7 +89,6 @@ class DatastoreRequest { | Array<(err: Error | null, resp: Entity | null) => void> | Entity; datastore!: Datastore; - // TODO: Replace protected with a symbol for better data hiding. [key: string]: Entity; /** From 623bb76b0bc1e0f20909543fef3138d38b92ac31 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 10:43:13 -0500 Subject: [PATCH 43/50] Move excludes to proper position Excludes should contain the right functions in the right order. --- src/transaction.ts | 3 +-- test/transaction.ts | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index fb257ce77..6474f2d46 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -1085,7 +1085,6 @@ export interface RunOptions { promisifyAll(Transaction, { exclude: [ 'createAggregationQuery', - '#commitAsync', 'createQuery', 'delete', 'insert', @@ -1093,9 +1092,9 @@ promisifyAll(Transaction, { 'parseTransactionResponse', 'runAsync', 'save', - '#withBeginTransaction', 'update', 'upsert', + '#withBeginTransaction', ], }); diff --git a/test/transaction.ts b/test/transaction.ts index 6e9b13a84..ace0e3a1c 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -62,7 +62,6 @@ const fakePfy = Object.assign({}, pfy, { promisified = true; assert.deepStrictEqual(options.exclude, [ 'createAggregationQuery', - '#commitAsync', 'createQuery', 'delete', 'insert', @@ -70,9 +69,9 @@ const fakePfy = Object.assign({}, pfy, { 'parseTransactionResponse', 'runAsync', 'save', - '#withBeginTransaction', 'update', 'upsert', + '#withBeginTransaction', ]); }, }); From 521f11c496240ffb98a1e45996cbd1bffb7c5d98 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 11:07:57 -0500 Subject: [PATCH 44/50] Simplify the run function a little bit Regroup functionality to simplify the run function --- src/transaction.ts | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 6474f2d46..7644367d1 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -208,7 +208,6 @@ class Transaction extends DatastoreRequest { ); } - // TODO: Simplify the generics here: remove PassThroughReturnType async #withBeginTransaction( gaxOptions: CallOptions | undefined, resolver: ( @@ -552,38 +551,28 @@ class Transaction extends DatastoreRequest { typeof optionsOrCallback === 'object' ? optionsOrCallback : {}; const callback = typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!; - // TODO: Whenever run is called a second time and a warning is emitted then do nothing. - // TODO: This means rewriting many tests so that they don't use the same transaction object. - const warningMessage = - 'run has already been called and should not be called again.'; - if (this.#state !== TransactionState.NOT_STARTED) { - process.emitWarning(warningMessage); + const runIfStarted = () => { + process.emitWarning( + 'run has already been called and should not be called again.' + ); callback(null, this, {transaction: this.id}); + }; + if (this.#state !== TransactionState.NOT_STARTED) { + runIfStarted(); } else { this.#mutex.acquire().then(release => { if (this.#state === TransactionState.NOT_STARTED) { - this.runAsync(options) - // TODO: Replace type with google.datastore.v1.IBeginTransactionResponse and address downstream issues - .then( - ( - response: PassThroughReturnType - ) => { - // TODO: Probably release the mutex after the id is recorded, but likely doesn't matter since node is single threaded. - release(); - this.#parseRunAsync(response, callback); - } - ) - .catch((err: any) => { - // TODO: Remove this catch block - callback( - Error('The error should always be caught by then'), - this - ); - }); + this.runAsync(options).then( + ( + response: PassThroughReturnType + ) => { + release(); + this.#parseRunAsync(response, callback); + } + ); } else { release(); - process.emitWarning(warningMessage); - callback(null, this, {transaction: this.id}); + runIfStarted(); } }); } From 32d40d2568ed8ec3e26761755b8f0ef7656495a2 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 13:46:03 -0500 Subject: [PATCH 45/50] Add comments to tests Explain purpose of testing objects --- test/transaction.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/test/transaction.ts b/test/transaction.ts index b8cc49015..915a55082 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -163,7 +163,7 @@ async.each( }); }); - describe.only('run without setting up transaction id', () => { + 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 testRunResp = { @@ -674,6 +674,11 @@ async.each( transaction: Buffer.from(Array.from(Array(100).keys())), }; + /* + MockedTransactionWrapper is a helper class for mocking out various + Gapic functions and ensuring that responses and errors actually make it + back to the user. + */ class MockedTransactionWrapper { datastore: Datastore; transaction: Transaction; @@ -738,7 +743,14 @@ async.each( error: Error | null ) { const dataClient = this.dataClient; - // TODO: Check here that function hasn't been mocked out already + // Check here that function hasn't been mocked out already + // Ensures that this mocking object is not being misused. + this.functionsMocked.forEach(fn => { + if (fn.name === functionName) { + console.log('test'); + // throw Error('${functionName} has already been mocked out'); + } + }); if (dataClient && dataClient[functionName]) { this.functionsMocked.push({ name: functionName, @@ -764,12 +776,16 @@ async.each( } } + // This resets beginTransaction from the Gapic layer to what it originally was. + // Resetting beginTransaction ensures other tests don't use the beginTransaction mock. resetBeginTransaction() { if (this.dataClient && this.dataClient.beginTransaction) { this.dataClient.beginTransaction = this.mockedBeginTransaction; } } - // TODO: Allow several functions to be mocked, eliminate string parameter + + // This resets Gapic functions mocked out by the tests to what they originally were. + // Resetting mocked out Gapic functions ensures other tests don't use these mocks. resetGapicFunctions() { this.functionsMocked.forEach(functionMocked => { this.dataClient[functionMocked.name] = From 04496764750ea724ca6dc6237b0e1200cd9ff28d Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 14:14:12 -0500 Subject: [PATCH 46/50] Modify tests and fix a bug from the merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The merge added a bug because the pound sign wasn’t used and it should have been. --- src/transaction.ts | 2 +- test/transaction.ts | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 263a24d75..935bfe59b 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -221,7 +221,7 @@ class Transaction extends DatastoreRequest { try { try { if (this.#state === TransactionState.NOT_STARTED) { - const runResults = await this.runAsync({gaxOptions}); + const runResults = await this.#runAsync({gaxOptions}); this.#parseRunSuccess(runResults); } } finally { diff --git a/test/transaction.ts b/test/transaction.ts index 915a55082..f56488908 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -747,8 +747,7 @@ async.each( // Ensures that this mocking object is not being misused. this.functionsMocked.forEach(fn => { if (fn.name === functionName) { - console.log('test'); - // throw Error('${functionName} has already been mocked out'); + throw Error('${functionName} has already been mocked out'); } }); if (dataClient && dataClient[functionName]) { @@ -1292,15 +1291,23 @@ async.each( error: Error | null | undefined, response?: any ) => { - this.callbackOrder.push('run callback'); - this.checkForCompletion(); + try { + this.callbackOrder.push('run callback'); + this.checkForCompletion(); + } catch (e) { + this.done(e); + } }; commitCallback: CommitCallback = ( error: Error | null | undefined, response?: google.datastore.v1.ICommitResponse ) => { - this.callbackOrder.push('commit callback'); - this.checkForCompletion(); + try { + this.callbackOrder.push('commit callback'); + this.checkForCompletion(); + } catch (e) { + this.done(e); + } }; constructor( From 36189dea38ceead3690d970f81829e640f1fd73c Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 14:22:20 -0500 Subject: [PATCH 47/50] Fix error message --- test/transaction.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/transaction.ts b/test/transaction.ts index f56488908..f795a3a66 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -747,7 +747,7 @@ async.each( // Ensures that this mocking object is not being misused. this.functionsMocked.forEach(fn => { if (fn.name === functionName) { - throw Error('${functionName} has already been mocked out'); + throw Error(`${functionName} has already been mocked out`); } }); if (dataClient && dataClient[functionName]) { From 1fb109b2f9c9299013ddb42007baff2508ed2a29 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 14:49:15 -0500 Subject: [PATCH 48/50] Comments and general cleanup Remove constructor parameter that is not used. Add comments to functions so that the tests are more readable. --- test/transaction.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/transaction.ts b/test/transaction.ts index f795a3a66..2551aafe1 100644 --- a/test/transaction.ts +++ b/test/transaction.ts @@ -666,7 +666,6 @@ async.each( }); }); - // TODO: Add a test here for calling commit describe('various functions without setting up transaction id when run returns a response', () => { // These tests were created so that when transaction.run is restructured we // can be confident that it works the same way as before. @@ -674,11 +673,9 @@ async.each( transaction: Buffer.from(Array.from(Array(100).keys())), }; - /* - MockedTransactionWrapper is a helper class for mocking out various - Gapic functions and ensuring that responses and errors actually make it - back to the user. - */ + // MockedTransactionWrapper is a helper class for mocking out various + // Gapic functions and ensuring that responses and errors actually make it + // back to the user. class MockedTransactionWrapper { datastore: Datastore; transaction: Transaction; @@ -686,11 +683,9 @@ async.each( mockedBeginTransaction: any; mockedFunction: any; // TODO: replace with type functionsMocked: {name: string; mockedFunction: any}[]; - callBackSignaler: (callbackReached: string) => void; + callBackSignaler: (callbackReached: string) => void = () => {}; - constructor(callBackSignaler?: (callbackReached: string) => void) { - const defaultCallback = () => {}; - this.callBackSignaler = callBackSignaler ?? defaultCallback; + constructor() { const namespace = 'run-without-mock'; const projectId = 'project-id'; const options = { @@ -729,6 +724,8 @@ async.each( {} | null | undefined > ) => { + // Calls a user provided function that will receive this string + // Usually used to track when this code was reached relative to other code this.callBackSignaler('beginTransaction called'); callback(null, testRunResp); }; @@ -737,6 +734,9 @@ async.each( this.functionsMocked = []; this.datastore = datastore; } + + // This mocks out a gapic function to just call the callback received in the Gapic function. + // The callback will send back the error and response arguments provided as parameters. mockGapicFunction( functionName: string, response: ResponseType, From f09b6f8e64bbc86b6911d0a0ecacc2ded5ad3ba8 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 15:34:55 -0500 Subject: [PATCH 49/50] Added comments for functions in transaction.ts Functions in transaction.ts need JSdoc comments. --- src/transaction.ts | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/transaction.ts b/src/transaction.ts index 935bfe59b..24f02ddcf 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -709,9 +709,9 @@ class Transaction extends DatastoreRequest { /** * This function parses results from a beginTransaction call * - * @param {RequestPromiseReturnType} response The response from a call to + * @param {RequestPromiseReturnType} [response] The response from a call to * begin a transaction. - * @param {RunCallback} callback A callback that accepts an error and a + * @param {RunCallback} [callback] A callback that accepts an error and a * response as arguments. * **/ @@ -729,6 +729,13 @@ class Transaction extends DatastoreRequest { } } + /** + * This function saves results from a successful beginTransaction call. + * + * @param {PassThroughReturnType} [response] The response from a call to + * begin a transaction that completed successfully. + * + **/ #parseRunSuccess(response: PassThroughReturnType) { const resp = response.resp; this.id = resp!.transaction; From 6b73bbf234c01b5de95c5eee1ff1fe39e8e180b3 Mon Sep 17 00:00:00 2001 From: Daniel Bruce Date: Fri, 10 Nov 2023 15:49:40 -0500 Subject: [PATCH 50/50] Add a comment for aggregate queries Add JSdoc comment --- src/request.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/request.ts b/src/request.ts index d39f572ef..1231e595a 100644 --- a/src/request.ts +++ b/src/request.ts @@ -546,6 +546,18 @@ class DatastoreRequest { ); } + /** + * Datastore allows you to run aggregate queries by aggregate field. + * + * The query is run, and the results are returned as the second argument to + * your callback. + * + * @param {AggregateQuery} query AggregateQuery object. + * @param {RunQueryOptions} options Optional configuration + * @param {function} [callback] The callback function. If omitted, a promise is + * returned. + * + **/ runAggregationQuery( query: AggregateQuery, options?: RunQueryOptions