Throw last failed attempt's error in failed transactions (#3287)

Author: Brian Chen

Firestore/CHANGELOG.md

@@ -2,6 +2,8 @@
 - [fixed] Fixed an internal assertion that was triggered when an update
    with a `FieldValue.serverTimestamp()` and an update with a
   `FieldValue.increment()` were pending for the same document.
+- [changed] Failed transactions now return the failure from the last attempt,
+  instead of `ABORTED.`
 
 # 1.4.1
 - [fixed] Fixed certificate loading for non-CocoaPods builds that may not

Firestore/Example/Tests/Integration/API/FIRServerTimestampTests.mm

@@ -306,10 +306,7 @@ - (void)testServerTimestampsFailViaTransactionUpdateOnNonexistentDocument {
       completion:^(id result, NSError *error) {
         XCTAssertNotNil(error);
         XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-        // TODO(b/35201829): This should be NotFound, but right now we retry transactions on any
-        // error and so this turns into Aborted instead.
-        // TODO(mikelehen): Actually it's FailedPrecondition, unlike Android. What do we want???
-        XCTAssertEqual(error.code, FIRFirestoreErrorCodeFailedPrecondition);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeNotFound);
         [expectation fulfill];
       }];
   [self awaitExpectations];

Firestore/Example/Tests/Integration/FSTTransactionTests.mm

@@ -43,6 +43,7 @@ - (void)testGetDocuments {
         // We currently require every document read to also be written.
         // TODO(b/34879758): Fix this check once we drop that requirement.
         XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -115,9 +116,7 @@ - (void)testGetNonexistentDocumentThenFailPatch {
         XCTAssertNil(result);
         XCTAssertNotNil(error);
         XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-        // TODO(dimond): This is probably the wrong error code, but it's what we use today. We
-        // should update the code once the underlying error was fixed.
-        XCTAssertEqual(error.code, FIRFirestoreErrorCodeFailedPrecondition);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -143,9 +142,7 @@ - (void)testDeleteDocumentAndPatch {
         XCTAssertNil(result);
         XCTAssertNotNil(error);
         XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-        // TODO(dimond): This is probably the wrong error code, but it's what we use today. We
-        // should update the code once the underlying error was fixed.
-        XCTAssertEqual(error.code, FIRFirestoreErrorCodeFailedPrecondition);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -172,9 +169,8 @@ - (void)testDeleteDocumentAndSet {
         XCTAssertNil(result);
         XCTAssertNotNil(error);
         XCTAssertEqualObjects(error.domain, FIRFirestoreErrorDomain);
-        // TODO(dimond): This is probably the wrong error code, but it's what we use today. We
-        // should update the code once the underlying error was fixed.
-        XCTAssertEqual(error.code, FIRFirestoreErrorCodeFailedPrecondition);
+        // This is the error surfaced by the backend.
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -237,6 +233,8 @@ - (void)testCannotUpdateNonExistentDocument {
       }
       completion:^(id _Nullable result, NSError *_Nullable error) {
         XCTAssertNotNil(error);
+        // This is the error surfaced by the backend.
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeNotFound);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -373,6 +371,7 @@ - (void)testHandleReadingOneDocAndWritingAnother {
         // XCTAssertNil(error);
         // XCTAssertEqualObjects(@(16), snapshot[@"count"]);
         XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];
@@ -410,13 +409,49 @@ - (void)testReadingADocTwiceWithDifferentVersions {
       }
       completion:^(id _Nullable result, NSError *_Nullable error) {
         [expectation fulfill];
+        XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeAborted);
       }];
   [self awaitExpectations];
 
   FIRDocumentSnapshot *snapshot = [self readDocumentForRef:doc];
   XCTAssertEqualObjects(@(1234.0), snapshot[@"count"]);
 }
 
+- (void)testReadAndUpdateNonExistentDocumentWithExternalWrite {
+  FIRFirestore *firestore = [self firestore];
+  XCTestExpectation *expectation = [self expectationWithDescription:@"transaction"];
+  [firestore
+      runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) {
+        // Get and update a document that doesn't exist so that the transaction fails.
+        FIRDocumentReference *doc =
+            [[firestore collectionWithPath:@"nonexistent"] documentWithAutoID];
+        [transaction getDocument:doc error:error];
+        XCTAssertNil(*error);
+        // Do a write outside of the transaction.
+        dispatch_semaphore_t writeSemaphore = dispatch_semaphore_create(0);
+        [doc setData:@{
+          @"count" : @(1234)
+        }
+            completion:^(NSError *_Nullable error) {
+              dispatch_semaphore_signal(writeSemaphore);
+            }];
+        // We can block on it, because transactions run on a background queue.
+        dispatch_semaphore_wait(writeSemaphore, DISPATCH_TIME_FOREVER);
+        // Now try to update the other doc from within the transaction.
+        // This should fail, because the document didn't exist at the
+        // start of the transaction.
+        [transaction updateData:@{@"count" : @(16)} forDocument:doc];
+        return nil;
+      }
+      completion:^(id _Nullable result, NSError *_Nullable error) {
+        [expectation fulfill];
+        XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
+      }];
+  [self awaitExpectations];
+}
+
 - (void)testCannotHaveAGetWithoutMutations {
   FIRFirestore *firestore = [self firestore];
   FIRDocumentReference *doc = [[firestore collectionWithPath:@"foo"] documentWithAutoID];
@@ -433,6 +468,7 @@ - (void)testCannotHaveAGetWithoutMutations {
         // We currently require every document read to also be written.
         // TODO(b/34879758): Fix this check once we drop that requirement.
         XCTAssertNotNil(error);
+        XCTAssertEqual(error.code, FIRFirestoreErrorCodeInvalidArgument);
         [expectation fulfill];
       }];
   [self awaitExpectations];

Firestore/Source/Core/FSTSyncEngine.mm

@@ -315,13 +315,9 @@ - (void)transactionWithRetries:(int)retries
               resultCallback(std::move(maybe_result));
               return;
             }
-
             // TODO(b/35201829): Only retry on real transaction failures.
             if (retries == 0) {
-              Status wrappedError =
-                  Status(FirestoreErrorCode::FailedPrecondition, "Transaction failed all retries.")
-                      .CausedBy(std::move(status));
-              resultCallback(std::move(wrappedError));
+              resultCallback(std::move(status));
               return;
             }
             workerQueue->VerifyIsCurrentQueue();

Firestore/core/src/firebase/firestore/core/transaction.h

@@ -128,8 +128,8 @@ class Transaction {
   bool committed_ = false;
 
   /**
-   * An error that may have occurred as a consequence of a write. If set, needs
-   * to be raised in the callback instead of trying to commit.
+   * A deferred usage error that occurred previously in this transaction that
+   * will cause the transaction to fail once it actually commits.
    */
   util::Status last_write_error_;
 

Firestore/core/src/firebase/firestore/core/transaction.mm

@@ -79,8 +79,13 @@
                          LookupCallback&& callback) {
   EnsureCommitNotCalled();
 
-  HARD_ASSERT(mutations_.empty(),
-              "Transactions lookups are invalid after writes.");
+  if (!mutations_.empty()) {
+    Status lookup_error = Status{FirestoreErrorCode::InvalidArgument,
+                                 "Firestore transactions require all reads to "
+                                 "be executed before all writes"};
+    callback({}, lookup_error);
+    return;
+  }
 
   datastore_->LookupDocuments(
       keys, [this, callback](const std::vector<FSTMaybeDocument*>& documents,
@@ -124,7 +129,7 @@
 
   if (version.has_value() && version.value() == SnapshotVersion::None()) {
     // The document to update doesn't exist, so fail the transaction.
-    return Status{FirestoreErrorCode::Aborted,
+    return Status{FirestoreErrorCode::InvalidArgument,
                   "Can't update a document that doesn't exist."};
   } else if (version.has_value()) {
     // Document exists, just base precondition on document update time.
@@ -184,7 +189,7 @@
     // TODO(klimt): This is a temporary restriction, until "verify" is supported
     // on the backend.
     callback(
-        Status{FirestoreErrorCode::FailedPrecondition,
+        Status{FirestoreErrorCode::InvalidArgument,
                "Every document read in a transaction must also be written in "
                "that transaction."});
   } else {