Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: new transaction feature #1239

Merged
Merged
Show file tree
Hide file tree
Changes from 58 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
f9bb82e
Move the state up, make it protected
danieljbruce Mar 4, 2024
e6c1265
Build new transaction options method
danieljbruce Mar 4, 2024
974eee2
Correct the fn that builds transactions
danieljbruce Mar 4, 2024
9cf180a
Set transaction to readOnly in system test.
danieljbruce Mar 5, 2024
a58d4ec
Change #parseRunSuccess
danieljbruce Mar 5, 2024
bf110eb
Move parseRunSuccess up to protected
danieljbruce Mar 5, 2024
f1cb78d
Saves the transaction id
danieljbruce Mar 5, 2024
8a420c3
Add tests for testing read only transactions
danieljbruce Mar 5, 2024
6ca70ac
Add tests that measure read time
danieljbruce Mar 5, 2024
46121e0
ran linter
danieljbruce Mar 6, 2024
8b5a311
Add a test for testing requests
danieljbruce Mar 6, 2024
5a91c4e
Final changes to make test work
danieljbruce Mar 6, 2024
38d76ff
Add the transaction.run test
danieljbruce Mar 6, 2024
6299bf3
run linter
danieljbruce Mar 6, 2024
e36b158
runQuery, lookup, put, commit
danieljbruce Mar 6, 2024
28a89ed
runAggregationQuery, lookup, put, commit
danieljbruce Mar 6, 2024
fa4f200
put, put, lookup, commit
danieljbruce Mar 6, 2024
6c976e1
Add tests for the commits
danieljbruce Mar 6, 2024
920f282
Add testing to ensure begin tx is called
danieljbruce Mar 7, 2024
7f9c2c9
Document #blockWithMutex
danieljbruce Mar 7, 2024
f762bb1
Document transaction state
danieljbruce Mar 7, 2024
4375378
feat: new transaction feature branch
danieljbruce Mar 7, 2024
599bec1
Add a check for expired
danieljbruce Mar 8, 2024
dc23b45
Add commit and rollback blocks
danieljbruce Mar 8, 2024
dac429a
run the linter
danieljbruce Mar 8, 2024
637ec5d
Don’t allow readtime to be specified in a txn
danieljbruce May 30, 2024
4a5fdae
throw error for both read time and consistency
danieljbruce May 30, 2024
e0ad5e8
Merge branch 'main' of https://github.com/googleapis/nodejs-datastore…
danieljbruce May 30, 2024
06661c4
Add test for specifying readtime
danieljbruce May 30, 2024
30f5b79
Remove the console logs
danieljbruce May 30, 2024
a3872bd
Refactor the test
danieljbruce May 30, 2024
3b03bf7
Improve test to make sure Gapic layer isn’t called
danieljbruce May 30, 2024
5891af8
Run linter
danieljbruce Jun 3, 2024
5fd45e7
Make change to allow the runAggregationQuery go up
danieljbruce Jun 3, 2024
4aedb17
Should error when get is used
danieljbruce Jun 3, 2024
38bd702
Move tests over and refactor initialized datastore
danieljbruce Jun 3, 2024
d8821d9
Remove only and remove imports
danieljbruce Jun 3, 2024
5eed19f
Introduce parameterized testing for errors
danieljbruce Jun 3, 2024
ecb9737
Use parameters in parameterized tests
danieljbruce Jun 3, 2024
c08df99
Migrate error tests to parameterized testing
danieljbruce Jun 3, 2024
316c651
Change description
danieljbruce Jun 3, 2024
974117b
Prepare second describe block for tests
danieljbruce Jun 3, 2024
5edf87a
Revert "Prepare second describe block for tests"
danieljbruce Jun 3, 2024
9f3b5ca
Always return after the error is sent back
danieljbruce Jun 3, 2024
602280e
Add headers
danieljbruce Jun 3, 2024
82f3b03
Tests and implementation for expired on rollback
danieljbruce Jun 3, 2024
57e7575
fix test
danieljbruce Jun 3, 2024
ff7d200
Wrap rollbacks with a withBeginTransaction.
danieljbruce Jun 3, 2024
d43f02f
Update the test so that it begins the tx before
danieljbruce Jun 4, 2024
1c74b6b
Throw error if transaction not started on rollback
danieljbruce Jun 4, 2024
a1a5ba6
Remove only
danieljbruce Jun 4, 2024
52f4199
Add a comment to the test regarding new txn.run()
danieljbruce Jun 4, 2024
13dcb58
Ensure that the errors get bubbled up
danieljbruce Jun 5, 2024
8091062
Run. linter
danieljbruce Jun 5, 2024
4a74f4d
Merge branch 'main' into new-transaction-feature-branch
danieljbruce Jun 5, 2024
d52c773
read time and consistency error
danieljbruce Jun 5, 2024
66b0286
Merge branch 'new-transaction-feature-branch' of https://github.com/d…
danieljbruce Jun 5, 2024
2b93852
Remove unnecessary change
danieljbruce Jun 5, 2024
22e7b9d
Eliminate the call to withBeginTransaction
danieljbruce Jun 5, 2024
a54b6c6
Throw error reported in documentation
danieljbruce Jun 10, 2024
418c43b
Work on streaming errors
danieljbruce Jun 10, 2024
2edd410
Move the error throwing up the stack
danieljbruce Jun 11, 2024
5c1dee5
Fix linting errors
danieljbruce Jun 11, 2024
7bf7a90
Generate unit tests for getTransactionRequest
danieljbruce Jun 20, 2024
2022006
Generate unit tests
danieljbruce Jun 20, 2024
f94c37e
Merge branch 'main' into new-transaction-feature-branch
danieljbruce Jun 20, 2024
03b7cd3
Make function one ternary operation
danieljbruce Jun 20, 2024
b16afbf
Merge branch 'new-transaction-feature-branch' of https://github.com/d…
danieljbruce Jun 20, 2024
9b05013
Add two comments
danieljbruce Jun 20, 2024
f10d1ef
Update the comment
danieljbruce Jun 20, 2024
7e453e1
Run the linter
danieljbruce Jun 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 163 additions & 18 deletions src/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,17 @@

// eslint-disable-next-line @typescript-eslint/no-var-requires
const streamEvents = require('stream-events');
export const transactionExpiredError = 'This transaction has already expired.';

export interface AbortableDuplex extends Duplex {
abort(): void;
}

interface TransactionRequestOptions {
readOnly?: {};
readWrite?: {previousTransaction?: string | Uint8Array | null};
}

// Import the clients for each version supported by this package.
const gapic = Object.freeze({
v1: require('./v1'),
Expand All @@ -55,9 +61,10 @@
RunQueryResponse,
RunQueryCallback,
} from './query';
import {Datastore} from '.';
import {Datastore, Transaction} from '.';
import ITimestamp = google.protobuf.ITimestamp;
import {AggregateQuery} from './aggregate';
import {RunOptions} from './transaction';
import * as protos from '../protos/protos';
import {serializer} from 'google-gax';
import * as gax from 'google-gax';
Expand Down Expand Up @@ -153,6 +160,16 @@
return {};
}

const readTimeAndConsistencyError =
'Read time and read consistency cannot both be specified.';

// Write function to check for readTime and readConsistency.
function throwOnReadTimeAndConsistency(options: RunQueryStreamOptions) {
if (options.readTime && options.consistency) {
throw new Error(readTimeAndConsistencyError);
}
}

/**
* A map of read consistency values to proto codes.
*
Expand All @@ -164,6 +181,19 @@
strong: 1,
};

/**
* By default a DatastoreRequest is in the NOT_TRANSACTION state. If the
* DatastoreRequest is a Transaction object, then initially it will be in
* the NOT_STARTED state, but then the state will become IN_PROGRESS after the
* transaction has started.
*/
export enum TransactionState {
NOT_TRANSACTION,
NOT_STARTED,
IN_PROGRESS,
EXPIRED,
}

/**
* Handle logic for Datastore API operations. Handles request logic for
* Datastore.
Expand All @@ -184,6 +214,7 @@
| Array<(err: Error | null, resp: Entity | null) => void>
| Entity;
datastore!: Datastore;
protected state: TransactionState = TransactionState.NOT_TRANSACTION;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like using protected here because typescript doesn't technically hide protected properties completely, but it seems that there is no simple way around it because both Transaction and DatastoreRequest need access to the state variable. There is more information about some complex ways to avoid this in the design doc in Alternative 3, but they require major architecture changes.

[key: string]: Entity;

/**
Expand Down Expand Up @@ -335,6 +366,15 @@
);
}

/* This throws an error if the transaction has already expired.
*
*/
protected checkExpired() {
if (this.state === TransactionState.EXPIRED) {
throw Error(transactionExpiredError);
}
}

/**
* Retrieve the entities as a readable object stream.
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: update L381 with new added errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Done.

Expand Down Expand Up @@ -365,12 +405,19 @@
keys: Entities,
options: CreateReadStreamOptions = {}
): Transform {
this.checkExpired();
keys = arrify(keys).map(entity.keyToKeyProto);
if (keys.length === 0) {
throw new Error('At least one Key object is required.');
}

const makeRequest = (keys: entity.Key[] | KeyProto[]) => {
try {
throwOnReadTimeAndConsistency(options);
} catch (error: any) {

Check warning on line 417 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
stream.destroy(error);
return;
}
const reqOpts = this.getRequestOptions(options);
Object.assign(reqOpts, {keys});
this.request_(
Expand All @@ -381,6 +428,7 @@
gaxOpts: options.gaxOptions,
},
(err, resp) => {
this.parseTransactionResponse(resp);
if (err) {
stream.destroy(err);
return;
Expand Down Expand Up @@ -631,6 +679,10 @@
const callback =
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;

if (this.state === TransactionState.EXPIRED) {
callback(new Error(transactionExpiredError));
return;
}
this.createReadStream(keys, options)
.on('error', callback)
.pipe(
Expand All @@ -641,6 +693,22 @@
);
}

/**
* This function saves results from a successful beginTransaction call.
*
* @param {BeginAsyncResponse} [response] The response from a call to
* begin a transaction that completed successfully.
*
**/
protected parseTransactionResponse(resp?: {
transaction?: Uint8Array | string | undefined | null;
}): void {
if (resp && resp.transaction && Buffer.byteLength(resp.transaction) > 0) {
this.id = resp!.transaction;
this.state = TransactionState.IN_PROGRESS;
}
}

/**
* Datastore allows you to run aggregate queries by supplying aggregate fields
* which will determine the type of aggregation that is performed.
Expand Down Expand Up @@ -677,6 +745,14 @@
const callback =
typeof optionsOrCallback === 'function' ? optionsOrCallback : cb!;

if (this.state === TransactionState.EXPIRED) {
callback(new Error(transactionExpiredError));
return;
}
if (options.readTime && options.consistency) {
callback(new Error(readTimeAndConsistencyError));
return;
}
query.query = extend(true, new Query(), query.query);
let queryProto: QueryProto;
try {
Expand All @@ -687,7 +763,13 @@
setImmediate(callback, e as Error);
return;
}
const sharedQueryOpts = this.getQueryOptions(query.query, options);
let sharedQueryOpts;
try {
sharedQueryOpts = this.getQueryOptions(query.query, options);
} catch (error: any) {

Check warning on line 769 in src/request.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
callback(error);
return;
}
const aggregationQueryOptions: AggregationQueryOptions = {
nestedQuery: queryProto,
aggregations: query.toProto(),
Expand All @@ -704,6 +786,7 @@
},
(err, res) => {
const info = getInfoFromStats(res);
this.parseTransactionResponse(res);
if (res && res.batch) {
const results = res.batch.aggregationResults;
const finalResults = results
Expand Down Expand Up @@ -846,6 +929,10 @@

let info: RunQueryInfo;

if (this.state === TransactionState.EXPIRED) {
callback(new Error(transactionExpiredError));
return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check readTimeAndConsistencyError here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. That check is done further downstream in the call to runQueryStream. But I did spot some other cases where errors weren't bubbling up and fixed those bugs.

this.runQueryStream(query, options)
.on('error', callback)
.on('info', info_ => {
Expand Down Expand Up @@ -892,11 +979,13 @@
* ```
*/
runQueryStream(query: Query, options: RunQueryStreamOptions = {}): Transform {
this.checkExpired();
query = extend(true, new Query(), query);
const makeRequest = (query: Query) => {
let queryProto: QueryProto;
try {
queryProto = entity.queryToQueryProto(query);
throwOnReadTimeAndConsistency(options);
} catch (e) {
// using setImmediate here to make sure this doesn't throw a
// synchronous error
Expand All @@ -918,7 +1007,8 @@
);
};

function onResultSet(err?: Error | null, resp?: Entity) {
const onResultSet = (err?: Error | null, resp?: Entity) => {
this.parseTransactionResponse(resp);
if (err) {
stream.destroy(err);
return;
Expand Down Expand Up @@ -977,7 +1067,7 @@

makeRequest(query);
});
}
};

const stream = streamEvents(new Transform({objectMode: true}));
stream.once('reading', () => {
Expand All @@ -990,11 +1080,24 @@
options: RunQueryStreamOptions
): SharedQueryOptions {
const sharedQueryOpts = {} as SharedQueryOptions;
if (isTransaction(this)) {
if (this.state === TransactionState.NOT_STARTED) {
if (sharedQueryOpts.readOptions === undefined) {
sharedQueryOpts.readOptions = {};
}
sharedQueryOpts.readOptions.newTransaction = getTransactionRequest(
this,
{}
);
sharedQueryOpts.readOptions.consistencyType = 'newTransaction';
}
}
if (options.consistency) {
const code = CONSISTENCY_PROTO_CODE[options.consistency.toLowerCase()];
sharedQueryOpts.readOptions = {
readConsistency: code,
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug because it would overwrite newTransaction if readConsistency was provided.

if (sharedQueryOpts.readOptions === undefined) {
sharedQueryOpts.readOptions = {};
}
sharedQueryOpts.readOptions.readConsistency = code;
}
if (options.readTime) {
if (sharedQueryOpts.readOptions === undefined) {
Expand Down Expand Up @@ -1122,18 +1225,27 @@
reqOpts.transaction = this.id;
}

if (
isTransaction ||
(reqOpts.readOptions && reqOpts.readOptions.newTransaction)
) {
if (reqOpts.readOptions && reqOpts.readOptions.readConsistency) {
callback(
new Error('Read consistency cannot be specified in a transaction.')
);
return;
}
if (reqOpts.readOptions && reqOpts.readOptions.readTime) {
callback(new Error('Read time cannot be specified in a transaction.'));
return;
}
}
if (
isTransaction &&
(method === 'lookup' ||
method === 'runQuery' ||
method === 'runAggregationQuery')
) {
if (reqOpts.readOptions && reqOpts.readOptions.readConsistency) {
throw new Error(
'Read consistency cannot be specified in a transaction.'
);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just moved up to also be included for new transaction calls. Also, the error was not propogating up and needs to be wrapped in a callback.

if (reqOpts.readOptions) {
Object.assign(reqOpts.readOptions, {transaction: this.id});
} else {
Expand Down Expand Up @@ -1229,6 +1341,36 @@
}
}

function isTransaction(request: DatastoreRequest): request is Transaction {
return request instanceof Transaction;
}

export function getTransactionRequest(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used by DatastoreRequest in request.ts and Transaction in transaction.ts. Transaction inherits from DatastoreRequest so even though this could be moved to a protected method in DatastoreRequest, we want to avoid doing so. This is because adding the protected reserved word in Typescript doesn't entirely hide the method from the user in the class like protected should.

transaction: Transaction,
options: RunOptions
): TransactionRequestOptions {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add some comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

let reqOpts: TransactionRequestOptions = {};
if (options.readOnly || transaction.readOnly) {
reqOpts.readOnly = {};
}
if (options.transactionId || transaction.id) {
reqOpts.readWrite = {
previousTransaction: options.transactionId || transaction.id,
};
}
if (options.transactionOptions) {
reqOpts = {};
if (options.transactionOptions.readOnly) {
reqOpts.readOnly = {};
}
const id = options.transactionOptions.id;
if (id) {
reqOpts.readWrite = {previousTransaction: id};
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to trim down this logic a bit? Seems a bit redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generated unit tests, corrected them and then reduced this function down to one ternary operation. Previously this was just copy pasted code from before. Search describe('getTransactionRequest', () => { to see how this function should behave.

return reqOpts;
}

export interface ConsistencyProtoCode {
[key: string]: number;
}
Expand Down Expand Up @@ -1294,15 +1436,18 @@
readConsistency?: number;
transaction?: string | Uint8Array | null;
readTime?: ITimestamp;
newTransaction?: TransactionRequestOptions;
consistencyType?:
| 'readConsistency'
| 'transaction'
| 'newTransaction'
| 'readTime';
};
}
export interface RequestOptions extends SharedQueryOptions {
mutations?: google.datastore.v1.IMutation[];
keys?: Entity;
transactionOptions?: {
readOnly?: {};
readWrite?: {previousTransaction?: string | Uint8Array | null};
} | null;
transactionOptions?: TransactionRequestOptions | null;
transaction?: string | null | Uint8Array;
mode?: string;
query?: QueryProto;
Expand Down Expand Up @@ -1333,7 +1478,7 @@
* that a callback is omitted.
*/
promisifyAll(DatastoreRequest, {
exclude: ['getQueryOptions', 'getRequestOptions'],
exclude: ['checkExpired', 'getQueryOptions', 'getRequestOptions'],
});

/**
Expand Down
Loading
Loading