Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) {
documentChanges,
newSnapshot.isFromCache(),
newSnapshot.getMutatedKeys(),
newSnapshot.hasLimboDocuments(),
newSnapshot.isSynced(),
newSnapshot.didSyncStateChange(),
/* excludesMetadataChanges= */ true);
}
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -312,7 +311,7 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh
viewChanges,
fromCache,
docChanges.mutatedKeys,
hasLimboDocuments,
synced,
syncStatedChanged,
/* excludesMetadataChanges= */ false);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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;
}
Expand All @@ -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) {
Expand All @@ -81,7 +81,7 @@ public static ViewSnapshot fromInitialDocuments(
viewChanges,
fromCache,
mutatedKeys,
hasLimboDocuments,
synced,
/* didSyncStateChange= */ true,
excludesMetadataChanges);
}
Expand All @@ -90,8 +90,8 @@ public Query getQuery() {
return query;
}

public boolean hasLimboDocuments() {
return hasLimboDocuments;
public boolean isSynced() {
return synced;
}

public DocumentSet getDocuments() {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -192,8 +192,8 @@ public String toString() {
+ isFromCache
+ ", mutatedKeys="
+ mutatedKeys.size()
+ ", hasLimboDocuments="
+ hasLimboDocuments
+ ", synced="
+ synced
+ ", didSyncStateChange="
+ didSyncStateChange
+ ", excludesMetadataChanges="
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public void testIncludeMetadataChanges() {
documentChanges,
/*isFromCache=*/ false,
/*mutatedKeys=*/ keySet(),
/*hasLimboDocuments=*/ true,
/*isSynced=*/ true,
/*didSyncStateChange=*/ true,
/* excludesMetadataChanges= */ false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -308,7 +308,7 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() {
asList(change3),
snap2.isFromCache(),
snap2.getMutatedKeys(),
snap2.hasLimboDocuments(),
snap2.isSynced(),
snap2.didSyncStateChange(),
/* excludesMetadataChanges= */ true);
assertEquals(
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -390,7 +390,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() {
asList(change1),
/* isFromCache= */ true,
snap1.getMutatedKeys(),
snap1.hasLimboDocuments(),
snap1.isSynced(),
/* didSyncStateChange= */ true,
/* excludesMetadataChanges= */ true);
ViewSnapshot expectedSnapshot2 =
Expand All @@ -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);
Expand Down Expand Up @@ -429,7 +429,7 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() {
asList(),
/* isFromCache= */ true,
snap1.getMutatedKeys(),
snap1.hasLimboDocuments(),
snap1.isSynced(),
/* didSyncStateChange= */ true,
/* excludesMetadataChanges= */ true);
assertEquals(asList(expectedSnapshot), events);
Expand All @@ -456,7 +456,7 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() {
asList(),
/* isFromCache= */ true,
snap1.getMutatedKeys(),
snap1.hasLimboDocuments(),
snap1.isSynced(),
/* didSyncStateChange= */ true,
/* excludesMetadataChanges= */ true);
assertEquals(asList(expectedSnapshot), events);
Expand All @@ -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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -59,7 +59,7 @@ public void testConstructor() {
changes,
fromCache,
mutatedKeys,
hasLimboDocuments,
synced,
syncStateChanges,
excludesMetadataChanges);

Expand All @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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
Expand Down