Fix FAILED_PRECONDITION when writing to a deleted document in a transaction

Author: milaGGL

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,5 @@
 # Unreleased
+- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction. Ported from web SDK  (#5871).JMASW N
 - [fixed] Fixed Firestore failing to raise initial snapshot from empty local cache result.
 
 # 24.4.0
@@ -10,7 +11,7 @@
 The Kotlin extensions library transitively includes the updated
   `firebase-firestore` library. The Kotlin extensions library has the following
   additional updates:
-
+SDEW
 * [feature] Firebase now supports Kotlin coroutines.
   With this release, we added
   [`kotlinx-coroutines-play-services`](https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-play-services/){: .external}

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java

@@ -69,20 +69,29 @@ void runStage(Transaction transaction, DocumentReference docRef)
 
   private static TransactionStage get = Transaction::get;
 
+  private enum FromDocumentType {
+    // The operation will be performed on a document that exists.
+    EXISTING,
+    // The operation will be performed on a document that has never existed.
+    NON_EXISTENT,
+    // The operation will be performed on a document that existed, but was deleted.
+    DELETED,
+  }
+
   /**
    * Used for testing that all possible combinations of executing transactions result in the desired
    * document value or error.
    *
-   * <p>`run()`, `withExistingDoc()`, and `withNonexistentDoc()` don't actually do anything except
-   * assign variables into the TransactionTester.
+   * <p>`run()`, `withExistingDoc()`,`withNonexistentDoc()` and `withDeletedDoc()` don't actually do
+   * anything except assign variables into the TransactionTester.
    *
    * <p>`expectDoc()`, `expectNoDoc()`, and `expectError()` will trigger the transaction to run and
    * assert that the end result matches the input.
    */
   private static class TransactionTester {
     private FirebaseFirestore db;
     private DocumentReference docRef;
-    private boolean fromExistingDoc = false;
+    private FromDocumentType fromDocumentType = FromDocumentType.NON_EXISTENT;
     private List<TransactionStage> stages = new ArrayList<>();
 
     TransactionTester(FirebaseFirestore inputDb) {
@@ -91,13 +100,19 @@ private static class TransactionTester {
 
     @CanIgnoreReturnValue
     public TransactionTester withExistingDoc() {
-      fromExistingDoc = true;
+      fromDocumentType = FromDocumentType.EXISTING;
       return this;
     }
 
     @CanIgnoreReturnValue
     public TransactionTester withNonexistentDoc() {
-      fromExistingDoc = false;
+      fromDocumentType = FromDocumentType.NON_EXISTENT;
+      return this;
+    }
+
+    @CanIgnoreReturnValue
+    public TransactionTester withDeletedDoc() {
+      fromDocumentType = FromDocumentType.DELETED;
       return this;
     }
 
@@ -160,8 +175,20 @@ private void expectError(Code expected) {
 
     private void prepareDoc() {
       docRef = db.collection("tester-docref").document();
-      if (fromExistingDoc) {
-        waitFor(docRef.set(map("foo", "bar0")));
+
+      switch (fromDocumentType) {
+        case EXISTING:
+          waitFor(docRef.set(map("foo", "bar0")));
+          break;
+        case NON_EXISTENT:
+          // Nothing to do; document does not exist.
+          break;
+        case DELETED:
+          waitFor(docRef.set(map("foo", "bar0")));
+          waitFor(docRef.delete());
+          break;
+        default:
+          throw new Error("invalid fromDocumentType: " + fromDocumentType);
       }
     }
 
@@ -223,6 +250,11 @@ public void testRunsTransactionsAfterGettingExistingDoc() {
     tt.withExistingDoc().run(get, set1, set2).expectDoc(map("foo", "bar2"));
   }
 
+  // This test is identical to the test above, except that withNonexistentDoc()
+  // is replaced by withDeletedDoc(), to guard against regression of
+  // https://github.com/firebase/firebase-js-sdk/issues/5871, where transactions
+  // would incorrectly fail with FAILED_PRECONDITION when operations were
+  // performed on a deleted document (rather than a non-existent document).
   @Test
   public void testRunsTransactionsAfterGettingNonexistentDoc() {
     FirebaseFirestore firestore = testFirestore();
@@ -241,6 +273,24 @@ public void testRunsTransactionsAfterGettingNonexistentDoc() {
     tt.withNonexistentDoc().run(get, set1, set2).expectDoc(map("foo", "bar2"));
   }
 
+  @Test
+  public void testRunsTransactionsAfterGettingDeletedDoc() {
+    FirebaseFirestore firestore = testFirestore();
+    TransactionTester tt = new TransactionTester(firestore);
+
+    tt.withDeletedDoc().run(get, delete1, delete1).expectNoDoc();
+    tt.withDeletedDoc().run(get, delete1, update2).expectError(Code.INVALID_ARGUMENT);
+    tt.withDeletedDoc().run(get, delete1, set2).expectDoc(map("foo", "bar2"));
+
+    tt.withDeletedDoc().run(get, update1, delete1).expectError(Code.INVALID_ARGUMENT);
+    tt.withDeletedDoc().run(get, update1, update2).expectError(Code.INVALID_ARGUMENT);
+    tt.withDeletedDoc().run(get, update1, set2).expectError(Code.INVALID_ARGUMENT);
+
+    tt.withDeletedDoc().run(get, set1, delete1).expectNoDoc();
+    tt.withDeletedDoc().run(get, set1, update2).expectDoc(map("foo", "bar2"));
+    tt.withDeletedDoc().run(get, set1, set2).expectDoc(map("foo", "bar2"));
+  }
+
   @Test
   public void testRunsTransactionsOnExistingDoc() {
     FirebaseFirestore firestore = testFirestore();
@@ -277,6 +327,24 @@ public void testRunsTransactionsOnNonexistentDoc() {
     tt.withNonexistentDoc().run(set1, set2).expectDoc(map("foo", "bar2"));
   }
 
+  @Test
+  public void testRunsTransactionsOnDeletedDoc() {
+    FirebaseFirestore firestore = testFirestore();
+    TransactionTester tt = new TransactionTester(firestore);
+
+    tt.withDeletedDoc().run(delete1, delete1).expectNoDoc();
+    tt.withDeletedDoc().run(delete1, update2).expectError(Code.INVALID_ARGUMENT);
+    tt.withDeletedDoc().run(delete1, set2).expectDoc(map("foo", "bar2"));
+
+    tt.withDeletedDoc().run(update1, delete1).expectError(Code.NOT_FOUND);
+    tt.withDeletedDoc().run(update1, update2).expectError(Code.NOT_FOUND);
+    tt.withDeletedDoc().run(update1, set2).expectError(Code.NOT_FOUND);
+
+    tt.withDeletedDoc().run(set1, delete1).expectNoDoc();
+    tt.withDeletedDoc().run(set1, update2).expectDoc(map("foo", "bar2"));
+    tt.withDeletedDoc().run(set1, set2).expectDoc(map("foo", "bar2"));
+  }
+
   @Test
   public void testSetDocumentWithMerge() {
     FirebaseFirestore firestore = testFirestore();
@@ -637,6 +705,29 @@ public void testDoesNotRetryOnPermanentError() {
     assertEquals(1, count.get());
   }
 
+  @Test
+  public void testRetryOnAlreadyExistsError() {
+    final FirebaseFirestore firestore = testFirestore();
+    DocumentReference doc = firestore.collection("foo").document();
+    CountDownLatch latch = new CountDownLatch(2);
+    waitFor(
+        firestore.runTransaction(
+            transaction -> {
+              latch.countDown();
+              transaction.get(doc);
+              // Do a write outside of the transaction.
+              waitFor(doc.set(map("count", 1)));
+              // Now try to set the doc within the transaction.This should fail once
+              // with ALREADY_EXISTS error.
+              transaction.set(doc, map("count", 2));
+              return null;
+            }));
+    DocumentSnapshot snapshot = waitFor(doc.get());
+    assertEquals(0, latch.getCount());
+    assertTrue(snapshot.exists());
+    assertEquals(2L, snapshot.getData().get("count"));
+  }
+
   @Test
   public void testMakesDefaultMaxAttempts() {
     FirebaseFirestore firestore = testFirestore();

firebase-firestore/src/main/java/com/google/firebase/firestore/core/Transaction.java

@@ -199,7 +199,11 @@ private void recordVersion(MutableDocument doc) throws FirebaseFirestoreExceptio
   private Precondition precondition(DocumentKey key) {
     @Nullable SnapshotVersion version = readVersions.get(key);
     if (!writtenDocs.contains(key) && version != null) {
-      return Precondition.updateTime(version);
+      if (version.equals(SnapshotVersion.NONE)) {
+        return Precondition.exists(false);
+      } else {
+        return Precondition.updateTime(version);
+      }
     } else {
       return Precondition.NONE;
     }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java

@@ -95,11 +95,13 @@ private void handleTransactionError(Task task) {
 
   private static boolean isRetryableTransactionError(Exception e) {
     if (e instanceof FirebaseFirestoreException) {
-      // In transactions, the backend will fail outdated reads with FAILED_PRECONDITION and
-      // non-matching document versions with ABORTED. These errors should be retried.
+      // In transactions, the backend will fail outdated reads with FAILED_PRECONDITION,non-matching
+      // document versions with ABORTED, and attempts to create already exists with ALREADY_EXISTS.
+      // These errors should be retried.
       FirebaseFirestoreException.Code code = ((FirebaseFirestoreException) e).getCode();
       return code == FirebaseFirestoreException.Code.ABORTED
           || code == FirebaseFirestoreException.Code.FAILED_PRECONDITION
+          || code == FirebaseFirestoreException.Code.ALREADY_EXISTS
           || !Datastore.isPermanentError(((FirebaseFirestoreException) e).getCode());
     }
     return false;