Standardize Firestore transaction errors

packages/firestore/src/core/transaction.ts

@@ -36,6 +36,12 @@ export class Transaction {
   private mutations: Mutation[] = [];
   private committed = false;
 
+  /**
+   * An error that may have occurred as a consequence of a write.
+   * If set, commit() will throw the stored error.
+   */
+  private lastWriteError: FirestoreError;
+
   constructor(private datastore: Datastore) {}
 
   private recordVersion(doc: MaybeDocument): void {
@@ -66,13 +72,15 @@ export class Transaction {
 
   lookup(keys: DocumentKey[]): Promise<MaybeDocument[]> {
     if (this.committed) {
-      return Promise.reject<MaybeDocument[]>(
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
         'Transaction has already completed.'
       );
     }
     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 +96,6 @@ export class Transaction {
   }
 
   private write(mutations: Mutation[]): void {
-    if (this.committed) {
-      throw new FirestoreError(
-        Code.FAILED_PRECONDITION,
-        'Transaction has already completed.'
-      );
-    }
     this.mutations = this.mutations.concat(mutations);
   }
 
@@ -118,7 +120,7 @@ export class Transaction {
     if (version && version.isEqual(SnapshotVersion.forDeletedDoc())) {
       // The document doesn't exist, so fail 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 +138,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,14 +156,24 @@ export class Transaction {
   }
 
   commit(): Promise<void> {
+    if (this.committed) {
+      throw new FirestoreError(
+        Code.INVALID_ARGUMENT,
+        'Transaction has already completed.'
+      );
+    }
+    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(() => {

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

@@ -117,11 +117,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 +137,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 +164,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'
             );
@@ -447,7 +439,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 +461,52 @@ 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 => {
+        let counter = 0;
+        return db
+          .runTransaction(transaction => {
+            counter += 1;
+            const doc = db.collection('nonexistent' + counter).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: counter }))
+                // 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: 1234 }))
+            );
+          })
+          .then(() => expect.fail('transaction should fail'))
+          .catch(err => {
+            expect(err).to.exist;
+            expect((err as firestore.FirestoreError).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 +557,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'
             );