From 9967098225eff969ef8c6d0fdbaa5651f5be8e8c Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 09:19:20 -0800 Subject: [PATCH 01/10] WIP --- .../src/third_party/crashpad | 2 +- firebase-crashlytics-ndk/src/third_party/lss | 2 +- .../src/third_party/mini_chromium | 2 +- .../google/firebase/database/QueryTest.java | 18 +- .../google/firebase/database/core/Repo.java | 5 +- .../firebase/database/core/SyncTree.java | 159 +++++++++--------- 6 files changed, 107 insertions(+), 81 deletions(-) diff --git a/firebase-crashlytics-ndk/src/third_party/crashpad b/firebase-crashlytics-ndk/src/third_party/crashpad index c902f6b1c9e..e5e47bc2775 160000 --- a/firebase-crashlytics-ndk/src/third_party/crashpad +++ b/firebase-crashlytics-ndk/src/third_party/crashpad @@ -1 +1 @@ -Subproject commit c902f6b1c9e43224181969110b83e0053b2ddd3c +Subproject commit e5e47bc277532ae109a444cbb3646977c93fc077 diff --git a/firebase-crashlytics-ndk/src/third_party/lss b/firebase-crashlytics-ndk/src/third_party/lss index 9719c1e1e67..29f7c7e018f 160000 --- a/firebase-crashlytics-ndk/src/third_party/lss +++ b/firebase-crashlytics-ndk/src/third_party/lss @@ -1 +1 @@ -Subproject commit 9719c1e1e676814c456b55f5f070eabad6709d31 +Subproject commit 29f7c7e018f4ce706a709f0b0afbf8bacf869480 diff --git a/firebase-crashlytics-ndk/src/third_party/mini_chromium b/firebase-crashlytics-ndk/src/third_party/mini_chromium index 4332ddb6963..0e22eed71ee 160000 --- a/firebase-crashlytics-ndk/src/third_party/mini_chromium +++ b/firebase-crashlytics-ndk/src/third_party/mini_chromium @@ -1 +1 @@ -Subproject commit 4332ddb6963750e1106efdcece6d6e2de6dc6430 +Subproject commit 0e22eed71eec97dacbe80822a14c5cd0b580d793 diff --git a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java index d2826c7cded..d92a978a1f3 100644 --- a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java +++ b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java @@ -38,6 +38,9 @@ import com.google.firebase.database.core.RepoManager; import com.google.firebase.database.future.ReadFuture; import com.google.firebase.database.future.WriteFuture; + +import junit.framework.TestFailure; + import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -56,6 +59,9 @@ import org.junit.Rule; import org.junit.Test; +import kotlin.collections.builders.MapBuilder; + + @org.junit.runner.RunWith(AndroidJUnit4.class) public class QueryTest { @Rule public RetryRule retryRule = new RetryRule(3); @@ -449,7 +455,7 @@ public void passingInvalidKeysToStartAfterOrEndBeforeThrows() throws DatabaseExc @Test public void listenerCanBeRemovedFromSpecificQuery() - throws DatabaseException, TestFailure, ExecutionException, TimeoutException, + throws DatabaseException, ExecutionException, TimeoutException, InterruptedException { DatabaseReference ref = getRandomNode(); @@ -4617,6 +4623,16 @@ public void testGetReturnsNullForEmptyNodeWhenOnline() assertNull(await(db.getReference(UUID.randomUUID().toString()).get()).getValue()); } + @Test + public void testGetCleansUpTags() + throws DatabaseException, InterruptedException, ExecutionException { + FirebaseDatabase db = getNewDatabase(); + DatabaseReference myRef = db.getReference(UUID.randomUUID().toString()); + Query query = myRef.startAfter(1); + await(db.getReference(UUID.randomUUID().toString()).get()).getValue(); + assertNull(myRef.repo.serverSyncTree.tagForQuery(query.getSpec())); + } + @Test public void testGetWaitsForConnection() throws DatabaseException, InterruptedException { FirebaseDatabase db = getNewDatabase(); diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java index 160a5c6ff57..b7aca39cf81 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java @@ -17,6 +17,8 @@ import static com.google.firebase.database.core.utilities.Utilities.hardAssert; import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; + import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.database.DataSnapshot; @@ -78,7 +80,8 @@ public class Repo implements PersistentConnection.Delegate { public long dataUpdateCount = 0; // for testing. private long nextWriteId = 1; private SyncTree infoSyncTree; - private SyncTree serverSyncTree; + @VisibleForTesting + public SyncTree serverSyncTree; private FirebaseDatabase database; private boolean loggedTransactionPersistenceWarning = false; diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java index 4f22419fdc4..9b9413d44f9 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java @@ -16,6 +16,8 @@ import static com.google.firebase.database.core.utilities.Utilities.hardAssert; +import androidx.annotation.VisibleForTesting; + import com.google.firebase.database.DataSnapshot; import com.google.firebase.database.DatabaseError; import com.google.firebase.database.InternalHelpers; @@ -156,8 +158,10 @@ public List onListenComplete(DatabaseError error) { */ private final WriteTree pendingWriteTree; - private final Map tagToQueryMap; - private final Map queryToTagMap; + // TODO(mtewani): Come back to this. + @VisibleForTesting + public final Map tagToQueryMap; + public final Map queryToTagMap; private final Set keepSyncedQueries; private final ListenProvider listenProvider; private final PersistenceManager persistenceManager; @@ -704,85 +708,88 @@ public List call() { List removed = removedAndEvents.getFirst(); cancelEvents = removedAndEvents.getSecond(); - // We may have just removed one of many listeners and can short-circuit this whole - // process. We may also not have removed a default listener, in which case all of the - // descendant listeners should already be properly set up. - // - // Since indexed queries can shadow if they don't have other query constraints, check - // for loadsAllData(), instead of isDefault(). - boolean removingDefault = false; - for (QuerySpec queryRemoved : removed) { - persistenceManager.setQueryInactive(query); - removingDefault = removingDefault || queryRemoved.loadsAllData(); - } + if(!skipDedup) { + + // We may have just removed one of many listeners and can short-circuit this whole + // process. We may also not have removed a default listener, in which case all of the + // descendant listeners should already be properly set up. + // + // Since indexed queries can shadow if they don't have other query constraints, check + // for loadsAllData(), instead of isDefault(). + boolean removingDefault = false; + for (QuerySpec queryRemoved : removed) { + persistenceManager.setQueryInactive(query); + removingDefault = removingDefault || queryRemoved.loadsAllData(); + } - /** - * This is to handle removeRegistration by {@link Repo#getValue(Query)}. Specifically - * to avoid the scenario where: A listener is attached at a child path, and {@link - * Repo#getValue(Query)} is called on the parent path. Normally, when a listener is - * attached on a child path and then a parent path has a listener attached to it, to - * reduce the number of listeners, the listen() function will unlisten to the child - * path and listen instead on the parent path. And then, when removeRegistration is - * called on the parent path, the child path will get listened to, since it doesn't - * have anything covering its path. However, for {@link Repo#getValue(Query)}, we do - * not call listen on the parent path, and the child path is still listened to and so - * when the deduping happens below, the SyncTree assumes that the child listener has - * been removed and attempts to call listen again, but since we are still listening on - * that location, listen would be called twice on the same query. skipDedup allows us - * to skip this deduping process altogether. - */ - if (skipDedup) { - return null; - } - ImmutableTree currentTree = syncPointTree; - boolean covered = - currentTree.getValue() != null && currentTree.getValue().hasCompleteView(); - for (ChildKey component : path) { - currentTree = currentTree.getChild(component); - covered = - covered - || (currentTree.getValue() != null - && currentTree.getValue().hasCompleteView()); - if (covered || currentTree.isEmpty()) { - break; + /** + * This is to handle removeRegistration by {@link Repo#getValue(Query)}. Specifically + * to avoid the scenario where: A listener is attached at a child path, and {@link + * Repo#getValue(Query)} is called on the parent path. Normally, when a listener is + * attached on a child path and then a parent path has a listener attached to it, to + * reduce the number of listeners, the listen() function will unlisten to the child + * path and listen instead on the parent path. And then, when removeRegistration is + * called on the parent path, the child path will get listened to, since it doesn't + * have anything covering its path. However, for {@link Repo#getValue(Query)}, we do + * not call listen on the parent path, and the child path is still listened to and so + * when the deduping happens below, the SyncTree assumes that the child listener has + * been removed and attempts to call listen again, but since we are still listening on + * that location, listen would be called twice on the same query. skipDedup allows us + * to skip this deduping process altogether. + */ + if (skipDedup) { + return null; + } + ImmutableTree currentTree = syncPointTree; + boolean covered = + currentTree.getValue() != null && currentTree.getValue().hasCompleteView(); + for (ChildKey component : path) { + currentTree = currentTree.getChild(component); + covered = + covered + || (currentTree.getValue() != null + && currentTree.getValue().hasCompleteView()); + if (covered || currentTree.isEmpty()) { + break; + } } - } - if (removingDefault && !covered) { - ImmutableTree subtree = syncPointTree.subtree(path); - // There are potentially child listeners. Determine what if any listens we need to - // send before executing the removal. - if (!subtree.isEmpty()) { - // We need to fold over our subtree and collect the listeners to send - List newViews = collectDistinctViewsForSubTree(subtree); - - // Ok, we've collected all the listens we need. Set them up. - for (View view : newViews) { - ListenContainer container = new ListenContainer(view); - QuerySpec newQuery = view.getQuery(); - listenProvider.startListening( - queryForListening(newQuery), container.tag, container, container); + if (removingDefault && !covered) { + ImmutableTree subtree = syncPointTree.subtree(path); + // There are potentially child listeners. Determine what if any listens we need to + // send before executing the removal. + if (!subtree.isEmpty()) { + // We need to fold over our subtree and collect the listeners to send + List newViews = collectDistinctViewsForSubTree(subtree); + + // Ok, we've collected all the listens we need. Set them up. + for (View view : newViews) { + ListenContainer container = new ListenContainer(view); + QuerySpec newQuery = view.getQuery(); + listenProvider.startListening( + queryForListening(newQuery), container.tag, container, container); + } + } else { + // There's nothing below us, so nothing we need to start listening on } - } else { - // There's nothing below us, so nothing we need to start listening on } - } - // If we removed anything and we're not covered by a higher up listen, we need to stop - // listening on this query. The above block has us covered in terms of making sure - // we're set up on listens lower in the tree. - // Also, note that if we have a cancelError, it's already been removed at the provider - // level. - if (!covered && !removed.isEmpty() && cancelError == null) { - // If we removed a default, then we weren't listening on any of the other queries - // here. Just cancel the one default. Otherwise, we need to iterate through and - // cancel each individual query - if (removingDefault) { - listenProvider.stopListening(queryForListening(query), null); - } else { - for (QuerySpec queryToRemove : removed) { - Tag tag = tagForQuery(queryToRemove); - hardAssert(tag != null); - listenProvider.stopListening(queryForListening(queryToRemove), tag); + // If we removed anything and we're not covered by a higher up listen, we need to stop + // listening on this query. The above block has us covered in terms of making sure + // we're set up on listens lower in the tree. + // Also, note that if we have a cancelError, it's already been removed at the provider + // level. + if (!covered && !removed.isEmpty() && cancelError == null) { + // If we removed a default, then we weren't listening on any of the other queries + // here. Just cancel the one default. Otherwise, we need to iterate through and + // cancel each individual query + if (removingDefault) { + listenProvider.stopListening(queryForListening(query), null); + } else { + for (QuerySpec queryToRemove : removed) { + Tag tag = tagForQuery(queryToRemove); + hardAssert(tag != null); + listenProvider.stopListening(queryForListening(queryToRemove), tag); + } } } } From ded39b2f324c40846518d5ee91664557944b7505 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 09:34:49 -0800 Subject: [PATCH 02/10] Fixed tests --- .../google/firebase/database/QueryTest.java | 8 ++--- .../firebase/database/core/SyncTree.java | 34 +++++++++---------- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java index d92a978a1f3..075e942af91 100644 --- a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java +++ b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java @@ -39,8 +39,6 @@ import com.google.firebase.database.future.ReadFuture; import com.google.firebase.database.future.WriteFuture; -import junit.framework.TestFailure; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -59,8 +57,6 @@ import org.junit.Rule; import org.junit.Test; -import kotlin.collections.builders.MapBuilder; - @org.junit.runner.RunWith(AndroidJUnit4.class) public class QueryTest { @@ -455,7 +451,7 @@ public void passingInvalidKeysToStartAfterOrEndBeforeThrows() throws DatabaseExc @Test public void listenerCanBeRemovedFromSpecificQuery() - throws DatabaseException, ExecutionException, TimeoutException, + throws DatabaseException, TestFailure, ExecutionException, TimeoutException, InterruptedException { DatabaseReference ref = getRandomNode(); @@ -4629,7 +4625,7 @@ public void testGetCleansUpTags() FirebaseDatabase db = getNewDatabase(); DatabaseReference myRef = db.getReference(UUID.randomUUID().toString()); Query query = myRef.startAfter(1); - await(db.getReference(UUID.randomUUID().toString()).get()).getValue(); + await(query.get()).getValue(); assertNull(myRef.repo.serverSyncTree.tagForQuery(query.getSpec())); } diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java index 9b9413d44f9..22823ff05d7 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java @@ -708,6 +708,21 @@ public List call() { List removed = removedAndEvents.getFirst(); cancelEvents = removedAndEvents.getSecond(); + /** + * This is to handle removeRegistration by {@link Repo#getValue(Query)}. Specifically + * to avoid the scenario where: A listener is attached at a child path, and {@link + * Repo#getValue(Query)} is called on the parent path. Normally, when a listener is + * attached on a child path and then a parent path has a listener attached to it, to + * reduce the number of listeners, the listen() function will unlisten to the child + * path and listen instead on the parent path. And then, when removeRegistration is + * called on the parent path, the child path will get listened to, since it doesn't + * have anything covering its path. However, for {@link Repo#getValue(Query)}, we do + * not call listen on the parent path, and the child path is still listened to and so + * when the deduping happens below, the SyncTree assumes that the child listener has + * been removed and attempts to call listen again, but since we are still listening on + * that location, listen would be called twice on the same query. skipDedup allows us + * to skip this deduping process altogether. + */ if(!skipDedup) { // We may have just removed one of many listeners and can short-circuit this whole @@ -722,24 +737,7 @@ public List call() { removingDefault = removingDefault || queryRemoved.loadsAllData(); } - /** - * This is to handle removeRegistration by {@link Repo#getValue(Query)}. Specifically - * to avoid the scenario where: A listener is attached at a child path, and {@link - * Repo#getValue(Query)} is called on the parent path. Normally, when a listener is - * attached on a child path and then a parent path has a listener attached to it, to - * reduce the number of listeners, the listen() function will unlisten to the child - * path and listen instead on the parent path. And then, when removeRegistration is - * called on the parent path, the child path will get listened to, since it doesn't - * have anything covering its path. However, for {@link Repo#getValue(Query)}, we do - * not call listen on the parent path, and the child path is still listened to and so - * when the deduping happens below, the SyncTree assumes that the child listener has - * been removed and attempts to call listen again, but since we are still listening on - * that location, listen would be called twice on the same query. skipDedup allows us - * to skip this deduping process altogether. - */ - if (skipDedup) { - return null; - } + ImmutableTree currentTree = syncPointTree; boolean covered = currentTree.getValue() != null && currentTree.getValue().hasCompleteView(); From 26f07ed68e6b0abc17ff1bc067b5a1e92c544171 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 09:36:02 -0800 Subject: [PATCH 03/10] Removed firebase-crashlytics updates --- .gitmodules | 9 --------- firebase-crashlytics-ndk/src/third_party/crashpad | 1 - firebase-crashlytics-ndk/src/third_party/lss | 1 - firebase-crashlytics-ndk/src/third_party/mini_chromium | 1 - 4 files changed, 12 deletions(-) delete mode 160000 firebase-crashlytics-ndk/src/third_party/crashpad delete mode 160000 firebase-crashlytics-ndk/src/third_party/lss delete mode 160000 firebase-crashlytics-ndk/src/third_party/mini_chromium diff --git a/.gitmodules b/.gitmodules index 8b9fc0157f3..e69de29bb2d 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,9 +0,0 @@ -[submodule "firebase-crashlytics-ndk/src/third_party/crashpad"] - path = firebase-crashlytics-ndk/src/third_party/crashpad - url = https://chromium.googlesource.com/crashpad/crashpad.git -[submodule "firebase-crashlytics-ndk/src/third_party/mini_chromium"] - path = firebase-crashlytics-ndk/src/third_party/mini_chromium - url = https://chromium.googlesource.com/chromium/mini_chromium -[submodule "firebase-crashlytics-ndk/src/third_party/lss"] - path = firebase-crashlytics-ndk/src/third_party/lss - url = https://chromium.googlesource.com/linux-syscall-support.git diff --git a/firebase-crashlytics-ndk/src/third_party/crashpad b/firebase-crashlytics-ndk/src/third_party/crashpad deleted file mode 160000 index e5e47bc2775..00000000000 --- a/firebase-crashlytics-ndk/src/third_party/crashpad +++ /dev/null @@ -1 +0,0 @@ -Subproject commit e5e47bc277532ae109a444cbb3646977c93fc077 diff --git a/firebase-crashlytics-ndk/src/third_party/lss b/firebase-crashlytics-ndk/src/third_party/lss deleted file mode 160000 index 29f7c7e018f..00000000000 --- a/firebase-crashlytics-ndk/src/third_party/lss +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 29f7c7e018f4ce706a709f0b0afbf8bacf869480 diff --git a/firebase-crashlytics-ndk/src/third_party/mini_chromium b/firebase-crashlytics-ndk/src/third_party/mini_chromium deleted file mode 160000 index 0e22eed71ee..00000000000 --- a/firebase-crashlytics-ndk/src/third_party/mini_chromium +++ /dev/null @@ -1 +0,0 @@ -Subproject commit 0e22eed71eec97dacbe80822a14c5cd0b580d793 From fa74779edc6099481a1d385b40a76a5715250380 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 09:36:24 -0800 Subject: [PATCH 04/10] Updated gitmodules --- .gitmodules | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.gitmodules b/.gitmodules index e69de29bb2d..8b9fc0157f3 100644 --- a/.gitmodules +++ b/.gitmodules @@ -0,0 +1,9 @@ +[submodule "firebase-crashlytics-ndk/src/third_party/crashpad"] + path = firebase-crashlytics-ndk/src/third_party/crashpad + url = https://chromium.googlesource.com/crashpad/crashpad.git +[submodule "firebase-crashlytics-ndk/src/third_party/mini_chromium"] + path = firebase-crashlytics-ndk/src/third_party/mini_chromium + url = https://chromium.googlesource.com/chromium/mini_chromium +[submodule "firebase-crashlytics-ndk/src/third_party/lss"] + path = firebase-crashlytics-ndk/src/third_party/lss + url = https://chromium.googlesource.com/linux-syscall-support.git From b9b4915eec2fff6bca2e14a416397ae4e0aa4a2b Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 09:37:09 -0800 Subject: [PATCH 05/10] Reverted --- firebase-crashlytics-ndk/src/third_party/crashpad | 1 + firebase-crashlytics-ndk/src/third_party/lss | 1 + firebase-crashlytics-ndk/src/third_party/mini_chromium | 1 + 3 files changed, 3 insertions(+) create mode 160000 firebase-crashlytics-ndk/src/third_party/crashpad create mode 160000 firebase-crashlytics-ndk/src/third_party/lss create mode 160000 firebase-crashlytics-ndk/src/third_party/mini_chromium diff --git a/firebase-crashlytics-ndk/src/third_party/crashpad b/firebase-crashlytics-ndk/src/third_party/crashpad new file mode 160000 index 00000000000..c902f6b1c9e --- /dev/null +++ b/firebase-crashlytics-ndk/src/third_party/crashpad @@ -0,0 +1 @@ +Subproject commit c902f6b1c9e43224181969110b83e0053b2ddd3c diff --git a/firebase-crashlytics-ndk/src/third_party/lss b/firebase-crashlytics-ndk/src/third_party/lss new file mode 160000 index 00000000000..9719c1e1e67 --- /dev/null +++ b/firebase-crashlytics-ndk/src/third_party/lss @@ -0,0 +1 @@ +Subproject commit 9719c1e1e676814c456b55f5f070eabad6709d31 diff --git a/firebase-crashlytics-ndk/src/third_party/mini_chromium b/firebase-crashlytics-ndk/src/third_party/mini_chromium new file mode 160000 index 00000000000..4332ddb6963 --- /dev/null +++ b/firebase-crashlytics-ndk/src/third_party/mini_chromium @@ -0,0 +1 @@ +Subproject commit 4332ddb6963750e1106efdcece6d6e2de6dc6430 From 4a069c99bdbb562e24d9e43929587e82b40175bd Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 10:48:24 -0800 Subject: [PATCH 06/10] Fixed tests --- .../google/firebase/database/QueryTest.java | 19 +++++++--- .../google/firebase/database/core/Repo.java | 4 +-- .../firebase/database/core/SyncTree.java | 36 ++++++++++--------- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java index 075e942af91..7afdb956991 100644 --- a/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java +++ b/firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java @@ -38,7 +38,6 @@ import com.google.firebase.database.core.RepoManager; import com.google.firebase.database.future.ReadFuture; import com.google.firebase.database.future.WriteFuture; - import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -57,7 +56,6 @@ import org.junit.Rule; import org.junit.Test; - @org.junit.runner.RunWith(AndroidJUnit4.class) public class QueryTest { @Rule public RetryRule retryRule = new RetryRule(3); @@ -4621,12 +4619,25 @@ public void testGetReturnsNullForEmptyNodeWhenOnline() @Test public void testGetCleansUpTags() - throws DatabaseException, InterruptedException, ExecutionException { + throws DatabaseException, InterruptedException, ExecutionException, TimeoutException, + TestFailure { FirebaseDatabase db = getNewDatabase(); DatabaseReference myRef = db.getReference(UUID.randomUUID().toString()); Query query = myRef.startAfter(1); await(query.get()).getValue(); - assertNull(myRef.repo.serverSyncTree.tagForQuery(query.getSpec())); + /** + * If we add a listener for the same query and the tag still exists, but the view doesn't, + * {{@link com.google.firebase.database.core.SyncTree#addEventRegistration(EventRegistration, + * boolean)} throws an error + */ + ReadFuture future = + new ReadFuture( + query, + events -> { + assertEquals(1, events.size()); + return true; + }); + future.timedGet(10000, TimeUnit.MILLISECONDS); } @Test diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java index b7aca39cf81..a9264a0b51b 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java @@ -18,7 +18,6 @@ import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; - import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.database.DataSnapshot; @@ -80,8 +79,7 @@ public class Repo implements PersistentConnection.Delegate { public long dataUpdateCount = 0; // for testing. private long nextWriteId = 1; private SyncTree infoSyncTree; - @VisibleForTesting - public SyncTree serverSyncTree; + @VisibleForTesting protected SyncTree serverSyncTree; private FirebaseDatabase database; private boolean loggedTransactionPersistenceWarning = false; diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java index 22823ff05d7..cb2ec5f871d 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java @@ -16,8 +16,6 @@ import static com.google.firebase.database.core.utilities.Utilities.hardAssert; -import androidx.annotation.VisibleForTesting; - import com.google.firebase.database.DataSnapshot; import com.google.firebase.database.DatabaseError; import com.google.firebase.database.InternalHelpers; @@ -158,10 +156,8 @@ public List onListenComplete(DatabaseError error) { */ private final WriteTree pendingWriteTree; - // TODO(mtewani): Come back to this. - @VisibleForTesting - public final Map tagToQueryMap; - public final Map queryToTagMap; + private final Map tagToQueryMap; + private final Map queryToTagMap; private final Set keepSyncedQueries; private final ListenProvider listenProvider; private final PersistenceManager persistenceManager; @@ -633,8 +629,11 @@ public List call() { } boolean viewAlreadyExists = syncPoint.viewExistsForQuery(query); + System.out.println("Map: " + queryToTagMap); + System.out.println("Query: " + query); if (!viewAlreadyExists && !query.loadsAllData()) { // We need to track a tag for this query + System.out.println("assert" + !queryToTagMap.containsKey(query)); hardAssert( !queryToTagMap.containsKey(query), "View does not exist but we have a tag"); Tag tag = getNextQueryTag(); @@ -723,13 +722,15 @@ public List call() { * that location, listen would be called twice on the same query. skipDedup allows us * to skip this deduping process altogether. */ - if(!skipDedup) { + if (!skipDedup) { // We may have just removed one of many listeners and can short-circuit this whole - // process. We may also not have removed a default listener, in which case all of the + // process. We may also not have removed a default listener, in which case all of + // the // descendant listeners should already be properly set up. // - // Since indexed queries can shadow if they don't have other query constraints, check + // Since indexed queries can shadow if they don't have other query constraints, + // check // for loadsAllData(), instead of isDefault(). boolean removingDefault = false; for (QuerySpec queryRemoved : removed) { @@ -737,16 +738,15 @@ public List call() { removingDefault = removingDefault || queryRemoved.loadsAllData(); } - ImmutableTree currentTree = syncPointTree; boolean covered = - currentTree.getValue() != null && currentTree.getValue().hasCompleteView(); + currentTree.getValue() != null && currentTree.getValue().hasCompleteView(); for (ChildKey component : path) { currentTree = currentTree.getChild(component); covered = - covered - || (currentTree.getValue() != null - && currentTree.getValue().hasCompleteView()); + covered + || (currentTree.getValue() != null + && currentTree.getValue().hasCompleteView()); if (covered || currentTree.isEmpty()) { break; } @@ -765,16 +765,18 @@ public List call() { ListenContainer container = new ListenContainer(view); QuerySpec newQuery = view.getQuery(); listenProvider.startListening( - queryForListening(newQuery), container.tag, container, container); + queryForListening(newQuery), container.tag, container, container); } } else { // There's nothing below us, so nothing we need to start listening on } } - // If we removed anything and we're not covered by a higher up listen, we need to stop + // If we removed anything and we're not covered by a higher up listen, we need to + // stop // listening on this query. The above block has us covered in terms of making sure // we're set up on listens lower in the tree. - // Also, note that if we have a cancelError, it's already been removed at the provider + // Also, note that if we have a cancelError, it's already been removed at the + // provider // level. if (!covered && !removed.isEmpty() && cancelError == null) { // If we removed a default, then we weren't listening on any of the other queries From dacb2596f34a994536314f222c366965b0e6aa28 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 10:49:15 -0800 Subject: [PATCH 07/10] Reverted protected change --- .../src/main/java/com/google/firebase/database/core/Repo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java index a9264a0b51b..fb1d583b82b 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java @@ -79,7 +79,7 @@ public class Repo implements PersistentConnection.Delegate { public long dataUpdateCount = 0; // for testing. private long nextWriteId = 1; private SyncTree infoSyncTree; - @VisibleForTesting protected SyncTree serverSyncTree; + private SyncTree serverSyncTree; private FirebaseDatabase database; private boolean loggedTransactionPersistenceWarning = false; From 5697546c6a0cc491811ef6b928c66cb6a7951953 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 10:49:40 -0800 Subject: [PATCH 08/10] Removed System out --- .../main/java/com/google/firebase/database/core/SyncTree.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java index cb2ec5f871d..d3a96d70269 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java @@ -629,11 +629,8 @@ public List call() { } boolean viewAlreadyExists = syncPoint.viewExistsForQuery(query); - System.out.println("Map: " + queryToTagMap); - System.out.println("Query: " + query); if (!viewAlreadyExists && !query.loadsAllData()) { // We need to track a tag for this query - System.out.println("assert" + !queryToTagMap.containsKey(query)); hardAssert( !queryToTagMap.containsKey(query), "View does not exist but we have a tag"); Tag tag = getNextQueryTag(); From 81ca1babbca639e26540992f960a321a3321ca65 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 10:50:16 -0800 Subject: [PATCH 09/10] Removed import --- .../src/main/java/com/google/firebase/database/core/Repo.java | 1 - 1 file changed, 1 deletion(-) diff --git a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java index fb1d583b82b..160a5c6ff57 100644 --- a/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java +++ b/firebase-database/src/main/java/com/google/firebase/database/core/Repo.java @@ -17,7 +17,6 @@ import static com.google.firebase.database.core.utilities.Utilities.hardAssert; import androidx.annotation.NonNull; -import androidx.annotation.VisibleForTesting; import com.google.android.gms.tasks.Task; import com.google.android.gms.tasks.TaskCompletionSource; import com.google.firebase.database.DataSnapshot; From 22097d1298a410ec76cc90e508e827da19bf2594 Mon Sep 17 00:00:00 2001 From: Maneesh Tewani Date: Wed, 18 Jan 2023 10:55:39 -0800 Subject: [PATCH 10/10] Updated changelog --- firebase-database/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-database/CHANGELOG.md b/firebase-database/CHANGELOG.md index ebb5b3ed8f5..ec007de3e97 100644 --- a/firebase-database/CHANGELOG.md +++ b/firebase-database/CHANGELOG.md @@ -1,4 +1,5 @@ # Unreleased +* [fixed] Fixed issue where `Query.get()` resulted in query tags being added but not cleaned up, potentially causing errors when attempting to add a listener to the query the `get()` was performed on. # 20.1.0 * [unchanged] Updated to accommodate the release of the updated