diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts index 06c30f4cf83..a623170db4d 100644 --- a/packages/firestore/src/core/transaction.ts +++ b/packages/firestore/src/core/transaction.ts @@ -20,7 +20,12 @@ import { documentVersionMap } from '../model/collections'; import { Document, NoDocument, MaybeDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; -import { DeleteMutation, Mutation, Precondition } from '../model/mutation'; +import { + DeleteMutation, + Mutation, + Precondition, + VerifyMutation +} from '../model/mutation'; import { Datastore } from '../remote/datastore'; import { fail, assert } from '../util/assert'; import { Code, FirestoreError } from '../util/error'; @@ -46,7 +51,7 @@ export class Transaction { * Set of documents that have been written in the transaction. * * When there's more than one write to the same key in a transaction, any - * writes after hte first are handled differently. + * writes after the first are handled differently. */ private writtenDocs: Set = new Set(); @@ -102,12 +107,11 @@ export class Transaction { this.mutations.forEach(mutation => { unwritten = unwritten.remove(mutation.key); }); - if (!unwritten.isEmpty()) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - 'Every document read in a transaction must also be written.' - ); - } + // For each document that was read but not written to, we want to perform + // a `verify` operation. + unwritten.forEach((key, _version) => { + this.mutations.push(new VerifyMutation(key, this.precondition(key))); + }); await this.datastore.commit(this.mutations); this.committed = true; } diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 4ba967abee1..661a381a69e 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -17,7 +17,7 @@ import { Timestamp } from '../api/timestamp'; import { SnapshotVersion } from '../core/snapshot_version'; -import { assert } from '../util/assert'; +import { assert, fail } from '../util/assert'; import * as misc from '../util/misc'; import { SortedSet } from '../util/sorted_set'; @@ -121,7 +121,8 @@ export enum MutationType { Set, Patch, Transform, - Delete + Delete, + Verify } /** @@ -187,7 +188,7 @@ export class Precondition { * A mutation describes a self-contained change to a document. Mutations can * create, replace, delete, and update subsets of documents. * - * Mutations not only act on the value of the document but also it version. + * Mutations not only act on the value of the document but also its version. * * For local mutations (mutations that haven't been committed yet), we preserve * the existing version for Set, Patch, and Transform mutations. For Delete @@ -283,7 +284,7 @@ export abstract class Mutation { * * The base value is a sparse object that consists of only the document * fields for which this mutation contains a non-idempotent transformation - * (e.g. a numeric increment). The provided alue guarantees consistent + * (e.g. a numeric increment). The provided value guarantees consistent * behavior for non-idempotent transforms and allow us to return the same * latency-compensated value even if the backend has already applied the * mutation. The base value is null for idempotent mutations, as they can be @@ -817,3 +818,45 @@ export class DeleteMutation extends Mutation { ); } } + +/** + * A mutation that verifies the existence of the document at the given key with + * the provided precondition. + * + * The `verify` operation is only used in Transactions, and this class serves + * primarily to facilitate serialization into protos. + */ +export class VerifyMutation extends Mutation { + constructor(readonly key: DocumentKey, readonly precondition: Precondition) { + super(); + } + + readonly type: MutationType = MutationType.Verify; + + applyToRemoteDocument( + maybeDoc: MaybeDocument | null, + mutationResult: MutationResult + ): MaybeDocument { + fail('VerifyMutation should only be used in Transactions.'); + } + + applyToLocalView( + maybeDoc: MaybeDocument | null, + baseDoc: MaybeDocument | null, + localWriteTime: Timestamp + ): MaybeDocument | null { + fail('VerifyMutation should only be used in Transactions.'); + } + + extractBaseValue(maybeDoc: MaybeDocument | null): null { + fail('VerifyMutation should only be used in Transactions.'); + } + + isEqual(other: Mutation): boolean { + return ( + other instanceof VerifyMutation && + this.key.isEqual(other.key) && + this.precondition.isEqual(other.precondition) + ); + } +} diff --git a/packages/firestore/src/protos/firestore_proto_api.d.ts b/packages/firestore/src/protos/firestore_proto_api.d.ts index 70b9efd7586..895dc1f3676 100644 --- a/packages/firestore/src/protos/firestore_proto_api.d.ts +++ b/packages/firestore/src/protos/firestore_proto_api.d.ts @@ -374,6 +374,7 @@ export declare namespace firestoreV1ApiClientInterfaces { interface Write { update?: Document; delete?: string; + verify?: string; transform?: DocumentTransform; updateMask?: DocumentMask; currentDocument?: Precondition; diff --git a/packages/firestore/src/protos/google/firestore/v1/write.proto b/packages/firestore/src/protos/google/firestore/v1/write.proto index bffd0790997..f838cdc497c 100644 --- a/packages/firestore/src/protos/google/firestore/v1/write.proto +++ b/packages/firestore/src/protos/google/firestore/v1/write.proto @@ -42,6 +42,11 @@ message Write { // `projects/{project_id}/databases/{database_id}/documents/{document_path}`. string delete = 2; + // The name of a document on which to verify the `current_document` + // precondition. + // This only requires read access to the document. + string verify = 5; + // Applies a tranformation to a document. // At most one `transform` per document is allowed in a given request. // An `update` cannot follow a `transform` on the same document in a given diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 5d86d7e0485..40e1ddc88c6 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -45,7 +45,8 @@ import { PatchMutation, Precondition, SetMutation, - TransformMutation + TransformMutation, + VerifyMutation } from '../model/mutation'; import { FieldPath, ResourcePath } from '../model/path'; import * as api from '../protos/firestore_proto_api'; @@ -844,6 +845,10 @@ export class JsonProtoSerializer { ) } }; + } else if (mutation instanceof VerifyMutation) { + result = { + verify: this.toName(mutation.key) + }; } else { return fail('Unknown mutation type ' + mutation.type); } @@ -883,6 +888,9 @@ export class JsonProtoSerializer { 'Transforms only support precondition "exists == true"' ); return new TransformMutation(key, fieldTransforms); + } else if (proto.verify) { + const key = this.fromName(proto.verify); + return new VerifyMutation(key, precondition); } else { return fail('unknown mutation proto: ' + JSON.stringify(proto)); } diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts index 35c743b3b2b..a20e860d617 100644 --- a/packages/firestore/test/integration/api/transactions.test.ts +++ b/packages/firestore/test/integration/api/transactions.test.ts @@ -206,45 +206,6 @@ apiDescribe('Database transactions', (persistence: boolean) => { } } - // TODO(klimt): Test that transactions don't see latency compensation - // changes, using the other kind of integration test. - // We currently require every document read to also be written. - it('get documents', () => { - return integrationHelpers.withTestDb(persistence, db => { - const doc = db.collection('spaces').doc(); - return doc - .set({ - foo: 1, - desc: 'Stuff related to Firestore project...', - owner: 'Jonny' - }) - .then(() => { - return ( - db - .runTransaction(transaction => { - return transaction.get(doc); - }) - // We currently require every document read to also be - // written. - // TODO(b/34879758): Add this check back once we drop that. - // .then((snapshot) => { - // expect(snapshot).to.exist; - // expect(snapshot.data()['owner']).to.equal('Jonny'); - // }); - .then(() => expect.fail('transaction should fail')) - .catch((err: firestore.FirestoreError) => { - expect(err).to.exist; - expect(err.code).to.equal('invalid-argument'); - expect(err.message).to.contain( - 'Every document read in a transaction must also be' + - ' written' - ); - }) - ); - }); - }); - }); - it('runs transactions after getting existing document', async () => { return integrationHelpers.withTestDb(persistence, async db => { const tt = new TransactionTester(db); @@ -604,54 +565,40 @@ apiDescribe('Database transactions', (persistence: boolean) => { }); }); - it('handle reading one doc and writing another', () => { + it('retry when a document that was read without being written changes', () => { return integrationHelpers.withTestDb(persistence, db => { const doc1 = db.collection('counters').doc(); const doc2 = db.collection('counters').doc(); - // let tries = 0; - return ( - doc1 - .set({ - count: 15 - }) - .then(() => { - return db.runTransaction(transaction => { - // ++tries; - - // Get the first doc. - return ( - transaction - .get(doc1) - // Do a write outside of the transaction. The first time the - // transaction is tried, this will bump the version, which - // will cause the write to doc2 to fail. The second time, it - // will be a no-op and not bump the version. - .then(() => doc1.set({ count: 1234 })) - // Now try to update the other doc from within the - // transaction. - // This should fail once, because we read 15 earlier. - .then(() => transaction.set(doc2, { count: 16 })) - ); - }); - }) - // We currently require every document read to also be written. - // TODO(b/34879758): Add this check back once we drop that. - // .then(() => doc1.get()) - // .then(snapshot => { - // expect(tries).to.equal(2); - // expect(snapshot.data()['count']).to.equal(1234); - // return doc2.get(); - // }) - // .then(snapshot => expect(snapshot.data()['count']).to.equal(16)); - .then(() => expect.fail('transaction should fail')) - .catch((err: firestore.FirestoreError) => { - expect(err).to.exist; - expect(err.code).to.equal('invalid-argument'); - expect(err.message).to.contain( - 'Every document read in a transaction must also be ' + 'written' + let tries = 0; + return doc1 + .set({ + count: 15 + }) + .then(() => { + return db.runTransaction(transaction => { + ++tries; + + // Get the first doc. + return ( + transaction + .get(doc1) + // Do a write outside of the transaction. The first time the + // transaction is tried, this will bump the version, which + // will cause the write to doc2 to fail. The second time, it + // will be a no-op and not bump the version. + .then(() => doc1.set({ count: 1234 })) + // Now try to update the other doc from within the + // transaction. + // This should fail once, because we read 15 earlier. + .then(() => transaction.set(doc2, { count: 16 })) ); - }) - ); + }); + }) + .then(async () => { + const snapshot = await doc1.get(); + expect(tries).to.equal(2); + expect(snapshot.data()!['count']).to.equal(1234); + }); }); }); @@ -781,32 +728,22 @@ apiDescribe('Database transactions', (persistence: boolean) => { } }); - it('cannot have a get without mutations', () => { + it('can have gets without mutations', () => { return integrationHelpers.withTestDb(persistence, db => { const docRef = db.collection('foo').doc(); - return ( - docRef - .set({ foo: 'bar' }) - .then(() => { - return db.runTransaction(txn => { - return txn.get(docRef); - }); - }) - // We currently require every document read to also be written. - // TODO(b/34879758): Add this check back once we drop that. - // .then((snapshot) => { - // expect(snapshot).to.exist; - // expect(snapshot.data()['foo']).to.equal('bar'); - // }); - .then(() => expect.fail('transaction should fail')) - .catch((err: firestore.FirestoreError) => { - expect(err).to.exist; - expect(err.code).to.equal('invalid-argument'); - expect(err.message).to.contain( - 'Every document read in a transaction must also be ' + 'written' - ); - }) - ); + const docRef2 = db.collection('foo').doc(); + return docRef + .set({ foo: 'bar' }) + .then(() => { + return db.runTransaction(async txn => { + await txn.get(docRef2); + return txn.get(docRef); + }); + }) + .then(snapshot => { + expect(snapshot).to.exist; + expect(snapshot.data()!['foo']).to.equal('bar'); + }); }); }); diff --git a/packages/firestore/test/unit/remote/node/serializer.test.ts b/packages/firestore/test/unit/remote/node/serializer.test.ts index bdf8965048c..ba009a08473 100644 --- a/packages/firestore/test/unit/remote/node/serializer.test.ts +++ b/packages/firestore/test/unit/remote/node/serializer.test.ts @@ -44,7 +44,8 @@ import { FieldMask, Mutation, Precondition, - SetMutation + SetMutation, + VerifyMutation } from '../../../../src/model/mutation'; import { DOCUMENT_KEY_NAME, FieldPath } from '../../../../src/model/path'; import { @@ -696,6 +697,20 @@ describe('Serializer', () => { }; verifyMutation(mutation, proto); }); + + it('VerifyMutation', () => { + const mutation = new VerifyMutation( + key('foo/bar'), + Precondition.updateTime(version(4)) + ); + const proto = { + verify: s.toName(mutation.key), + currentDocument: { + updateTime: { seconds: '0', nanos: 4000 } + } + }; + verifyMutation(mutation, proto); + }); }); it('toDocument() / fromDocument', () => {