From bd68acd9f95c8d4cd829b75c709ed94963c5aa9d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Sep 2022 20:22:54 -0400 Subject: [PATCH 1/5] ci: enable ITQueryCountTest in CI, since the project is whitelists for COUNT --- .../com/google/cloud/firestore/ITQueryCountTest.java | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java index b9e3a671e..a06908069 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java @@ -47,21 +47,11 @@ public class ITQueryCountTest { @Rule public TestName testName = new TestName(); - private FirestoreOptions firestoreOptions; private Firestore firestore; - @Before - public void setUpFirestoreOptions() { - firestoreOptions = FirestoreOptions.newBuilder().build(); - // TODO(count) Remove the assumeTrue() below once count queries are supported in prod. - assumeTrue( - "Count queries are only supported in the Firestore Emulator (for now)", - firestoreOptions.getHost().startsWith("localhost")); - } - @Before public void setUpFirestore() { - firestore = firestoreOptions.getService(); + firestore = FirestoreOptions.newBuilder().build().getService(); Preconditions.checkNotNull( firestore, "Error instantiating Firestore. Check that the service account credentials were properly set."); From a7e90ddaf770f3f4a67072688ddefc28a1928f5d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 21 Sep 2022 20:32:03 -0400 Subject: [PATCH 2/5] ITQueryCountTest.java: removed unused iport of assumeTrue --- .../test/java/com/google/cloud/firestore/ITQueryCountTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java index a06908069..9d7c4492b 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java @@ -20,7 +20,6 @@ import static com.google.common.truth.Truth.assertThat; import static java.util.Collections.singletonMap; import static org.junit.Assert.assertThrows; -import static org.junit.Assume.assumeTrue; import com.google.api.core.ApiFuture; import com.google.auto.value.AutoValue; From 17543221150e9d0aa3e2e752b5d3d77f1d4e079e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 22 Sep 2022 00:09:30 -0400 Subject: [PATCH 3/5] Skip aggregateQueryInATransactionShouldLockTheCountedDocuments when running against prod (b/248152832) --- .../com/google/cloud/firestore/ITQueryCountTest.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java index 9d7c4492b..3d93456c8 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java @@ -20,6 +20,7 @@ import static com.google.common.truth.Truth.assertThat; import static java.util.Collections.singletonMap; import static org.junit.Assert.assertThrows; +import static org.junit.Assume.assumeTrue; import com.google.api.core.ApiFuture; import com.google.auto.value.AutoValue; @@ -215,6 +216,11 @@ public void aggregateQueryShouldWorkInATransaction() throws Exception { @Test public void aggregateQueryInATransactionShouldLockTheCountedDocuments() throws Exception { + assumeTrue( + "Skip this test when running against production because " + + "it appears that production is failing to lock the counted documents b/248152832", + isRunningAgainstFirestoreEmulator()); + CollectionReference collection = createEmptyCollection(); DocumentReference document = createDocumentInCollection(collection); CountDownLatch aggregateQueryExecutedSignal = new CountDownLatch(1); @@ -388,6 +394,11 @@ private static void await(ApiFuture future) throws InterruptedException { executor.shutdown(); } + /** Returns whether the tests are running against the Firestore emulator. */ + private boolean isRunningAgainstFirestoreEmulator() { + return firestore.getOptions().getHost().startsWith("localhost:"); + } + @AutoValue abstract static class CreatedCollectionInfo { From b47297cda22b4c5a1d77ad47ef55e1a281c9d134 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 22 Sep 2022 00:11:17 -0400 Subject: [PATCH 4/5] Revert "Skip aggregateQueryInATransactionShouldLockTheCountedDocuments when running against prod (b/248152832)" This reverts commit 17543221150e9d0aa3e2e752b5d3d77f1d4e079e. --- .../com/google/cloud/firestore/ITQueryCountTest.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java index 3d93456c8..9d7c4492b 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/ITQueryCountTest.java @@ -20,7 +20,6 @@ import static com.google.common.truth.Truth.assertThat; import static java.util.Collections.singletonMap; import static org.junit.Assert.assertThrows; -import static org.junit.Assume.assumeTrue; import com.google.api.core.ApiFuture; import com.google.auto.value.AutoValue; @@ -216,11 +215,6 @@ public void aggregateQueryShouldWorkInATransaction() throws Exception { @Test public void aggregateQueryInATransactionShouldLockTheCountedDocuments() throws Exception { - assumeTrue( - "Skip this test when running against production because " - + "it appears that production is failing to lock the counted documents b/248152832", - isRunningAgainstFirestoreEmulator()); - CollectionReference collection = createEmptyCollection(); DocumentReference document = createDocumentInCollection(collection); CountDownLatch aggregateQueryExecutedSignal = new CountDownLatch(1); @@ -394,11 +388,6 @@ private static void await(ApiFuture future) throws InterruptedException { executor.shutdown(); } - /** Returns whether the tests are running against the Firestore emulator. */ - private boolean isRunningAgainstFirestoreEmulator() { - return firestore.getOptions().getHost().startsWith("localhost:"); - } - @AutoValue abstract static class CreatedCollectionInfo { From c1856505a9f8b40628579ce2214229d9e306d0c7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 11 Oct 2022 12:12:39 -0400 Subject: [PATCH 5/5] ITQueryCountTest.java: tweak aggregateQueryInATransactionShouldLockTheCountedDocuments to do what I had expected --- .../cloud/firestore/it/ITQueryCountTest.java | 36 +++++++++---------- 1 file changed, 17 insertions(+), 19 deletions(-) diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java index 91ee8ef61..5d052d28a 100644 --- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java +++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java @@ -35,8 +35,8 @@ import com.google.cloud.firestore.QueryDocumentSnapshot; import com.google.cloud.firestore.TransactionOptions; import com.google.cloud.firestore.WriteBatch; -import com.google.cloud.firestore.WriteResult; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import java.util.ArrayList; import java.util.List; import java.util.concurrent.CountDownLatch; @@ -44,6 +44,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.After; import org.junit.Before; import org.junit.Rule; @@ -234,44 +235,41 @@ public void aggregateQueryShouldWorkInATransaction() throws Exception { @Test public void aggregateQueryInATransactionShouldLockTheCountedDocuments() throws Exception { - assumeTrue( - "Skip this test when running against production because " - + "it appears that production is failing to lock the counted documents b/248152832", - isRunningAgainstFirestoreEmulator()); + CollectionReference collection = createCollectionWithDocuments(7).collection(); + DocumentReference resultsDocument = createEmptyCollection().document(); - CollectionReference collection = createEmptyCollection(); - DocumentReference document = createDocumentInCollection(collection); CountDownLatch aggregateQueryExecutedSignal = new CountDownLatch(1); CountDownLatch transactionContinueSignal = new CountDownLatch(1); + AtomicInteger transactionInvokeCount = new AtomicInteger(0); ApiFuture transactionFuture = collection .getFirestore() .runTransaction( t -> { - t.get(collection.count()).get(); - aggregateQueryExecutedSignal.countDown(); - transactionContinueSignal.await(); + int invokeCount = transactionInvokeCount.getAndIncrement(); + long count = t.get(collection.count()).get().getCount(); + if (invokeCount == 0) { + aggregateQueryExecutedSignal.countDown(); + transactionContinueSignal.await(); + } + t.set(resultsDocument, ImmutableMap.of("count", count)); return null; }); - ExecutionException executionException; try { aggregateQueryExecutedSignal.await(); - ApiFuture documentSetTask = document.set(singletonMap("abc", "def")); - executionException = assertThrows(ExecutionException.class, documentSetTask::get); + // Add a document to the collection so the count retrieved in the transaction is stale. + collection.document().set(ImmutableMap.of("key", 42L)).get(); } finally { transactionContinueSignal.countDown(); } - assertThat(executionException) - .hasCauseThat() - .hasMessageThat() - .ignoringCase() - .contains("transaction lock timeout"); - // Wait for the transaction to finish. transactionFuture.get(); + + // Verify that the correct count was written in the transaction. + assertThat(resultsDocument.get().get().getLong("count")).isEqualTo(8); } @Test