Only advance the limbo free snapshot if query is synced (#699)

Author: schmidt-sebastian

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

@@ -117,7 +117,7 @@ public static QuerySnapshot querySnapshot(
             documentChanges,
             isFromCache,
             mutatedKeys,
-            /* hasLimboDocuments= */ false,
+            /* synced= */ false,
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ false);
     return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);

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

@@ -80,7 +80,7 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) {
               documentChanges,
               newSnapshot.isFromCache(),
               newSnapshot.getMutatedKeys(),
-              newSnapshot.hasLimboDocuments(),
+              newSnapshot.isSynced(),
               newSnapshot.didSyncStateChange(),
               /* excludesMetadataChanges= */ true);
     }
@@ -159,7 +159,7 @@ private void raiseInitialEvent(ViewSnapshot snapshot) {
             snapshot.getDocuments(),
             snapshot.getMutatedKeys(),
             snapshot.isFromCache(),
-            snapshot.hasLimboDocuments(),
+            snapshot.isSynced(),
             snapshot.excludesMetadataChanges());
     raisedInitialEvent = true;
     listener.onEvent(snapshot, null);

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

@@ -296,8 +296,7 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
         });
     applyTargetChange(targetChange);
     List<LimboDocumentChange> limboDocumentChanges = updateLimboDocuments();
-    boolean hasLimboDocuments = !(limboDocuments.size() == 0);
-    boolean synced = !hasLimboDocuments && current;
+    boolean synced = limboDocuments.size() == 0 && current;
     SyncState newSyncState = synced ? SyncState.SYNCED : SyncState.LOCAL;
     boolean syncStatedChanged = newSyncState != syncState;
     syncState = newSyncState;
@@ -312,7 +311,7 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
               viewChanges,
               fromCache,
               docChanges.mutatedKeys,
-              hasLimboDocuments,
+              synced,
               syncStatedChanged,
               /* excludesMetadataChanges= */ false);
     }

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

@@ -37,7 +37,7 @@ public enum SyncState {
   private final List<DocumentViewChange> changes;
   private final boolean isFromCache;
   private final ImmutableSortedSet<DocumentKey> mutatedKeys;
-  private final boolean hasLimboDocuments;
+  private final boolean synced;
   private final boolean didSyncStateChange;
   private boolean excludesMetadataChanges;
 
@@ -48,7 +48,7 @@ public ViewSnapshot(
       List<DocumentViewChange> changes,
       boolean isFromCache,
       ImmutableSortedSet<DocumentKey> mutatedKeys,
-      boolean hasLimboDocuments,
+      boolean synced,
       boolean didSyncStateChange,
       boolean excludesMetadataChanges) {
     this.query = query;
@@ -57,7 +57,7 @@ public ViewSnapshot(
     this.changes = changes;
     this.isFromCache = isFromCache;
     this.mutatedKeys = mutatedKeys;
-    this.hasLimboDocuments = hasLimboDocuments;
+    this.synced = synced;
     this.didSyncStateChange = didSyncStateChange;
     this.excludesMetadataChanges = excludesMetadataChanges;
   }
@@ -68,7 +68,7 @@ public static ViewSnapshot fromInitialDocuments(
       DocumentSet documents,
       ImmutableSortedSet<DocumentKey> mutatedKeys,
       boolean fromCache,
-      boolean hasLimboDocuments,
+      boolean synced,
       boolean excludesMetadataChanges) {
     List<DocumentViewChange> viewChanges = new ArrayList<>();
     for (Document doc : documents) {
@@ -81,7 +81,7 @@ public static ViewSnapshot fromInitialDocuments(
         viewChanges,
         fromCache,
         mutatedKeys,
-        hasLimboDocuments,
+        synced,
         /* didSyncStateChange= */ true,
         excludesMetadataChanges);
   }
@@ -90,8 +90,8 @@ public Query getQuery() {
     return query;
   }
 
-  public boolean hasLimboDocuments() {
-    return hasLimboDocuments;
+  public boolean isSynced() {
+    return synced;
   }
 
   public DocumentSet getDocuments() {
@@ -140,7 +140,7 @@ public final boolean equals(Object o) {
     if (isFromCache != that.isFromCache) {
       return false;
     }
-    if (hasLimboDocuments != that.hasLimboDocuments) {
+    if (synced != that.synced) {
       return false;
     }
     if (didSyncStateChange != that.didSyncStateChange) {
@@ -172,7 +172,7 @@ public int hashCode() {
     result = 31 * result + changes.hashCode();
     result = 31 * result + mutatedKeys.hashCode();
     result = 31 * result + (isFromCache ? 1 : 0);
-    result = 31 * result + (hasLimboDocuments ? 1 : 0);
+    result = 31 * result + (synced ? 1 : 0);
     result = 31 * result + (didSyncStateChange ? 1 : 0);
     result = 31 * result + (excludesMetadataChanges ? 1 : 0);
     return result;
@@ -192,8 +192,8 @@ public String toString() {
         + isFromCache
         + ", mutatedKeys="
         + mutatedKeys.size()
-        + ", hasLimboDocuments="
-        + hasLimboDocuments
+        + ", synced="
+        + synced
         + ", didSyncStateChange="
         + didSyncStateChange
         + ", excludesMetadataChanges="

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

@@ -491,7 +491,7 @@ public void notifyLocalViewChanges(List<LocalViewChanges> viewChanges) {
             }
             localViewReferences.removeReferences(removed, targetId);
 
-            if (!viewChange.hasUnresolvedLimboDocuments()) {
+            if (viewChange.isSynced()) {
               QueryData queryData = targetIds.get(targetId);
               hardAssert(
                   queryData != null,

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java

@@ -49,22 +49,22 @@ public static LocalViewChanges fromViewSnapshot(int targetId, ViewSnapshot snaps
       }
     }
 
-    return new LocalViewChanges(targetId, snapshot.hasLimboDocuments(), addedKeys, removedKeys);
+    return new LocalViewChanges(targetId, snapshot.isSynced(), addedKeys, removedKeys);
   }
 
   private final int targetId;
-  private final boolean hasUnresolvedLimboDocuments;
+  private final boolean synced;
 
   private final ImmutableSortedSet<DocumentKey> added;
   private final ImmutableSortedSet<DocumentKey> removed;
 
   public LocalViewChanges(
       int targetId,
-      boolean hasUnresolvedLimboDocuments,
+      boolean synced,
       ImmutableSortedSet<DocumentKey> added,
       ImmutableSortedSet<DocumentKey> removed) {
     this.targetId = targetId;
-    this.hasUnresolvedLimboDocuments = hasUnresolvedLimboDocuments;
+    this.synced = synced;
     this.added = added;
     this.removed = removed;
   }
@@ -73,12 +73,8 @@ public int getTargetId() {
     return targetId;
   }
 
-  /**
-   * Returns whether there were any unresolved limbo documents in the corresponding view when this
-   * change was computed.
-   */
-  public boolean hasUnresolvedLimboDocuments() {
-    return hasUnresolvedLimboDocuments;
+  public boolean isSynced() {
+    return synced;
   }
 
   public ImmutableSortedSet<DocumentKey> getAdded() {

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

@@ -129,7 +129,7 @@ public static QuerySnapshot querySnapshot(
             documentChanges,
             isFromCache,
             mutatedKeys,
-            /* hasLimboDocuments= */ false,
+            /* synced= */ false,
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ false);
     return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE);

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

@@ -125,7 +125,7 @@ public void testIncludeMetadataChanges() {
             documentChanges,
             /*isFromCache=*/ false,
             /*mutatedKeys=*/ keySet(),
-            /*hasLimboDocuments=*/ true,
+            /*isSynced=*/ true,
             /*didSyncStateChange=*/ true,
             /* excludesMetadataChanges= */ false);
 

firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java

@@ -108,7 +108,7 @@ public void testRaisesCollectionEvents() {
             asList(change1, change4),
             snap2.isFromCache(),
             snap2.getMutatedKeys(),
-            /* hasLimboDocuments= */ false,
+            /* synced= */ false,
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ false);
     assertEquals(asList(snap2Prime), otherAccum);
@@ -266,7 +266,7 @@ public void testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChang
             asList(),
             snap4.isFromCache(),
             snap4.getMutatedKeys(),
-            snap4.hasLimboDocuments(),
+            snap4.isSynced(),
             snap4.didSyncStateChange(),
             /* excludeMetadataChanges= */ true); // This test excludes document metadata changes
 
@@ -308,7 +308,7 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() {
             asList(change3),
             snap2.isFromCache(),
             snap2.getMutatedKeys(),
-            snap2.hasLimboDocuments(),
+            snap2.isSynced(),
             snap2.didSyncStateChange(),
             /* excludesMetadataChanges= */ true);
     assertEquals(
@@ -351,7 +351,7 @@ public void testWillWaitForSyncIfOnline() {
             asList(change1, change2),
             /* isFromCache= */ false,
             snap3.getMutatedKeys(),
-            /* hasLimboDocuments= */ false,
+            /* synced= */ true,
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ true);
     assertEquals(asList(expectedSnapshot), events);
@@ -390,7 +390,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() {
             asList(change1),
             /* isFromCache= */ true,
             snap1.getMutatedKeys(),
-            snap1.hasLimboDocuments(),
+            snap1.isSynced(),
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ true);
     ViewSnapshot expectedSnapshot2 =
@@ -401,7 +401,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() {
             asList(change2),
             /* isFromCache= */ true,
             snap2.getMutatedKeys(),
-            snap2.hasLimboDocuments(),
+            snap2.isSynced(),
             /* didSyncStateChange= */ false,
             /* excludesMetadataChanges= */ true);
     assertEquals(asList(expectedSnapshot1, expectedSnapshot2), events);
@@ -429,7 +429,7 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() {
             asList(),
             /* isFromCache= */ true,
             snap1.getMutatedKeys(),
-            snap1.hasLimboDocuments(),
+            snap1.isSynced(),
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ true);
     assertEquals(asList(expectedSnapshot), events);
@@ -456,7 +456,7 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() {
             asList(),
             /* isFromCache= */ true,
             snap1.getMutatedKeys(),
-            snap1.hasLimboDocuments(),
+            snap1.isSynced(),
             /* didSyncStateChange= */ true,
             /* excludesMetadataChanges= */ true);
     assertEquals(asList(expectedSnapshot), events);
@@ -470,7 +470,7 @@ private ViewSnapshot applyExpectedMetadata(ViewSnapshot snap, MetadataChanges me
         snap.getChanges(),
         snap.isFromCache(),
         snap.getMutatedKeys(),
-        snap.hasLimboDocuments(),
+        snap.isSynced(),
         snap.didSyncStateChange(),
         MetadataChanges.EXCLUDE.equals(metadata));
   }

firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java

@@ -45,7 +45,7 @@ public void testConstructor() {
     List<DocumentViewChange> changes =
         Arrays.asList(DocumentViewChange.create(Type.ADDED, doc("c/foo", 1, map())));
     ImmutableSortedSet<DocumentKey> mutatedKeys = keySet(key("c/foo"));
-    boolean hasLimboDocuments = true;
+    boolean synced = true;
     boolean fromCache = true;
     boolean hasPendingWrites = true;
     boolean syncStateChanges = true;
@@ -59,7 +59,7 @@ public void testConstructor() {
             changes,
             fromCache,
             mutatedKeys,
-            hasLimboDocuments,
+            synced,
             syncStateChanges,
             excludesMetadataChanges);
 
@@ -69,7 +69,7 @@ public void testConstructor() {
     assertEquals(changes, snapshot.getChanges());
     assertEquals(fromCache, snapshot.isFromCache());
     assertEquals(mutatedKeys, snapshot.getMutatedKeys());
-    assertEquals(hasLimboDocuments, snapshot.hasLimboDocuments());
+    assertEquals(synced, snapshot.isSynced());
     assertEquals(hasPendingWrites, snapshot.hasPendingWrites());
     assertEquals(syncStateChanges, snapshot.didSyncStateChange());
     assertEquals(excludesMetadataChanges, snapshot.excludesMetadataChanges());

firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java

@@ -302,7 +302,7 @@ public void testKeepsTrackOfLimboDocuments() {
   }
 
   @Test
-  public void testViewsWithLimboDocumentsAreMarkedAsSuch() {
+  public void testViewsWithLimboDocumentsAreNotMarkedSynced() {
     Query query = messageQuery();
     View view = new View(query, DocumentKey.emptyKeySet());
     Document doc1 = doc("rooms/eros/messages/0", 0, map());
@@ -311,20 +311,20 @@ public void testViewsWithLimboDocumentsAreMarkedAsSuch() {
     // Doc1 is contained in the local view, but we are not yet CURRENT so it is expected that the
     // backend hasn't told us about all documents yet.
     ViewChange change = applyChanges(view, doc1);
-    assertFalse(change.getSnapshot().hasLimboDocuments());
+    assertFalse(change.getSnapshot().isSynced());
 
     // Add doc2 to generate a snapshot. Doc1 is still missing.
     View.DocumentChanges viewDocChanges = view.computeDocChanges(docUpdates(doc2));
     change =
         view.applyChanges(
             viewDocChanges, targetChange(ByteString.EMPTY, true, asList(doc2), null, null));
-    assertTrue(change.getSnapshot().hasLimboDocuments());
+    assertFalse(change.getSnapshot().isSynced()); // We are CURRENT but doc1 is in limbo.
 
     // Add doc1 to the backend's result set.
     change =
         view.applyChanges(
             viewDocChanges, targetChange(ByteString.EMPTY, true, asList(doc1), null, null));
-    assertFalse(change.getSnapshot().hasLimboDocuments());
+    assertTrue(change.getSnapshot().isSynced());
   }
 
   @Test