From 307c6cc6a0e8fbc861ee0ab78d22e5df3890faba Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt <mrschmidt@google.com> Date: Wed, 23 Jun 2021 14:25:40 -0600 Subject: [PATCH 1/3] feat: add read-only transactions --- dev/src/index.ts | 94 +++++++++++++++++----- dev/src/transaction.ts | 23 ++++-- dev/src/validate.ts | 21 +++++ dev/system-test/firestore.ts | 35 ++++++++ dev/test/transaction.ts | 149 +++++++++++++++++++++++++++-------- types/firestore.d.ts | 49 +++++++++--- 6 files changed, 302 insertions(+), 69 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 9cf3a8483..367fd7762 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -68,6 +68,7 @@ import { validateMinNumberOfArguments, validateObject, validateString, + validateTimestamp, } from './validate'; import {WriteBatch} from './write-batch'; @@ -925,13 +926,37 @@ export class Firestore implements firestore.Firestore { * * @callback Firestore~updateFunction * @template T - * @param {Transaction} transaction The transaction object for this + * @param {Transaction} transaction The transaction object for this * transaction. * @returns {Promise<T>} The promise returned at the end of the transaction. * This promise will be returned by {@link Firestore#runTransaction} if the * transaction completed successfully. */ + /** + * Options object for {@link Firestore#runTransaction} to configure a + * read-only transaction. + * + * @callback Firestore~ReadOnlyTransactionOptions + * @template T + * @param {true} readOnly Set to true to indicate a read-write transaction. + * @param {Timestamp=} readTime If specified, documents are read at the given + * time. This may not be more than 60 seconds in the past from when the + * request is processed by the server. + */ + + /** + * Options object for {@link Firestore#runTransaction} to configure a + * read-write transaction. + * + * @callback Firestore~ReadWriteTransactionOptions + * @template T + * @param {false=} readOnly Set to false or omitted to start a read-write + * transaction. + * @param {number=} maxAttempts The maximum number of attempts for this + * transaction. Defaults to five. + */ + /** * Executes the given updateFunction and commits the changes applied within * the transaction. @@ -940,11 +965,18 @@ export class Firestore implements firestore.Firestore { * modify Firestore documents under lock. You have to perform all reads before * before you perform any write. * - * Documents read during a transaction are locked pessimistically. A - * transaction's lock on a document blocks other transactions, batched - * writes, and other non-transactional writes from changing that document. - * A transaction releases its document locks at commit time or once it times - * out or fails for any reason. + * Transaction can be performed as read-only or read-write transactions. By + * default, transactions are executed in read-write mode. + * + * A read-write transaction obtains a pessimistic lock on all documents that + * are read during the transaction. These locks block other transactions, + * batched writes, and other non-transactional writes from changing that + * document. A transaction releases its document locks at commit time or if + * it fails for any reason. + * + * Read-only transaction do not lock documents. They can be used to read + * documents at a consistent snapshot in time, which may be up to 60 seconds + * in the past. * * Transactions are committed once 'updateFunction' resolves. If a transaction * fails with contention, the transaction is retried up to five times. The @@ -952,14 +984,14 @@ export class Firestore implements firestore.Firestore { * * Transactions time out after 60 seconds if no documents are read. * Transactions that are not committed within than 270 seconds are also - * aborted. + * aborted. Any remaining locks are released when a transaction times out. * * @template T * @param {Firestore~updateFunction} updateFunction The user function to * execute within the transaction context. - * @param {object=} transactionOptions Transaction options. - * @param {number=} transactionOptions.maxAttempts - The maximum number of - * attempts for this transaction. + * @param { + * Firestore~ReadWriteTransactionOptions|Firestore~ReadOnlyTransactionOptions= + * } transactionOptions Transaction options. * @returns {Promise<T>} If the transaction completed successfully or was * explicitly aborted (by the updateFunction returning a failed Promise), the * Promise returned by the updateFunction will be returned here. Else if the @@ -990,30 +1022,52 @@ export class Firestore implements firestore.Firestore { */ runTransaction<T>( updateFunction: (transaction: Transaction) => Promise<T>, - transactionOptions?: {maxAttempts?: number} + transactionOptions?: + | firestore.ReadWriteTransactionOptions + | firestore.ReadOnlyTransactionOptions ): Promise<T> { validateFunction('updateFunction', updateFunction); const defaultAttempts = 5; const tag = requestTag(); - let maxAttempts: number; + let maxAttempts = defaultAttempts; + let readOnly = false; + let readTime: Timestamp | undefined; if (transactionOptions) { validateObject('transactionOptions', transactionOptions); - validateInteger( - 'transactionOptions.maxAttempts', - transactionOptions.maxAttempts, - {optional: true, minValue: 1} + validateBoolean( + 'transactionOptions.readOnly', + transactionOptions.readOnly, + {optional: true} ); - maxAttempts = transactionOptions.maxAttempts || defaultAttempts; - } else { - maxAttempts = defaultAttempts; + if (transactionOptions.readOnly) { + readOnly = true; + validateTimestamp( + 'transactionOptions.readTime', + transactionOptions.readTime, + {optional: true} + ); + readTime = transactionOptions.readTime as Timestamp | undefined; + maxAttempts = 1; + } else { + validateInteger( + 'transactionOptions.maxAttempts', + transactionOptions.maxAttempts, + {optional: true, minValue: 1} + ); + maxAttempts = transactionOptions.maxAttempts || defaultAttempts; + } } const transaction = new Transaction(this, tag); return this.initializeIfNeeded(tag).then(() => - transaction.runTransaction(updateFunction, maxAttempts) + transaction.runTransaction(updateFunction, { + maxAttempts, + readOnly, + readTime, + }) ); } diff --git a/dev/src/transaction.ts b/dev/src/transaction.ts index e7c5766c5..bf2103ac3 100644 --- a/dev/src/transaction.ts +++ b/dev/src/transaction.ts @@ -22,6 +22,7 @@ import * as proto from '../protos/firestore_v1_proto_api'; import {ExponentialBackoff} from './backoff'; import {DocumentSnapshot} from './document'; import {Firestore, WriteBatch} from './index'; +import {Timestamp} from './timestamp'; import {logger} from './logger'; import {FieldPath, validateFieldPath} from './path'; import {StatusCode} from './status-code'; @@ -355,12 +356,18 @@ export class Transaction implements firestore.Transaction { * * @private */ - begin(): Promise<void> { + begin(readOnly: boolean, readTime: Timestamp | undefined): Promise<void> { const request: api.IBeginTransactionRequest = { database: this._firestore.formattedName, }; - if (this._transactionId) { + if (readOnly) { + request.options = { + readOnly: { + readTime: readTime?.toProto()?.timestampValue, + }, + }; + } else if (this._transactionId) { request.options = { readWrite: { retryTransaction: this._transactionId, @@ -415,16 +422,20 @@ export class Transaction implements firestore.Transaction { * context. * @param requestTag A unique client-assigned identifier for the scope of * this transaction. - * @param maxAttempts The maximum number of attempts for this transaction. + * @param options The user-defined options for this transaction. */ async runTransaction<T>( updateFunction: (transaction: Transaction) => Promise<T>, - maxAttempts: number + options: { + maxAttempts: number; + readOnly: boolean; + readTime?: Timestamp; + } ): Promise<T> { let result: T; let lastError: GoogleError | undefined = undefined; - for (let attempt = 0; attempt < maxAttempts; ++attempt) { + for (let attempt = 0; attempt < options.maxAttempts; ++attempt) { try { if (lastError) { logger( @@ -439,7 +450,7 @@ export class Transaction implements firestore.Transaction { this._writeBatch._reset(); await this.maybeBackoff(lastError); - await this.begin(); + await this.begin(options.readOnly, options.readTime); const promise = updateFunction(this); if (!(promise instanceof Promise)) { diff --git a/dev/src/validate.ts b/dev/src/validate.ts index 3db6390f1..0270b5f50 100644 --- a/dev/src/validate.ts +++ b/dev/src/validate.ts @@ -17,6 +17,7 @@ import {URL} from 'url'; import {FieldPath} from './path'; import {isFunction, isObject} from './util'; +import {Timestamp} from './timestamp'; /** * Options to allow argument omission. @@ -278,6 +279,26 @@ export function validateInteger( } } +/** + * Validates that 'value' is a Timestamp. + * + * @private + * @param arg The argument name or argument index (for varargs methods). + * @param value The input to validate. + * @param options Options that specify whether the Timestamp can be omitted. + */ +export function validateTimestamp( + arg: string | number, + value: unknown, + options?: RequiredArgumentOptions +): void { + if (!validateOptional(value, options)) { + if (!(value instanceof Timestamp)) { + throw new Error(invalidArgumentMessage(arg, 'Timestamp')); + } + } +} + /** * Generates an error message to use with invalid arguments. * diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 10379e830..52ed3a848 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -2345,6 +2345,41 @@ describe('Transaction class', () => { const finalSnapshot = await ref.get(); expect(finalSnapshot.data()).to.deep.equal({first: true, second: true}); }); + + it('supports read-only transactions', async () => { + const ref = randomCol.doc('doc'); + await ref.set({foo: 'bar'}); + const snapshot = await firestore.runTransaction( + updateFunction => updateFunction.get(ref), + {readOnly: true} + ); + expect(snapshot.exists).to.be.true; + }); + + it('supports read-only transactions with custom read-time', async () => { + const ref = randomCol.doc('doc'); + const writeResult = await ref.set({foo: 1}); + await ref.set({foo: 2}); + const snapshot = await firestore.runTransaction( + updateFunction => updateFunction.get(ref), + {readOnly: true, readTime: writeResult.writeTime} + ); + expect(snapshot.exists).to.be.true; + expect(snapshot.get('foo')).to.equal(1); + }); + + it('fails read-only with writes', async () => { + const ref = randomCol.doc('doc'); + try { + await firestore.runTransaction( + async updateFunction => updateFunction.set(ref, {}), + {readOnly: true} + ); + expect.fail(); + } catch (e) { + expect(e.code).to.equal(Status.INVALID_ARGUMENT); + } + }); }); describe('WriteBatch class', () => { diff --git a/dev/test/transaction.ts b/dev/test/transaction.ts index 5434c86ff..8854bc159 100644 --- a/dev/test/transaction.ts +++ b/dev/test/transaction.ts @@ -22,7 +22,7 @@ import * as through2 from 'through2'; import * as proto from '../protos/firestore_v1_proto_api'; import * as Firestore from '../src'; -import {DocumentReference, FieldPath, Transaction} from '../src'; +import {DocumentReference, FieldPath, Timestamp, Transaction} from '../src'; import {setTimeoutHandler} from '../src/backoff'; import { ApiOverride, @@ -35,6 +35,10 @@ import { } from './util/helpers'; import api = proto.google.firestore.v1; +import { + ReadOnlyTransactionOptions, + ReadWriteTransactionOptions, +} from '@google-cloud/firestore'; use(chaiAsPromised); @@ -128,29 +132,38 @@ function rollback( }; } -function begin( - transaction?: Uint8Array | string, - prevTransaction?: Uint8Array | string, - error?: Error -): TransactionStep { +function begin(options?: { + transactionId?: Uint8Array | string; + readOnly?: {readTime?: {seconds?: number; nanos?: number}}; + readWrite?: { + prevTransactionId?: Uint8Array | string; + }; + error?: Error; +}): TransactionStep { const proto: api.IBeginTransactionRequest = {database: DATABASE_ROOT}; - if (prevTransaction) { + if (options?.readOnly) { + proto.options = { + readOnly: { + readTime: options.readOnly.readTime, + }, + }; + } else if (options?.readWrite?.prevTransactionId) { proto.options = { readWrite: { - retryTransaction: transactionId(prevTransaction), + retryTransaction: transactionId(options.readWrite.prevTransactionId), }, }; } const response = { - transaction: transactionId(transaction), + transaction: transactionId(options?.transactionId), }; return { type: 'begin', request: proto, - error, + error: options?.error, response, }; } @@ -278,6 +291,7 @@ function backoff(maxDelay?: boolean): TransactionStep { * Asserts that the given transaction function issues the expected requests. */ function runTransaction<T>( + transactionOptions: ReadWriteTransactionOptions | ReadOnlyTransactionOptions, transactionCallback: ( transaction: Transaction, docRef: DocumentReference @@ -352,7 +366,7 @@ function runTransaction<T>( return await firestore.runTransaction(transaction => { const docRef = firestore.doc('collectionId/documentId'); return transactionCallback(transaction, docRef); - }); + }, transactionOptions); } finally { setTimeoutHandler(setTimeout); expect(expectedRequests.length).to.equal( @@ -366,6 +380,7 @@ function runTransaction<T>( describe('successful transactions', () => { it('empty transaction', () => { return runTransaction( + /* transactionOptions= */ {}, () => { return Promise.resolve(); }, @@ -376,6 +391,7 @@ describe('successful transactions', () => { it('returns value', () => { return runTransaction( + /* transactionOptions= */ {}, () => { return Promise.resolve('bar'); }, @@ -415,19 +431,24 @@ describe('failed transactions', () => { if (retry) { await runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', undefined, serverError), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({ + transactionId: 'foo2', + readWrite: {prevTransactionId: 'foo1'}, + }), commit('foo2') ); } else { await expect( runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', undefined, serverError), rollback('foo1') ) @@ -445,12 +466,13 @@ describe('failed transactions', () => { serverError.code = Status.INVALID_ARGUMENT; await runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', undefined, serverError), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({transactionId: 'foo2', readWrite: {prevTransactionId: 'foo1'}}), commit('foo2') ); }); @@ -470,20 +492,25 @@ describe('failed transactions', () => { if (retry) { await runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), query('foo1', serverError), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({ + transactionId: 'foo2', + readWrite: {prevTransactionId: 'foo1'}, + }), query('foo2'), commit('foo2') ); } else { await expect( runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), query('foo1', serverError), rollback('foo1') ) @@ -506,20 +533,25 @@ describe('failed transactions', () => { if (retry) { await runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), getDocument('foo1', serverError), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({ + transactionId: 'foo2', + readWrite: {prevTransactionId: 'foo1'}, + }), getDocument('foo2'), commit('foo2') ); } else { await expect( runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), getDocument('foo1', serverError), rollback('foo1') ) @@ -537,20 +569,25 @@ describe('failed transactions', () => { if (retry) { await runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', /* writes=*/ undefined, serverError), rollback('foo1', serverError), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({ + transactionId: 'foo2', + readWrite: {prevTransactionId: 'foo1'}, + }), commit('foo2') ); } else { await expect( runTransaction( + /* transactionOptions= */ {}, transactionFunction, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', /* writes=*/ undefined, serverError), rollback('foo1', serverError) ) @@ -595,7 +632,12 @@ describe('failed transactions', () => { it('requires a promise', () => { return expect( - runTransaction((() => {}) as InvalidApiUsage, begin(), rollback()) + runTransaction( + /* transactionOptions= */ {}, + (() => {}) as InvalidApiUsage, + begin(), + rollback() + ) ).to.eventually.be.rejectedWith( 'You must return a Promise in your transaction()-callback.' ); @@ -618,6 +660,7 @@ describe('failed transactions', () => { it("doesn't retry custom user exceptions in callback", () => { return expect( runTransaction( + /* transactionOptions= */ {}, () => { return Promise.reject('request exception'); }, @@ -633,24 +676,25 @@ describe('failed transactions', () => { return expect( runTransaction( + /* transactionOptions= */ {}, () => Promise.resolve(), - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', [], err), rollback('foo1'), backoff(), - begin('foo2', 'foo1'), + begin({transactionId: 'foo2', readWrite: {prevTransactionId: 'foo1'}}), commit('foo2', [], err), rollback('foo2'), backoff(), - begin('foo3', 'foo2'), + begin({transactionId: 'foo3', readWrite: {prevTransactionId: 'foo2'}}), commit('foo3', [], err), rollback('foo3'), backoff(), - begin('foo4', 'foo3'), + begin({transactionId: 'foo4', readWrite: {prevTransactionId: 'foo3'}}), commit('foo4', [], err), rollback('foo4'), backoff(), - begin('foo5', 'foo4'), + begin({transactionId: 'foo5', readWrite: {prevTransactionId: 'foo4'}}), commit('foo5', [], new Error('Final exception')), rollback('foo5') ) @@ -662,12 +706,13 @@ describe('failed transactions', () => { err.code = Status.RESOURCE_EXHAUSTED; return runTransaction( + /* transactionOptions= */ {}, async () => {}, - begin('foo1'), + begin({transactionId: 'foo1'}), commit('foo1', [], err), rollback('foo1'), backoff(/* maxDelay= */ true), - begin('foo2', 'foo1'), + begin({transactionId: 'foo2', readWrite: {prevTransactionId: 'foo1'}}), commit('foo2') ); }); @@ -676,6 +721,7 @@ describe('failed transactions', () => { describe('transaction operations', () => { it('support get with document ref', () => { return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { return transaction.get(docRef).then(doc => { expect(doc.id).to.equal('documentId'); @@ -689,6 +735,7 @@ describe('transaction operations', () => { it('requires a query or document for get', () => { return runTransaction( + /* transactionOptions= */ {}, (transaction: InvalidApiUsage) => { expect(() => transaction.get()).to.throw( 'Value for argument "refOrQuery" must be a DocumentReference or a Query.' @@ -708,6 +755,7 @@ describe('transaction operations', () => { it('enforce that gets come before writes', () => { return expect( runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.set(docRef, {foo: 'bar'}); return transaction.get(docRef); @@ -722,6 +770,7 @@ describe('transaction operations', () => { it('support get with query', () => { return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { const query = docRef.parent.where('foo', '==', 'bar'); return transaction.get(query).then(results => { @@ -734,8 +783,32 @@ describe('transaction operations', () => { ); }); + it('supports read-only transactions', () => { + return runTransaction( + {readOnly: true}, + (transaction, docRef) => transaction.get(docRef), + begin({readOnly: {}}), + getDocument(), + commit() + ); + }); + + it('supports read-only transactions with read time', () => { + return runTransaction( + { + readOnly: true, + readTime: Timestamp.fromMillis(1), + }, + (transaction, docRef) => transaction.get(docRef), + begin({readOnly: {readTime: {nanos: 1000000}}}), + getDocument(), + commit() + ); + }); + it('support getAll', () => { return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { const firstDoc = docRef.parent.doc('firstDocument'); const secondDoc = docRef.parent.doc('secondDocument'); @@ -754,6 +827,7 @@ describe('transaction operations', () => { it('support getAll with field mask', () => { return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { const doc = docRef.parent.doc('doc'); @@ -770,6 +844,7 @@ describe('transaction operations', () => { it('enforce that getAll come before writes', () => { return expect( runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.set(docRef, {foo: 'bar'}); return transaction.getAll(docRef); @@ -794,6 +869,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.create(docRef, {}); return Promise.resolve(); @@ -828,6 +904,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.update(docRef, {'a.b': 'c'}); transaction.update(docRef, 'a.b', 'c'); @@ -852,6 +929,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.set(docRef, {'a.b': 'c'}); return Promise.resolve(); @@ -877,6 +955,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.set(docRef, {'a.b': 'c'}, {merge: true}); return Promise.resolve(); @@ -902,6 +981,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { const postRef = docRef.withConverter(postConverterMerge); transaction.set(postRef, {title: 'story'} as Partial<Post>, { @@ -930,6 +1010,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { const postRef = docRef.withConverter(postConverter); transaction.set( @@ -952,6 +1033,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.delete(docRef); return Promise.resolve(); @@ -974,6 +1056,7 @@ describe('transaction operations', () => { }; return runTransaction( + /* transactionOptions= */ {}, (transaction, docRef) => { transaction.delete(docRef).set(docRef, {}); return Promise.resolve(); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index ad393532f..8d65ed6a4 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -174,6 +174,28 @@ declare namespace FirebaseFirestore { [key: string]: any; // Accept other properties, such as GRPC settings. } + /** Options to configure a read-only transaction. */ + export interface ReadOnlyTransactionOptions { + /** Set to true to indicate a read-write transaction. */ + readOnly: true; + /** + * If specified, documents are read at the given time. This may not be more + * than 60 seconds in the past from when the request is processed by the + * server. + */ + readTime?: Timestamp; + } + + /** Options to configure a read-write transaction. */ + export interface ReadWriteTransactionOptions { + /** Set to false or omitted to start a read-write transaction. */ + readOnly?: false; + /** + * The maximum number of attempts for this transaction. Defaults to five. + */ + maxAttempts?: number; + } + /** * `Firestore` represents a Firestore Database and is the entry point for all * Firestore operations. @@ -311,11 +333,18 @@ declare namespace FirebaseFirestore { * modify Firestore documents under lock. You have to perform all reads * before before you perform any write. * - * Documents read during a transaction are locked pessimistically. A - * transaction's lock on a document blocks other transactions, batched - * writes, and other non-transactional writes from changing that document. - * A transaction releases its document locks at commit time or once it times - * out or fails for any reason. + * Transaction can be performed as read-only or read-write transactions. By + * default, transactions are executed in read-write mode. + * + * A read-write transaction obtains a pessimistic lock on all documents that + * are read during the transaction. These locks block other transactions, + * batched writes, and other non-transactional writes from changing that + * document. A transaction releases its document locks at commit time or if + * it fails for any reason. + * + * Read-only transaction do not lock documents. They can be used to read + * documents at a consistent snapshot in time, which may be up to 60 seconds + * in the past. * * Transactions are committed once 'updateFunction' resolves. If a * transaction fails with contention, the transaction is retried up to five @@ -323,13 +352,11 @@ declare namespace FirebaseFirestore { * * Transactions time out after 60 seconds if no documents are read. * Transactions that are not committed within than 270 seconds are also - * aborted. + * aborted. Any remaining locks are released when a transaction times out. * * @param updateFunction The function to execute within the transaction * context. - * @param {object=} transactionOptions Transaction options. - * @param {number=} transactionOptions.maxAttempts The maximum number of - * attempts for this transaction. + * @param transactionOptions Transaction options. * @return If the transaction completed successfully or was explicitly * aborted (by the updateFunction returning a failed Promise), the Promise * returned by the updateFunction will be returned here. Else if the @@ -338,7 +365,9 @@ declare namespace FirebaseFirestore { */ runTransaction<T>( updateFunction: (transaction: Transaction) => Promise<T>, - transactionOptions?: {maxAttempts?: number} + transactionOptions?: + | ReadWriteTransactionOptions + | ReadOnlyTransactionOptions ): Promise<T>; /** From 96fb1dfad012a526a887469a42699695eb9e295a Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt <mrschmidt@google.com> Date: Mon, 28 Jun 2021 19:14:43 -0600 Subject: [PATCH 2/3] Review --- dev/src/index.ts | 22 +++++++++++----------- dev/system-test/firestore.ts | 8 +++++++- types/firestore.d.ts | 22 +++++++++++----------- 3 files changed, 29 insertions(+), 23 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 367fd7762..a1414347f 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -939,7 +939,7 @@ export class Firestore implements firestore.Firestore { * * @callback Firestore~ReadOnlyTransactionOptions * @template T - * @param {true} readOnly Set to true to indicate a read-write transaction. + * @param {true} readOnly Set to true to indicate a read-only transaction. * @param {Timestamp=} readTime If specified, documents are read at the given * time. This may not be more than 60 seconds in the past from when the * request is processed by the server. @@ -951,7 +951,7 @@ export class Firestore implements firestore.Firestore { * * @callback Firestore~ReadWriteTransactionOptions * @template T - * @param {false=} readOnly Set to false or omitted to start a read-write + * @param {false=} readOnly Set to false or omit to indicate a read-write * transaction. * @param {number=} maxAttempts The maximum number of attempts for this * transaction. Defaults to five. @@ -965,22 +965,22 @@ export class Firestore implements firestore.Firestore { * modify Firestore documents under lock. You have to perform all reads before * before you perform any write. * - * Transaction can be performed as read-only or read-write transactions. By + * Transactions can be performed as read-only or read-write transactions. By * default, transactions are executed in read-write mode. * * A read-write transaction obtains a pessimistic lock on all documents that * are read during the transaction. These locks block other transactions, * batched writes, and other non-transactional writes from changing that - * document. A transaction releases its document locks at commit time or if - * it fails for any reason. + * document. Any writes in a read-write transactions are committed once + * 'updateFunction' resolves, which also releases all locks. * - * Read-only transaction do not lock documents. They can be used to read - * documents at a consistent snapshot in time, which may be up to 60 seconds - * in the past. + * If a read-write transaction fails with contention, the transaction is + * retried up to five times. The `updateFunction` is invoked once for each + * attempt. * - * Transactions are committed once 'updateFunction' resolves. If a transaction - * fails with contention, the transaction is retried up to five times. The - * `updateFunction` is invoked once for each attempt. + * Read-only transactions do not lock documents. They can be used to read + * documents at a consistent snapshot in time, which may be up to 60 seconds + * in the past. Read-only transactions are not retried. * * Transactions time out after 60 seconds if no documents are read. * Transactions that are not committed within than 270 seconds are also diff --git a/dev/system-test/firestore.ts b/dev/system-test/firestore.ts index 52ed3a848..11e45e1aa 100644 --- a/dev/system-test/firestore.ts +++ b/dev/system-test/firestore.ts @@ -2369,14 +2369,20 @@ describe('Transaction class', () => { }); it('fails read-only with writes', async () => { + let attempts = 0; + const ref = randomCol.doc('doc'); try { await firestore.runTransaction( - async updateFunction => updateFunction.set(ref, {}), + async updateFunction => { + ++attempts; + updateFunction.set(ref, {}); + }, {readOnly: true} ); expect.fail(); } catch (e) { + expect(attempts).to.equal(1); expect(e.code).to.equal(Status.INVALID_ARGUMENT); } }); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 8d65ed6a4..558ae812a 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -176,7 +176,7 @@ declare namespace FirebaseFirestore { /** Options to configure a read-only transaction. */ export interface ReadOnlyTransactionOptions { - /** Set to true to indicate a read-write transaction. */ + /** Set to true to indicate a read-only transaction. */ readOnly: true; /** * If specified, documents are read at the given time. This may not be more @@ -188,7 +188,7 @@ declare namespace FirebaseFirestore { /** Options to configure a read-write transaction. */ export interface ReadWriteTransactionOptions { - /** Set to false or omitted to start a read-write transaction. */ + /** Set to false or omit to indicate a read-write transaction. */ readOnly?: false; /** * The maximum number of attempts for this transaction. Defaults to five. @@ -333,22 +333,22 @@ declare namespace FirebaseFirestore { * modify Firestore documents under lock. You have to perform all reads * before before you perform any write. * - * Transaction can be performed as read-only or read-write transactions. By + * Transactions can be performed as read-only or read-write transactions. By * default, transactions are executed in read-write mode. * * A read-write transaction obtains a pessimistic lock on all documents that * are read during the transaction. These locks block other transactions, * batched writes, and other non-transactional writes from changing that - * document. A transaction releases its document locks at commit time or if - * it fails for any reason. + * document. Any writes in a read-write transactions are committed once + * 'updateFunction' resolves, which also releases all locks. * - * Read-only transaction do not lock documents. They can be used to read - * documents at a consistent snapshot in time, which may be up to 60 seconds - * in the past. + * If a read-write transaction fails with contention, the transaction is + * retried up to five times. The `updateFunction` is invoked once for each + * attempt. * - * Transactions are committed once 'updateFunction' resolves. If a - * transaction fails with contention, the transaction is retried up to five - * times. The `updateFunction` is invoked once for each attempt. + * Read-only transactions do not lock documents. They can be used to read + * documents at a consistent snapshot in time, which may be up to 60 seconds + * in the past. Read-only transactions are not retried. * * Transactions time out after 60 seconds if no documents are read. * Transactions that are not committed within than 270 seconds are also From 63dc44b4c9e290c408af9c9ea541c2e8c4473a1b Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt <mrschmidt@google.com> Date: Mon, 28 Jun 2021 19:22:53 -0600 Subject: [PATCH 3/3] Lint --- dev/src/index.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index c2efd9c9d..5d90c452f 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -1063,7 +1063,8 @@ export class Firestore implements firestore.Firestore { {optional: true, minValue: 1} ); - maxAttempts = transactionOptions.maxAttempts || DEFAULT_MAX_TRANSACTION_ATTEMPTS; + maxAttempts = + transactionOptions.maxAttempts || DEFAULT_MAX_TRANSACTION_ATTEMPTS; } }