Fix FAILED_PRECONDITION when writing to a deleted document in a transaction (#4257)

Author: milaGGL

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,7 @@
 # Unreleased
+- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction.
+  (#5871).
+
 - [fixed] Fixed Firestore failing to raise initial snapshot from empty local cache result.
 
 # 24.4.0

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 RuntimeException("invalid fromDocumentType: " + fromDocumentType);
       }
     }
 
@@ -241,6 +268,29 @@ public void testRunsTransactionsAfterGettingNonexistentDoc() {
     tt.withNonexistentDoc().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 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();
+    AtomicInteger transactionCallbackCount = new AtomicInteger(0);
+    waitFor(
+        firestore.runTransaction(
+            transaction -> {
+              int currentCount = transactionCallbackCount.incrementAndGet();
+              transaction.get(doc);
+              // Do a write outside of the transaction.
+              if (currentCount == 1) waitFor(doc.set(map("foo1", "bar1")));
+              // Now try to set the doc within the transaction. This should fail once
+              // with ALREADY_EXISTS error.
+              transaction.set(doc, map("foo2", "bar2"));
+              return null;
+            }));
+    DocumentSnapshot snapshot = waitFor(doc.get());
+    assertEquals(2, transactionCallbackCount.get());
+    assertTrue(snapshot.exists());
+    assertEquals(map("foo2", "bar2"), snapshot.getData());
+  }
+
   @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,10 +95,12 @@ 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.ALREADY_EXISTS
           || code == FirebaseFirestoreException.Code.FAILED_PRECONDITION
           || !Datastore.isPermanentError(((FirebaseFirestoreException) e).getCode());
     }