Improved handling of reads after writes in transactions if the get is… (#7715)

* Improved handling of reads after writes in transactions if the get is not awaited or returned.

Author: MarkDuckworth

packages/firestore/src/core/transaction.ts

@@ -48,7 +48,7 @@ export class Transaction {
    * A deferred usage error that occurred previously in this transaction that
    * will cause the transaction to fail once it actually commits.
    */
-  private lastWriteError: FirestoreError | null = null;
+  private lastTransactionError: FirestoreError | null = null;
 
   /**
    * Set of documents that have been written in the transaction.
@@ -64,10 +64,11 @@ export class Transaction {
     this.ensureCommitNotCalled();
 
     if (this.mutations.length > 0) {
-      throw new FirestoreError(
+      this.lastTransactionError = new FirestoreError(
         Code.INVALID_ARGUMENT,
         'Firestore transactions require all reads to be executed before all writes.'
       );
+      throw this.lastTransactionError;
     }
     const docs = await invokeBatchGetDocumentsRpc(this.datastore, keys);
     docs.forEach(doc => this.recordVersion(doc));
@@ -83,7 +84,7 @@ export class Transaction {
     try {
       this.write(data.toMutation(key, this.preconditionForUpdate(key)));
     } catch (e) {
-      this.lastWriteError = e as FirestoreError | null;
+      this.lastTransactionError = e as FirestoreError | null;
     }
     this.writtenDocs.add(key.toString());
   }
@@ -96,8 +97,8 @@ export class Transaction {
   async commit(): Promise<void> {
     this.ensureCommitNotCalled();
 
-    if (this.lastWriteError) {
-      throw this.lastWriteError;
+    if (this.lastTransactionError) {
+      throw this.lastTransactionError;
     }
     const unwritten = this.readVersions;
     // For each mutation, note that the doc was written.

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

@@ -506,10 +506,11 @@ apiDescribe('Database transactions', persistence => {
     });
   });
 
-  it('cannot read after writing', () => {
-    return withTestDb(persistence, db => {
-      return runTransaction(db, transaction => {
-        const docRef = doc(collection(db, 'anything'));
+  it('cannot read after writing and does not commit', () => {
+    return withTestDb(persistence, async db => {
+      const docRef = doc(collection(db, '00000-anything'));
+      await setDoc(docRef, { foo: 'baz' });
+      await runTransaction(db, async transaction => {
         transaction.set(docRef, { foo: 'bar' });
         return transaction.get(docRef);
       })
@@ -523,6 +524,41 @@ apiDescribe('Database transactions', persistence => {
             'Firestore transactions require all reads to be executed'
           );
         });
+
+      const postSnap = await getDoc(docRef);
+      expect(postSnap.get('foo')).to.equal('baz');
+    });
+  });
+
+  it('cannot read after writing and does not commit, even if the user transaction does not bubble up the error', () => {
+    return withTestDb(persistence, async db => {
+      const docRef = doc(collection(db, '00000-anything'));
+      await setDoc(docRef, { foo: 'baz' });
+      await runTransaction(db, async transaction => {
+        transaction.set(docRef, { foo: 'bar' });
+
+        // The following statement `transaction.get(...)` is problematic because
+        // it occurs after `transaction.set(...)`. In previous versions of the
+        // SDK this un-awaited `transaction.get(...)` failed but the transaction
+        // still committed successfully. This regression test ensures that the
+        // commit will fail even if the code does not await
+        // `transaction.get(...)`.
+        // eslint-disable-next-line
+        transaction.get(docRef);
+      })
+        .then(() => {
+          expect.fail('transaction should fail');
+        })
+        .catch((err: FirestoreError) => {
+          expect(err).to.exist;
+          expect(err.code).to.equal('invalid-argument');
+          expect(err.message).to.contain(
+            'Firestore transactions require all reads to be executed'
+          );
+        });
+
+      const postSnap = await getDoc(docRef);
+      expect(postSnap.get('foo')).to.equal('baz');
     });
   });