Fix cleanup of query tags from get

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

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();

firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java

@@ -704,18 +704,6 @@ public List<Event> call() {
 
  List<QuerySpec> 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<Event> 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<SyncPoint> 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<SyncPoint> 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<View> 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<SyncPoint> 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<SyncPoint> 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<View> 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);
+ }
  }
  }
  }