Add verify support (#2436)

Author: Brian Chen

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<DocumentKey> = 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;
   }

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)
+    );
+  }
+}

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;

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

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));
     }

packages/firestore/test/integration/api/transactions.test.ts

@@ -205,45 +205,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);
@@ -484,54 +445,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);
+        });
     });
   });
 
@@ -621,32 +568,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');
+        });
     });
   });
 

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', () => {