Standardize Firestore transaction errors (#1937)

Author: Brian Chen

packages/firestore/src/core/transaction.ts

@@ -22,7 +22,7 @@ import { Document, NoDocument, MaybeDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
 import { Datastore } from '../remote/datastore';
-import { fail } from '../util/assert';
+import { fail, assert } from '../util/assert';
 import { Code, FirestoreError } from '../util/error';
 import { SnapshotVersion } from './snapshot_version';
 
@@ -36,6 +36,12 @@ export class Transaction {
   private mutations: Mutation[] = [];
   private committed = false;
 
+  /**
+   * A deferred usage error that occurred previously in this transaction that
+   * will cause the transaction to fail once it actually commits.
+   */
+  private lastWriteError: FirestoreError;
+
   constructor(private datastore: Datastore) {}
 
   private recordVersion(doc: MaybeDocument): void {
@@ -65,14 +71,12 @@ export class Transaction {
   }
 
   lookup(keys: DocumentKey[]): Promise<MaybeDocument[]> {
-    if (this.committed) {
-      return Promise.reject<MaybeDocument[]>(
-        'Transaction has already completed.'
-      );
-    }
+    this.ensureCommitNotCalled();
+
     if (this.mutations.length > 0) {
-      return Promise.reject<MaybeDocument[]>(
-        'Transactions lookups are invalid after writes.'
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        'Firestore transactions require all reads to be executed before all writes.'
       );
     }
     return this.datastore.lookup(keys).then(docs => {
@@ -88,12 +92,7 @@ export class Transaction {
   }
 
   private write(mutations: Mutation[]): void {
-    if (this.committed) {
-      throw new FirestoreError(
-        Code.FAILED_PRECONDITION,
-        'Transaction has already completed.'
-      );
-    }
+    this.ensureCommitNotCalled();
     this.mutations = this.mutations.concat(mutations);
   }
 
@@ -117,8 +116,17 @@ export class Transaction {
     const version = this.readVersions.get(key);
     if (version && version.isEqual(SnapshotVersion.forDeletedDoc())) {
       // The document doesn't exist, so fail the transaction.
+
+      // This has to be validated locally because you can't send a precondition
+      // that a document does not exist without changing the semantics of the
+      // backend write to be an insert. This is the reverse of what we want,
+      // since we want to assert that the document doesn't exist but then send
+      // the update and have it fail. Since we can't express that to the backend,
+      // we have to validate locally.
+
+      // Note: this can change once we can send separate verify writes in the transaction.
       throw new FirestoreError(
-        Code.FAILED_PRECONDITION,
+        Code.INVALID_ARGUMENT,
         "Can't update a document that doesn't exist."
       );
     } else if (version) {
@@ -136,7 +144,11 @@ export class Transaction {
   }
 
   update(key: DocumentKey, data: ParsedUpdateData): void {
-    this.write(data.toMutations(key, this.preconditionForUpdate(key)));
+    try {
+      this.write(data.toMutations(key, this.preconditionForUpdate(key)));
+    } catch (e) {
+      this.lastWriteError = e;
+    }
   }
 
   delete(key: DocumentKey): void {
@@ -150,18 +162,31 @@ export class Transaction {
   }
 
   commit(): Promise<void> {
+    this.ensureCommitNotCalled();
+
+    if (this.lastWriteError) {
+      throw this.lastWriteError;
+    }
     let unwritten = this.readVersions;
     // For each mutation, note that the doc was written.
     this.mutations.forEach(mutation => {
       unwritten = unwritten.remove(mutation.key);
     });
     if (!unwritten.isEmpty()) {
-      return Promise.reject(
-        Error('Every document read in a transaction must also be written.')
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        'Every document read in a transaction must also be written.'
       );
     }
     return this.datastore.commit(this.mutations).then(() => {
       this.committed = true;
     });
   }
+
+  private ensureCommitNotCalled(): void {
+    assert(
+      !this.committed,
+      'A transaction object cannot be used after its update callback has been invoked.'
+    );
+  }
 }

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

@@ -50,8 +50,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
               //   expect(snapshot.data()['owner']).to.equal('Jonny');
               // });
               .then(() => expect.fail('transaction should fail'))
-              .catch(err => {
+              .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'
@@ -117,11 +118,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           });
         })
         .then(() => expect.fail('transaction should fail'))
-        .catch(err => {
+        .catch((err: firestore.FirestoreError) => {
           expect(err).to.exist;
-          expect(err.message).to.equal(
-            "Can't update a document that doesn't exist."
-          );
+          expect(err.code).to.equal('invalid-argument');
         });
     });
   });
@@ -139,16 +138,10 @@ apiDescribe('Database transactions', (persistence: boolean) => {
               transaction.update(docRef, { foo: 'new-bar' });
             });
           })
-          .then(() => expect.fail('transaction should fail'))
-          .catch(err => {
+          .catch((err: firestore.FirestoreError) => {
             expect(err).to.exist;
-            // TODO(b/65409268): This is likely the wrong error code
-            expect((err as firestore.FirestoreError).code).to.equal(
-              'failed-precondition'
-            );
-            expect(err.message).to.equal(
-              "Can't update a document that doesn't exist."
-            );
+            // This is the error surfaced by the backend.
+            expect(err.code).to.equal('invalid-argument');
           });
       });
     });
@@ -172,7 +165,7 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           .then(() => expect.fail('transaction should fail'))
           .catch(err => {
             expect(err).to.exist;
-            // TODO(b/65409268): This is likely the wrong error code
+            // This is the error surfaced by the backend.
             expect((err as firestore.FirestoreError).code).to.equal(
               'invalid-argument'
             );
@@ -412,8 +405,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           // })
           // .then(snapshot => expect(snapshot.data()['count']).to.equal(16));
           .then(() => expect.fail('transaction should fail'))
-          .catch(err => {
+          .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'
             );
@@ -447,7 +441,10 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           });
         })
         .then(() => expect.fail('transaction should fail'))
-        .catch(err => expect(err).to.exist)
+        .catch(err => {
+          expect(err).to.exist;
+          expect((err as firestore.FirestoreError).code).to.equal('aborted');
+        })
         .then(() => doc.get())
         .then(snapshot => {
           expect(snapshot.data()!['count']).to.equal(1234);
@@ -466,12 +463,48 @@ apiDescribe('Database transactions', (persistence: boolean) => {
         .then(() => {
           expect.fail('transaction should fail');
         })
-        .catch(err => {
+        .catch((err: firestore.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'
+          );
         });
     });
   });
 
+  it(
+    'cannot read non-existent document then update, even if ' +
+      'document is written after the read',
+    () => {
+      return integrationHelpers.withTestDb(persistence, db => {
+        return db
+          .runTransaction(transaction => {
+            const doc = db.collection('nonexistent').doc();
+            return (
+              transaction
+                // Get and update a document that doesn't exist so that the transaction fails.
+                .get(doc)
+                // Do a write outside of the transaction.
+                .then(() => doc.set({ count: 1234 }))
+                // Now try to update the doc from within the transaction. This
+                // should fail, because the document didn't exist at the start
+                // of the transaction.
+                .then(() => transaction.update(doc, { count: 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(
+              "Can't update a document that doesn't exist."
+            );
+          });
+      });
+    }
+  );
+
   describe('must return a promise:', () => {
     const noop = (): void => {
       /* -_- */
@@ -522,8 +555,9 @@ apiDescribe('Database transactions', (persistence: boolean) => {
           //   expect(snapshot.data()['foo']).to.equal('bar');
           // });
           .then(() => expect.fail('transaction should fail'))
-          .catch(err => {
+          .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'
             );