From 2450dc96d9f37b47e65e603f889a060497e9e71a Mon Sep 17 00:00:00 2001
From: Brian Chen <chenbrian@google.com>
Date: Thu, 5 Dec 2019 10:41:47 -0800
Subject: [PATCH 1/3] WIP need plumb through

---
 packages/firestore/src/core/transaction.ts    | 24 +++--
 packages/firestore/src/model/mutation.ts      | 89 ++++++++++++++-----
 .../src/protos/firestore_proto_api.d.ts       |  1 +
 .../protos/google/firestore/v1/write.proto    |  5 ++
 packages/firestore/src/util/log.ts            |  4 +-
 .../test/integration/api/transactions.test.ts | 67 ++++----------
 6 files changed, 105 insertions(+), 85 deletions(-)

diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts
index 06c30f4cf83..731db5bec00 100644
--- a/packages/firestore/src/core/transaction.ts
+++ b/packages/firestore/src/core/transaction.ts
@@ -20,7 +20,7 @@ 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 +46,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();
 
@@ -100,14 +100,22 @@ export class Transaction {
     let unwritten = this.readVersions;
     // For each mutation, note that the doc was written.
     this.mutations.forEach(mutation => {
+      // console.log('readVersion', this.readVersions);
+      // console.log('mutation', this.mutations);
       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. We append to the start of the transaction because
+    // verify calls are cheaper than writes.
+    unwritten.forEach((key, _version) => {
+      this.mutations.unshift(new VerifyMutation(key, Precondition.NONE));
+    });
+    // if (!unwritten.isEmpty()) {
+    //   throw new FirestoreError(
+    //     Code.INVALID_ARGUMENT,
+    //     'Every document read in a transaction must also be written.'
+    //   );
+    // }
     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..8e664b3af40 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
 }
 
 /**
@@ -586,27 +587,8 @@ export class TransformMutation extends Mutation {
     baseDoc: MaybeDocument | null,
     localWriteTime: Timestamp
   ): MaybeDocument | null {
-    this.verifyKeyMatches(maybeDoc);
-
-    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
-    }
-
-    const doc = this.requireDocument(maybeDoc);
-    const transformResults = this.localTransformResults(
-      localWriteTime,
-      maybeDoc,
-      baseDoc
-    );
-    const newData = this.transformObject(doc.data(), transformResults);
-    return new Document(
-      this.key,
-      doc.version,
-      {
-        hasLocalMutations: true
-      },
-      newData
-    );
+    fail('This operation is not supported');
+    return null;
   }
 
   extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null {
@@ -817,3 +799,64 @@ export class DeleteMutation extends Mutation {
     );
   }
 }
+
+/** A mutation that verifies the existence of the document at the given key. */
+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 {
+    this.verifyKeyMatches(maybeDoc);
+
+    assert(
+      mutationResult.transformResults == null,
+      'Transform results received by DeleteMutation.'
+    );
+
+    // Unlike applyToLocalView, if we're applying a mutation to a remote
+    // document the server has accepted the mutation so the precondition must
+    // have held.
+
+    return new NoDocument(this.key, mutationResult.version, {
+      hasCommittedMutations: true
+    });
+  }
+
+  applyToLocalView(
+    maybeDoc: MaybeDocument | null,
+    baseDoc: MaybeDocument | null,
+    localWriteTime: Timestamp
+  ): MaybeDocument | null {
+    this.verifyKeyMatches(maybeDoc);
+
+    if (!this.precondition.isValidFor(maybeDoc)) {
+      return maybeDoc;
+    }
+
+    if (maybeDoc) {
+      assert(
+        maybeDoc.key.isEqual(this.key),
+        'Can only apply mutation to document with same key'
+      );
+    }
+    return new NoDocument(this.key, SnapshotVersion.forDeletedDoc());
+  }
+
+  extractBaseValue(maybeDoc: MaybeDocument | null): null {
+    return null;
+  }
+
+  isEqual(other: Mutation): boolean {
+    return (
+      other instanceof DeleteMutation &&
+      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..32f0e20e90e 100644
--- a/packages/firestore/src/protos/google/firestore/v1/write.proto
+++ b/packages/firestore/src/protos/google/firestore/v1/write.proto
@@ -41,6 +41,11 @@ message Write {
     // A document name to delete. In the format:
     // `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.
diff --git a/packages/firestore/src/util/log.ts b/packages/firestore/src/util/log.ts
index 124a9a73735..ea2306f42d5 100644
--- a/packages/firestore/src/util/log.ts
+++ b/packages/firestore/src/util/log.ts
@@ -59,10 +59,10 @@ export function setLogLevel(newLevel: LogLevel): void {
 }
 
 export function debug(tag: string, msg: string, ...obj: unknown[]): void {
-  if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
+  // if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
     const args = obj.map(argToString);
     logClient.debug(`Firestore (${SDK_VERSION}) [${tag}]: ${msg}`, ...args);
-  }
+  // }
 }
 
 export function error(msg: string, ...obj: unknown[]): void {
diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts
index a1cd6c15c85..5c0dbc90b8d 100644
--- a/packages/firestore/test/integration/api/transactions.test.ts
+++ b/packages/firestore/test/integration/api/transactions.test.ts
@@ -206,10 +206,7 @@ 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', () => {
+  it('gets documents in a transaction', () => {
     return integrationHelpers.withTestDb(persistence, db => {
       const doc = db.collection('spaces').doc();
       return doc
@@ -224,21 +221,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
               .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'
-                );
+             .then((snapshot) => {
+                expect(snapshot).to.exist;
+                expect(snapshot.data()!['owner']).to.equal('Jonny');
               })
           );
         });
@@ -604,11 +589,11 @@ apiDescribe('Database transactions', (persistence: boolean) => {
     });
   });
 
-  it('handle reading one doc and writing another', () => {
+  it.only('handles reading one doc and writing another', () => {
     return integrationHelpers.withTestDb(persistence, db => {
       const doc1 = db.collection('counters').doc();
       const doc2 = db.collection('counters').doc();
-      // let tries = 0;
+      let tries = 0;
       return (
         doc1
           .set({
@@ -616,7 +601,7 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           })
           .then(() => {
             return db.runTransaction(transaction => {
-              // ++tries;
+              ++tries;
 
               // Get the first doc.
               return (
@@ -634,22 +619,10 @@ apiDescribe('Database transactions', (persistence: boolean) => {
               );
             });
           })
-          // 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'
-            );
+          .then(async () => {
+            const snapshot = await doc1.get();
+            expect(tries).to.equal(2);
+            expect(snapshot.data()!['count']).to.equal(1234);
           })
       );
     });
@@ -781,7 +754,7 @@ apiDescribe('Database transactions', (persistence: boolean) => {
     }
   });
 
-  it('cannot have a get without mutations', () => {
+  it('can have a get without mutations', () => {
     return integrationHelpers.withTestDb(persistence, db => {
       const docRef = db.collection('foo').doc();
       return (
@@ -792,19 +765,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
               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'
-            );
+          .then((snapshot) => {
+            expect(snapshot).to.exist;
+            expect(snapshot.data()!['foo']).to.equal('bar');
           })
       );
     });

From 7635f875e36134623994005c582fb48b306e0ef4 Mon Sep 17 00:00:00 2001
From: Brian Chen <chenbrian@google.com>
Date: Wed, 11 Dec 2019 17:03:30 -0800
Subject: [PATCH 2/3] first draft with e2e working

---
 packages/firestore/src/core/transaction.ts    |  20 ++-
 packages/firestore/src/model/mutation.ts      |  68 +++++------
 .../protos/google/firestore/v1/write.proto    |   2 +-
 packages/firestore/src/remote/serializer.ts   |  10 +-
 packages/firestore/src/util/log.ts            |   4 +-
 .../test/integration/api/transactions.test.ts | 114 +++++++-----------
 .../test/unit/remote/node/serializer.test.ts  |  17 ++-
 7 files changed, 114 insertions(+), 121 deletions(-)

diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts
index 731db5bec00..20f48781e0c 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, VerifyMutation } 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';
@@ -100,22 +105,13 @@ export class Transaction {
     let unwritten = this.readVersions;
     // For each mutation, note that the doc was written.
     this.mutations.forEach(mutation => {
-      // console.log('readVersion', this.readVersions);
-      // console.log('mutation', this.mutations);
       unwritten = unwritten.remove(mutation.key);
     });
     // For each document that was read but not written to, we want to perform
-    // a `verify` operation. We append to the start of the transaction because
-    // verify calls are cheaper than writes.
+    // a `verify` operation.
     unwritten.forEach((key, _version) => {
-      this.mutations.unshift(new VerifyMutation(key, Precondition.NONE));
+      this.mutations.unshift(new VerifyMutation(key, this.precondition(key)));
     });
-    // if (!unwritten.isEmpty()) {
-    //   throw new FirestoreError(
-    //     Code.INVALID_ARGUMENT,
-    //     'Every document read in a transaction must also be written.'
-    //   );
-    // }
     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 8e664b3af40..661a381a69e 100644
--- a/packages/firestore/src/model/mutation.ts
+++ b/packages/firestore/src/model/mutation.ts
@@ -188,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
@@ -284,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
@@ -587,8 +587,27 @@ export class TransformMutation extends Mutation {
     baseDoc: MaybeDocument | null,
     localWriteTime: Timestamp
   ): MaybeDocument | null {
-    fail('This operation is not supported');
-    return null;
+    this.verifyKeyMatches(maybeDoc);
+
+    if (!this.precondition.isValidFor(maybeDoc)) {
+      return maybeDoc;
+    }
+
+    const doc = this.requireDocument(maybeDoc);
+    const transformResults = this.localTransformResults(
+      localWriteTime,
+      maybeDoc,
+      baseDoc
+    );
+    const newData = this.transformObject(doc.data(), transformResults);
+    return new Document(
+      this.key,
+      doc.version,
+      {
+        hasLocalMutations: true
+      },
+      newData
+    );
   }
 
   extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null {
@@ -800,7 +819,13 @@ export class DeleteMutation extends Mutation {
   }
 }
 
-/** A mutation that verifies the existence of the document at the given key. */
+/**
+ * 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();
@@ -812,20 +837,7 @@ export class VerifyMutation extends Mutation {
     maybeDoc: MaybeDocument | null,
     mutationResult: MutationResult
   ): MaybeDocument {
-    this.verifyKeyMatches(maybeDoc);
-
-    assert(
-      mutationResult.transformResults == null,
-      'Transform results received by DeleteMutation.'
-    );
-
-    // Unlike applyToLocalView, if we're applying a mutation to a remote
-    // document the server has accepted the mutation so the precondition must
-    // have held.
-
-    return new NoDocument(this.key, mutationResult.version, {
-      hasCommittedMutations: true
-    });
+    fail('VerifyMutation should only be used in Transactions.');
   }
 
   applyToLocalView(
@@ -833,28 +845,16 @@ export class VerifyMutation extends Mutation {
     baseDoc: MaybeDocument | null,
     localWriteTime: Timestamp
   ): MaybeDocument | null {
-    this.verifyKeyMatches(maybeDoc);
-
-    if (!this.precondition.isValidFor(maybeDoc)) {
-      return maybeDoc;
-    }
-
-    if (maybeDoc) {
-      assert(
-        maybeDoc.key.isEqual(this.key),
-        'Can only apply mutation to document with same key'
-      );
-    }
-    return new NoDocument(this.key, SnapshotVersion.forDeletedDoc());
+    fail('VerifyMutation should only be used in Transactions.');
   }
 
   extractBaseValue(maybeDoc: MaybeDocument | null): null {
-    return null;
+    fail('VerifyMutation should only be used in Transactions.');
   }
 
   isEqual(other: Mutation): boolean {
     return (
-      other instanceof DeleteMutation &&
+      other instanceof VerifyMutation &&
       this.key.isEqual(other.key) &&
       this.precondition.isEqual(other.precondition)
     );
diff --git a/packages/firestore/src/protos/google/firestore/v1/write.proto b/packages/firestore/src/protos/google/firestore/v1/write.proto
index 32f0e20e90e..f838cdc497c 100644
--- a/packages/firestore/src/protos/google/firestore/v1/write.proto
+++ b/packages/firestore/src/protos/google/firestore/v1/write.proto
@@ -41,7 +41,7 @@ message Write {
     // A document name to delete. In the format:
     // `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.
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/src/util/log.ts b/packages/firestore/src/util/log.ts
index ea2306f42d5..124a9a73735 100644
--- a/packages/firestore/src/util/log.ts
+++ b/packages/firestore/src/util/log.ts
@@ -59,10 +59,10 @@ export function setLogLevel(newLevel: LogLevel): void {
 }
 
 export function debug(tag: string, msg: string, ...obj: unknown[]): void {
-  // if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
+  if (logClient.logLevel <= FirebaseLogLevel.DEBUG) {
     const args = obj.map(argToString);
     logClient.debug(`Firestore (${SDK_VERSION}) [${tag}]: ${msg}`, ...args);
-  // }
+  }
 }
 
 export function error(msg: string, ...obj: unknown[]): void {
diff --git a/packages/firestore/test/integration/api/transactions.test.ts b/packages/firestore/test/integration/api/transactions.test.ts
index 19ed1b33aa3..a20e860d617 100644
--- a/packages/firestore/test/integration/api/transactions.test.ts
+++ b/packages/firestore/test/integration/api/transactions.test.ts
@@ -206,30 +206,6 @@ apiDescribe('Database transactions', (persistence: boolean) => {
     }
   }
 
-  it('gets documents in a transaction', () => {
-    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);
-              })
-             .then((snapshot) => {
-                expect(snapshot).to.exist;
-                expect(snapshot.data()!['owner']).to.equal('Jonny');
-              })
-          );
-        });
-    });
-  });
-
   it('runs transactions after getting existing document', async () => {
     return integrationHelpers.withTestDb(persistence, async db => {
       const tt = new TransactionTester(db);
@@ -589,42 +565,40 @@ apiDescribe('Database transactions', (persistence: boolean) => {
     });
   });
 
-  it.only('handles 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 }))
-              );
-            });
-          })
-          .then(async () => {
-            const snapshot = await doc1.get();
-            expect(tries).to.equal(2);
-            expect(snapshot.data()!['count']).to.equal(1234);
-          })
-      );
+      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);
+        });
     });
   });
 
@@ -754,22 +728,22 @@ apiDescribe('Database transactions', (persistence: boolean) => {
     }
   });
 
-  it('can 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);
-            });
-          })
-          .then((snapshot) => {
-            expect(snapshot).to.exist;
-            expect(snapshot.data()!['foo']).to.equal('bar');
-          })
-      );
+      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', () => {

From 788acbe7a6683f0ccab31189bed0ad531d07c8c4 Mon Sep 17 00:00:00 2001
From: Brian Chen <chenbrian@google.com>
Date: Fri, 20 Dec 2019 09:42:44 -0800
Subject: [PATCH 3/3] use push instead of unshift

---
 packages/firestore/src/core/transaction.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/packages/firestore/src/core/transaction.ts b/packages/firestore/src/core/transaction.ts
index 20f48781e0c..a623170db4d 100644
--- a/packages/firestore/src/core/transaction.ts
+++ b/packages/firestore/src/core/transaction.ts
@@ -110,7 +110,7 @@ export class Transaction {
     // For each document that was read but not written to, we want to perform
     // a `verify` operation.
     unwritten.forEach((key, _version) => {
-      this.mutations.unshift(new VerifyMutation(key, this.precondition(key)));
+      this.mutations.push(new VerifyMutation(key, this.precondition(key)));
     });
     await this.datastore.commit(this.mutations);
     this.committed = true;