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 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..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 @@ -4617,6 +4617,29 @@ public void testGetReturnsNullForEmptyNodeWhenOnline() assertNull(await(db.getReference(UUID.randomUUID().toString()).get()).getValue()); } + @Test + public void testGetCleansUpTags() + 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(); + /** + * 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 public void testGetWaitsForConnection() throws DatabaseException, InterruptedException { FirebaseDatabase db = getNewDatabase(); 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..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 @@ -704,18 +704,6 @@ 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(); - } - /** * 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 @@ -731,58 +719,74 @@ 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) { - 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 (!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(); } - } - 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); + 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; } - } 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 (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 + } + } + // 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); + } } } }