diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 209d5794215..152ee3d64ba 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/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 diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java index 4045cdc0e06..f83289d3d5a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/TransactionTest.java @@ -69,12 +69,21 @@ 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. * - *

`run()`, `withExistingDoc()`, and `withNonexistentDoc()` don't actually do anything except - * assign variables into the TransactionTester. + *

`run()`, `withExistingDoc()`, `withNonexistentDoc()` and `withDeletedDoc()` don't actually + * do anything except assign variables into the TransactionTester. * *

`expectDoc()`, `expectNoDoc()`, and `expectError()` will trigger the transaction to run and * assert that the end result matches the input. @@ -82,7 +91,7 @@ void runStage(Transaction transaction, DocumentReference docRef) private static class TransactionTester { private FirebaseFirestore db; private DocumentReference docRef; - private boolean fromExistingDoc = false; + private FromDocumentType fromDocumentType = FromDocumentType.NON_EXISTENT; private List 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(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Transaction.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Transaction.java index 4a2cdbc3321..d235fc8e7b0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Transaction.java +++ b/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; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java index 70a249cc804..28db0e6902e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/TransactionRunner.java +++ b/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()); }