From 13aae0510a5e24c15251cd9dbbd88c7d9c829204 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 23 Mar 2023 09:47:02 -0400 Subject: [PATCH 1/4] Firestore: QueryTest.java: improve the test that resumes a query with existence filter to actually validate the existence filter. This is a port of https://github.com/firebase/firebase-js-sdk/pull/7149, and builds upon the test added in https://github.com/firebase/firebase-android-sdk/pull/4799. --- .../google/firebase/firestore/QueryTest.java | 48 ++++- ...hChangeAggregatorTestingHooksAccessor.java | 173 ++++++++++++++++++ .../remote/WatchChangeAggregator.java | 4 + .../WatchChangeAggregatorTestingHooks.java | 110 +++++++++++ 4 files changed, 333 insertions(+), 2 deletions(-) create mode 100644 firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.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 f607ddbbc85..ca88cd11ecb 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 @@ -37,6 +37,7 @@ 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; @@ -1053,6 +1054,7 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep createdDocuments.add(documentSnapshot.getReference()); } } + assertWithMessage("createdDocuments").that(createdDocuments).hasSize(100); // Delete 50 of the 100 documents. Do this in a transaction, rather than // DocumentReference.delete(), to avoid affecting the local cache. @@ -1069,13 +1071,35 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep } return null; })); + assertWithMessage("deletedDocumentIds").that(deletedDocumentIds).hasSize(50); // 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. - QuerySnapshot snapshot2 = waitFor(collection.get()); + // Resume the query and save the resulting snapshot for verification. Use some internal testing + // hooks to "capture" the existence filter mismatches to verify them. + QuerySnapshot snapshot2; + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo + existenceFilterMismatchInfo; + WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator + existenceFilterMismatchAccumulator = + new WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator(); + existenceFilterMismatchAccumulator.register(); + try { + snapshot2 = waitFor(collection.get()); + // 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(); + } // Verify that the snapshot from the resumed query contains the expected documents; that is, // that it contains the 50 documents that were _not_ deleted. @@ -1098,6 +1122,26 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep .that(actualDocumentIds) .containsExactlyElementsIn(expectedDocumentIds); } + + // Skip the verification of the existence filter mismatch when testing against the Firestore + // emulator because the Firestore emulator fails to to send an existence filter at all. + // TODO(b/270731363): Enable the verification of the existence filter mismatch once the + // Firestore emulator is fixed to send an existence filter. + if (isRunningAgainstEmulator()) { + return; + } + + // 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); } @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 new file mode 100644 index 00000000000..e1a386f0066 --- /dev/null +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java @@ -0,0 +1,173 @@ +// 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 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. + * + *

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(); + } + + 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(); + } + } + + 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)); + } + } + + 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); + } + } + } + } + } +} 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 1086af504db..a9fb909a24f 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 @@ -202,6 +202,10 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) { // `isFromCache:true`. resetTarget(targetId); pendingTargetResets.add(targetId); + + WatchChangeAggregatorTestingHooks.notifyOnExistenceFilterMismatch( + WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo.from( + (int) 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..9e686c9202e --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java @@ -0,0 +1,110 @@ +// 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.VisibleForTesting; +import com.google.auto.value.AutoValue; +import com.google.firebase.firestore.ListenerRegistration; +import com.google.firebase.firestore.util.Executors; +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) { + return new AutoValue_WatchChangeAggregatorTestingHooks_ExistenceFilterMismatchInfo( + localCacheCount, existenceFilterCount); + } + + /** 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(); + + static ExistenceFilterMismatchInfo from(int localCacheCount, ExistenceFilter existenceFilter) { + return create(localCacheCount, existenceFilter.getCount()); + } + } +} From 81a8f7360a4fb8c1bdead7bf9dca765cf6e4243e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 23 Mar 2023 21:24:21 -0400 Subject: [PATCH 2/4] Refactor: WatchChangeAggregatorTestingHooks -> TestingHooks and WatchChangeAggregatorTestingHooksAccessor -> ExistenceFilterMismatchListener --- .../google/firebase/firestore/QueryTest.java | 17 +- .../ExistenceFilterMismatchListener.java | 143 +++++++++++++++ ...hChangeAggregatorTestingHooksAccessor.java | 173 ------------------ ...torTestingHooks.java => TestingHooks.java} | 58 ++++-- .../remote/WatchChangeAggregator.java | 7 +- 5 files changed, 201 insertions(+), 197 deletions(-) create mode 100644 firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java delete mode 100644 firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java rename firebase-firestore/src/main/java/com/google/firebase/firestore/remote/{WatchChangeAggregatorTestingHooks.java => TestingHooks.java} (70%) 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 ca88cd11ecb..3464151b5e1 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 @@ -37,7 +37,7 @@ 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.remote.ExistenceFilterMismatchListener; import com.google.firebase.firestore.testutil.EventAccumulator; import com.google.firebase.firestore.testutil.IntegrationTestUtil; import java.util.ArrayList; @@ -1079,14 +1079,12 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep // Resume the query and save the resulting snapshot for verification. Use some internal testing // hooks to "capture" the existence filter mismatches to verify them. + ExistenceFilterMismatchListener existenceFilterMismatchListener = + new ExistenceFilterMismatchListener(); QuerySnapshot snapshot2; - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchInfo - existenceFilterMismatchInfo; - WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator - existenceFilterMismatchAccumulator = - new WatchChangeAggregatorTestingHooksAccessor.ExistenceFilterMismatchAccumulator(); - existenceFilterMismatchAccumulator.register(); + ExistenceFilterMismatchListener.ExistenceFilterMismatchInfo existenceFilterMismatchInfo; try { + existenceFilterMismatchListener.startListening(); snapshot2 = waitFor(collection.get()); // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed // to send an existence filter. @@ -1094,11 +1092,10 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep existenceFilterMismatchInfo = null; } else { existenceFilterMismatchInfo = - existenceFilterMismatchAccumulator.waitForExistenceFilterMismatch( - /*timeoutMillis=*/ 5000); + existenceFilterMismatchListener.waitForExistenceFilterMismatch(/*timeoutMillis=*/ 5000); } } finally { - existenceFilterMismatchAccumulator.unregister(); + existenceFilterMismatchListener.stopListening(); } // Verify that the snapshot from the resumed query contains the expected documents; that is, diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java new file mode 100644 index 00000000000..23a2bbc0d7e --- /dev/null +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java @@ -0,0 +1,143 @@ +// 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 android.os.SystemClock; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import com.google.firebase.firestore.ListenerRegistration; +import java.util.ArrayList; + +/** + * Provides a mechanism for tests to listen for existence filter mismatches in the Watch "listen" + * stream. + */ +public final class ExistenceFilterMismatchListener { + + private TestingHooksExistenceFilterMismatchListenerImpl listener; + private ListenerRegistration listenerRegistration; + + /** + * Starts listening for existence filter mismatches. + * + * @throws IllegalStateException if this object is already started. + * @see #stopListening + */ + public synchronized void startListening() { + if (listener != null) { + throw new IllegalStateException("already registered"); + } + listener = new TestingHooksExistenceFilterMismatchListenerImpl(); + listenerRegistration = TestingHooks.getInstance().addExistenceFilterMismatchListener(listener); + } + + /** + * Stops listening for existence filter mismatches. + * + *

If listening has not been started then this method does nothing. + * + * @see #startListening + */ + public synchronized void stopListening() { + if (listenerRegistration != null) { + listenerRegistration.remove(); + } + listenerRegistration = null; + listener = null; + } + + /** + * Waits for an existence filter mismatch. + * + * @param timeoutMillis the amount of time, in milliseconds, to wait for an existence filter + * mismatch. + * @return information about the existence filter mismatch that occurred. + * @throws InterruptedException if waiting is interrupted. + * @throws IllegalStateException if this object has not been started by {@link #startListening}. + * @throws IllegalArgumentException if the given timeout is less than or equal to zero. + */ + @Nullable + public ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) + throws InterruptedException { + if (timeoutMillis <= 0) { + throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); + } + + TestingHooksExistenceFilterMismatchListenerImpl registeredListener; + synchronized (this) { + registeredListener = listener; + } + + if (registeredListener == null) { + throw new IllegalStateException( + "must be registered before waiting for an existence filter mismatch"); + } + + return registeredListener.waitForExistenceFilterMismatch(timeoutMillis); + } + + private static final class TestingHooksExistenceFilterMismatchListenerImpl + implements TestingHooks.ExistenceFilterMismatchListener { + + private final ArrayList existenceFilterMismatches = + new ArrayList<>(); + + @Override + public synchronized void onExistenceFilterMismatch( + @NonNull TestingHooks.ExistenceFilterMismatchInfo info) { + existenceFilterMismatches.add(new ExistenceFilterMismatchInfo(info)); + notifyAll(); + } + + @Nullable + synchronized ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) + throws InterruptedException { + if (timeoutMillis <= 0) { + throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); + } + + 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; + } + + wait(currentWaitMillis); + } + } + } + + /** @see TestingHooks.ExistenceFilterMismatchInfo */ + public static final class ExistenceFilterMismatchInfo { + + private final TestingHooks.ExistenceFilterMismatchInfo info; + + ExistenceFilterMismatchInfo(@NonNull TestingHooks.ExistenceFilterMismatchInfo info) { + this.info = info; + } + + public int localCacheCount() { + return info.localCacheCount(); + } + + public int existenceFilterCount() { + return info.existenceFilterCount(); + } + } +} 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 deleted file mode 100644 index e1a386f0066..00000000000 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooksAccessor.java +++ /dev/null @@ -1,173 +0,0 @@ -// 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 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. - * - *

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(); - } - - 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(); - } - } - - 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)); - } - } - - 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); - } - } - } - } - } -} 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/TestingHooks.java similarity index 70% rename from firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java rename to firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java index 9e686c9202e..2e98c70f6eb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregatorTestingHooks.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java @@ -25,12 +25,27 @@ import java.util.HashMap; import java.util.Map; -final class WatchChangeAggregatorTestingHooks { - - private WatchChangeAggregatorTestingHooks() {} - - private static final Map - existenceFilterMismatchListeners = new HashMap<>(); +/** + * Manages "testing hooks", hooks into the internals of the SDK to verify internal state and events + * during integration tests. + * + *

Do not use this class except for testing purposes. + */ +@VisibleForTesting +final class TestingHooks { + + private static final TestingHooks instance = new TestingHooks(); + + private final Map existenceFilterMismatchListeners = + new HashMap<>(); + + private TestingHooks() {} + + /** Returns the singleton instance of this class. */ + @NonNull + static TestingHooks getInstance() { + return instance; + } /** * Notifies all registered {@link ExistenceFilterMismatchListener}` listeners registered via @@ -38,7 +53,7 @@ private WatchChangeAggregatorTestingHooks() {} * * @param info Information about the existence filter mismatch to deliver to the listeners. */ - static void notifyOnExistenceFilterMismatch(ExistenceFilterMismatchInfo info) { + void notifyOnExistenceFilterMismatch(@NonNull ExistenceFilterMismatchInfo info) { synchronized (existenceFilterMismatchListeners) { for (ExistenceFilterMismatchListener listener : existenceFilterMismatchListeners.values()) { Executors.BACKGROUND_EXECUTOR.execute(() -> listener.onExistenceFilterMismatch(info)); @@ -64,8 +79,7 @@ static void notifyOnExistenceFilterMismatch(ExistenceFilterMismatchInfo info) { * ListenerRegistration#remove} method; only the first unregistration request does anything; * all subsequent requests do nothing. */ - @VisibleForTesting - static ListenerRegistration addExistenceFilterMismatchListener( + ListenerRegistration addExistenceFilterMismatchListener( @NonNull ExistenceFilterMismatchListener listener) { checkNotNull(listener, "a null listener is not allowed"); @@ -81,16 +95,34 @@ static ListenerRegistration addExistenceFilterMismatchListener( }; } + /** + * Implementations of this interface can be registered with {@link + * #addExistenceFilterMismatchListener}. + */ interface ExistenceFilterMismatchListener { + + /** + * Invoked when an existence filter mismatch occurs. + * + * @param info information about the existence filter mismatch. + */ @AnyThread - void onExistenceFilterMismatch(ExistenceFilterMismatchInfo info); + void onExistenceFilterMismatch(@NonNull ExistenceFilterMismatchInfo info); } + /** + * Information about an existence filter mismatch, as specified to listeners registered with + * {@link #addExistenceFilterMismatchListener}. + */ @AutoValue abstract static class ExistenceFilterMismatchInfo { + /** + * Creates and returns a new instance of {@link ExistenceFilterMismatchInfo} with the given + * values. + */ static ExistenceFilterMismatchInfo create(int localCacheCount, int existenceFilterCount) { - return new AutoValue_WatchChangeAggregatorTestingHooks_ExistenceFilterMismatchInfo( + return new AutoValue_TestingHooks_ExistenceFilterMismatchInfo( localCacheCount, existenceFilterCount); } @@ -103,6 +135,10 @@ static ExistenceFilterMismatchInfo create(int localCacheCount, int existenceFilt */ abstract int existenceFilterCount(); + /** + * Convenience method to create and return a new instance of {@link ExistenceFilterMismatchInfo} + * with the values taken from the given arguments. + */ static ExistenceFilterMismatchInfo from(int localCacheCount, ExistenceFilter existenceFilter) { return create(localCacheCount, existenceFilter.getCount()); } 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 a9fb909a24f..157e73cf83f 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 @@ -203,9 +203,10 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) { resetTarget(targetId); pendingTargetResets.add(targetId); - WatchChangeAggregatorTestingHooks.notifyOnExistenceFilterMismatch( - WatchChangeAggregatorTestingHooks.ExistenceFilterMismatchInfo.from( - (int) currentSize, watchChange.getExistenceFilter())); + TestingHooks.getInstance() + .notifyOnExistenceFilterMismatch( + TestingHooks.ExistenceFilterMismatchInfo.from( + (int) currentSize, watchChange.getExistenceFilter())); } } } From db3e7c38b85650fa513bff0c46c00c1e2e65b19d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 23 Mar 2023 21:40:44 -0400 Subject: [PATCH 3/4] cleanup: rename waitForExistenceFilterMismatch -> getOrWaitForExistenceFilterMismatch --- .../google/firebase/firestore/QueryTest.java | 3 ++- .../ExistenceFilterMismatchListener.java | 19 +++++++++++++------ 2 files changed, 15 insertions(+), 7 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 3464151b5e1..b96986afcfd 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 @@ -1092,7 +1092,8 @@ public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Excep existenceFilterMismatchInfo = null; } else { existenceFilterMismatchInfo = - existenceFilterMismatchListener.waitForExistenceFilterMismatch(/*timeoutMillis=*/ 5000); + existenceFilterMismatchListener.getOrWaitForExistenceFilterMismatch( + /*timeoutMillis=*/ 5000); } } finally { existenceFilterMismatchListener.stopListening(); diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java index 23a2bbc0d7e..6f185deaba7 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/ExistenceFilterMismatchListener.java @@ -59,17 +59,24 @@ public synchronized void stopListening() { } /** - * Waits for an existence filter mismatch. + * Returns the oldest existence filter mismatch observed, waiting if none has yet been observed. * - * @param timeoutMillis the amount of time, in milliseconds, to wait for an existence filter - * mismatch. + *

The oldest existence filter mismatch observed since the most recent successful invocation of + * {@link #startListening} will be returned. A subsequent invocation of this method will return + * the second-oldest existence filter mismatch observed, and so on. An invocation of {@link + * #stopListening} followed by another invocation of {@link #startListening} will discard any + * existence filter mismatches that occurred while previously started and will start observing + * afresh. + * + * @param timeoutMillis the maximum amount of time, in milliseconds, to wait for an existence + * filter mismatch to occur. * @return information about the existence filter mismatch that occurred. * @throws InterruptedException if waiting is interrupted. * @throws IllegalStateException if this object has not been started by {@link #startListening}. * @throws IllegalArgumentException if the given timeout is less than or equal to zero. */ @Nullable - public ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) + public ExistenceFilterMismatchInfo getOrWaitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (timeoutMillis <= 0) { throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); @@ -85,7 +92,7 @@ public ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMi "must be registered before waiting for an existence filter mismatch"); } - return registeredListener.waitForExistenceFilterMismatch(timeoutMillis); + return registeredListener.getOrWaitForExistenceFilterMismatch(timeoutMillis); } private static final class TestingHooksExistenceFilterMismatchListenerImpl @@ -102,7 +109,7 @@ public synchronized void onExistenceFilterMismatch( } @Nullable - synchronized ExistenceFilterMismatchInfo waitForExistenceFilterMismatch(long timeoutMillis) + synchronized ExistenceFilterMismatchInfo getOrWaitForExistenceFilterMismatch(long timeoutMillis) throws InterruptedException { if (timeoutMillis <= 0) { throw new IllegalArgumentException("invalid timeout: " + timeoutMillis); From f62d94f2b25d1ec19698b350a7334e3167cd2660 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 23 Mar 2023 22:22:46 -0400 Subject: [PATCH 4/4] TestingHooks.java: use CopyOnWriteArrayList and AtomicReference to manage the registered ExistenceFilterMismatchListener objects --- .../firestore/remote/TestingHooks.java | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java index 2e98c70f6eb..3a4c1611b63 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TestingHooks.java @@ -22,8 +22,8 @@ import com.google.auto.value.AutoValue; import com.google.firebase.firestore.ListenerRegistration; import com.google.firebase.firestore.util.Executors; -import java.util.HashMap; -import java.util.Map; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicReference; /** * Manages "testing hooks", hooks into the internals of the SDK to verify internal state and events @@ -36,8 +36,10 @@ final class TestingHooks { private static final TestingHooks instance = new TestingHooks(); - private final Map existenceFilterMismatchListeners = - new HashMap<>(); + // Use CopyOnWriteArrayList to store the listeners so that we don't need to worry about + // synchronizing adds, removes, and traversals. + private final CopyOnWriteArrayList> + existenceFilterMismatchListeners = new CopyOnWriteArrayList<>(); private TestingHooks() {} @@ -48,16 +50,21 @@ static TestingHooks getInstance() { } /** - * Notifies all registered {@link ExistenceFilterMismatchListener}` listeners registered via - * {@link #addExistenceFilterMismatchListener}. + * Asynchronously notifies all registered {@link ExistenceFilterMismatchListener}` listeners + * registered via {@link #addExistenceFilterMismatchListener}. * * @param info Information about the existence filter mismatch to deliver to the listeners. */ void notifyOnExistenceFilterMismatch(@NonNull ExistenceFilterMismatchInfo info) { - synchronized (existenceFilterMismatchListeners) { - for (ExistenceFilterMismatchListener listener : existenceFilterMismatchListeners.values()) { - Executors.BACKGROUND_EXECUTOR.execute(() -> listener.onExistenceFilterMismatch(info)); - } + for (AtomicReference listenerRef : + existenceFilterMismatchListeners) { + Executors.BACKGROUND_EXECUTOR.execute( + () -> { + ExistenceFilterMismatchListener listener = listenerRef.get(); + if (listener != null) { + listener.onExistenceFilterMismatch(info); + } + }); } } @@ -83,15 +90,12 @@ ListenerRegistration addExistenceFilterMismatchListener( @NonNull ExistenceFilterMismatchListener listener) { checkNotNull(listener, "a null listener is not allowed"); - Object listenerId = new Object(); - synchronized (existenceFilterMismatchListeners) { - existenceFilterMismatchListeners.put(listenerId, listener); - } + AtomicReference listenerRef = new AtomicReference<>(listener); + existenceFilterMismatchListeners.add(listenerRef); return () -> { - synchronized (existenceFilterMismatchListeners) { - existenceFilterMismatchListeners.remove(listenerId); - } + listenerRef.set(null); + existenceFilterMismatchListeners.remove(listenerRef); }; }