From e0ea8c97d8aad9b1c450ebe27cea98fed85fd8e1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Oct 2022 12:14:10 -0700 Subject: [PATCH 1/6] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction --- firebase-firestore/CHANGELOG.md | 3 +- .../firebase/firestore/TransactionTest.java | 105 ++++++++++++++++-- .../firebase/firestore/core/Transaction.java | 6 +- .../firestore/core/TransactionRunner.java | 6 +- 4 files changed, 109 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 209d5794215..2d00cd51608 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/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} 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..b0070d460b6 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 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(); 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..1dfda4a80e1 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,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; From 09c50bb9b6b7d880f0dbbb67cfc3f6530dac2fd3 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Fri, 28 Oct 2022 12:58:25 -0700 Subject: [PATCH 2/6] fix format --- firebase-firestore/CHANGELOG.md | 4 ++-- .../com/google/firebase/firestore/TransactionTest.java | 10 +++++----- .../firebase/firestore/core/TransactionRunner.java | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 2d00cd51608..5f996458201 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased -- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction. Ported from web SDK (#5871).JMASW N +- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction. Ported from web SDK. + See the [issue report in GitHub](https://github.com/firebase/firebase-js-sdk/issues/5871). - [fixed] Fixed Firestore failing to raise initial snapshot from empty local cache result. # 24.4.0 @@ -11,7 +12,6 @@ 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} 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 b0070d460b6..47c3acb7bf6 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 @@ -250,11 +250,6 @@ 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(); @@ -273,6 +268,11 @@ 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(); 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 1dfda4a80e1..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,13 +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,non-matching - // document versions with ABORTED, and attempts to create already exists with ALREADY_EXISTS. - // 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 + || code == FirebaseFirestoreException.Code.FAILED_PRECONDITION || !Datastore.isPermanentError(((FirebaseFirestoreException) e).getCode()); } return false; From b8ce2b1d94b65825ff8a48d9828f1b9da559871a Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 31 Oct 2022 12:44:02 -0700 Subject: [PATCH 3/6] resolve comments --- firebase-firestore/CHANGELOG.md | 5 +++-- .../firebase/firestore/TransactionTest.java | 20 +++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 5f996458201..381652f4eab 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased -- [fixed] Fix FAILED_PRECONDITION when writing to a deleted document in a transaction. Ported from web SDK. - See the [issue report in GitHub](https://github.com/firebase/firebase-js-sdk/issues/5871). +- [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 47c3acb7bf6..72eb5b3b373 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 @@ -82,8 +82,8 @@ private enum FromDocumentType { * Used for testing that all possible combinations of executing transactions result in the desired * document value or error. * - *

`run()`, `withExistingDoc()`,`withNonexistentDoc()` and `withDeletedDoc()` 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. @@ -188,7 +188,7 @@ private void prepareDoc() { waitFor(docRef.delete()); break; default: - throw new Error("invalid fromDocumentType: " + fromDocumentType); + throw new RuntimeException("invalid fromDocumentType: " + fromDocumentType); } } @@ -709,23 +709,23 @@ public void testDoesNotRetryOnPermanentError() { public void testRetryOnAlreadyExistsError() { final FirebaseFirestore firestore = testFirestore(); DocumentReference doc = firestore.collection("foo").document(); - CountDownLatch latch = new CountDownLatch(2); + AtomicInteger retryCount = new AtomicInteger(0); waitFor( firestore.runTransaction( transaction -> { - latch.countDown(); + retryCount.getAndIncrement(); 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 + if (retryCount.get() == 1) waitFor(doc.set(map("count", retryCount.get()))); + // Now try to set the doc within the transaction. This should fail once // with ALREADY_EXISTS error. - transaction.set(doc, map("count", 2)); + transaction.set(doc, map("count", 22)); return null; })); DocumentSnapshot snapshot = waitFor(doc.get()); - assertEquals(0, latch.getCount()); + assertEquals(2, retryCount.get()); assertTrue(snapshot.exists()); - assertEquals(2L, snapshot.getData().get("count")); + assertEquals(22L, snapshot.getData().get("count")); } @Test From 1a9aecf014aebf827c2589912fde3a37085c02b1 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Mon, 31 Oct 2022 13:21:23 -0700 Subject: [PATCH 4/6] remove unintended change --- firebase-firestore/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 381652f4eab..152ee3d64ba 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -13,6 +13,7 @@ The Kotlin extensions library transitively includes the updated `firebase-firestore` library. The Kotlin extensions library has the following additional updates: + * [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} From fe38e791517e48ded15d2b8e3af0209d93640728 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 1 Nov 2022 10:10:43 -0700 Subject: [PATCH 5/6] resolve comments --- .../google/firebase/firestore/TransactionTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 72eb5b3b373..c9f56e54acc 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 @@ -709,23 +709,23 @@ public void testDoesNotRetryOnPermanentError() { public void testRetryOnAlreadyExistsError() { final FirebaseFirestore firestore = testFirestore(); DocumentReference doc = firestore.collection("foo").document(); - AtomicInteger retryCount = new AtomicInteger(0); + AtomicInteger transactionCallbackCount = new AtomicInteger(0); waitFor( firestore.runTransaction( transaction -> { - retryCount.getAndIncrement(); + int currentCount = transactionCallbackCount.incrementAndGet(); transaction.get(doc); // Do a write outside of the transaction. - if (retryCount.get() == 1) waitFor(doc.set(map("count", retryCount.get()))); + 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("count", 22)); + transaction.set(doc, map("foo2", "bar2")); return null; })); DocumentSnapshot snapshot = waitFor(doc.get()); - assertEquals(2, retryCount.get()); + assertEquals(2, transactionCallbackCount.get()); assertTrue(snapshot.exists()); - assertEquals(22L, snapshot.getData().get("count")); + assertEquals(map("foo1", "bar1", "foo2", "bar2"), snapshot.getData()); } @Test From 7e97f3423b08ac25b31b153e9327a8c6a19f5162 Mon Sep 17 00:00:00 2001 From: milaGGL <107142260+milaGGL@users.noreply.github.com> Date: Tue, 1 Nov 2022 11:27:41 -0700 Subject: [PATCH 6/6] fix failing test --- .../java/com/google/firebase/firestore/TransactionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c9f56e54acc..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 @@ -725,7 +725,7 @@ public void testRetryOnAlreadyExistsError() { DocumentSnapshot snapshot = waitFor(doc.get()); assertEquals(2, transactionCallbackCount.get()); assertTrue(snapshot.exists()); - assertEquals(map("foo1", "bar1", "foo2", "bar2"), snapshot.getData()); + assertEquals(map("foo2", "bar2"), snapshot.getData()); } @Test