Fix Firestore failing to return empty results from the local cache (#4207)

Author: milaGGL

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,5 @@
 # Unreleased
+- [fixed] Fixed Firestore failing to raise initial snapshot from empty local cache result.
 
 # 24.4.0
 * [unchanged] Updated to accommodate the release of the updated

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java

@@ -105,7 +105,8 @@ public static QuerySnapshot querySnapshot(
             isFromCache,
             mutatedKeys,
             /* didSyncStateChange= */ true,
-            /* excludesMetadataChanges= */ false);
+            /* excludesMetadataChanges= */ false,
+            /* hasCachedResults= */ false);
     return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);
   }
 }

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

@@ -550,6 +550,50 @@ public void testQueriesFireFromCacheWhenOffline() {
     listener.remove();
   }
 
+  @Test
+  public void testQueriesCanRaiseInitialSnapshotFromCachedEmptyResults() {
+    CollectionReference collectionReference = testCollection();
+
+    // Populate the cache with empty query result.
+    QuerySnapshot querySnapshotA = waitFor(collectionReference.get());
+    assertFalse(querySnapshotA.getMetadata().isFromCache());
+    assertEquals(asList(), querySnapshotToValues(querySnapshotA));
+
+    // Add a snapshot listener whose first event should be raised from cache.
+    EventAccumulator<QuerySnapshot> accum = new EventAccumulator<>();
+    ListenerRegistration listenerRegistration =
+        collectionReference.addSnapshotListener(accum.listener());
+    QuerySnapshot querySnapshotB = accum.await();
+    assertTrue(querySnapshotB.getMetadata().isFromCache());
+    assertEquals(asList(), querySnapshotToValues(querySnapshotB));
+
+    listenerRegistration.remove();
+  }
+
+  @Test
+  public void testQueriesCanRaiseInitialSnapshotFromEmptyDueToDeleteCachedResults() {
+    Map<String, Map<String, Object>> testDocs = map("a", map("foo", 1L));
+    CollectionReference collectionReference = testCollectionWithDocs(testDocs);
+    // Populate the cache with single document.
+    QuerySnapshot querySnapshotA = waitFor(collectionReference.get());
+    assertFalse(querySnapshotA.getMetadata().isFromCache());
+    assertEquals(asList(testDocs.get("a")), querySnapshotToValues(querySnapshotA));
+
+    // delete the document, make cached result empty.
+    DocumentReference docRef = collectionReference.document("a");
+    waitFor(docRef.delete());
+
+    // Add a snapshot listener whose first event should be raised from cache.
+    EventAccumulator<QuerySnapshot> accum = new EventAccumulator<>();
+    ListenerRegistration listenerRegistration =
+        collectionReference.addSnapshotListener(accum.listener());
+    QuerySnapshot querySnapshotB = accum.await();
+    assertTrue(querySnapshotB.getMetadata().isFromCache());
+    assertEquals(asList(), querySnapshotToValues(querySnapshotB));
+
+    listenerRegistration.remove();
+  }
+
   @Test
   public void testQueriesCanUseNotEqualFilters() {
     // These documents are ordered by value in "zip" since the notEquals filter is an inequality,

firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java

@@ -87,7 +87,8 @@ public boolean onViewSnapshot(ViewSnapshot newSnapshot) {
               newSnapshot.isFromCache(),
               newSnapshot.getMutatedKeys(),
               newSnapshot.didSyncStateChange(),
-              /* excludesMetadataChanges= */ true);
+              /* excludesMetadataChanges= */ true,
+              newSnapshot.hasCachedResults());
     }
 
     if (!raisedInitialEvent) {
@@ -139,8 +140,11 @@ private boolean shouldRaiseInitialEvent(ViewSnapshot snapshot, OnlineState onlin
       return false;
     }
 
-    // Raise data from cache if we have any documents or we are offline
-    return !snapshot.getDocuments().isEmpty() || onlineState.equals(OnlineState.OFFLINE);
+    // Raise data from cache if we have any documents, have cached results before,
+    // or we are offline.
+    return (!snapshot.getDocuments().isEmpty()
+        || snapshot.hasCachedResults()
+        || onlineState.equals(OnlineState.OFFLINE));
   }
 
   private boolean shouldRaiseEvent(ViewSnapshot snapshot) {
@@ -171,7 +175,8 @@ private void raiseInitialEvent(ViewSnapshot snapshot) {
             snapshot.getDocuments(),
             snapshot.getMutatedKeys(),
             snapshot.isFromCache(),
-            snapshot.excludesMetadataChanges());
+            snapshot.excludesMetadataChanges(),
+            snapshot.hasCachedResults());
     raisedInitialEvent = true;
     listener.onEvent(snapshot, null);
   }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -54,6 +54,7 @@
 import com.google.firebase.firestore.util.Function;
 import com.google.firebase.firestore.util.Logger;
 import com.google.firebase.firestore.util.Util;
+import com.google.protobuf.ByteString;
 import io.grpc.Status;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -204,13 +205,16 @@ public int listen(Query query) {
     TargetData targetData = localStore.allocateTarget(query.toTarget());
     remoteStore.listen(targetData);
 
-    ViewSnapshot viewSnapshot = initializeViewAndComputeSnapshot(query, targetData.getTargetId());
+    ViewSnapshot viewSnapshot =
+        initializeViewAndComputeSnapshot(
+            query, targetData.getTargetId(), targetData.getResumeToken());
     syncEngineListener.onViewSnapshots(Collections.singletonList(viewSnapshot));
 
     return targetData.getTargetId();
   }
 
-  private ViewSnapshot initializeViewAndComputeSnapshot(Query query, int targetId) {
+  private ViewSnapshot initializeViewAndComputeSnapshot(
+      Query query, int targetId, ByteString resumeToken) {
     QueryResult queryResult = localStore.executeQuery(query, /* usePreviousResults= */ true);
 
     SyncState currentTargetSyncState = SyncState.NONE;
@@ -221,10 +225,10 @@ private ViewSnapshot initializeViewAndComputeSnapshot(Query query, int targetId)
     if (this.queriesByTarget.get(targetId) != null) {
       Query mirrorQuery = this.queriesByTarget.get(targetId).get(0);
       currentTargetSyncState = this.queryViewsByQuery.get(mirrorQuery).getView().getSyncState();
-      synthesizedCurrentChange =
-          TargetChange.createSynthesizedTargetChangeForCurrentChange(
-              currentTargetSyncState == SyncState.SYNCED);
     }
+    synthesizedCurrentChange =
+        TargetChange.createSynthesizedTargetChangeForCurrentChange(
+            currentTargetSyncState == SyncState.SYNCED, resumeToken);
 
     // TODO(wuandy): Investigate if we can extract the logic of view change computation and
     // update tracked limbo in one place, and have both emitNewSnapsAndNotifyLocalStore

firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java

@@ -300,8 +300,11 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
     boolean syncStatedChanged = newSyncState != syncState;
     syncState = newSyncState;
     ViewSnapshot snapshot = null;
+
     if (viewChanges.size() != 0 || syncStatedChanged) {
       boolean fromCache = newSyncState == SyncState.LOCAL;
+      boolean hasCachedResults =
+          targetChange == null ? false : !targetChange.getResumeToken().isEmpty();
       snapshot =
           new ViewSnapshot(
               query,
@@ -311,7 +314,8 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
               fromCache,
               docChanges.mutatedKeys,
               syncStatedChanged,
-              /* excludesMetadataChanges= */ false);
+              /* excludesMetadataChanges= */ false,
+              hasCachedResults);
     }
     return new ViewChange(snapshot, limboDocumentChanges);
   }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java

@@ -39,6 +39,7 @@ public enum SyncState {
   private final ImmutableSortedSet<DocumentKey> mutatedKeys;
   private final boolean didSyncStateChange;
   private boolean excludesMetadataChanges;
+  private boolean hasCachedResults;
 
   public ViewSnapshot(
       Query query,
@@ -48,7 +49,8 @@ public ViewSnapshot(
       boolean isFromCache,
       ImmutableSortedSet<DocumentKey> mutatedKeys,
       boolean didSyncStateChange,
-      boolean excludesMetadataChanges) {
+      boolean excludesMetadataChanges,
+      boolean hasCachedResults) {
     this.query = query;
     this.documents = documents;
     this.oldDocuments = oldDocuments;
@@ -57,6 +59,7 @@ public ViewSnapshot(
     this.mutatedKeys = mutatedKeys;
     this.didSyncStateChange = didSyncStateChange;
     this.excludesMetadataChanges = excludesMetadataChanges;
+    this.hasCachedResults = hasCachedResults;
   }
 
   /** Returns a view snapshot as if all documents in the snapshot were added. */
@@ -65,7 +68,8 @@ public static ViewSnapshot fromInitialDocuments(
       DocumentSet documents,
       ImmutableSortedSet<DocumentKey> mutatedKeys,
       boolean fromCache,
-      boolean excludesMetadataChanges) {
+      boolean excludesMetadataChanges,
+      boolean hasCachedResults) {
     List<DocumentViewChange> viewChanges = new ArrayList<>();
     for (Document doc : documents) {
       viewChanges.add(DocumentViewChange.create(DocumentViewChange.Type.ADDED, doc));
@@ -78,7 +82,8 @@ public static ViewSnapshot fromInitialDocuments(
         fromCache,
         mutatedKeys,
         /* didSyncStateChange= */ true,
-        excludesMetadataChanges);
+        excludesMetadataChanges,
+        hasCachedResults);
   }
 
   public Query getQuery() {
@@ -117,6 +122,10 @@ public boolean excludesMetadataChanges() {
     return excludesMetadataChanges;
   }
 
+  public boolean hasCachedResults() {
+    return hasCachedResults;
+  }
+
   @Override
   public final boolean equals(Object o) {
     if (this == o) {
@@ -149,6 +158,9 @@ public final boolean equals(Object o) {
     if (!oldDocuments.equals(that.oldDocuments)) {
       return false;
     }
+    if (hasCachedResults != that.hasCachedResults) {
+      return false;
+    }
     return changes.equals(that.changes);
   }
 
@@ -162,6 +174,7 @@ public int hashCode() {
     result = 31 * result + (isFromCache ? 1 : 0);
     result = 31 * result + (didSyncStateChange ? 1 : 0);
     result = 31 * result + (excludesMetadataChanges ? 1 : 0);
+    result = 31 * result + (hasCachedResults ? 1 : 0);
     return result;
   }
 
@@ -183,6 +196,8 @@ public String toString() {
         + didSyncStateChange
         + ", excludesMetadataChanges="
         + excludesMetadataChanges
+        + ", hasCachedResults="
+        + hasCachedResults
         + ")";
   }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/TargetChange.java

@@ -31,9 +31,10 @@ public final class TargetChange {
   private final ImmutableSortedSet<DocumentKey> modifiedDocuments;
   private final ImmutableSortedSet<DocumentKey> removedDocuments;
 
-  public static TargetChange createSynthesizedTargetChangeForCurrentChange(boolean isCurrent) {
+  public static TargetChange createSynthesizedTargetChangeForCurrentChange(
+      boolean isCurrent, ByteString resumeToken) {
     return new TargetChange(
-        ByteString.EMPTY,
+        resumeToken,
         isCurrent,
         DocumentKey.emptyKeySet(),
         DocumentKey.emptyKeySet(),

firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java

@@ -83,7 +83,8 @@ public static QuerySnapshot querySnapshot(
       Map<String, ObjectValue> oldDocs,
       Map<String, ObjectValue> docsToAdd,
       boolean hasPendingWrites,
-      boolean isFromCache) {
+      boolean isFromCache,
+      boolean hasCachedResults) {
     DocumentSet oldDocuments = docSet(Document.KEY_COMPARATOR);
     ImmutableSortedSet<DocumentKey> mutatedKeys = DocumentKey.emptyKeySet();
     for (Map.Entry<String, ObjectValue> pair : oldDocs.entrySet()) {
@@ -116,7 +117,8 @@ public static QuerySnapshot querySnapshot(
             isFromCache,
             mutatedKeys,
             /* didSyncStateChange= */ true,
-            /* excludesMetadataChanges= */ false);
+            /* excludesMetadataChanges= */ false,
+            hasCachedResults);
     return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);
   }
 

firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java

@@ -56,21 +56,26 @@ public void testEquals() {
     ObjectValue firstValue = wrapObject("a", 1);
     ObjectValue secondValue = wrapObject("b", 1);
 
-    QuerySnapshot foo = TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, false);
-    QuerySnapshot fooDup = TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, false);
+    QuerySnapshot foo =
+        TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, false, false);
+    QuerySnapshot fooDup =
+        TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, false, false);
     QuerySnapshot differentPath =
-        TestUtil.querySnapshot("bar", map(), map("a", firstValue), true, false);
+        TestUtil.querySnapshot("bar", map(), map("a", firstValue), true, false, false);
     QuerySnapshot differentDoc =
-        TestUtil.querySnapshot("foo", map(), map("a", secondValue), true, false);
+        TestUtil.querySnapshot("foo", map(), map("a", secondValue), true, false, false);
     QuerySnapshot noPendingWrites =
-        TestUtil.querySnapshot("foo", map(), map("a", firstValue), false, false);
+        TestUtil.querySnapshot("foo", map(), map("a", firstValue), false, false, false);
     QuerySnapshot fromCache =
-        TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, true);
+        TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, true, false);
+    QuerySnapshot hasCachedResults =
+        TestUtil.querySnapshot("foo", map(), map("a", firstValue), true, false, true);
     assertEquals(foo, fooDup);
     assertNotEquals(foo, differentPath);
     assertNotEquals(foo, differentDoc);
     assertNotEquals(foo, noPendingWrites);
     assertNotEquals(foo, fromCache);
+    assertNotEquals(foo, hasCachedResults);
 
     // Note: `foo` and `differentDoc` have the same hash code since we no longer take document
     // contents into account.
@@ -88,7 +93,8 @@ public void testToObjects() {
 
     ObjectValue objectData =
         ObjectValue.fromMap(map("timestamp", ServerTimestamps.valueOf(Timestamp.now(), null)));
-    QuerySnapshot foo = TestUtil.querySnapshot("foo", map(), map("a", objectData), true, false);
+    QuerySnapshot foo =
+        TestUtil.querySnapshot("foo", map(), map("a", objectData), true, false, false);
 
     List<POJO> docs = foo.toObjects(POJO.class);
     assertEquals(1, docs.size());
@@ -126,7 +132,8 @@ public void testIncludeMetadataChanges() {
             /*isFromCache=*/ false,
             /*mutatedKeys=*/ keySet(),
             /*didSyncStateChange=*/ true,
-            /* excludesMetadataChanges= */ false);
+            /* excludesMetadataChanges= */ false,
+            /* hasCachedResults= */ false);
 
     QuerySnapshot snapshot =
         new QuerySnapshot(new Query(fooQuery, firestore), viewSnapshot, firestore);