From 378ddee8fea828f930ee785b1d0dadb39926e391 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 16:03:15 -0500 Subject: [PATCH 1/9] WatchChangeAggregatorTestingHooks added --- .../remote/WatchChangeAggregator.java | 4 + .../WatchChangeAggregatorTestingHooks.java | 166 ++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java index 435cbca9e24..2e3ee9513e7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java @@ -216,6 +216,10 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) { resetTarget(targetId); pendingTargetResets.add(targetId); } + + WatchChangeAggregatorTestingHooks.notifyOnExistenceFilterMismatch( + WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo.from( + bloomFilterApplied, currentSize, watchChange.getExistenceFilter())); } } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java new file mode 100644 index 00000000000..6a492cd77df --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java @@ -0,0 +1,166 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.remote; + +import static com.google.firebase.firestore.util.Preconditions.checkNotNull; + +import androidx.annotation.AnyThread; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; +import com.google.auto.value.AutoValue; +import com.google.firebase.firestore.ListenerRegistration; +import com.google.firebase.firestore.util.Executors; +import com.google.firestore.v1.BloomFilter; +import java.util.HashMap; +import java.util.Map; + +final class WatchChangeAggregatorTestingHooks { + + private WatchChangeAggregatorTestingHooks() {} + + private static final Map + existenceFilterMismatchListeners = new HashMap<>(); + + /** + * Notifies all registered {@link ExistenceFilterMismatchListener}` listeners registered via + * {@link #addExistenceFilterMismatchListener}. + * + * @param info Information about the existence filter mismatch to deliver to the listeners. + */ + static void notifyOnExistenceFilterMismatch(ExistenceFilterMismatchInfo info) { + synchronized (existenceFilterMismatchListeners) { + for (ExistenceFilterMismatchListener listener : existenceFilterMismatchListeners.values()) { + Executors.BACKGROUND_EXECUTOR.execute(() -> listener.onExistenceFilterMismatch(info)); + } + } + } + + /** + * Registers a {@link ExistenceFilterMismatchListener} to be notified when an existence filter + * mismatch occurs in the Watch listen stream. + * + *

The relative order in which callbacks are notified is unspecified; do not rely on any + * particular ordering. If a given callback is registered multiple times then it will be notified + * multiple times, once per registration. + * + *

The thread on which the callback occurs is unspecified; listeners should perform their work + * as quickly as possible and return to avoid blocking any critical work. In particular, the + * listener callbacks should not block or perform long-running operations. Listener + * callbacks can occur concurrently with other callbacks on the same and other listeners. + * + * @param listener the listener to register. + * @return an object that unregisters the given listener via its {@link + * ListenerRegistration#remove} method; only the first unregistration request does anything; + * all subsequent requests do nothing. + */ + @VisibleForTesting + static ListenerRegistration addExistenceFilterMismatchListener( + @NonNull ExistenceFilterMismatchListener listener) { + checkNotNull(listener, "a null listener is not allowed"); + + Object listenerId = new Object(); + synchronized (existenceFilterMismatchListeners) { + existenceFilterMismatchListeners.put(listenerId, listener); + } + + return () -> { + synchronized (existenceFilterMismatchListeners) { + existenceFilterMismatchListeners.remove(listenerId); + } + }; + } + + interface ExistenceFilterMismatchListener { + @AnyThread + void onExistenceFilterMismatch(ExistenceFilterMismatchInfo info); + } + + @AutoValue + abstract static class ExistenceFilterMismatchInfo { + + static ExistenceFilterMismatchInfo create( + int localCacheCount, + int existenceFilterCount, + @Nullable ExistenceFilterBloomFilterInfo bloomFilter) { + return new AutoValue_WatchChangeAggregatorTestingHooks_ExistenceFilterMismatchInfo( + localCacheCount, existenceFilterCount, bloomFilter); + } + + /** Returns the number of documents that matched the query in the local cache. */ + abstract int localCacheCount(); + + /** + * Returns the number of documents that matched the query on the server, as specified in the + * ExistenceFilter message's `count` field. + */ + abstract int existenceFilterCount(); + + /** + * Returns information about the bloom filter provided by Watch in the ExistenceFilter message's + * `unchangedNames` field. A `null` return value means that Watch did _not_ provide a bloom + * filter. + */ + @Nullable + abstract ExistenceFilterBloomFilterInfo bloomFilter(); + + static ExistenceFilterMismatchInfo from( + boolean bloomFilterApplied, int localCacheCount, ExistenceFilter existenceFilter) { + return create( + localCacheCount, + existenceFilter.getCount(), + ExistenceFilterBloomFilterInfo.from(bloomFilterApplied, existenceFilter)); + } + } + + @AutoValue + abstract static class ExistenceFilterBloomFilterInfo { + + static ExistenceFilterBloomFilterInfo create( + boolean applied, int hashCount, int bitmapLength, int padding) { + return new AutoValue_WatchChangeAggregatorTestingHooks_ExistenceFilterBloomFilterInfo( + applied, hashCount, bitmapLength, padding); + } + + /** + * Returns whether a full requery was averted by using the bloom filter. If false, then + * something happened, such as a false positive, to prevent using the bloom filter to avoid a + * full requery. + */ + abstract boolean applied(); + + /** Returns the number of hash functions used in the bloom filter. */ + abstract int hashCount(); + + /** Returns the number of bytes in the bloom filter's bitmask. */ + abstract int bitmapLength(); + + /** Returns the number of bits of padding in the last byte of the bloom filter. */ + abstract int padding(); + + static ExistenceFilterBloomFilterInfo from( + boolean bloomFilterApplied, ExistenceFilter existenceFilter) { + BloomFilter unchangedNames = existenceFilter.getUnchangedNames(); + if (unchangedNames == null) { + return null; + } + return create( + bloomFilterApplied, + unchangedNames.getHashCount(), + unchangedNames.getBits().getBitmap().size(), + unchangedNames.getBits().getPadding()); + } + } +} From 705a3ce79ae61cb044ce260015a4566cf3175138 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 12:27:12 -0500 Subject: [PATCH 2/9] Write most of QueryTest test, except for bloom filter validation logic --- .../google/firebase/firestore/QueryTest.java | 191 +++++++++++++++--- ...hChangeAggregatorTestingHooksAccessor.java | 142 +++++++++++++ 2 files changed, 300 insertions(+), 33 deletions(-) create mode 100644 firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 9202fe0376e..1157db83cef 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore; +import static com.google.common.truth.Truth.assertWithMessage; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.isRunningAgainstEmulator; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.nullList; import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToIds; @@ -32,14 +33,18 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeFalse; +import android.os.SystemClock; + import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; import com.google.common.collect.Lists; import com.google.firebase.firestore.Query.Direction; +import com.google.firebase.firestore.remote.WatchChangeAggregatorTestingHooksAccessor; import com.google.firebase.firestore.testutil.EventAccumulator; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -1033,43 +1038,163 @@ public void testMultipleUpdatesWhileOffline() { } @Test - public void resumingQueryShouldRemoveDeletedDocumentsIndicatedByExistenceFilter() - throws InterruptedException { - assumeFalse( - "Skip this test when running against the Firestore emulator as there is a bug related to " - + "sending existence filter in response: b/270731363.", - isRunningAgainstEmulator()); - + public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Exception { + // Prepare the names and contents of the 100 documents to create. Map> testData = new HashMap<>(); - for (int i = 1; i <= 100; i++) { - testData.put("doc" + i, map("key", i)); + for (int i = 0; i < 100; i++) { + testData.put("doc" + (1000 + i), map("key", 42)); } - CollectionReference collection = testCollectionWithDocs(testData); - // Populate the cache and save the resume token. - QuerySnapshot snapshot1 = waitFor(collection.get()); - assertEquals(snapshot1.size(), 100); - List documents = snapshot1.getDocuments(); + // Each iteration of the "while" loop below runs a single iteration of the test. The test will + // be run multiple times only if a bloom filter false positive occurs. + while (true) { + // Create 100 documents in a new collection. + CollectionReference collection = testCollectionWithDocs(testData); + + // Run a query to populate the local cache with the 100 documents and a resume token. + List createdDocuments = new ArrayList<>(); + { + QuerySnapshot querySnapshot = waitFor(collection.get()); + assertWithMessage("querySnapshot1").that(querySnapshot.size()).isEqualTo(100); + for (DocumentSnapshot documentSnapshot : querySnapshot.getDocuments()) { + createdDocuments.add(documentSnapshot.getReference()); + } + } + + // Delete 50 of the 100 documents. Do this in a transaction, rather than + // DocumentReference.delete(), to avoid affecting the local cache. + HashSet deletedDocumentIds = new HashSet<>(); + waitFor( + collection + .getFirestore() + .runTransaction( + transaction -> { + for (int i = 0; i < createdDocuments.size(); i+=2) { + DocumentReference documentToDelete = createdDocuments.get(i); + transaction.delete(documentToDelete); + deletedDocumentIds.add(documentToDelete.getId()); + } + return null; + })); + + // Wait for 10 seconds, during which Watch will stop tracking the query and will send an + // existence filter rather than "delete" events when the query is resumed. + Thread.sleep(10000); + + // Resume the query and save the resulting snapshot for verification. Use some internal + // testing hooks to "capture" the existence filter mismatches to verify that Watch sent a + // bloom filter, and it was used to avert a full requery. + QuerySnapshot snapshot2; + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo existenceFilterMismatchInfo; + ExistenceFilterMismatchAccumulator existenceFilterMismatchAccumulator = new ExistenceFilterMismatchAccumulator(); + existenceFilterMismatchAccumulator.register(); + try { + snapshot2 = waitFor(collection.get()); + existenceFilterMismatchInfo = existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch(/*timeoutMillis=*/5000); + } finally { + existenceFilterMismatchAccumulator.unregister(); + } + + // Verify that the snapshot from the resumed query contains the expected documents; that is, + // that it contains the 50 documents that were _not_ deleted. + // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to + // send an existence filter. At the time of writing, the Firestore emulator fails to send an + // existence filter, resulting in the client including the deleted documents in the snapshot + // of the resumed query. + if (!(isRunningAgainstEmulator() && snapshot2.size() == 100)) { + HashSet actualDocumentIds = new HashSet<>(); + for (DocumentSnapshot documentSnapshot : snapshot2.getDocuments()) { + actualDocumentIds.add(documentSnapshot.getId()); + } + HashSet expectedDocumentIds = new HashSet<>(); + for (DocumentReference documentRef : createdDocuments) { + if (!deletedDocumentIds.contains(documentRef.getId())) { + expectedDocumentIds.add(documentRef.getId()); + } + } + assertWithMessage("snapshot2.docs").that(actualDocumentIds).containsExactlyElementsIn(expectedDocumentIds); + } + + // Skip the verification of the existence filter mismatch when testing against the Firestore + // emulator because the Firestore emulator does not include the `unchanged_names` bloom filter + // when it sends ExistenceFilter messages. Some day the emulator _may_ implement this logic, + // at which time this short-circuit can be removed. + if (isRunningAgainstEmulator()) { + return; + } + + // Verify that Watch sent an existence filter with the correct counts when the query was + // resumed. + assertWithMessage("localCacheCount").that(existenceFilterMismatchInfo.localCacheCount()).isEqualTo(100); + assertWithMessage("existenceFilterCount").that(existenceFilterMismatchInfo.existenceFilterCount()).isEqualTo(50); + } + } + + private static final class ExistenceFilterMismatchAccumulator { + + private final ExistenceFilterMismatchListenerImpl listener = new ExistenceFilterMismatchListenerImpl(); + private ListenerRegistration listenerRegistration = null; + + void register() { + if (listenerRegistration != null) { + throw new IllegalStateException("already registered"); + } + listenerRegistration = WatchChangeAggregatorTestingHooksAccessor.addExistenceFilterMismatchListener(listener); + } + + void unregister() { + if (listenerRegistration == null) { + return; + } + listenerRegistration.remove(); + listenerRegistration = null; + } + + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + if (listenerRegistration == null) { + throw new IllegalStateException("must be registered before waiting for an existence filter mismatch"); + } + return listener.waitForExistenceFilterMismatch(timeoutMillis); + } + + private final class ExistenceFilterMismatchListenerImpl implements WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchListener { + + private final ArrayList existenceFilterMismatches = new ArrayList<>(); + + @Override + public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { + synchronized (existenceFilterMismatchesLock) { + existenceFilterMismatches.add(info); + existenceFilterMismatchesLock.notifyAll(); + } + } + + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + if (timeoutMillis <= 0) { + throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); + } + synchronized (existenceFilterMismatchesLock) { + long endTimeMillis = SystemClock.uptimeMillis() + timeoutMillis; + while (true) { + if (existenceFilterMismatches.size() > 0) { + return existenceFilterMismatches.remove(0); + } + long currentWaitMillis = endTimeMillis - SystemClock.uptimeMillis(); + if (currentWaitMillis <= 0) { + throw new WaitForExistenceFilterMismatchTimeoutException("timeout (" + timeoutMillis + "ms) waiting for an existence filter mismatch"); + } + existenceFilterMismatchesLock.wait(currentWaitMillis); + } + } + } + + final class WaitForExistenceFilterMismatchTimeoutException extends RuntimeException { + WaitForExistenceFilterMismatchTimeoutException(String message) { + super(message); + } + } + } - // Delete 50 docs in transaction so that it doesn't affect local cache. - waitFor( - collection - .getFirestore() - .runTransaction( - transaction -> { - for (int i = 1; i <= 50; i++) { - DocumentReference docRef = documents.get(i).getReference(); - transaction.delete(docRef); - } - return null; - })); - - // Wait 10 seconds, during which Watch will stop tracking the query - // and will send an existence filter rather than "delete" events. - Thread.sleep(10000); - - QuerySnapshot snapshot2 = waitFor(collection.get()); - assertEquals(snapshot2.size(), 50); } // TODO(orquery): Enable this test when prod supports OR queries. diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java new file mode 100644 index 00000000000..6c8616720d8 --- /dev/null +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java @@ -0,0 +1,142 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.remote; + +import static com.google.firebase.firestore.util.Preconditions.checkNotNull; + +import androidx.annotation.AnyThread; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; + +import com.google.firebase.firestore.ListenerRegistration; + +/** + * Provides access to the {@link WatchChangeAggregatorTestingHooks} class and its methods. + * + * The {@link WatchChangeAggregatorTestingHooks} class has default visibility, and, therefore, is + * only visible to other classes declared in the same package. This class effectively "re-exports" + * the functionality from {@link WatchChangeAggregatorTestingHooks} in a class with {@code public} + * visibility so that tests written in other packages can access its functionality. + */ +public final class WatchChangeAggregatorTestingHooksAccessor { + + private WatchChangeAggregatorTestingHooksAccessor() {} + + /** + * @see WatchChangeAggregatorTestingHooks#addExistenceFilterMismatchListener + */ + public static ListenerRegistration addExistenceFilterMismatchListener( + @NonNull ExistenceFilterMismatchListener listener) { + checkNotNull(listener, "a null listener is not allowed"); + return WatchChangeAggregatorTestingHooks.addExistenceFilterMismatchListener(new ExistenceFilterMismatchListenerWrapper(listener)); + } + + /** + * @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener + */ + public interface ExistenceFilterMismatchListener { + @AnyThread + void onExistenceFilterMismatch(ExistenceFilterMismatchInfo info); + } + + /** + * @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo + */ + public interface ExistenceFilterMismatchInfo { + int localCacheCount(); + int existenceFilterCount(); + @Nullable ExistenceFilterBloomFilterInfo bloomFilter(); + } + + /** + * @see WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo + */ + public interface ExistenceFilterBloomFilterInfo { + boolean applied(); + int hashCount(); + int bitmapLength(); + int padding(); + } + + private static final class ExistenceFilterMismatchInfoImpl implements ExistenceFilterMismatchInfo { + + private final WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info; + + ExistenceFilterMismatchInfoImpl(@NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { + this.info = info; + } + + @Override + public int localCacheCount() { + return info.localCacheCount(); + } + + @Override + public int existenceFilterCount() { + return info.existenceFilterCount(); + } + + @Nullable + @Override + public ExistenceFilterBloomFilterInfo bloomFilter() { + WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo bloomFilterInfo = info.bloomFilter(); + return bloomFilterInfo == null ? null : new ExistenceFilterBloomFilterInfoImpl(bloomFilterInfo); + } + } + + private static final class ExistenceFilterBloomFilterInfoImpl implements ExistenceFilterBloomFilterInfo { + + private final WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo info; + + ExistenceFilterBloomFilterInfoImpl(@NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo info) { + this.info = info; + } + + @Override + public boolean applied() { + return info.applied(); + } + + @Override + public int hashCount() { + return info.hashCount(); + } + + @Override + public int bitmapLength() { + return info.bitmapLength(); + } + + @Override + public int padding() { + return info.padding(); + } + } + + private static final class ExistenceFilterMismatchListenerWrapper implements WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener { + + private final ExistenceFilterMismatchListener wrappedListener; + + ExistenceFilterMismatchListenerWrapper(@NonNull ExistenceFilterMismatchListener listenerToWrap) { + this.wrappedListener = listenerToWrap; + } + + @Override + public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { + this.wrappedListener.onExistenceFilterMismatch(new ExistenceFilterMismatchInfoImpl(info)); + } + } + +} From f35e885098c34fa7badf21a0add2a17f75d033d8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 12:47:02 -0500 Subject: [PATCH 3/9] IntegrationTestUtil.java: write in batches, for efficiency --- .../testutil/IntegrationTestUtil.java | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java index e14f8bffb48..4efa914633a 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java @@ -36,6 +36,7 @@ import com.google.firebase.firestore.ListenerRegistration; import com.google.firebase.firestore.MetadataChanges; import com.google.firebase.firestore.QuerySnapshot; +import com.google.firebase.firestore.WriteBatch; import com.google.firebase.firestore.auth.User; import com.google.firebase.firestore.core.DatabaseInfo; import com.google.firebase.firestore.model.DatabaseId; @@ -347,8 +348,27 @@ public static CollectionReference testCollectionWithDocs(Map> docs) { + WriteBatch writeBatch = null; + int writeBatchSize = 0; + for (Map.Entry> doc : docs.entrySet()) { - waitFor(collection.document(doc.getKey()).set(doc.getValue())); + if (writeBatch == null) { + writeBatch = collection.getFirestore().batch(); + } + + writeBatch.set(collection.document(doc.getKey()), doc.getValue()); + writeBatchSize++; + + // Write batches are capped at 500 writes. Use 400 just to be safe. + if (writeBatchSize == 400) { + waitFor(writeBatch.commit()); + writeBatch = null; + writeBatchSize = 0; + } + } + + if (writeBatch != null) { + waitFor(writeBatch.commit()); } } From 0af81b1a2bff1235b127c0c22d97dfaf162a46ae Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 12:55:47 -0500 Subject: [PATCH 4/9] Fix test against firestore emulator --- .../google/firebase/firestore/QueryTest.java | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 1157db83cef..10143e62589 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -35,6 +35,7 @@ import android.os.SystemClock; +import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; import com.google.common.collect.Lists; @@ -1090,7 +1091,13 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except existenceFilterMismatchAccumulator.register(); try { snapshot2 = waitFor(collection.get()); - existenceFilterMismatchInfo = existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch(/*timeoutMillis=*/5000); + // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed + // to send an existence filter. + if (isRunningAgainstEmulator()) { + existenceFilterMismatchInfo = null; + } else { + existenceFilterMismatchInfo = existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch(/*timeoutMillis=*/5000); + } } finally { existenceFilterMismatchAccumulator.unregister(); } @@ -1125,6 +1132,7 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except // Verify that Watch sent an existence filter with the correct counts when the query was // resumed. + assertWithMessage("Watch should have sent an existence filter").that(existenceFilterMismatchInfo).isNotNull(); assertWithMessage("localCacheCount").that(existenceFilterMismatchInfo.localCacheCount()).isEqualTo(100); assertWithMessage("existenceFilterCount").that(existenceFilterMismatchInfo.existenceFilterCount()).isEqualTo(50); } @@ -1150,6 +1158,7 @@ void unregister() { listenerRegistration = null; } + @Nullable WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (listenerRegistration == null) { throw new IllegalStateException("must be registered before waiting for an existence filter mismatch"); @@ -1163,17 +1172,18 @@ private final class ExistenceFilterMismatchListenerImpl implements WatchChangeAg @Override public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { - synchronized (existenceFilterMismatchesLock) { + synchronized (existenceFilterMismatches) { existenceFilterMismatches.add(info); - existenceFilterMismatchesLock.notifyAll(); + existenceFilterMismatches.notifyAll(); } } + @Nullable WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (timeoutMillis <= 0) { throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); } - synchronized (existenceFilterMismatchesLock) { + synchronized (existenceFilterMismatches) { long endTimeMillis = SystemClock.uptimeMillis() + timeoutMillis; while (true) { if (existenceFilterMismatches.size() > 0) { @@ -1181,18 +1191,12 @@ WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExi } long currentWaitMillis = endTimeMillis - SystemClock.uptimeMillis(); if (currentWaitMillis <= 0) { - throw new WaitForExistenceFilterMismatchTimeoutException("timeout (" + timeoutMillis + "ms) waiting for an existence filter mismatch"); + return null; } - existenceFilterMismatchesLock.wait(currentWaitMillis); + existenceFilterMismatches.wait(currentWaitMillis); } } } - - final class WaitForExistenceFilterMismatchTimeoutException extends RuntimeException { - WaitForExistenceFilterMismatchTimeoutException(String message) { - super(message); - } - } } } From 6e7058880370efb2acc36dba359478f8f17ecbf0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 14:54:47 -0500 Subject: [PATCH 5/9] finish test --- .../google/firebase/firestore/QueryTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 10143e62589..ce1cf4aeec7 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -1048,7 +1048,10 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except // Each iteration of the "while" loop below runs a single iteration of the test. The test will // be run multiple times only if a bloom filter false positive occurs. + int attemptNumber = 0; while (true) { + attemptNumber++; + // Create 100 documents in a new collection. CollectionReference collection = testCollectionWithDocs(testData); @@ -1135,6 +1138,33 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except assertWithMessage("Watch should have sent an existence filter").that(existenceFilterMismatchInfo).isNotNull(); assertWithMessage("localCacheCount").that(existenceFilterMismatchInfo.localCacheCount()).isEqualTo(100); assertWithMessage("existenceFilterCount").that(existenceFilterMismatchInfo.existenceFilterCount()).isEqualTo(50); + + // Skip the verification of the bloom filter when testing against production because the bloom + // filter is only implemented in nightly. + // TODO(b/271949433) Remove this "if" block once the bloom filter logic is deployed to + // production. + if (IntegrationTestUtil.getTargetBackend() != IntegrationTestUtil.TargetBackend.NIGHTLY) { + return; + } + + // Verify that Watch sent a valid bloom filter. + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterBloomFilterInfo bloomFilter = existenceFilterMismatchInfo.bloomFilter(); + assertWithMessage("The bloom filter specified in the existence filter").that(bloomFilter).isNotNull(); + assertWithMessage("hashCount").that(bloomFilter.hashCount()).isGreaterThan(0); + assertWithMessage("bitmapLength").that(bloomFilter.bitmapLength()).isGreaterThan(0); + assertWithMessage("padding").that(bloomFilter.padding()).isGreaterThan(0); + assertWithMessage("padding").that(bloomFilter.padding()).isLessThan(8); + + // Verify that the bloom filter was successfully used to avert a full requery. If a false + // positive occurred then retry the entire test. Although statistically rare, false positives + // are expected to happen occasionally. When a false positive _does_ happen, just retry the + // test with a different set of documents. If that retry _also_ experiences a false positive, + // then fail the test because that is so improbable that something must have gone wrong. + if (attemptNumber == 1 && ! bloomFilter.applied()) { + continue; + } + + assertWithMessage("bloom filter successfully applied with attemptNumber=" + attemptNumber).that(bloomFilter.applied()).isTrue(); } } From a44de2e0f01d5c4e0eabc94ea6d1da06849373ad Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 14:56:22 -0500 Subject: [PATCH 6/9] ../gradlew googleJavaFormat --- .../google/firebase/firestore/QueryTest.java | 68 ++++++++++++------- ...hChangeAggregatorTestingHooksAccessor.java | 61 +++++++++-------- 2 files changed, 79 insertions(+), 50 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index ce1cf4aeec7..393be68ef1b 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -31,10 +31,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assume.assumeFalse; import android.os.SystemClock; - import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; @@ -1073,7 +1071,7 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except .getFirestore() .runTransaction( transaction -> { - for (int i = 0; i < createdDocuments.size(); i+=2) { + for (int i = 0; i < createdDocuments.size(); i += 2) { DocumentReference documentToDelete = createdDocuments.get(i); transaction.delete(documentToDelete); deletedDocumentIds.add(documentToDelete.getId()); @@ -1089,8 +1087,10 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except // testing hooks to "capture" the existence filter mismatches to verify that Watch sent a // bloom filter, and it was used to avert a full requery. QuerySnapshot snapshot2; - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo existenceFilterMismatchInfo; - ExistenceFilterMismatchAccumulator existenceFilterMismatchAccumulator = new ExistenceFilterMismatchAccumulator(); + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + existenceFilterMismatchInfo; + ExistenceFilterMismatchAccumulator existenceFilterMismatchAccumulator = + new ExistenceFilterMismatchAccumulator(); existenceFilterMismatchAccumulator.register(); try { snapshot2 = waitFor(collection.get()); @@ -1099,7 +1099,9 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except if (isRunningAgainstEmulator()) { existenceFilterMismatchInfo = null; } else { - existenceFilterMismatchInfo = existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch(/*timeoutMillis=*/5000); + existenceFilterMismatchInfo = + existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch( + /*timeoutMillis=*/ 5000); } } finally { existenceFilterMismatchAccumulator.unregister(); @@ -1122,7 +1124,9 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except expectedDocumentIds.add(documentRef.getId()); } } - assertWithMessage("snapshot2.docs").that(actualDocumentIds).containsExactlyElementsIn(expectedDocumentIds); + assertWithMessage("snapshot2.docs") + .that(actualDocumentIds) + .containsExactlyElementsIn(expectedDocumentIds); } // Skip the verification of the existence filter mismatch when testing against the Firestore @@ -1135,9 +1139,15 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except // Verify that Watch sent an existence filter with the correct counts when the query was // resumed. - assertWithMessage("Watch should have sent an existence filter").that(existenceFilterMismatchInfo).isNotNull(); - assertWithMessage("localCacheCount").that(existenceFilterMismatchInfo.localCacheCount()).isEqualTo(100); - assertWithMessage("existenceFilterCount").that(existenceFilterMismatchInfo.existenceFilterCount()).isEqualTo(50); + assertWithMessage("Watch should have sent an existence filter") + .that(existenceFilterMismatchInfo) + .isNotNull(); + assertWithMessage("localCacheCount") + .that(existenceFilterMismatchInfo.localCacheCount()) + .isEqualTo(100); + assertWithMessage("existenceFilterCount") + .that(existenceFilterMismatchInfo.existenceFilterCount()) + .isEqualTo(50); // Skip the verification of the bloom filter when testing against production because the bloom // filter is only implemented in nightly. @@ -1148,8 +1158,11 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except } // Verify that Watch sent a valid bloom filter. - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterBloomFilterInfo bloomFilter = existenceFilterMismatchInfo.bloomFilter(); - assertWithMessage("The bloom filter specified in the existence filter").that(bloomFilter).isNotNull(); + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterBloomFilterInfo bloomFilter = + existenceFilterMismatchInfo.bloomFilter(); + assertWithMessage("The bloom filter specified in the existence filter") + .that(bloomFilter) + .isNotNull(); assertWithMessage("hashCount").that(bloomFilter.hashCount()).isGreaterThan(0); assertWithMessage("bitmapLength").that(bloomFilter.bitmapLength()).isGreaterThan(0); assertWithMessage("padding").that(bloomFilter.padding()).isGreaterThan(0); @@ -1160,24 +1173,28 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except // are expected to happen occasionally. When a false positive _does_ happen, just retry the // test with a different set of documents. If that retry _also_ experiences a false positive, // then fail the test because that is so improbable that something must have gone wrong. - if (attemptNumber == 1 && ! bloomFilter.applied()) { + if (attemptNumber == 1 && !bloomFilter.applied()) { continue; } - assertWithMessage("bloom filter successfully applied with attemptNumber=" + attemptNumber).that(bloomFilter.applied()).isTrue(); + assertWithMessage("bloom filter successfully applied with attemptNumber=" + attemptNumber) + .that(bloomFilter.applied()) + .isTrue(); } } private static final class ExistenceFilterMismatchAccumulator { - private final ExistenceFilterMismatchListenerImpl listener = new ExistenceFilterMismatchListenerImpl(); + private final ExistenceFilterMismatchListenerImpl listener = + new ExistenceFilterMismatchListenerImpl(); private ListenerRegistration listenerRegistration = null; void register() { if (listenerRegistration != null) { throw new IllegalStateException("already registered"); } - listenerRegistration = WatchChangeAggregatorTestingHooksAccessor.addExistenceFilterMismatchListener(listener); + listenerRegistration = + WatchChangeAggregatorTestingHooksAccessor.addExistenceFilterMismatchListener(listener); } void unregister() { @@ -1189,19 +1206,24 @@ void unregister() { } @Nullable - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (listenerRegistration == null) { - throw new IllegalStateException("must be registered before waiting for an existence filter mismatch"); + throw new IllegalStateException( + "must be registered before waiting for an existence filter mismatch"); } return listener.waitForExistenceFilterMismatch(timeoutMillis); } - private final class ExistenceFilterMismatchListenerImpl implements WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchListener { + private final class ExistenceFilterMismatchListenerImpl + implements WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchListener { - private final ArrayList existenceFilterMismatches = new ArrayList<>(); + private final ArrayList + existenceFilterMismatches = new ArrayList<>(); @Override - public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { + public void onExistenceFilterMismatch( + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { synchronized (existenceFilterMismatches) { existenceFilterMismatches.add(info); existenceFilterMismatches.notifyAll(); @@ -1209,7 +1231,8 @@ public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooksAccessor. } @Nullable - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (timeoutMillis <= 0) { throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); } @@ -1228,7 +1251,6 @@ WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo waitForExi } } } - } // TODO(orquery): Enable this test when prod supports OR queries. diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java index 6c8616720d8..c0f1114617b 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java @@ -19,13 +19,12 @@ import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.Nullable; - import com.google.firebase.firestore.ListenerRegistration; /** * Provides access to the {@link WatchChangeAggregatorTestingHooks} class and its methods. * - * The {@link WatchChangeAggregatorTestingHooks} class has default visibility, and, therefore, is + *

The {@link WatchChangeAggregatorTestingHooks} class has default visibility, and, therefore, is * only visible to other classes declared in the same package. This class effectively "re-exports" * the functionality from {@link WatchChangeAggregatorTestingHooks} in a class with {@code public} * visibility so that tests written in other packages can access its functionality. @@ -34,47 +33,48 @@ public final class WatchChangeAggregatorTestingHooksAccessor { private WatchChangeAggregatorTestingHooksAccessor() {} - /** - * @see WatchChangeAggregatorTestingHooks#addExistenceFilterMismatchListener - */ + /** @see WatchChangeAggregatorTestingHooks#addExistenceFilterMismatchListener */ public static ListenerRegistration addExistenceFilterMismatchListener( - @NonNull ExistenceFilterMismatchListener listener) { + @NonNull ExistenceFilterMismatchListener listener) { checkNotNull(listener, "a null listener is not allowed"); - return WatchChangeAggregatorTestingHooks.addExistenceFilterMismatchListener(new ExistenceFilterMismatchListenerWrapper(listener)); + return WatchChangeAggregatorTestingHooks.addExistenceFilterMismatchListener( + new ExistenceFilterMismatchListenerWrapper(listener)); } - /** - * @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener - */ + /** @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener */ public interface ExistenceFilterMismatchListener { @AnyThread void onExistenceFilterMismatch(ExistenceFilterMismatchInfo info); } - /** - * @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo - */ + /** @see WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo */ public interface ExistenceFilterMismatchInfo { int localCacheCount(); + int existenceFilterCount(); - @Nullable ExistenceFilterBloomFilterInfo bloomFilter(); + + @Nullable + ExistenceFilterBloomFilterInfo bloomFilter(); } - /** - * @see WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo - */ + /** @see WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo */ public interface ExistenceFilterBloomFilterInfo { boolean applied(); + int hashCount(); + int bitmapLength(); + int padding(); } - private static final class ExistenceFilterMismatchInfoImpl implements ExistenceFilterMismatchInfo { + private static final class ExistenceFilterMismatchInfoImpl + implements ExistenceFilterMismatchInfo { private final WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info; - ExistenceFilterMismatchInfoImpl(@NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { + ExistenceFilterMismatchInfoImpl( + @NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { this.info = info; } @@ -91,16 +91,21 @@ public int existenceFilterCount() { @Nullable @Override public ExistenceFilterBloomFilterInfo bloomFilter() { - WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo bloomFilterInfo = info.bloomFilter(); - return bloomFilterInfo == null ? null : new ExistenceFilterBloomFilterInfoImpl(bloomFilterInfo); + WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo bloomFilterInfo = + info.bloomFilter(); + return bloomFilterInfo == null + ? null + : new ExistenceFilterBloomFilterInfoImpl(bloomFilterInfo); } } - private static final class ExistenceFilterBloomFilterInfoImpl implements ExistenceFilterBloomFilterInfo { + private static final class ExistenceFilterBloomFilterInfoImpl + implements ExistenceFilterBloomFilterInfo { private final WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo info; - ExistenceFilterBloomFilterInfoImpl(@NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo info) { + ExistenceFilterBloomFilterInfoImpl( + @NonNull WatchChangeAggregatorTestingHooks.ExistenceFilterBloomFilterInfo info) { this.info = info; } @@ -125,18 +130,20 @@ public int padding() { } } - private static final class ExistenceFilterMismatchListenerWrapper implements WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener { + private static final class ExistenceFilterMismatchListenerWrapper + implements WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchListener { private final ExistenceFilterMismatchListener wrappedListener; - ExistenceFilterMismatchListenerWrapper(@NonNull ExistenceFilterMismatchListener listenerToWrap) { + ExistenceFilterMismatchListenerWrapper( + @NonNull ExistenceFilterMismatchListener listenerToWrap) { this.wrappedListener = listenerToWrap; } @Override - public void onExistenceFilterMismatch(WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { + public void onExistenceFilterMismatch( + WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo info) { this.wrappedListener.onExistenceFilterMismatch(new ExistenceFilterMismatchInfoImpl(info)); } } - } From b6d62b5dc8a6d693f6896d3beba311da54bf537b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 16:44:50 -0500 Subject: [PATCH 7/9] minor refactor --- .../google/firebase/firestore/QueryTest.java | 77 +----------------- ...hChangeAggregatorTestingHooksAccessor.java | 79 +++++++++++++++++++ 2 files changed, 82 insertions(+), 74 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 393be68ef1b..e2be229fd38 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -32,8 +32,6 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import android.os.SystemClock; -import androidx.annotation.Nullable; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; import com.google.common.collect.Lists; @@ -1089,8 +1087,9 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except QuerySnapshot snapshot2; WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo existenceFilterMismatchInfo; - ExistenceFilterMismatchAccumulator existenceFilterMismatchAccumulator = - new ExistenceFilterMismatchAccumulator(); + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator + existenceFilterMismatchAccumulator = + new WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator(); existenceFilterMismatchAccumulator.register(); try { snapshot2 = waitFor(collection.get()); @@ -1183,76 +1182,6 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except } } - private static final class ExistenceFilterMismatchAccumulator { - - private final ExistenceFilterMismatchListenerImpl listener = - new ExistenceFilterMismatchListenerImpl(); - private ListenerRegistration listenerRegistration = null; - - void register() { - if (listenerRegistration != null) { - throw new IllegalStateException("already registered"); - } - listenerRegistration = - WatchChangeAggregatorTestingHooksAccessor.addExistenceFilterMismatchListener(listener); - } - - void unregister() { - if (listenerRegistration == null) { - return; - } - listenerRegistration.remove(); - listenerRegistration = null; - } - - @Nullable - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo - waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { - if (listenerRegistration == null) { - throw new IllegalStateException( - "must be registered before waiting for an existence filter mismatch"); - } - return listener.waitForExistenceFilterMismatch(timeoutMillis); - } - - private final class ExistenceFilterMismatchListenerImpl - implements WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchListener { - - private final ArrayList - existenceFilterMismatches = new ArrayList<>(); - - @Override - public void onExistenceFilterMismatch( - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { - synchronized (existenceFilterMismatches) { - existenceFilterMismatches.add(info); - existenceFilterMismatches.notifyAll(); - } - } - - @Nullable - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo - waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { - if (timeoutMillis <= 0) { - throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); - } - synchronized (existenceFilterMismatches) { - long endTimeMillis = SystemClock.uptimeMillis() + timeoutMillis; - while (true) { - if (existenceFilterMismatches.size() > 0) { - return existenceFilterMismatches.remove(0); - } - long currentWaitMillis = endTimeMillis - SystemClock.uptimeMillis(); - if (currentWaitMillis <= 0) { - return null; - } - existenceFilterMismatches.wait(currentWaitMillis); - } - } - } - } - } - // TODO(orquery): Enable this test when prod supports OR queries. @Ignore @Test diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java index c0f1114617b..836d32d9132 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java @@ -16,10 +16,12 @@ import static com.google.firebase.firestore.util.Preconditions.checkNotNull; +import android.os.SystemClock; import androidx.annotation.AnyThread; import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.firebase.firestore.ListenerRegistration; +import java.util.ArrayList; /** * Provides access to the {@link WatchChangeAggregatorTestingHooks} class and its methods. @@ -146,4 +148,81 @@ public void onExistenceFilterMismatch( this.wrappedListener.onExistenceFilterMismatch(new ExistenceFilterMismatchInfoImpl(info)); } } + + public static final class ExistenceFilterMismatchAccumulator { + + private ExistenceFilterMismatchListenerImpl listener; + private ListenerRegistration listenerRegistration = null; + + /** Registers the accumulator to begin listening for existence filter mismatches. */ + public synchronized void register() { + if (listener != null) { + throw new IllegalStateException("already registered"); + } + listener = new ExistenceFilterMismatchListenerImpl(); + listenerRegistration = + WatchChangeAggregatorTestingHooksAccessor.addExistenceFilterMismatchListener(listener); + } + + /** Unregisters the accumulator from listening for existence filter mismatches. */ + public synchronized void unregister() { + if (listener == null) { + return; + } + listenerRegistration.remove(); + listenerRegistration = null; + listener = null; + } + + @Nullable + public WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + ExistenceFilterMismatchListenerImpl capturedListener; + synchronized (this) { + capturedListener = listener; + } + if (capturedListener == null) { + throw new IllegalStateException( + "must be registered before waiting for an existence filter mismatch"); + } + return capturedListener.waitForExistenceFilterMismatch(timeoutMillis); + } + + private static final class ExistenceFilterMismatchListenerImpl + implements WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchListener { + + private final ArrayList existenceFilterMismatches = + new ArrayList<>(); + + @Override + public void onExistenceFilterMismatch( + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo info) { + synchronized (existenceFilterMismatches) { + existenceFilterMismatches.add(info); + existenceFilterMismatches.notifyAll(); + } + } + + @Nullable + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + waitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { + if (timeoutMillis <= 0) { + throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); + } + synchronized (existenceFilterMismatches) { + long endTimeMillis = SystemClock.uptimeMillis() + timeoutMillis; + while (true) { + if (existenceFilterMismatches.size() > 0) { + return existenceFilterMismatches.remove(0); + } + long currentWaitMillis = endTimeMillis - SystemClock.uptimeMillis(); + if (currentWaitMillis <= 0) { + return null; + } + existenceFilterMismatches.wait(currentWaitMillis); + } + } + } + } + } } From 6d010985f90d2e9bf7bd97218001ebb036f86768 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 22:32:17 -0500 Subject: [PATCH 8/9] Remove @Ignore annotation left behind by a bad merge --- .../java/com/google/firebase/firestore/QueryTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index e85b07c2692..5f84e0a0c99 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -1181,8 +1181,6 @@ public void resumingAQueryShouldUseBloomFilterToAvoidFullRequery() throws Except } } - // TODO(orquery): Enable this test when prod supports OR queries. - @Ignore @Test public void testOrQueries() { Map> testDocs = From e38db6408e74422bb61caabb4700da19a14d0fa2 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 10 Mar 2023 23:34:42 -0500 Subject: [PATCH 9/9] Add missing import of assumeTrue, which was also lost during a bad merge --- .../java/com/google/firebase/firestore/QueryTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java index 5f84e0a0c99..ef11f4006ad 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java @@ -31,6 +31,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeTrue; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task;