Fixed get() to make sure it updates the correct paths (#3887)

Fixed `get()` issue where wrong cache paths were being updated in the SyncTree.

Co-authored-by: Jan Wyszynski <[email protected]>

Author: maneesht

firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java

@@ -334,6 +334,14 @@ public static void waitFor(Semaphore semaphore, int count, long timeout, TimeUni
     assertTrue("Operation timed out", success);
   }
 
+  public static DatabaseReference translateReference(DatabaseReference node, FirebaseDatabase db) {
+    return db.getReference().child(node.getPath().toString());
+  }
+
+  public static DataSnapshot referenceAtPath(DatabaseReference node, EventRecord record) {
+    return record.getSnapshot().child(node.getPath().toString());
+  }
+
   public static DataSnapshot getSnap(Query ref) throws InterruptedException {
 
     final Semaphore semaphore = new Semaphore(0);

firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java

@@ -34,7 +34,8 @@
 public class ReadFuture implements Future<List<EventRecord>> {
 
   public interface CompletionCondition {
-    public boolean isComplete(List<EventRecord> events);
+    public boolean isComplete(List<EventRecord> events)
+        throws ExecutionException, InterruptedException;
   }
 
   private List<EventRecord> events = new ArrayList<EventRecord>();
@@ -66,13 +67,15 @@ public void onDataChange(DataSnapshot snapshot) {
               }
             } catch (Exception e) {
               exception = e;
+              ref.removeEventListener(valueEventListener);
               finish();
             }
           }
 
           @Override
           public void onCancelled(DatabaseError error) {
             wasCancelled = true;
+            ref.removeEventListener(valueEventListener);
             finish();
           }
         };

firebase-database/src/androidTest/java/com/google/firebase/database/future/ReadFuture.java

@@ -58,7 +58,7 @@
 /**
  * This class is an Android/SQL-backed implementation of PersistenceStorageEngine.
  *
- * <p>The implementation uses 3 tables for persistence: - writes: A list of all currently
+ * <p>The implementation uses 4 tables for persistence: - writes: A list of all currently
  * outstanding (visible) writes. A row represents a single write containing a unique-across-restarts
  * write id, the path string, the type which can be 'o' for an overwrite or 'm' for a merge, and the
  * serialized node wrapped. Part number is NULL for normal writes. Large writes (>256K) are split
@@ -1058,6 +1058,7 @@ private Cursor loadNestedQuery(Path path, String[] columns) {
     arguments[path.size() + 1] = pathPrefixStart;
     arguments[path.size() + 2] = pathPrefixEnd;
     String orderBy = PATH_COLUMN_NAME;
+    // TODO: We need to modify this to make sure that we limit based on trackedquerykeys
 
     return database.query(SERVER_CACHE_TABLE, columns, whereClause, arguments, null, null, orderBy);
   }

firebase-database/src/main/java/com/google/firebase/database/android/SqlPersistenceStorageEngine.java

@@ -419,7 +419,6 @@ public Task<Object> get(List<String> path, Map<String, Object> queryParams) {
               String status = (String) response.get(REQUEST_STATUS);
               if (status.equals("ok")) {
                 Object body = response.get(SERVER_DATA_UPDATE_BODY);
-                delegate.onDataUpdate(query.path, body, /*isMerge=*/ false, /*tagNumber=*/ null);
                 source.setResult(body);
               } else {
                 source.setException(new Exception((String) response.get(SERVER_DATA_UPDATE_BODY)));

firebase-database/src/main/java/com/google/firebase/database/connection/PersistentConnectionImpl.java

@@ -75,6 +75,8 @@ public Path(String pathString) {
         count++;
       }
     }
+    // We are essentially doing the same work twice; once here, and once again in the constructor on
+    // line 60
     pieces = new ChildKey[count];
     int j = 0;
     for (String segment : segments) {

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

@@ -514,12 +514,13 @@ public void onRequestResult(String optErrorCode, String optErrorMessage) {
    */
   public Task<DataSnapshot> getValue(Query query) {
     TaskCompletionSource<DataSnapshot> source = new TaskCompletionSource<>();
+    final Repo repo = this;
     this.scheduleNow(
         new Runnable() {
           @Override
           public void run() {
             // Always check active-listener in-memory caches first. These are always at least as
-            // up to date as the persistence cache.
+            // up to date as the persistence cache
             Node serverValue = serverSyncTree.getServerValue(query.getSpec());
             if (serverValue != null) {
               source.setResult(
@@ -548,15 +549,44 @@ public void run() {
                           source.setException(Objects.requireNonNull(task.getException()));
                         }
                       } else {
+                        /*
+                         * We need to replicate the behavior that occurs when running `once()`. In other words,
+                         * we need to create a new eventRegistration, register it with a view and then
+                         * overwrite the data at that location, and then remove the view.
+                         */
                         Node serverNode = NodeUtilities.NodeFromJSON(task.getResult());
-                        postEvents(
-                            serverSyncTree.applyServerOverwrite(query.getPath(), serverNode));
+                        QuerySpec spec = query.getSpec();
+                        // EventRegistrations require a listener to be attached, so a dummy
+                        // ValueEventListener was created.
+                        ValueEventListener listener =
+                            new ValueEventListener() {
+                              @Override
+                              public void onDataChange(@NonNull DataSnapshot snapshot) {
+                                // noOp
+                              }
+
+                              @Override
+                              public void onCancelled(@NonNull DatabaseError error) {
+                                // noOp
+                              }
+                            };
+                        ValueEventRegistration eventRegistration =
+                            new ValueEventRegistration(repo, listener, spec);
+                        serverSyncTree.addEventRegistration(
+                            eventRegistration, /*skipListenerSetup=*/ true);
+                        if (spec.loadsAllData()) {
+                          serverSyncTree.applyServerOverwrite(spec.getPath(), serverNode);
+                        } else {
+                          serverSyncTree.applyTaggedQueryOverwrite(
+                              spec.getPath(), serverNode, getServerSyncTree().tagForQuery(spec));
+                        }
                         source.setResult(
                             InternalHelpers.createDataSnapshot(
                                 query.getRef(),
                                 IndexedNode.from(serverNode, query.getSpec().getIndex())));
+                        serverSyncTree.removeEventRegistration(
+                            eventRegistration, /*skipDedup=*/ true);
                       }
-                      serverSyncTree.setQueryInactive(query.getSpec());
                     });
           }
         });

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

@@ -235,8 +235,9 @@ public List<View> getQueryViews() {
 
   public Node getCompleteServerCache(Path path) {
     for (View view : this.views.values()) {
-      if (view.getCompleteServerCache(path) != null) {
-        return view.getCompleteServerCache(path);
+      Node serverCache = view.getCompleteServerCache(path);
+      if (serverCache != null) {
+        return serverCache;
       }
     }
     return null;

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

@@ -538,9 +538,14 @@ public Node getServerValue(QuerySpec query) {
         });
   }
 
-  /** Add an event callback for the specified query. */
   public List<? extends Event> addEventRegistration(
       @NotNull final EventRegistration eventRegistration) {
+    return addEventRegistration(eventRegistration, false);
+  }
+
+  /** Add an event callback for the specified query. */
+  public List<? extends Event> addEventRegistration(
+      @NotNull final EventRegistration eventRegistration, final boolean skipListenerSetup) {
     return persistenceManager.runInTransaction(
         new Callable<List<? extends Event>>() {
           @Override
@@ -635,7 +640,7 @@ public List<? extends Event> call() {
             WriteTreeRef writesCache = pendingWriteTree.childWrites(path);
             List<? extends Event> events =
                 syncPoint.addEventRegistration(eventRegistration, writesCache, serverCache);
-            if (!viewAlreadyExists && !foundAncestorDefaultView) {
+            if (!viewAlreadyExists && !foundAncestorDefaultView && !skipListenerSetup) {
               View view = syncPoint.viewForQuery(query);
               setupListener(query, view);
             }
@@ -650,7 +655,14 @@ public List<? extends Event> call() {
    * <p>If query is the default query, we'll check all queries for the specified eventRegistration.
    */
   public List<Event> removeEventRegistration(@NotNull EventRegistration eventRegistration) {
-    return this.removeEventRegistration(eventRegistration.getQuerySpec(), eventRegistration, null);
+    return this.removeEventRegistration(
+        eventRegistration.getQuerySpec(), eventRegistration, null, false);
+  }
+
+  public List<Event> removeEventRegistration(
+      @NotNull EventRegistration eventRegistration, boolean skipDedup) {
+    return this.removeEventRegistration(
+        eventRegistration.getQuerySpec(), eventRegistration, null, skipDedup);
   }
 
   /**
@@ -660,13 +672,14 @@ public List<Event> removeEventRegistration(@NotNull EventRegistration eventRegis
    */
   public List<Event> removeAllEventRegistrations(
       @NotNull QuerySpec query, @NotNull DatabaseError error) {
-    return this.removeEventRegistration(query, null, error);
+    return this.removeEventRegistration(query, null, error, false);
   }
 
   private List<Event> removeEventRegistration(
       final @NotNull QuerySpec query,
       final @Nullable EventRegistration eventRegistration,
-      final @Nullable DatabaseError cancelError) {
+      final @Nullable DatabaseError cancelError,
+      final boolean skipDedup) {
     return persistenceManager.runInTransaction(
         new Callable<List<Event>>() {
           @Override
@@ -688,6 +701,7 @@ public List<Event> call() {
               if (maybeSyncPoint.isEmpty()) {
                 syncPointTree = syncPointTree.remove(path);
               }
+
               List<QuerySpec> removed = removedAndEvents.getFirst();
               cancelEvents = removedAndEvents.getSecond();
               // We may have just removed one of many listeners and can short-circuit this whole
@@ -701,6 +715,25 @@ public List<Event> call() {
                 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<SyncPoint> currentTree = syncPointTree;
               boolean covered =
                   currentTree.getValue() != null && currentTree.getValue().hasCompleteView();
@@ -915,7 +948,7 @@ private QuerySpec queryForTag(Tag tag) {
   }
 
   /** Return the tag associated with the given query. */
-  private Tag tagForQuery(QuerySpec query) {
+  public Tag tagForQuery(QuerySpec query) {
     return this.queryToTagMap.get(query);
   }
 

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

@@ -122,7 +122,7 @@ public void zombifyForRemove(EventRegistration registration) {
       if (registrationList != null && !registrationList.isEmpty()) {
         if (registration.getQuerySpec().isDefault()) {
           // The behavior here has to match the behavior of SyncPoint.removeEventRegistration.
-          // If the query is default, it remove a single instance of the registration
+          // If the query is default, it removes a single instance of the registration
           // from each unique query.  So for example, if you had 3 copies registered under default,
           // you would end up with 2 still registered.
           // If you had 1 registration in default and 2 in query a', you'd end up with just