From 3d66aaa2ba15c6163248bb1acc3e5a4ccd69fcb5 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 31 Jul 2019 11:16:38 -0700 Subject: [PATCH 01/13] Add update_time to SQLLiteRemoteDocument schema --- .../local/SQLiteRemoteDocumentCache.java | 8 ++++++- .../firestore/local/SQLiteSchema.java | 24 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index fae8f366192..0d64b832a36 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -18,6 +18,7 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import androidx.annotation.Nullable; +import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.Document; @@ -51,13 +52,18 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { @Override public void add(MaybeDocument maybeDocument) { String path = pathForKey(maybeDocument.getKey()); + Timestamp timestamp = maybeDocument.getVersion().getTimestamp(); MessageLite message = serializer.encodeMaybeDocument(maybeDocument); statsCollector.recordRowsWritten(STATS_TAG, 1); db.execute( - "INSERT OR REPLACE INTO remote_documents (path, contents) VALUES (?, ?)", + "INSERT OR REPLACE INTO remote_documents " + + "(path, update_time_seconds, update_time_nanos, contents) " + + "VALUES (?, ?, ?, ?)", path, + timestamp.getSeconds(), + timestamp.getNanoseconds(), message.toByteArray()); db.getIndexManager().addToCollectionParentIndex(maybeDocument.getKey().getPath().popLast()); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index cd7ef9ac52b..4eecb273d2e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -45,8 +45,16 @@ class SQLiteSchema { /** * The version of the schema. Increase this by one for each migration added to runMigrations * below. + * + *

TODO(index-free): The migration to schema version 9 doesn't backfill `update_time` as this + * requires rewriting the RemoteDocumentCache. For index-free queries to efficiently handle + * existing documents, we still need to populate update_time for all existing entries, drop the + * RemoteDocumentCache or ask users to invoke `clearPersistence()` manually. If we decide to + * backfill or drop the contents of the RemoteDocumentCache, we need to perform an additional + * schema migration. */ - static final int VERSION = 8; + static final int VERSION = 9; + // Remove this constant and increment VERSION to enable indexing support static final int INDEXING_SUPPORT_VERSION = VERSION + 1; @@ -127,6 +135,10 @@ void runMigrations(int fromVersion, int toVersion) { createV8CollectionParentsIndex(); } + if (fromVersion < 9 && toVersion >= 9) { + addUpdateTime(); + } + /* * Adding a new migration? READ THIS FIRST! * @@ -351,6 +363,16 @@ private void addSequenceNumber() { } } + private void addUpdateTime() { + if (!tableContainsColumn("remote_documents", "update_time_seconds")) { + hardAssert( + !tableContainsColumn("remote_documents", "update_time_nanos"), + "Table contained update_time_seconds, but is missing update_time_nanos"); + db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_seconds INTEGER"); + db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_nanos INTEGER"); + } + } + /** * Ensures that each entry in the remote document cache has a corresponding sentinel row. Any * entries that lack a sentinel row are given one with the sequence number set to the highest From 42ec7f12e8dc5fdb8b9b3e0d8aaa0ac817d261b2 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 5 Aug 2019 12:52:59 -0700 Subject: [PATCH 02/13] Index-Free (3/6): Persist a Query's Sync State (#616) --- .../google/firebase/firestore/TestUtil.java | 3 +- .../firestore/core/QueryListener.java | 2 + .../google/firebase/firestore/core/View.java | 4 +- .../firebase/firestore/core/ViewSnapshot.java | 15 ++++++++ .../firestore/local/LocalSerializer.java | 12 +++++- .../firebase/firestore/local/LocalStore.java | 37 ++++++++++++++++++- .../firestore/local/LocalViewChanges.java | 13 ++++++- .../firebase/firestore/local/QueryData.java | 28 +++++++++++++- .../firebase/firestore/proto/target.proto | 4 ++ .../google/firebase/firestore/TestUtil.java | 3 +- .../firebase/firestore/QuerySnapshotTest.java | 1 + .../firestore/core/QueryListenerTest.java | 9 +++++ .../firestore/core/ViewSnapshotTest.java | 3 ++ .../firebase/firestore/core/ViewTest.java | 26 +++++++++++++ .../firestore/local/LocalSerializerTest.java | 14 ++++++- .../firestore/local/QueryCacheTestCase.java | 1 + .../remote/RemoteSerializerTest.java | 17 ++++++++- .../firebase/firestore/spec/SpecTestCase.java | 1 + .../firebase/firestore/testutil/TestUtil.java | 2 +- 19 files changed, 181 insertions(+), 14 deletions(-) diff --git a/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java index 96ea22889a9..54f93fd0cb4 100644 --- a/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java +++ b/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java @@ -117,7 +117,8 @@ public static QuerySnapshot querySnapshot( documentChanges, isFromCache, mutatedKeys, - true, + /* hasLimboDocuments= */ false, + /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ false); return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index fb8d99d3afb..5414dfc7f65 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java @@ -80,6 +80,7 @@ public void onViewSnapshot(ViewSnapshot newSnapshot) { documentChanges, newSnapshot.isFromCache(), newSnapshot.getMutatedKeys(), + newSnapshot.hasLimboDocuments(), newSnapshot.didSyncStateChange(), /* excludesMetadataChanges= */ true); } @@ -158,6 +159,7 @@ private void raiseInitialEvent(ViewSnapshot snapshot) { snapshot.getDocuments(), snapshot.getMutatedKeys(), snapshot.isFromCache(), + snapshot.hasLimboDocuments(), snapshot.excludesMetadataChanges()); raisedInitialEvent = true; listener.onEvent(snapshot, null); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java index 32d5e2a6827..81082d341ca 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java @@ -296,7 +296,8 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh }); applyTargetChange(targetChange); List limboDocumentChanges = updateLimboDocuments(); - boolean synced = limboDocuments.size() == 0 && current; + boolean hasLimboDocuments = !(limboDocuments.size() == 0); + boolean synced = !hasLimboDocuments && current; SyncState newSyncState = synced ? SyncState.SYNCED : SyncState.LOCAL; boolean syncStatedChanged = newSyncState != syncState; syncState = newSyncState; @@ -311,6 +312,7 @@ public ViewChange applyChanges(DocumentChanges docChanges, TargetChange targetCh viewChanges, fromCache, docChanges.mutatedKeys, + hasLimboDocuments, syncStatedChanged, /* excludesMetadataChanges= */ false); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index 1e05a3d8e98..239f44a2dc8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -37,6 +37,7 @@ public enum SyncState { private final List changes; private final boolean isFromCache; private final ImmutableSortedSet mutatedKeys; + private final boolean hasLimboDocuments; private final boolean didSyncStateChange; private boolean excludesMetadataChanges; @@ -47,6 +48,7 @@ public ViewSnapshot( List changes, boolean isFromCache, ImmutableSortedSet mutatedKeys, + boolean hasLimboDocuments, boolean didSyncStateChange, boolean excludesMetadataChanges) { this.query = query; @@ -55,6 +57,7 @@ public ViewSnapshot( this.changes = changes; this.isFromCache = isFromCache; this.mutatedKeys = mutatedKeys; + this.hasLimboDocuments = hasLimboDocuments; this.didSyncStateChange = didSyncStateChange; this.excludesMetadataChanges = excludesMetadataChanges; } @@ -65,6 +68,7 @@ public static ViewSnapshot fromInitialDocuments( DocumentSet documents, ImmutableSortedSet mutatedKeys, boolean fromCache, + boolean hasLimboDocuments, boolean excludesMetadataChanges) { List viewChanges = new ArrayList<>(); for (Document doc : documents) { @@ -77,6 +81,7 @@ public static ViewSnapshot fromInitialDocuments( viewChanges, fromCache, mutatedKeys, + hasLimboDocuments, /* didSyncStateChange= */ true, excludesMetadataChanges); } @@ -85,6 +90,10 @@ public Query getQuery() { return query; } + public boolean hasLimboDocuments() { + return hasLimboDocuments; + } + public DocumentSet getDocuments() { return documents; } @@ -131,6 +140,9 @@ public final boolean equals(Object o) { if (isFromCache != that.isFromCache) { return false; } + if (hasLimboDocuments != that.hasLimboDocuments) { + return false; + } if (didSyncStateChange != that.didSyncStateChange) { return false; } @@ -160,6 +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 + (didSyncStateChange ? 1 : 0); result = 31 * result + (excludesMetadataChanges ? 1 : 0); return result; @@ -179,6 +192,8 @@ public String toString() { + isFromCache + ", mutatedKeys=" + mutatedKeys.size() + + ", hasLimboDocuments=" + + hasLimboDocuments + ", didSyncStateChange=" + didSyncStateChange + ", excludesMetadataChanges=" diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java index d9bf81d2c57..8ca8bdf1e21 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java @@ -205,6 +205,8 @@ com.google.firebase.firestore.proto.Target encodeQueryData(QueryData queryData) result .setTargetId(queryData.getTargetId()) .setLastListenSequenceNumber(queryData.getSequenceNumber()) + .setLastLimboFreeSnapshotVersion( + rpcSerializer.encodeVersion(queryData.getLastLimboFreeSnapshotVersion())) .setSnapshotVersion(rpcSerializer.encodeVersion(queryData.getSnapshotVersion())) .setResumeToken(queryData.getResumeToken()); @@ -221,6 +223,8 @@ com.google.firebase.firestore.proto.Target encodeQueryData(QueryData queryData) QueryData decodeQueryData(com.google.firebase.firestore.proto.Target target) { int targetId = target.getTargetId(); SnapshotVersion version = rpcSerializer.decodeVersion(target.getSnapshotVersion()); + SnapshotVersion lastLimboFreeSnapshotVersion = + rpcSerializer.decodeVersion(target.getLastLimboFreeSnapshotVersion()); ByteString resumeToken = target.getResumeToken(); long sequenceNumber = target.getLastListenSequenceNumber(); @@ -239,6 +243,12 @@ QueryData decodeQueryData(com.google.firebase.firestore.proto.Target target) { } return new QueryData( - query, targetId, sequenceNumber, QueryPurpose.LISTEN, version, resumeToken); + query, + targetId, + sequenceNumber, + QueryPurpose.LISTEN, + version, + lastLimboFreeSnapshotVersion, + resumeToken); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 0e92e76c2c2..dedc474b08d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -469,12 +469,35 @@ public void notifyLocalViewChanges(List viewChanges) { "notifyLocalViewChanges", () -> { for (LocalViewChanges viewChange : viewChanges) { - localViewReferences.addReferences(viewChange.getAdded(), viewChange.getTargetId()); + int targetId = viewChange.getTargetId(); + + localViewReferences.addReferences(viewChange.getAdded(), targetId); ImmutableSortedSet removed = viewChange.getRemoved(); for (DocumentKey key : removed) { persistence.getReferenceDelegate().removeReference(key); } - localViewReferences.removeReferences(removed, viewChange.getTargetId()); + localViewReferences.removeReferences(removed, targetId); + + if (!viewChange.hasUnresolvedLimboDocuments()) { + QueryData queryData = targetIds.get(targetId); + hardAssert( + queryData != null, + "Can't set limbo-free snapshot version for unknown target: %s", + targetId); + + // Advance the last limbo free snapshot version + SnapshotVersion lastLimboFreeSnapshotVersion = queryData.getSnapshotVersion(); + QueryData updatedQueryData = + new QueryData( + queryData.getQuery(), + queryData.getTargetId(), + queryData.getSequenceNumber(), + queryData.getPurpose(), + queryData.getSnapshotVersion(), + lastLimboFreeSnapshotVersion, + queryData.getResumeToken()); + targetIds.put(targetId, updatedQueryData); + } } }); } @@ -548,10 +571,20 @@ public void releaseQuery(Query query) { int targetId = queryData.getTargetId(); QueryData cachedQueryData = targetIds.get(targetId); + + boolean needsUpdate = false; if (cachedQueryData.getSnapshotVersion().compareTo(queryData.getSnapshotVersion()) > 0) { // If we've been avoiding persisting the resumeToken (see shouldPersistQueryData for // conditions and rationale) we need to persist the token now because there will no // longer be an in-memory version to fall back on. + needsUpdate = true; + } else if (!cachedQueryData + .getLastLimboFreeSnapshotVersion() + .equals(queryData.getLastLimboFreeSnapshotVersion())) { + needsUpdate = true; + } + + if (needsUpdate) { queryData = cachedQueryData; queryCache.updateQueryData(queryData); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java index 541f3f444b6..540cbad5b3d 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java @@ -49,19 +49,22 @@ public static LocalViewChanges fromViewSnapshot(int targetId, ViewSnapshot snaps } } - return new LocalViewChanges(targetId, addedKeys, removedKeys); + return new LocalViewChanges(targetId, snapshot.hasLimboDocuments(), addedKeys, removedKeys); } private final int targetId; + private final boolean hasUnresolvedLimboDocuments; private final ImmutableSortedSet added; private final ImmutableSortedSet removed; public LocalViewChanges( int targetId, + boolean hasUnresolvedLimboDocuments, ImmutableSortedSet added, ImmutableSortedSet removed) { this.targetId = targetId; + this.hasUnresolvedLimboDocuments = hasUnresolvedLimboDocuments; this.added = added; this.removed = removed; } @@ -70,6 +73,14 @@ 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 ImmutableSortedSet getAdded() { return added; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java index ab52bb22192..2c46babbc58 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java @@ -28,6 +28,7 @@ public final class QueryData { private final long sequenceNumber; private final QueryPurpose purpose; private final SnapshotVersion snapshotVersion; + private final SnapshotVersion lastLimboFreeSnapshotVersion; private final ByteString resumeToken; /** @@ -36,12 +37,14 @@ public final class QueryData { * @param query The query being listened to. * @param targetId The target to which the query corresponds, assigned by the LocalStore for user * queries or the SyncEngine for limbo queries. + * @param sequenceNumber The sequence number, denoting the last time this query was used. * @param purpose The purpose of the query. * @param snapshotVersion The latest snapshot version seen for this target. + * @param lastLimboFreeSnapshotVersion The maximum snapshot version at which the associated query + * view contained no limbo documents. * @param resumeToken An opaque, server-assigned token that allows watching a query to be resumed * after disconnecting without retransmitting all the data that matches the query. The resume * token essentially identifies a point in time from which the server should resume sending - * results. */ public QueryData( Query query, @@ -49,10 +52,12 @@ public QueryData( long sequenceNumber, QueryPurpose purpose, SnapshotVersion snapshotVersion, + SnapshotVersion lastLimboFreeSnapshotVersion, ByteString resumeToken) { this.query = checkNotNull(query); this.targetId = targetId; this.sequenceNumber = sequenceNumber; + this.lastLimboFreeSnapshotVersion = lastLimboFreeSnapshotVersion; this.purpose = purpose; this.snapshotVersion = checkNotNull(snapshotVersion); this.resumeToken = checkNotNull(resumeToken); @@ -66,6 +71,7 @@ public QueryData(Query query, int targetId, long sequenceNumber, QueryPurpose pu sequenceNumber, purpose, SnapshotVersion.NONE, + SnapshotVersion.NONE, WatchStream.EMPTY_RESUME_TOKEN); } @@ -93,6 +99,13 @@ public ByteString getResumeToken() { return resumeToken; } + /** + * Returns the last snapshot version for which the associated view contained no limbo documents. + */ + public SnapshotVersion getLastLimboFreeSnapshotVersion() { + return lastLimboFreeSnapshotVersion; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -108,6 +121,7 @@ public boolean equals(Object o) { && sequenceNumber == queryData.sequenceNumber && purpose.equals(queryData.purpose) && snapshotVersion.equals(queryData.snapshotVersion) + && lastLimboFreeSnapshotVersion.equals(queryData.lastLimboFreeSnapshotVersion) && resumeToken.equals(queryData.resumeToken); } @@ -118,6 +132,7 @@ public int hashCode() { result = 31 * result + (int) sequenceNumber; result = 31 * result + purpose.hashCode(); result = 31 * result + snapshotVersion.hashCode(); + result = 31 * result + lastLimboFreeSnapshotVersion.hashCode(); result = 31 * result + resumeToken.hashCode(); return result; } @@ -135,6 +150,8 @@ public String toString() { + purpose + ", snapshotVersion=" + snapshotVersion + + ", lastLimboFreeSnapshotVersion=" + + lastLimboFreeSnapshotVersion + ", resumeToken=" + resumeToken + '}'; @@ -143,6 +160,13 @@ public String toString() { /** Creates a new query data instance with an updated snapshot version and resume token. */ public QueryData copy( SnapshotVersion snapshotVersion, ByteString resumeToken, long sequenceNumber) { - return new QueryData(query, targetId, sequenceNumber, purpose, snapshotVersion, resumeToken); + return new QueryData( + query, + targetId, + sequenceNumber, + purpose, + snapshotVersion, + lastLimboFreeSnapshotVersion, + resumeToken); } } diff --git a/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto b/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto index eb8453f4630..e835cde4c1c 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto @@ -78,6 +78,10 @@ message Target { // A target specified by a set of document names. google.firestore.v1.Target.DocumentsTarget documents = 6; } + + // Denotes the maximum snapshot version at which the associated querye view + // contained no limbo documents. + google.protobuf.Timestamp last_limbo_free_snapshot_version = 7; } // Global state tracked across all Targets, tracked separately to avoid the diff --git a/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java index 63960964eff..6997822c2e5 100644 --- a/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java +++ b/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java @@ -129,7 +129,8 @@ public static QuerySnapshot querySnapshot( documentChanges, isFromCache, mutatedKeys, - true, + /* hasLimboDocuments= */ false, + /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ false); return new QuerySnapshot(query(path), viewSnapshot, FIRESTORE); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java index d11013b34a2..e1f7a99f36a 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java @@ -125,6 +125,7 @@ public void testIncludeMetadataChanges() { documentChanges, /*isFromCache=*/ false, /*mutatedKeys=*/ keySet(), + /*hasLimboDocuments=*/ true, /*didSyncStateChange=*/ true, /* excludesMetadataChanges= */ false); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java index d3e0a4b9d6c..d093eb58e5b 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java @@ -108,6 +108,7 @@ public void testRaisesCollectionEvents() { asList(change1, change4), snap2.isFromCache(), snap2.getMutatedKeys(), + /* hasLimboDocuments= */ false, /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ false); assertEquals(asList(snap2Prime), otherAccum); @@ -265,6 +266,7 @@ public void testRaisesQueryMetadataEventsOnlyWhenHasPendingWritesOnTheQueryChang asList(), snap4.isFromCache(), snap4.getMutatedKeys(), + snap4.hasLimboDocuments(), snap4.didSyncStateChange(), /* excludeMetadataChanges= */ true); // This test excludes document metadata changes @@ -306,6 +308,7 @@ public void testMetadataOnlyDocumentChangesAreFilteredOut() { asList(change3), snap2.isFromCache(), snap2.getMutatedKeys(), + snap2.hasLimboDocuments(), snap2.didSyncStateChange(), /* excludesMetadataChanges= */ true); assertEquals( @@ -348,6 +351,7 @@ public void testWillWaitForSyncIfOnline() { asList(change1, change2), /* isFromCache= */ false, snap3.getMutatedKeys(), + /* hasLimboDocuments= */ false, /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); @@ -386,6 +390,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { asList(change1), /* isFromCache= */ true, snap1.getMutatedKeys(), + snap1.hasLimboDocuments(), /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ true); ViewSnapshot expectedSnapshot2 = @@ -396,6 +401,7 @@ public void testWillRaiseInitialEventWhenGoingOffline() { asList(change2), /* isFromCache= */ true, snap2.getMutatedKeys(), + snap2.hasLimboDocuments(), /* didSyncStateChange= */ false, /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot1, expectedSnapshot2), events); @@ -423,6 +429,7 @@ public void testWillRaiseInitialEventWhenGoingOfflineAndThereAreNoDocs() { asList(), /* isFromCache= */ true, snap1.getMutatedKeys(), + snap1.hasLimboDocuments(), /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); @@ -449,6 +456,7 @@ public void testWillRaiseInitialEventWhenStartingOfflineAndThereAreNoDocs() { asList(), /* isFromCache= */ true, snap1.getMutatedKeys(), + snap1.hasLimboDocuments(), /* didSyncStateChange= */ true, /* excludesMetadataChanges= */ true); assertEquals(asList(expectedSnapshot), events); @@ -462,6 +470,7 @@ private ViewSnapshot applyExpectedMetadata(ViewSnapshot snap, MetadataChanges me snap.getChanges(), snap.isFromCache(), snap.getMutatedKeys(), + snap.hasLimboDocuments(), snap.didSyncStateChange(), MetadataChanges.EXCLUDE.equals(metadata)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java index 554f81e5b2c..8abe6a50ff3 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java @@ -45,6 +45,7 @@ public void testConstructor() { List changes = Arrays.asList(DocumentViewChange.create(Type.ADDED, doc("c/foo", 1, map()))); ImmutableSortedSet mutatedKeys = keySet(key("c/foo")); + boolean hasLimboDocuments = true; boolean fromCache = true; boolean hasPendingWrites = true; boolean syncStateChanges = true; @@ -58,6 +59,7 @@ public void testConstructor() { changes, fromCache, mutatedKeys, + hasLimboDocuments, syncStateChanges, excludesMetadataChanges); @@ -67,6 +69,7 @@ public void testConstructor() { assertEquals(changes, snapshot.getChanges()); assertEquals(fromCache, snapshot.isFromCache()); assertEquals(mutatedKeys, snapshot.getMutatedKeys()); + assertEquals(hasLimboDocuments, snapshot.hasLimboDocuments()); assertEquals(hasPendingWrites, snapshot.hasPendingWrites()); assertEquals(syncStateChanges, snapshot.didSyncStateChange()); assertEquals(excludesMetadataChanges, snapshot.excludesMetadataChanges()); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java index efac909cf79..cdefee313ba 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java @@ -301,6 +301,32 @@ public void testKeepsTrackOfLimboDocuments() { change.getLimboChanges()); } + @Test + public void testViewsWithLimboDocumentsAreMarkedAsSuch() { + Query query = messageQuery(); + View view = new View(query, DocumentKey.emptyKeySet()); + Document doc1 = doc("rooms/eros/messages/0", 0, map()); + Document doc2 = doc("rooms/eros/messages/1", 0, map()); + + // 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()); + + // 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()); + + // Add doc1 to the backend's result set. + change = + view.applyChanges( + viewDocChanges, targetChange(ByteString.EMPTY, true, asList(doc1), null, null)); + assertFalse(change.getSnapshot().hasLimboDocuments()); + } + @Test public void testResumingQueryCreatesNoLimbos() { Query query = messageQuery(); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java index c97f3637fbc..b671db6546b 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalSerializerTest.java @@ -203,11 +203,19 @@ public void testEncodesQueryData() { Query query = TestUtil.query("room"); int targetId = 42; long sequenceNumber = 10; - SnapshotVersion version = TestUtil.version(1039); + SnapshotVersion snapshotVersion = TestUtil.version(1039); + SnapshotVersion limboFreeVersion = TestUtil.version(1000); ByteString resumeToken = TestUtil.resumeToken(1039); QueryData queryData = - new QueryData(query, targetId, sequenceNumber, QueryPurpose.LISTEN, version, resumeToken); + new QueryData( + query, + targetId, + sequenceNumber, + QueryPurpose.LISTEN, + snapshotVersion, + limboFreeVersion, + resumeToken); // Let the RPC serializer test various permutations of query serialization. com.google.firestore.v1.Target.QueryTarget queryTarget = @@ -223,6 +231,8 @@ public void testEncodesQueryData() { com.google.firestore.v1.Target.QueryTarget.newBuilder() .setParent(queryTarget.getParent()) .setStructuredQuery(queryTarget.getStructuredQuery())) + .setLastLimboFreeSnapshotVersion( + com.google.protobuf.Timestamp.newBuilder().setNanos(1000000)) .build(); assertEquals(expected, serializer.encodeQueryData(queryData)); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryCacheTestCase.java index f5d95486ac3..ba7cd12f93b 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/QueryCacheTestCase.java @@ -329,6 +329,7 @@ private QueryData newQueryData(Query query, int targetId, long version) { sequenceNumber, QueryPurpose.LISTEN, version(version), + version(version), resumeToken(version)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 66a5f45e20b..4265e39b808 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -476,6 +476,7 @@ public void testEncodesFirstLevelKeyQueries() { 2, QueryPurpose.LISTEN, SnapshotVersion.NONE, + SnapshotVersion.NONE, WatchStream.EMPTY_RESUME_TOKEN)); DocumentsTarget.Builder docs = @@ -883,7 +884,13 @@ public void testEncodesResumeTokens() { Target actual = serializer.encodeTarget( new QueryData( - q, 1, 2, QueryPurpose.LISTEN, SnapshotVersion.NONE, TestUtil.resumeToken(1000))); + q, + 1, + 2, + QueryPurpose.LISTEN, + SnapshotVersion.NONE, + SnapshotVersion.NONE, + TestUtil.resumeToken(1000))); StructuredQuery.Builder structuredQueryBuilder = StructuredQuery.newBuilder() @@ -911,7 +918,13 @@ public void testEncodesResumeTokens() { */ private QueryData wrapQueryData(Query query) { return new QueryData( - query, 1, 2, QueryPurpose.LISTEN, SnapshotVersion.NONE, WatchStream.EMPTY_RESUME_TOKEN); + query, + 1, + 2, + QueryPurpose.LISTEN, + SnapshotVersion.NONE, + SnapshotVersion.NONE, + WatchStream.EMPTY_RESUME_TOKEN); } @Test diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java index 696c0598381..77ab1121c36 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java @@ -887,6 +887,7 @@ private void validateStateExpectations(@Nullable JSONObject expected) throws JSO ARBITRARY_SEQUENCE_NUMBER, QueryPurpose.LISTEN, SnapshotVersion.NONE, + SnapshotVersion.NONE, ByteString.copyFromUtf8(resumeToken))); } } diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index 61f3dbfcf7a..476e6d05f15 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -518,7 +518,7 @@ public static LocalViewChanges viewChanges( for (String keyPath : removedKeys) { removed = removed.insert(key(keyPath)); } - return new LocalViewChanges(targetId, added, removed); + return new LocalViewChanges(targetId, false, added, removed); } /** Creates a resume token to match the given snapshot version. */ From 016fd1297787fc263665d64fb7a3d91647ad3b03 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 7 Aug 2019 13:54:40 -0700 Subject: [PATCH 03/13] Add QueryData.with() helpers (#687) --- .../firebase/firestore/local/LocalStore.java | 13 ++--- .../local/MemoryLruReferenceDelegate.java | 4 +- .../firebase/firestore/local/QueryData.java | 51 ++++++++++++++----- .../local/SQLiteLruReferenceDelegate.java | 4 +- .../firestore/remote/RemoteStore.java | 8 +-- .../local/LruGarbageCollectorTestCase.java | 5 +- .../firestore/remote/MockDatastore.java | 3 +- .../remote/RemoteSerializerTest.java | 34 +++---------- .../firebase/firestore/spec/SpecTestCase.java | 14 ++--- 9 files changed, 59 insertions(+), 77 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index dedc474b08d..40aada51ad1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -359,7 +359,9 @@ public ImmutableSortedMap applyRemoteEvent(RemoteEve if (!resumeToken.isEmpty()) { QueryData oldQueryData = queryData; queryData = - queryData.copy(remoteEvent.getSnapshotVersion(), resumeToken, sequenceNumber); + queryData + .withResumeToken(resumeToken, remoteEvent.getSnapshotVersion()) + .withSequenceNumber(sequenceNumber); targetIds.put(boxedTargetId, queryData); if (shouldPersistQueryData(oldQueryData, queryData, change)) { @@ -488,14 +490,7 @@ public void notifyLocalViewChanges(List viewChanges) { // Advance the last limbo free snapshot version SnapshotVersion lastLimboFreeSnapshotVersion = queryData.getSnapshotVersion(); QueryData updatedQueryData = - new QueryData( - queryData.getQuery(), - queryData.getTargetId(), - queryData.getSequenceNumber(), - queryData.getPurpose(), - queryData.getSnapshotVersion(), - lastLimboFreeSnapshotVersion, - queryData.getResumeToken()); + queryData.withLastLimboFreeSnapshotVersion(lastLimboFreeSnapshotVersion); targetIds.put(targetId, updatedQueryData); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java index 6ff4cb74dcc..f2430ad86d3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java @@ -135,9 +135,7 @@ public void removeMutationReference(DocumentKey key) { @Override public void removeTarget(QueryData queryData) { - QueryData updated = - queryData.copy( - queryData.getSnapshotVersion(), queryData.getResumeToken(), getCurrentSequenceNumber()); + QueryData updated = queryData.withSequenceNumber(getCurrentSequenceNumber()); persistence.getQueryCache().updateQueryData(updated); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java index 2c46babbc58..0c6361f1e78 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryData.java @@ -46,7 +46,7 @@ public final class QueryData { * after disconnecting without retransmitting all the data that matches the query. The resume * token essentially identifies a point in time from which the server should resume sending */ - public QueryData( + QueryData( Query query, int targetId, long sequenceNumber, @@ -75,6 +75,42 @@ public QueryData(Query query, int targetId, long sequenceNumber, QueryPurpose pu WatchStream.EMPTY_RESUME_TOKEN); } + /** Creates a new query data instance with an updated sequence number. */ + public QueryData withSequenceNumber(long sequenceNumber) { + return new QueryData( + query, + targetId, + sequenceNumber, + purpose, + snapshotVersion, + lastLimboFreeSnapshotVersion, + resumeToken); + } + + /** Creates a new query data instance with an updated resume token and snapshot version. */ + public QueryData withResumeToken(ByteString resumeToken, SnapshotVersion snapshotVersion) { + return new QueryData( + query, + targetId, + sequenceNumber, + purpose, + snapshotVersion, + lastLimboFreeSnapshotVersion, + resumeToken); + } + + /** Creates a new query data instance with an updated last limbo free snapshot version number. */ + public QueryData withLastLimboFreeSnapshotVersion(SnapshotVersion lastLimboFreeSnapshotVersion) { + return new QueryData( + query, + targetId, + sequenceNumber, + purpose, + snapshotVersion, + lastLimboFreeSnapshotVersion, + resumeToken); + } + public Query getQuery() { return query; } @@ -156,17 +192,4 @@ public String toString() { + resumeToken + '}'; } - - /** Creates a new query data instance with an updated snapshot version and resume token. */ - public QueryData copy( - SnapshotVersion snapshotVersion, ByteString resumeToken, long sequenceNumber) { - return new QueryData( - query, - targetId, - sequenceNumber, - purpose, - snapshotVersion, - lastLimboFreeSnapshotVersion, - resumeToken); - } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java index d2823ac3990..1793c6e73b7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteLruReferenceDelegate.java @@ -167,9 +167,7 @@ public int removeOrphanedDocuments(long upperBound) { @Override public void removeTarget(QueryData queryData) { - QueryData updated = - queryData.copy( - queryData.getSnapshotVersion(), queryData.getResumeToken(), getCurrentSequenceNumber()); + QueryData updated = queryData.withSequenceNumber(getCurrentSequenceNumber()); persistence.getQueryCache().updateQueryData(updated); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java index a964a00d72b..78b0e111412 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java @@ -504,9 +504,7 @@ private void raiseWatchSnapshot(SnapshotVersion snapshotVersion) { // A watched target might have been removed already. if (queryData != null) { this.listenTargets.put( - targetId, - queryData.copy( - snapshotVersion, targetChange.getResumeToken(), queryData.getSequenceNumber())); + targetId, queryData.withResumeToken(targetChange.getResumeToken(), snapshotVersion)); } } } @@ -519,9 +517,7 @@ private void raiseWatchSnapshot(SnapshotVersion snapshotVersion) { if (queryData != null) { // Clear the resume token for the query, since we're in a known mismatch state. this.listenTargets.put( - targetId, - queryData.copy( - queryData.getSnapshotVersion(), ByteString.EMPTY, queryData.getSequenceNumber())); + targetId, queryData.withResumeToken(ByteString.EMPTY, queryData.getSnapshotVersion())); // Cause a hard reset by unwatching and rewatching immediately, but deliberately don't send // a resume token so that we get a full update. diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java index 45cbd8035f5..63eef368751 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java @@ -109,8 +109,9 @@ private void updateTargetInTransaction(QueryData queryData) { SnapshotVersion version = version(2); ByteString resumeToken = resumeToken(2); QueryData updated = - queryData.copy( - version, resumeToken, persistence.getReferenceDelegate().getCurrentSequenceNumber()); + queryData + .withResumeToken(resumeToken, version) + .withSequenceNumber(persistence.getReferenceDelegate().getCurrentSequenceNumber()); queryCache.updateQueryData(updated); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java index 84911d286cd..b13b8b89d1f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/MockDatastore.java @@ -90,8 +90,7 @@ public void watchQuery(QueryData queryData) { + ")"); // Snapshot version is ignored on the wire QueryData sentQueryData = - queryData.copy( - SnapshotVersion.NONE, queryData.getResumeToken(), queryData.getSequenceNumber()); + queryData.withResumeToken(queryData.getResumeToken(), SnapshotVersion.NONE); watchStreamRequestCount += 1; this.activeTargets.put(queryData.getTargetId(), sentQueryData); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java index 4265e39b808..dbb6b5437c5 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/remote/RemoteSerializerTest.java @@ -468,16 +468,7 @@ public void testEncodesListenRequestLabels() { @Test public void testEncodesFirstLevelKeyQueries() { Query q = Query.atPath(ResourcePath.fromString("docs/1")); - Target actual = - serializer.encodeTarget( - new QueryData( - q, - 1, - 2, - QueryPurpose.LISTEN, - SnapshotVersion.NONE, - SnapshotVersion.NONE, - WatchStream.EMPTY_RESUME_TOKEN)); + Target actual = serializer.encodeTarget(new QueryData(q, 1, 2, QueryPurpose.LISTEN)); DocumentsTarget.Builder docs = DocumentsTarget.newBuilder().addDocuments("projects/p/databases/d/documents/docs/1"); @@ -881,16 +872,10 @@ public void testEncodesBounds() { @Test public void testEncodesResumeTokens() { Query q = Query.atPath(ResourcePath.fromString("docs")); - Target actual = - serializer.encodeTarget( - new QueryData( - q, - 1, - 2, - QueryPurpose.LISTEN, - SnapshotVersion.NONE, - SnapshotVersion.NONE, - TestUtil.resumeToken(1000))); + QueryData queryData = + new QueryData(q, 1, 2, QueryPurpose.LISTEN) + .withResumeToken(TestUtil.resumeToken(1000), SnapshotVersion.NONE); + Target actual = serializer.encodeTarget(queryData); StructuredQuery.Builder structuredQueryBuilder = StructuredQuery.newBuilder() @@ -917,14 +902,7 @@ public void testEncodesResumeTokens() { * QueryData, but for the most part we're just testing variations on Query. */ private QueryData wrapQueryData(Query query) { - return new QueryData( - query, - 1, - 2, - QueryPurpose.LISTEN, - SnapshotVersion.NONE, - SnapshotVersion.NONE, - WatchStream.EMPTY_RESUME_TOKEN); + return new QueryData(query, 1, 2, QueryPurpose.LISTEN); } @Test diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java index 77ab1121c36..e6c5f6f966a 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java @@ -879,16 +879,10 @@ private void validateStateExpectations(@Nullable JSONObject expected) throws JSO // TODO: populate the purpose of the target once it's possible to encode that in the // spec tests. For now, hard-code that it's a listen despite the fact that it's not always // the right value. - expectedActiveTargets.put( - targetId, - new QueryData( - query, - targetId, - ARBITRARY_SEQUENCE_NUMBER, - QueryPurpose.LISTEN, - SnapshotVersion.NONE, - SnapshotVersion.NONE, - ByteString.copyFromUtf8(resumeToken))); + QueryData queryData = + new QueryData(query, targetId, ARBITRARY_SEQUENCE_NUMBER, QueryPurpose.LISTEN) + .withResumeToken(ByteString.copyFromUtf8(resumeToken), SnapshotVersion.NONE); + expectedActiveTargets.put(targetId, queryData); } } } From 8ea9471cb5c003b3f84caa40c8e724ebd8a3f3d3 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 7 Aug 2019 16:01:53 -0700 Subject: [PATCH 04/13] Index-Free: Using readTime instead of updateTime (#686) --- .../firebase/firestore/local/LocalStore.java | 4 +- .../local/MemoryLruReferenceDelegate.java | 4 +- .../local/MemoryRemoteDocumentCache.java | 57 ++++++++++++++----- .../firestore/local/RemoteDocumentCache.java | 4 +- .../local/SQLiteRemoteDocumentCache.java | 7 ++- .../firestore/local/SQLiteSchema.java | 14 ++--- .../local/IndexedQueryEngineTest.java | 5 +- .../local/LruGarbageCollectorTestCase.java | 4 +- .../local/RemoteDocumentCacheTestCase.java | 14 +++-- 9 files changed, 74 insertions(+), 39 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 40aada51ad1..5d8122cbe47 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -392,7 +392,7 @@ public ImmutableSortedMap applyRemoteEvent(RemoteEve || doc.getVersion().equals(SnapshotVersion.NONE) || (authoritativeUpdates.contains(doc.getKey()) && !existingDoc.hasPendingWrites()) || doc.getVersion().compareTo(existingDoc.getVersion()) >= 0) { - remoteDocuments.add(doc); + remoteDocuments.add(doc, remoteEvent.getSnapshotVersion()); changedDocs.put(key, doc); } else { Logger.debug( @@ -629,7 +629,7 @@ private void applyWriteToRemoteDocuments(MutationBatchResult batchResult) { batch, remoteDoc); } else { - remoteDocuments.add(doc); + remoteDocuments.add(doc, batchResult.getCommitVersion()); } } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java index f2430ad86d3..860eb5385c0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryLruReferenceDelegate.java @@ -117,8 +117,8 @@ public int removeTargets(long upperBound, SparseArray activeTargetIds) { public int removeOrphanedDocuments(long upperBound) { int count = 0; MemoryRemoteDocumentCache cache = persistence.getRemoteDocumentCache(); - for (Map.Entry entry : cache.getDocuments()) { - DocumentKey key = entry.getKey(); + for (MaybeDocument doc : cache.getDocuments()) { + DocumentKey key = doc.getKey(); if (!isPinned(key, upperBound)) { cache.remove(key); orphanedSequenceNumbers.remove(key); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index 6a9dd59cef1..a13b53fb1b2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -15,9 +15,10 @@ package com.google.firebase.firestore.local; import static com.google.firebase.firestore.model.DocumentCollections.emptyDocumentMap; -import static com.google.firebase.firestore.model.DocumentCollections.emptyMaybeDocumentMap; import static com.google.firebase.firestore.util.Assert.hardAssert; +import android.util.Pair; +import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.core.Query; @@ -25,6 +26,7 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.model.ResourcePath; +import com.google.firebase.firestore.model.SnapshotVersion; import java.util.HashMap; import java.util.Iterator; import java.util.Map; @@ -32,21 +34,21 @@ /** In-memory cache of remote documents. */ final class MemoryRemoteDocumentCache implements RemoteDocumentCache { - /** Underlying cache of documents. */ - private ImmutableSortedMap docs; + /** Underlying cache of documents and their read times. */ + private ImmutableSortedMap> docs; private final MemoryPersistence persistence; private StatsCollector statsCollector; MemoryRemoteDocumentCache(MemoryPersistence persistence, StatsCollector statsCollector) { - docs = emptyMaybeDocumentMap(); + docs = ImmutableSortedMap.Builder.emptyMap(DocumentKey.comparator()); this.statsCollector = statsCollector; this.persistence = persistence; } @Override - public void add(MaybeDocument document) { - docs = docs.insert(document.getKey(), document); + public void add(MaybeDocument document, SnapshotVersion readTime) { + docs = docs.insert(document.getKey(), new Pair<>(document, readTime)); persistence.getIndexManager().addToCollectionParentIndex(document.getKey().getPath().popLast()); } @@ -61,7 +63,8 @@ public void remove(DocumentKey key) { @Override public MaybeDocument get(DocumentKey key) { statsCollector.recordRowsRead(STATS_TAG, 1); - return docs.get(key); + Pair entry = docs.get(key); + return entry != null ? entry.first : null; } @Override @@ -89,12 +92,13 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu // we need to match the query against. ResourcePath queryPath = query.getPath(); DocumentKey prefix = DocumentKey.fromPath(queryPath.append("")); - Iterator> iterator = docs.iteratorFrom(prefix); + Iterator>> iterator = + docs.iteratorFrom(prefix); int rowsRead = 0; while (iterator.hasNext()) { - Map.Entry entry = iterator.next(); + Map.Entry> entry = iterator.next(); ++rowsRead; @@ -103,7 +107,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu break; } - MaybeDocument maybeDoc = entry.getValue(); + MaybeDocument maybeDoc = entry.getValue().first; if (!(maybeDoc instanceof Document)) { continue; } @@ -119,8 +123,8 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu return result; } - ImmutableSortedMap getDocuments() { - return docs; + Iterable getDocuments() { + return new DocumentIterable(); } /** @@ -140,10 +144,33 @@ private static long getKeySize(DocumentKey key) { long getByteSize(LocalSerializer serializer) { long count = 0; - for (Map.Entry entry : docs) { - count += getKeySize(entry.getKey()); - count += serializer.encodeMaybeDocument(entry.getValue()).getSerializedSize(); + for (MaybeDocument doc : new DocumentIterable()) { + count += getKeySize(doc.getKey()); + count += serializer.encodeMaybeDocument(doc).getSerializedSize(); } return count; } + + /** + * A proxy that exposes an iterator over the current set of documents in the RemoteDocumentCache. + */ + private class DocumentIterable implements Iterable { + @NonNull + @Override + public Iterator iterator() { + Iterator>> iterator = + MemoryRemoteDocumentCache.this.docs.iterator(); + return new Iterator() { + @Override + public boolean hasNext() { + return iterator.hasNext(); + } + + @Override + public MaybeDocument next() { + return iterator.next().getValue().first; + } + }; + } + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index bf3c2f7290a..dbd59eca9cd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -20,6 +20,7 @@ import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; +import com.google.firebase.firestore.model.SnapshotVersion; import java.util.Map; /** @@ -40,8 +41,9 @@ interface RemoteDocumentCache { * for the key, it will be replaced. * * @param maybeDocument A Document or NoDocument to put in the cache. + * @param readTime The time at which the document was read or committed. */ - void add(MaybeDocument maybeDocument); + void add(MaybeDocument maybeDocument, SnapshotVersion readTime); /** Removes the cached entry for the given key (no-op if no entry exists). */ void remove(DocumentKey documentKey); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 0d64b832a36..6032842e8e9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -26,6 +26,7 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.model.ResourcePath; +import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.util.BackgroundQueue; import com.google.firebase.firestore.util.Executors; import com.google.protobuf.InvalidProtocolBufferException; @@ -50,16 +51,16 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { } @Override - public void add(MaybeDocument maybeDocument) { + public void add(MaybeDocument maybeDocument, SnapshotVersion readTime) { String path = pathForKey(maybeDocument.getKey()); - Timestamp timestamp = maybeDocument.getVersion().getTimestamp(); + Timestamp timestamp = readTime.getTimestamp(); MessageLite message = serializer.encodeMaybeDocument(maybeDocument); statsCollector.recordRowsWritten(STATS_TAG, 1); db.execute( "INSERT OR REPLACE INTO remote_documents " - + "(path, update_time_seconds, update_time_nanos, contents) " + + "(path, read_time_seconds, read_time_nanos, contents) " + "VALUES (?, ?, ?, ?)", path, timestamp.getSeconds(), diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 4eecb273d2e..272ec183756 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -136,7 +136,7 @@ void runMigrations(int fromVersion, int toVersion) { } if (fromVersion < 9 && toVersion >= 9) { - addUpdateTime(); + addReadTime(); } /* @@ -363,13 +363,13 @@ private void addSequenceNumber() { } } - private void addUpdateTime() { - if (!tableContainsColumn("remote_documents", "update_time_seconds")) { + private void addReadTime() { + if (!tableContainsColumn("remote_documents", "read_time_seconds")) { hardAssert( - !tableContainsColumn("remote_documents", "update_time_nanos"), - "Table contained update_time_seconds, but is missing update_time_nanos"); - db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_seconds INTEGER"); - db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_nanos INTEGER"); + !tableContainsColumn("remote_documents", "read_time_nanos"), + "Table contained read_time_nanos, but is missing read_time_seconds"); + db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER"); + db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER"); } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java index 8cf85f30173..48ab21b2e11 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java @@ -73,7 +73,8 @@ public void setUp() { } private void addDocument(Document newDoc) { - remoteDocuments.add(newDoc); + // Use document version as read time as the IndexedQueryEngine does not rely on read time. + remoteDocuments.add(newDoc, newDoc.getVersion()); queryEngine.handleDocumentChange( deletedDoc(newDoc.getKey().toString(), ORIGINAL_VERSION), newDoc); } @@ -85,7 +86,7 @@ private void removeDocument(Document oldDoc) { } private void updateDocument(Document oldDoc, Document newDoc) { - remoteDocuments.add(newDoc); + remoteDocuments.add(newDoc, newDoc.getVersion()); queryEngine.handleDocumentChange(oldDoc, newDoc); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java index 63eef368751..50522e31e2d 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LruGarbageCollectorTestCase.java @@ -140,7 +140,7 @@ private Document nextTestDocument() { private Document cacheADocumentInTransaction() { Document doc = nextTestDocument(); - documentCache.add(doc); + documentCache.add(doc, doc.getVersion()); return doc; } @@ -555,7 +555,7 @@ public void testRemoveTargetsThenGC() { SnapshotVersion newVersion = version(3); Document doc = new Document(middleDocToUpdate, newVersion, Document.DocumentState.SYNCED, testValue); - documentCache.add(doc); + documentCache.add(doc, newVersion); updateTargetInTransaction(middleTarget); }); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java index 5b575dfaa5b..6b5d49072c1 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java @@ -21,6 +21,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.path; import static com.google.firebase.firestore.testutil.TestUtil.values; +import static com.google.firebase.firestore.testutil.TestUtil.version; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotEquals; @@ -32,6 +33,7 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.model.NoDocument; +import com.google.firebase.firestore.model.SnapshotVersion; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -134,7 +136,7 @@ public void testSetAndReadLotsOfDocuments() { public void testSetAndReadDeletedDocument() { String path = "a/b"; NoDocument deletedDoc = deletedDoc(path, 42); - add(deletedDoc); + add(deletedDoc, version(42)); assertEquals(deletedDoc, get(path)); } @@ -144,7 +146,7 @@ public void testSetDocumentToNewValue() { Document written = addTestDocumentAtPath(path); Document newDoc = doc(path, 57, map("data", 5)); - add(newDoc); + add(newDoc, version(57)); assertNotEquals(written, newDoc); assertEquals(newDoc, get(path)); @@ -182,12 +184,14 @@ public void testDocumentsMatchingQuery() { private Document addTestDocumentAtPath(String path) { Document doc = doc(path, 42, map("data", 2)); - add(doc); + add(doc, version(42)); return doc; } - private void add(MaybeDocument doc) { - persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc)); + // TODO(mrschmidt): Add a test uses different update and read times and verifies that we correctly + // filter by read time + private void add(MaybeDocument doc, SnapshotVersion readTime) { + persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc, readTime)); } @Nullable From 47d0230d329a33650e18a026e0483e54d504c2da Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 8 Aug 2019 15:14:34 -0700 Subject: [PATCH 05/13] Index-Free (4/6): Filter document queries by read time (#617) --- .../firestore/local/IndexedQueryEngine.java | 5 +- .../firestore/local/LocalDocumentsView.java | 23 +++--- .../local/MemoryRemoteDocumentCache.java | 11 ++- .../firestore/local/RemoteDocumentCache.java | 5 +- .../firestore/local/SQLitePersistence.java | 17 ++++- .../local/SQLiteRemoteDocumentCache.java | 20 +++++- .../firestore/local/SQLiteSchema.java | 4 +- .../firestore/local/SimpleQueryEngine.java | 3 +- .../firestore/local/LocalStoreTestCase.java | 8 +-- .../local/RemoteDocumentCacheTestCase.java | 39 ++++++++-- .../firestore/local/SQLiteSchemaTest.java | 72 ++++++++++++++++++- 11 files changed, 176 insertions(+), 31 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java index 11a8e48e15b..2cb85b05461 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java @@ -29,6 +29,7 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.FieldPath; import com.google.firebase.firestore.model.MaybeDocument; +import com.google.firebase.firestore.model.SnapshotVersion; import com.google.firebase.firestore.model.value.ArrayValue; import com.google.firebase.firestore.model.value.BooleanValue; import com.google.firebase.firestore.model.value.DoubleValue; @@ -99,7 +100,7 @@ public IndexedQueryEngine( @Override public ImmutableSortedMap getDocumentsMatchingQuery(Query query) { return query.isDocumentQuery() - ? localDocuments.getDocumentsMatchingQuery(query) + ? localDocuments.getDocumentsMatchingQuery(query, SnapshotVersion.NONE) : performCollectionQuery(query); } @@ -118,7 +119,7 @@ private ImmutableSortedMap performCollectionQuery(Query q "If there are any filters, we should be able to use an index."); // TODO: Call overlay.getCollectionDocuments(query.getPath()) and filter the // results (there may still be startAt/endAt bounds that apply). - filteredResults = localDocuments.getDocumentsMatchingQuery(query); + filteredResults = localDocuments.getDocumentsMatchingQuery(query, SnapshotVersion.NONE); } return filteredResults; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index 535db58a542..670b55cd874 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -131,15 +131,22 @@ ImmutableSortedMap getLocalViewOfDocuments( // documents in a given collection so that SimpleQueryEngine can do that and then filter in // memory. - /** Performs a query against the local view of all documents. */ - ImmutableSortedMap getDocumentsMatchingQuery(Query query) { + /** + * Performs a query against the local view of all documents. + * + * @param query The query to match documents against. + * @param sinceReadTime If not set to SnapshotVersion.MIN, return only documents that have been + * read since this snapshot version (exclusive). + */ + ImmutableSortedMap getDocumentsMatchingQuery( + Query query, SnapshotVersion sinceReadTime) { ResourcePath path = query.getPath(); if (query.isDocumentQuery()) { return getDocumentsMatchingDocumentQuery(path); } else if (query.isCollectionGroupQuery()) { - return getDocumentsMatchingCollectionGroupQuery(query); + return getDocumentsMatchingCollectionGroupQuery(query, sinceReadTime); } else { - return getDocumentsMatchingCollectionQuery(query); + return getDocumentsMatchingCollectionQuery(query, sinceReadTime); } } @@ -156,7 +163,7 @@ private ImmutableSortedMap getDocumentsMatchingDocumentQu } private ImmutableSortedMap getDocumentsMatchingCollectionGroupQuery( - Query query) { + Query query, SnapshotVersion sinceReadTime) { hardAssert( query.getPath().isEmpty(), "Currently we only support collection group queries at the root."); @@ -169,7 +176,7 @@ private ImmutableSortedMap getDocumentsMatchingCollection for (ResourcePath parent : parents) { Query collectionQuery = query.asCollectionQueryAtPath(parent.append(collectionId)); ImmutableSortedMap collectionResults = - getDocumentsMatchingCollectionQuery(collectionQuery); + getDocumentsMatchingCollectionQuery(collectionQuery, sinceReadTime); for (Map.Entry docEntry : collectionResults) { results = results.insert(docEntry.getKey(), docEntry.getValue()); } @@ -179,9 +186,9 @@ private ImmutableSortedMap getDocumentsMatchingCollection /** Queries the remote documents and overlays mutations. */ private ImmutableSortedMap getDocumentsMatchingCollectionQuery( - Query query) { + Query query, SnapshotVersion sinceReadTime) { ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query); + remoteDocumentCache.getAllDocumentsMatchingQuery(query, sinceReadTime); List matchingBatches = mutationQueue.getAllMutationBatchesAffectingQuery(query); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java index a13b53fb1b2..2e9f5a815e5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryRemoteDocumentCache.java @@ -48,6 +48,9 @@ final class MemoryRemoteDocumentCache implements RemoteDocumentCache { @Override public void add(MaybeDocument document, SnapshotVersion readTime) { + hardAssert( + !readTime.equals(SnapshotVersion.NONE), + "Cannot add document to the RemoteDocumentCache with a read time of zero"); docs = docs.insert(document.getKey(), new Pair<>(document, readTime)); persistence.getIndexManager().addToCollectionParentIndex(document.getKey().getPath().popLast()); @@ -82,7 +85,8 @@ public Map getAll(Iterable keys) { } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery(Query query) { + public ImmutableSortedMap getAllDocumentsMatchingQuery( + Query query, SnapshotVersion sinceReadTime) { hardAssert( !query.isCollectionGroupQuery(), "CollectionGroup queries should be handled in LocalDocumentsView"); @@ -112,6 +116,11 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu continue; } + SnapshotVersion readTime = entry.getValue().second; + if (readTime.compareTo(sinceReadTime) <= 0) { + continue; + } + Document doc = (Document) maybeDoc; if (query.matches(doc)) { result = result.insert(doc.getKey(), doc); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java index dbd59eca9cd..c79ca10eab6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/RemoteDocumentCache.java @@ -75,7 +75,10 @@ interface RemoteDocumentCache { *

Cached NoDocument entries have no bearing on query results. * * @param query The query to match documents against. + * @param sinceReadTime If not set to SnapshotVersion.MIN, return only documents that have been + * read since this snapshot version (exclusive). * @return The set of matching documents. */ - ImmutableSortedMap getAllDocumentsMatchingQuery(Query query); + ImmutableSortedMap getAllDocumentsMatchingQuery( + Query query, SnapshotVersion sinceReadTime); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java index f3ddd3b028f..0ffc415d84c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLitePersistence.java @@ -77,7 +77,7 @@ public static String databaseName(String persistenceKey, DatabaseId databaseId) } } - private final OpenHelper opener; + private final SQLiteOpenHelper opener; private final LocalSerializer serializer; private final StatsCollector statsCollector; private final SQLiteQueryCache queryCache; @@ -125,8 +125,19 @@ public SQLitePersistence( LocalSerializer serializer, StatsCollector statsCollector, LruGarbageCollector.Params params) { - String databaseName = databaseName(persistenceKey, databaseId); - this.opener = new OpenHelper(context, databaseName); + this( + serializer, + statsCollector, + params, + new OpenHelper(context, databaseName(persistenceKey, databaseId))); + } + + public SQLitePersistence( + LocalSerializer serializer, + StatsCollector statsCollector, + LruGarbageCollector.Params params, + SQLiteOpenHelper openHelper) { + this.opener = openHelper; this.serializer = serializer; this.statsCollector = statsCollector; this.queryCache = new SQLiteQueryCache(this, this.serializer); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index 6032842e8e9..cf8ef713903 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -52,6 +52,10 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache { @Override public void add(MaybeDocument maybeDocument, SnapshotVersion readTime) { + hardAssert( + !readTime.equals(SnapshotVersion.NONE), + "Cannot add document to the RemoteDocumentCache with a read time of zero"); + String path = pathForKey(maybeDocument.getKey()); Timestamp timestamp = readTime.getTimestamp(); MessageLite message = serializer.encodeMaybeDocument(maybeDocument); @@ -131,7 +135,8 @@ public Map getAll(Iterable documentKeys } @Override - public ImmutableSortedMap getAllDocumentsMatchingQuery(Query query) { + public ImmutableSortedMap getAllDocumentsMatchingQuery( + Query query, SnapshotVersion sinceReadTime) { hardAssert( !query.isCollectionGroupQuery(), "CollectionGroup queries should be handled in LocalDocumentsView"); @@ -142,6 +147,7 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu String prefixPath = EncodedPath.encode(prefix); String prefixSuccessorPath = EncodedPath.prefixSuccessor(prefixPath); + Timestamp readTime = sinceReadTime.getTimestamp(); BackgroundQueue backgroundQueue = new BackgroundQueue(); @@ -150,8 +156,16 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery(Qu new ImmutableSortedMap[] {DocumentCollections.emptyDocumentMap()}; int rowsProcessed = - db.query("SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ?") - .binding(prefixPath, prefixSuccessorPath) + db.query( + "SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ? " + + "AND (read_time_seconds IS NULL OR read_time_seconds > ? " + + "OR (read_time_seconds = ? AND read_time_nanos > ?))") + .binding( + prefixPath, + prefixSuccessorPath, + readTime.getSeconds(), + readTime.getSeconds(), + readTime.getNanoseconds()) .forEach( row -> { // TODO: Actually implement a single-collection query diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 272ec183756..97c09efc415 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -46,9 +46,9 @@ class SQLiteSchema { * The version of the schema. Increase this by one for each migration added to runMigrations * below. * - *

TODO(index-free): The migration to schema version 9 doesn't backfill `update_time` as this + *

TODO(index-free): The migration to schema version 9 doesn't backfill `read_time` as this * requires rewriting the RemoteDocumentCache. For index-free queries to efficiently handle - * existing documents, we still need to populate update_time for all existing entries, drop the + * existing documents, we still need to populate read_time for all existing entries, drop the * RemoteDocumentCache or ask users to invoke `clearPersistence()` manually. If we decide to * backfill or drop the contents of the RemoteDocumentCache, we need to perform an additional * schema migration. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java index 983445a5651..13c524771aa 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java @@ -19,6 +19,7 @@ import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; +import com.google.firebase.firestore.model.SnapshotVersion; /** * A naive implementation of QueryEngine that just loads all the documents in the queried collection @@ -36,7 +37,7 @@ public SimpleQueryEngine(LocalDocumentsView localDocumentsView) { public ImmutableSortedMap getDocumentsMatchingQuery(Query query) { // TODO: Once LocalDocumentsView provides a getCollectionDocuments() method, we // should call that here and then filter the results. - return localDocumentsView.getDocumentsMatchingQuery(query); + return localDocumentsView.getDocumentsMatchingQuery(query, SnapshotVersion.NONE); } @Override diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 93e45b30848..181ff211d50 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -727,12 +727,12 @@ public void testCollectsGarbageAfterAcknowledgedMutation() { Query query = Query.atPath(ResourcePath.fromString("foo")); int targetId = allocateQuery(query); applyRemoteEvent( - updateRemoteEvent(doc("foo/bar", 0, map("foo", "old")), asList(targetId), emptyList())); + updateRemoteEvent(doc("foo/bar", 1, map("foo", "old")), asList(targetId), emptyList())); writeMutation(patchMutation("foo/bar", map("foo", "bar"))); releaseQuery(query); writeMutation(setMutation("foo/bah", map("foo", "bah"))); writeMutation(deleteMutation("foo/baz")); - assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(doc("foo/bah", 0, map("foo", "bah"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(deletedDoc("foo/baz", 0)); @@ -761,13 +761,13 @@ public void testCollectsGarbageAfterRejectedMutation() { Query query = Query.atPath(ResourcePath.fromString("foo")); int targetId = allocateQuery(query); applyRemoteEvent( - updateRemoteEvent(doc("foo/bar", 0, map("foo", "old")), asList(targetId), emptyList())); + updateRemoteEvent(doc("foo/bar", 1, map("foo", "old")), asList(targetId), emptyList())); writeMutation(patchMutation("foo/bar", map("foo", "bar"))); // Release the query so that our target count goes back to 0 and we are considered up-to-date. releaseQuery(query); writeMutation(setMutation("foo/bah", map("foo", "bah"))); writeMutation(deleteMutation("foo/baz")); - assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); + assertContains(doc("foo/bar", 1, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(doc("foo/bah", 0, map("foo", "bah"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(deletedDoc("foo/baz", 0)); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java index 6b5d49072c1..91e8407a871 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/RemoteDocumentCacheTestCase.java @@ -177,19 +177,48 @@ public void testDocumentsMatchingQuery() { Query query = Query.atPath(path("b")); ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query); + remoteDocumentCache.getAllDocumentsMatchingQuery(query, SnapshotVersion.NONE); List expected = asList(doc("b/1", 42, docData), doc("b/2", 42, docData)); assertEquals(expected, values(results)); } + @Test + public void testDocumentsMatchingQuerySinceReadTime() { + Map docData = map("data", 2); + addTestDocumentAtPath("b/old", /* updateTime= */ 1, /* readTime= */ 11); + addTestDocumentAtPath("b/current", /* updateTime= */ 2, /* readTime= = */ 12); + addTestDocumentAtPath("b/new", /* updateTime= */ 3, /* readTime= = */ 13); + + Query query = Query.atPath(path("b")); + ImmutableSortedMap results = + remoteDocumentCache.getAllDocumentsMatchingQuery(query, version(12)); + List expected = asList(doc("b/new", 3, docData)); + assertEquals(expected, values(results)); + } + + @Test + public void testDocumentsMatchingUsesReadTimeNotUpdateTime() { + Map docData = map("data", 2); + addTestDocumentAtPath("b/old", /* updateTime= */ 1, /* readTime= */ 2); + addTestDocumentAtPath("b/new", /* updateTime= */ 2, /* readTime= */ 1); + + Query query = Query.atPath(path("b")); + ImmutableSortedMap results = + remoteDocumentCache.getAllDocumentsMatchingQuery(query, version(1)); + List expected = asList(doc("b/old", 1, docData)); + assertEquals(expected, values(results)); + } + private Document addTestDocumentAtPath(String path) { - Document doc = doc(path, 42, map("data", 2)); - add(doc, version(42)); + return addTestDocumentAtPath(path, 42, 42); + } + + private Document addTestDocumentAtPath(String path, int updateTime, int readTime) { + Document doc = doc(path, updateTime, map("data", 2)); + add(doc, version(readTime)); return doc; } - // TODO(mrschmidt): Add a test uses different update and read times and verifies that we correctly - // filter by read time private void add(MaybeDocument doc, SnapshotVersion readTime) { persistence.runTransaction("add entry", () -> remoteDocumentCache.add(doc, readTime)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 1318987867e..e3fbbeb9c43 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -16,8 +16,11 @@ import static com.google.firebase.firestore.local.EncodedPath.decodeResourcePath; import static com.google.firebase.firestore.local.EncodedPath.encode; +import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.path; +import static com.google.firebase.firestore.testutil.TestUtil.query; +import static com.google.firebase.firestore.testutil.TestUtil.version; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -28,9 +31,13 @@ import android.database.sqlite.SQLiteDatabase; import android.database.sqlite.SQLiteOpenHelper; import androidx.test.core.app.ApplicationProvider; +import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.firestore.model.DatabaseId; +import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; +import com.google.firebase.firestore.proto.MaybeDocument; import com.google.firebase.firestore.proto.WriteBatch; +import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firestore.v1.Document; import com.google.firestore.v1.Write; import java.util.ArrayList; @@ -55,10 +62,11 @@ public class SQLiteSchemaTest { private SQLiteDatabase db; private SQLiteSchema schema; + private SQLiteOpenHelper opener; @Before public void setUp() { - SQLiteOpenHelper opener = + opener = new SQLiteOpenHelper(ApplicationProvider.getApplicationContext(), "foo", null, 1) { @Override public void onCreate(SQLiteDatabase db) {} @@ -394,6 +402,68 @@ public void canCreateCollectionParentsIndex() { assertEquals(expectedParents, actualParents); } + @Test + public void existingDocumentsRemainReadableAfterIndexFreeMigration() { + // Initialize the schema to the state prior to the index-free migration. + schema.runMigrations(0, 8); + db.execSQL( + "INSERT INTO remote_documents (path, contents) VALUES (?, ?)", + new Object[] {encode(path("coll/existing")), createDummyDocument("coll/existing")}); + + // Run the index-free migration. + schema.runMigrations(8, 9); + db.execSQL( + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", + new Object[] {encode(path("coll/old")), 0, 1000, createDummyDocument("coll/old")}); + db.execSQL( + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", + new Object[] {encode(path("coll/current")), 0, 2000, createDummyDocument("coll/current")}); + db.execSQL( + "INSERT INTO remote_documents (path, read_time_seconds, read_time_nanos, contents) VALUES (?, ?, ?, ?)", + new Object[] {encode(path("coll/new")), 0, 3000, createDummyDocument("coll/new")}); + + SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache(); + ImmutableSortedMap results = + remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(2)); + + // Verify that queries return the documents that existed prior to the index-free migration, as + // well as any documents with a newer read time than the one passed in. + assertResultsContain(results, "coll/existing", "coll/new"); + } + + private SQLiteRemoteDocumentCache createRemoteDocumentCache() { + DatabaseId databaseId = DatabaseId.forProject("foo"); + LocalSerializer serializer = new LocalSerializer(new RemoteSerializer(databaseId)); + SQLitePersistence persistence = + new SQLitePersistence( + serializer, + StatsCollector.NO_OP_STATS_COLLECTOR, + LruGarbageCollector.Params.Default(), + opener); + persistence.start(); + return new SQLiteRemoteDocumentCache( + persistence, serializer, StatsCollector.NO_OP_STATS_COLLECTOR); + } + + private byte[] createDummyDocument(String name) { + return MaybeDocument.newBuilder() + .setDocument( + Document.newBuilder() + .setName("projects/foo/databases/(default)/documents/" + name) + .build()) + .build() + .toByteArray(); + } + + private void assertResultsContain( + ImmutableSortedMap actualResults, + String... docs) { + for (String doc : docs) { + assertTrue("Expected result for " + doc, actualResults.containsKey(key(doc))); + } + assertEquals("Results contain unexpected entries", docs.length, actualResults.size()); + } + private void assertNoResultsForQuery(String query, String[] args) { Cursor cursor = null; try { From aafbec43e9b4e4abcde2e797f2e052e8afd2f335 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 13 Aug 2019 21:01:08 -0700 Subject: [PATCH 06/13] Only advance the limbo free snapshot if query is synced (#699) --- .../google/firebase/firestore/TestUtil.java | 2 +- .../firestore/core/QueryListener.java | 4 ++-- .../google/firebase/firestore/core/View.java | 5 ++--- .../firebase/firestore/core/ViewSnapshot.java | 22 +++++++++---------- .../firebase/firestore/local/LocalStore.java | 2 +- .../firestore/local/LocalViewChanges.java | 16 +++++--------- .../google/firebase/firestore/TestUtil.java | 2 +- .../firebase/firestore/QuerySnapshotTest.java | 2 +- .../firestore/core/QueryListenerTest.java | 18 +++++++-------- .../firestore/core/ViewSnapshotTest.java | 6 ++--- .../firebase/firestore/core/ViewTest.java | 8 +++---- 11 files changed, 41 insertions(+), 46 deletions(-) diff --git a/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java index 54f93fd0cb4..d2ec7b475a7 100644 --- a/firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/TestUtil.java +++ b/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); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java index 5414dfc7f65..c98d02c6fae 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/QueryListener.java +++ b/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); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java index 81082d341ca..0cf8208d4d5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/View.java +++ b/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 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); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java index 239f44a2dc8..2b60db02867 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/ViewSnapshot.java @@ -37,7 +37,7 @@ public enum SyncState { private final List changes; private final boolean isFromCache; private final ImmutableSortedSet mutatedKeys; - private final boolean hasLimboDocuments; + private final boolean synced; private final boolean didSyncStateChange; private boolean excludesMetadataChanges; @@ -48,7 +48,7 @@ public ViewSnapshot( List changes, boolean isFromCache, ImmutableSortedSet 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 mutatedKeys, boolean fromCache, - boolean hasLimboDocuments, + boolean synced, boolean excludesMetadataChanges) { List 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=" diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index c329bde7c2e..65c59483a1a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -491,7 +491,7 @@ public void notifyLocalViewChanges(List viewChanges) { } localViewReferences.removeReferences(removed, targetId); - if (!viewChange.hasUnresolvedLimboDocuments()) { + if (viewChange.isSynced()) { QueryData queryData = targetIds.get(targetId); hardAssert( queryData != null, diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java index 540cbad5b3d..1845b44a649 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalViewChanges.java +++ b/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 added; private final ImmutableSortedSet removed; public LocalViewChanges( int targetId, - boolean hasUnresolvedLimboDocuments, + boolean synced, ImmutableSortedSet added, ImmutableSortedSet 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 getAdded() { diff --git a/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java b/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java index 6997822c2e5..d7515737424 100644 --- a/firebase-firestore/src/roboUtil/java/com/google/firebase/firestore/TestUtil.java +++ b/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); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java index e1f7a99f36a..afed0affa5d 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/QuerySnapshotTest.java +++ b/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); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java index d093eb58e5b..f5f782752b4 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryListenerTest.java +++ b/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)); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java index 8abe6a50ff3..e980566aef5 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewSnapshotTest.java @@ -45,7 +45,7 @@ public void testConstructor() { List changes = Arrays.asList(DocumentViewChange.create(Type.ADDED, doc("c/foo", 1, map()))); ImmutableSortedSet 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()); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java index cdefee313ba..aabd70c3381 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/ViewTest.java +++ b/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 From 715f3f03a96681e8dfd816b2645276b2cd58b8d7 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 21 Aug 2019 10:17:47 -0700 Subject: [PATCH 07/13] Add Index-Free query engine (#697) --- .../collection/ImmutableSortedMap.java | 8 + .../firestore/remote/RemoteStoreTest.java | 4 +- .../firestore/core/FirestoreClient.java | 6 +- .../google/firebase/firestore/core/Query.java | 12 + .../firebase/firestore/core/SyncEngine.java | 6 +- .../firestore/local/IndexFreeQueryEngine.java | 150 +++++++++ .../firestore/local/IndexedQueryEngine.java | 17 +- .../firestore/local/LocalDocumentsView.java | 2 +- .../firebase/firestore/local/LocalStore.java | 44 ++- .../firebase/firestore/local/QueryEngine.java | 15 +- .../firestore/local/SimpleQueryEngine.java | 16 +- .../firebase/firestore/core/QueryTest.java | 25 ++ .../local/IndexFreeQueryEngineTest.java | 290 ++++++++++++++++++ .../local/IndexedQueryEngineTest.java | 17 +- .../firestore/local/LocalStoreTestCase.java | 259 ++++++++++++++-- .../firestore/local/MemoryLocalStoreTest.java | 23 +- .../firestore/local/SQLiteLocalStoreTest.java | 23 +- .../firebase/firestore/spec/QueryEvent.java | 2 +- .../firebase/firestore/spec/SpecTestCase.java | 6 +- .../firebase/firestore/testutil/TestUtil.java | 28 +- 20 files changed, 893 insertions(+), 60 deletions(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java create mode 100644 firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java diff --git a/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java b/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java index f62c973aa1d..d7a648b9684 100644 --- a/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java +++ b/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java @@ -56,6 +56,14 @@ public abstract class ImmutableSortedMap implements Iterable getComparator(); + public ImmutableSortedMap insertAll(ImmutableSortedMap source) { + ImmutableSortedMap result = this; + for (Map.Entry entry : source) { + result = result.insert(entry.getKey(), entry.getValue()); + } + return result; + } + @Override @SuppressWarnings("unchecked") public boolean equals(Object o) { diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java index eb703a1af99..da03bc2253d 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/remote/RemoteStoreTest.java @@ -24,6 +24,7 @@ import com.google.firebase.firestore.local.LocalStore; import com.google.firebase.firestore.local.MemoryPersistence; import com.google.firebase.firestore.local.Persistence; +import com.google.firebase.firestore.local.SimpleQueryEngine; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.mutation.MutationBatchResult; import com.google.firebase.firestore.testutil.IntegrationTestUtil; @@ -72,9 +73,10 @@ public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { }; FakeConnectivityMonitor connectivityMonitor = new FakeConnectivityMonitor(); + SimpleQueryEngine queryEngine = new SimpleQueryEngine(); Persistence persistence = MemoryPersistence.createEagerGcMemoryPersistence(); persistence.start(); - LocalStore localStore = new LocalStore(persistence, User.UNAUTHENTICATED); + LocalStore localStore = new LocalStore(persistence, queryEngine, User.UNAUTHENTICATED); RemoteStore remoteStore = new RemoteStore(callback, localStore, datastore, testQueue, connectivityMonitor); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java index fb8b23c7948..84aa3aa76b1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FirestoreClient.java @@ -37,7 +37,9 @@ import com.google.firebase.firestore.local.LruGarbageCollector; import com.google.firebase.firestore.local.MemoryPersistence; import com.google.firebase.firestore.local.Persistence; +import com.google.firebase.firestore.local.QueryEngine; import com.google.firebase.firestore.local.SQLitePersistence; +import com.google.firebase.firestore.local.SimpleQueryEngine; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; @@ -263,7 +265,9 @@ private void initialize(Context context, User user, boolean usePersistence, long } persistence.start(); - localStore = new LocalStore(persistence, user); + // TODO(index-free): Use IndexFreeQueryEngine/IndexedQueryEngine as appropriate. + QueryEngine queryEngine = new SimpleQueryEngine(); + localStore = new LocalStore(persistence, queryEngine, user); if (gc != null) { lruScheduler = gc.newScheduler(asyncQueue, localStore); lruScheduler.start(); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index 107bab94089..b4206485503 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -117,6 +117,18 @@ public boolean isCollectionGroupQuery() { return collectionGroup != null; } + /** + * Returns true if this query does not specify any query constraints that could remove results. + */ + public boolean matchesAllDocuments() { + return filters.isEmpty() + && limit == NO_LIMIT + && startAt == null + && endAt == null + && (getExplicitOrderBy().isEmpty() + || (getExplicitOrderBy().size() == 1 && getFirstOrderByField().isKeyField())); + } + /** The filters on the documents returned by the query. */ public List getFilters() { return filters; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java index e03694637e4..6fc55434886 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java @@ -193,9 +193,10 @@ public int listen(Query query) { private ViewSnapshot initializeViewAndComputeSnapshot(QueryData queryData) { Query query = queryData.getQuery(); - ImmutableSortedMap docs = localStore.executeQuery(query); ImmutableSortedSet remoteKeys = localStore.getRemoteDocumentKeys(queryData.getTargetId()); + ImmutableSortedMap docs = + localStore.executeQuery(query, queryData, remoteKeys); View view = new View(query, remoteKeys); View.DocumentChanges viewDocChanges = view.computeDocChanges(docs); @@ -557,7 +558,8 @@ private void emitNewSnapsAndNotifyLocalStore( // against the local store to make sure we didn't lose any good docs that had been past the // limit. ImmutableSortedMap docs = - localStore.executeQuery(queryView.getQuery()); + localStore.executeQuery( + queryView.getQuery(), /* queryData= */ null, DocumentKey.emptyKeySet()); viewDocChanges = view.computeDocChanges(docs, viewDocChanges); } TargetChange targetChange = diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java new file mode 100644 index 00000000000..a6b10572f35 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java @@ -0,0 +1,150 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.local; + +import static com.google.firebase.firestore.util.Assert.hardAssert; + +import androidx.annotation.Nullable; +import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.ImmutableSortedSet; +import com.google.firebase.firestore.core.Query; +import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.model.DocumentCollections; +import com.google.firebase.firestore.model.DocumentKey; +import com.google.firebase.firestore.model.MaybeDocument; +import com.google.firebase.firestore.model.SnapshotVersion; +import java.util.Map; + +/** + * A query engine that takes advantage of the target document mapping in the QueryCache. The + * IndexFreeQueryEngine optimizes query execution by only reading the documents previously matched a + * query plus any documents that were edited after the query was last listened to. + * + *

There are some cases where Index-Free queries are not guaranteed to produce to the same + * results as full collection scans. In these case, the IndexFreeQueryEngine falls back to a full + * query processing. These cases are: + * + *

    + *
  1. Limit queries where a document that matched the query previously no longer matches the + * query. In this case, we have to scan all local documents since a document that was sent to + * us as part of a different query result may now fall into the limit. + *
  2. Limit queries that include edits that occurred after the last remote snapshot (both + * latency-compensated or committed). Even if an edited document continues to match the query, + * an edit may cause a document to sort below another document that is in the local cache. + *
  3. Queries where the last snapshot contained Limbo documents. Even though a Limbo document is + * not part of the backend result set, we need to include Limbo documents in local views to + * ensure consistency between different Query views. If there exists a previous query snapshot + * that contained no limbo documents, we can instead use the older snapshot version for + * Index-Free processing. + *
+ */ +public class IndexFreeQueryEngine implements QueryEngine { + private LocalDocumentsView localDocumentsView; + + @Override + public void setLocalDocumentsView(LocalDocumentsView localDocuments) { + this.localDocumentsView = localDocuments; + } + + @Override + public ImmutableSortedMap getDocumentsMatchingQuery( + Query query, @Nullable QueryData queryData, ImmutableSortedSet remoteKeys) { + hardAssert(localDocumentsView != null, "setLocalDocumentsView() not called"); + + // Queries that match all document don't benefit from using IndexFreeQueries. It is more + // efficient to scan all documents in a collection, rather than to perform individual lookups. + if (query.matchesAllDocuments()) { + return executeFullCollectionScan(query); + } + + // Queries that have never seen a snapshot without limbo free documents should also be run as a + // full collection scan. + if (queryData == null + || queryData.getLastLimboFreeSnapshotVersion().equals(SnapshotVersion.NONE)) { + return executeFullCollectionScan(query); + } + + ImmutableSortedMap result = + executeIndexFreeQuery(query, queryData, remoteKeys); + + return result != null ? result : executeFullCollectionScan(query); + } + + /** + * Attempts index-free query execution. Returns the set of query results on success, otherwise + * returns null. + */ + private @Nullable ImmutableSortedMap executeIndexFreeQuery( + Query query, QueryData queryData, ImmutableSortedSet remoteKeys) { + // Fetch the documents that matched the query at the last snapshot. + ImmutableSortedMap previousResults = + localDocumentsView.getDocuments(remoteKeys); + + // Limit queries are not eligible for index-free query execution if any part of the result was + // modified after we received the last query snapshot. This makes sure that we re-populate the + // view with older documents that may sort before the modified document. + if (query.hasLimit() + && containsUpdatesSinceSnapshotVersion(previousResults, queryData.getSnapshotVersion())) { + return null; + } + + ImmutableSortedMap results = DocumentCollections.emptyDocumentMap(); + + // Re-apply the query filter since previously matching documents do not necessarily still + // match the query. + for (Map.Entry entry : previousResults) { + MaybeDocument maybeDoc = entry.getValue(); + if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) { + Document doc = (Document) maybeDoc; + results = results.insert(entry.getKey(), doc); + } else if (query.hasLimit()) { + // Limit queries with documents that no longer match need to be re-filled from cache. + return null; + } + } + + // Retrieve all results for documents that were updated since the last limbo-document free + // remote snapshot. + ImmutableSortedMap updatedResults = + localDocumentsView.getDocumentsMatchingQuery( + query, queryData.getLastLimboFreeSnapshotVersion()); + + results = results.insertAll(updatedResults); + + return results; + } + + @Override + public void handleDocumentChange(MaybeDocument oldDocument, MaybeDocument newDocument) { + // No indexes to update. + } + + private boolean containsUpdatesSinceSnapshotVersion( + ImmutableSortedMap previousResults, + SnapshotVersion sinceSnapshotVersion) { + for (Map.Entry doc : previousResults) { + if (doc.getValue().hasPendingWrites() + || doc.getValue().getVersion().compareTo(sinceSnapshotVersion) > 0) { + return true; + } + } + + return false; + } + + private ImmutableSortedMap executeFullCollectionScan(Query query) { + return localDocumentsView.getDocumentsMatchingQuery(query, SnapshotVersion.NONE); + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java index 2cb85b05461..fd54260f486 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexedQueryEngine.java @@ -19,6 +19,7 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.FieldFilter; import com.google.firebase.firestore.core.Filter; import com.google.firebase.firestore.core.Filter.Operator; @@ -88,17 +89,23 @@ public class IndexedQueryEngine implements QueryEngine { private static final List lowCardinalityTypes = Arrays.asList(BooleanValue.class, ArrayValue.class, ObjectValue.class); - private final LocalDocumentsView localDocuments; private final SQLiteCollectionIndex collectionIndex; + private LocalDocumentsView localDocuments; - public IndexedQueryEngine( - LocalDocumentsView localDocuments, SQLiteCollectionIndex collectionIndex) { - this.localDocuments = localDocuments; + public IndexedQueryEngine(SQLiteCollectionIndex collectionIndex) { this.collectionIndex = collectionIndex; } @Override - public ImmutableSortedMap getDocumentsMatchingQuery(Query query) { + public void setLocalDocumentsView(LocalDocumentsView localDocuments) { + this.localDocuments = localDocuments; + } + + @Override + public ImmutableSortedMap getDocumentsMatchingQuery( + Query query, @Nullable QueryData queryData, ImmutableSortedSet remoteKeys) { + hardAssert(localDocuments != null, "setLocalDocumentsView() not called"); + return query.isDocumentQuery() ? localDocuments.getDocumentsMatchingQuery(query, SnapshotVersion.NONE) : performCollectionQuery(query); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index 670b55cd874..99858b8d51b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -40,7 +40,7 @@ * mutations in the MutationQueue to the RemoteDocumentCache. */ // TODO: Turn this into the UnifiedDocumentCache / whatever. -final class LocalDocumentsView { +class LocalDocumentsView { private final RemoteDocumentCache remoteDocumentCache; private final MutationQueue mutationQueue; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java index 65c59483a1a..ecb44399a24 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java @@ -19,6 +19,7 @@ import android.util.SparseArray; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import com.google.firebase.Timestamp; import com.google.firebase.database.collection.ImmutableSortedMap; import com.google.firebase.database.collection.ImmutableSortedSet; @@ -125,7 +126,7 @@ public final class LocalStore { /** Used to generate targetIds for queries tracked locally. */ private final TargetIdGenerator targetIdGenerator; - public LocalStore(Persistence persistence, User initialUser) { + public LocalStore(Persistence persistence, QueryEngine queryEngine, User initialUser) { hardAssert( persistence.isStarted(), "LocalStore was passed an unstarted persistence implementation"); this.persistence = persistence; @@ -135,8 +136,9 @@ public LocalStore(Persistence persistence, User initialUser) { remoteDocuments = persistence.getRemoteDocumentCache(); localDocuments = new LocalDocumentsView(remoteDocuments, mutationQueue, persistence.getIndexManager()); - // TODO: Use IndexedQueryEngine as appropriate. - queryEngine = new SimpleQueryEngine(localDocuments); + + this.queryEngine = queryEngine; + queryEngine.setLocalDocumentsView(localDocuments); localViewReferences = new ReferenceSet(); persistence.getReferenceDelegate().setInMemoryPins(localViewReferences); @@ -170,8 +172,7 @@ public ImmutableSortedMap handleUserChange(User user // Recreate our LocalDocumentsView using the new MutationQueue. localDocuments = new LocalDocumentsView(remoteDocuments, mutationQueue, persistence.getIndexManager()); - // TODO: Use IndexedQueryEngine as appropriate. - queryEngine = new SimpleQueryEngine(localDocuments); + queryEngine.setLocalDocumentsView(localDocuments); // Union the old/new changed keys. ImmutableSortedSet changedKeys = DocumentKey.emptyKeySet(); @@ -561,6 +562,21 @@ public QueryData allocateQuery(Query query) { return cached; } + /** + * Returns the QueryData as seen by the LocalStore, including updates that may have not yet been + * persisted to the QueryCache. + */ + @VisibleForTesting + @Nullable + QueryData getQueryData(Query query) { + QueryData queryData = queryCache.getQueryData(query); + if (queryData == null) { + return null; + } + QueryData updatedQueryData = targetIds.get(queryData.getTargetId()); + return updatedQueryData != null ? updatedQueryData : queryData; + } + /** Mutable state for the transaction in allocateQuery. */ private static class AllocateQueryHolder { QueryData cached; @@ -611,7 +627,23 @@ public void releaseQuery(Query query) { /** Runs the given query against all the documents in the local store and returns the results. */ public ImmutableSortedMap executeQuery(Query query) { - return queryEngine.getDocumentsMatchingQuery(query); + QueryData queryData = getQueryData(query); + if (queryData != null) { + ImmutableSortedSet remoteKeys = + this.queryCache.getMatchingKeysForTargetId(queryData.getTargetId()); + return executeQuery(query, queryData, remoteKeys); + } else { + return executeQuery(query, null, DocumentKey.emptyKeySet()); + } + } + + /** + * Runs the given query against the local store and returns the results, potentially taking + * advantage of the provided query data and the set of remote document keys. + */ + public ImmutableSortedMap executeQuery( + Query query, @Nullable QueryData queryData, ImmutableSortedSet remoteKeys) { + return queryEngine.getDocumentsMatchingQuery(query, queryData, remoteKeys); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java index 45e7afc7888..d071316718b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java @@ -14,17 +14,26 @@ package com.google.firebase.firestore.local; +import androidx.annotation.Nullable; import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; -/** Represents a query engine capable of performing queries over the local document cache. */ -interface QueryEngine { +/** + * Represents a query engine capable of performing queries over the local document cache. You must + * call setQueryDataProvider() and setLocalDocumentsView() before using. + */ +public interface QueryEngine { + + /** Sets the document view to query against. */ + void setLocalDocumentsView(LocalDocumentsView localDocuments); /** Returns all local documents matching the specified query. */ - ImmutableSortedMap getDocumentsMatchingQuery(Query query); + ImmutableSortedMap getDocumentsMatchingQuery( + Query query, @Nullable QueryData queryData, ImmutableSortedSet remoteKeys); /** * Notifies the query engine of a document change in case it would like to update indexes and the diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java index 13c524771aa..998aba3bb3f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SimpleQueryEngine.java @@ -14,7 +14,11 @@ package com.google.firebase.firestore.local; +import static com.google.firebase.firestore.util.Assert.hardAssert; + +import androidx.annotation.Nullable; import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -27,14 +31,18 @@ */ public class SimpleQueryEngine implements QueryEngine { - private final LocalDocumentsView localDocumentsView; + private LocalDocumentsView localDocumentsView; - public SimpleQueryEngine(LocalDocumentsView localDocumentsView) { - this.localDocumentsView = localDocumentsView; + @Override + public void setLocalDocumentsView(LocalDocumentsView localDocuments) { + this.localDocumentsView = localDocuments; } @Override - public ImmutableSortedMap getDocumentsMatchingQuery(Query query) { + public ImmutableSortedMap getDocumentsMatchingQuery( + Query query, @Nullable QueryData queryData, ImmutableSortedSet remoteKeys) { + hardAssert(localDocumentsView != null, "setLocalDocumentsView() not called"); + // TODO: Once LocalDocumentsView provides a getCollectionDocuments() method, we // should call that here and then filter the results. return localDocumentsView.getDocumentsMatchingQuery(query, SnapshotVersion.NONE); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java index e72ce1c12cc..1665bf72c20 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/core/QueryTest.java @@ -31,6 +31,7 @@ import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.testutil.ComparatorTester; +import java.util.Collections; import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -528,4 +529,28 @@ public void testImplicitOrderBy() { asList(orderBy("foo", "desc"), orderBy("bar", "asc"), orderBy(KEY_FIELD_NAME, "asc")), baseQuery.orderBy(orderBy("foo", "desc")).orderBy(orderBy("bar", "asc")).getOrderBy()); } + + @Test + public void testMatchesAllDocuments() { + Query baseQuery = Query.atPath(ResourcePath.fromString("collection")); + assertTrue(baseQuery.matchesAllDocuments()); + + Query query = baseQuery.orderBy(orderBy("__name__")); + assertTrue(query.matchesAllDocuments()); + + query = baseQuery.orderBy(orderBy("foo")); + assertFalse(query.matchesAllDocuments()); + + query = baseQuery.filter(filter("foo", "==", "bar")); + assertFalse(query.matchesAllDocuments()); + + query = baseQuery.limit(1); + assertFalse(query.matchesAllDocuments()); + + query = baseQuery.startAt(new Bound(Collections.emptyList(), true)); + assertFalse(query.matchesAllDocuments()); + + query = baseQuery.endAt(new Bound(Collections.emptyList(), true)); + assertFalse(query.matchesAllDocuments()); + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java new file mode 100644 index 00000000000..29e73f89a26 --- /dev/null +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java @@ -0,0 +1,290 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.local; + +import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.docSet; +import static com.google.firebase.firestore.testutil.TestUtil.filter; +import static com.google.firebase.firestore.testutil.TestUtil.map; +import static com.google.firebase.firestore.testutil.TestUtil.orderBy; +import static com.google.firebase.firestore.testutil.TestUtil.query; +import static com.google.firebase.firestore.testutil.TestUtil.version; +import static org.junit.Assert.assertEquals; + +import com.google.android.gms.common.internal.Preconditions; +import com.google.firebase.database.collection.ImmutableSortedMap; +import com.google.firebase.database.collection.ImmutableSortedSet; +import com.google.firebase.firestore.auth.User; +import com.google.firebase.firestore.core.Query; +import com.google.firebase.firestore.core.View; +import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.model.DocumentKey; +import com.google.firebase.firestore.model.DocumentSet; +import com.google.firebase.firestore.model.SnapshotVersion; +import com.google.protobuf.ByteString; +import java.util.Collections; +import java.util.concurrent.Callable; +import javax.annotation.Nullable; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class IndexFreeQueryEngineTest { + + private static final int TEST_TARGET_ID = 1; + + private static final Document MATCHING_DOC_A = + doc("coll/a", 1, map("matches", true, "order", 1), Document.DocumentState.SYNCED); + private static final Document NON_MATCHING_DOC_A = + doc("coll/a", 1, map("matches", false, "order", 1), Document.DocumentState.SYNCED); + private static final Document PENDING_MATCHING_DOC_A = + doc("coll/a", 1, map("matches", true, "order", 1), Document.DocumentState.LOCAL_MUTATIONS); + private static final Document PENDING_NON_MATCHING_DOC_A = + doc("coll/a", 1, map("matches", false, "order", 1), Document.DocumentState.LOCAL_MUTATIONS); + private static final Document UDPATED_DOC_A = + doc("coll/a", 11, map("matches", true, "order", 1), Document.DocumentState.SYNCED); + private static final Document MATCHING_DOC_B = + doc("coll/b", 1, map("matches", true, "order", 2), Document.DocumentState.SYNCED); + private static final Document NON_MATCHING_DOC_B = + doc("coll/b", 1, map("matches", false, "order", 2), Document.DocumentState.SYNCED); + private static final Document UPDATED_MATCHING_DOC_B = + doc("coll/b", 11, map("matches", true, "order", 2), Document.DocumentState.SYNCED); + + private MemoryPersistence persistence; + private MemoryRemoteDocumentCache remoteDocumentCache; + private QueryCache queryCache; + private QueryEngine queryEngine; + + private @Nullable Boolean expectIndexFreeExecution; + + @Before + public void setUp() { + expectIndexFreeExecution = null; + + persistence = MemoryPersistence.createEagerGcMemoryPersistence(); + queryCache = new MemoryQueryCache(persistence); + queryEngine = new IndexFreeQueryEngine(); + + remoteDocumentCache = persistence.getRemoteDocumentCache(); + + LocalDocumentsView localDocuments = + new LocalDocumentsView( + remoteDocumentCache, + persistence.getMutationQueue(User.UNAUTHENTICATED), + new MemoryIndexManager()) { + @Override + public ImmutableSortedMap getDocumentsMatchingQuery( + Query query, SnapshotVersion sinceReadTime) { + assertEquals( + "Observed query execution mode did not match expectation", + expectIndexFreeExecution, + !SnapshotVersion.NONE.equals(sinceReadTime)); + return super.getDocumentsMatchingQuery(query, sinceReadTime); + } + }; + queryEngine.setLocalDocumentsView(localDocuments); + } + + /** Adds the provided documents to the query target mapping. */ + private void persistQueryMapping(Document... docs) { + persistence.runTransaction( + "persistQueryMapping", + () -> { + ImmutableSortedSet remoteKeys = DocumentKey.emptyKeySet(); + for (Document doc : docs) { + remoteKeys = remoteKeys.insert(doc.getKey()); + } + queryCache.addMatchingKeys(remoteKeys, TEST_TARGET_ID); + }); + } + + /** Adds the provided documents to the remote document cache. */ + private void addDocument(Document... docs) { + persistence.runTransaction( + "addDocument", + () -> { + for (Document doc : docs) { + remoteDocumentCache.add(doc, doc.getVersion()); + } + }); + } + + private T expectIndexFreeQuery(Callable c) throws Exception { + try { + expectIndexFreeExecution = true; + return c.call(); + } finally { + expectIndexFreeExecution = null; + } + } + + private T expectFullCollectionQuery(Callable c) throws Exception { + try { + expectIndexFreeExecution = false; + return c.call(); + } finally { + expectIndexFreeExecution = null; + } + } + + private DocumentSet runQuery(Query query, QueryData queryData) { + Preconditions.checkNotNull( + expectIndexFreeExecution, + "Encountered runQuery() call not wrapped in expectIndexFreeQuery()/expectFullCollectionQuery()"); + ImmutableSortedMap docs = + queryEngine.getDocumentsMatchingQuery( + query, queryData, queryCache.getMatchingKeysForTargetId(TEST_TARGET_ID)); + View view = + new View(query, new ImmutableSortedSet<>(Collections.emptyList(), DocumentKey::compareTo)); + View.DocumentChanges viewDocChanges = view.computeDocChanges(docs); + return view.applyChanges(viewDocChanges).getSnapshot().getDocuments(); + } + + @Test + public void usesTargetMappingForInitialView() throws Exception { + Query query = query("coll").filter(filter("matches", "==", true)); + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + addDocument(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + + DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs); + } + + @Test + public void filtersNonMatchingInitialResults() throws Exception { + Query query = query("coll").filter(filter("matches", "==", true)); + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + addDocument(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + + // Add a mutated document that is not yet part of query's set of remote keys. + addDocument(PENDING_NON_MATCHING_DOC_A); + + DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs); + } + + @Test + public void includesChangesSinceInitialResults() throws Exception { + Query query = query("coll").filter(filter("matches", "==", true)); + QueryData originalQueryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + addDocument(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + + DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, originalQueryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs); + + addDocument(UPDATED_MATCHING_DOC_B); + + docs = expectIndexFreeQuery(() -> runQuery(query, originalQueryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_A, UPDATED_MATCHING_DOC_B), docs); + } + + @Test + public void doesNotUseInitialResultsWithoutLimboFreeSnapshotVersion() throws Exception { + Query query = query("coll").filter(filter("matches", "==", true)); + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ false); + + DocumentSet docs = expectFullCollectionQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator()), docs); + } + + @Test + public void doesNotUseInitialResultsForUnfilteredCollectionQuery() throws Exception { + Query query = query("coll"); + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + DocumentSet docs = expectFullCollectionQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator()), docs); + } + + @Test + public void doesNotUseInitialResultsForLimitQueryWithDocumentRemoval() throws Exception { + Query query = query("coll").filter(filter("matches", "==", true)).limit(1); + + // While the backend would never add DocA to the set of remote keys, this allows us to easily + // simulate what would happen when a document no longer matches due to an out-of-band update. + addDocument(NON_MATCHING_DOC_A); + persistQueryMapping(NON_MATCHING_DOC_A); + + addDocument(MATCHING_DOC_B); + + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + DocumentSet docs = expectFullCollectionQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs); + } + + @Test + public void doesNotUseInitialResultsForLimitQueryWithPendingWrite() throws Exception { + Query query = + query("coll") + .filter(filter("matches", "==", true)) + .orderBy(orderBy("order", "desc")) + .limit(1); + + // Add a query mapping for a document that matches, but that sorts below another document due to + // a pending write. + addDocument(PENDING_MATCHING_DOC_A); + persistQueryMapping(PENDING_MATCHING_DOC_A); + + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + addDocument(MATCHING_DOC_B); + + DocumentSet docs = expectFullCollectionQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs); + } + + @Test + public void doesNotUseInitialResultsForLimitQueryWithDocumentThatHasBeenUpdatedOutOfBand() + throws Exception { + Query query = + query("coll") + .filter(filter("matches", "==", true)) + .orderBy(orderBy("order", "desc")) + .limit(1); + + // Add a query mapping for a document that matches, but that sorts below another document based + // due to an update that the SDK received after the query's snapshot was persisted. + addDocument(UDPATED_DOC_A); + persistQueryMapping(UDPATED_DOC_A); + + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + addDocument(MATCHING_DOC_B); + + DocumentSet docs = expectFullCollectionQuery(() -> runQuery(query, queryData)); + assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs); + } + + private QueryData queryData(Query query, boolean hasLimboFreeSnapshot) { + return new QueryData( + query, + TEST_TARGET_ID, + 1, + QueryPurpose.LISTEN, + version(10), + hasLimboFreeSnapshot ? version(10) : SnapshotVersion.NONE, + ByteString.EMPTY); + } +} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java index 48ab21b2e11..b18efe0d9aa 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexedQueryEngineTest.java @@ -69,7 +69,7 @@ public void setUp() { remoteDocuments = persistence.getRemoteDocumentCache(); LocalDocumentsView localDocuments = new LocalDocumentsView(remoteDocuments, mutationQueue, persistence.getIndexManager()); - queryEngine = new IndexedQueryEngine(localDocuments, index); + queryEngine = new IndexedQueryEngine(index); } private void addDocument(Document newDoc) { @@ -214,7 +214,8 @@ public void addDocumentQuery() { Query query = query("coll").filter(filter("a", "==", "a")); ImmutableSortedMap results = - queryEngine.getDocumentsMatchingQuery(query); + queryEngine.getDocumentsMatchingQuery( + query, /* queryData= */ null, DocumentKey.emptyKeySet()); assertThat(results).doesNotContain(IGNORED_DOC.getKey()); assertThat(results).contains(MATCHING_DOC.getKey()); @@ -229,7 +230,8 @@ public void updateDocumentQuery() { Query query = query("coll").filter(filter("a", "==", "a")); ImmutableSortedMap results = - queryEngine.getDocumentsMatchingQuery(query); + queryEngine.getDocumentsMatchingQuery( + query, /* queryData= */ null, DocumentKey.emptyKeySet()); assertThat(results).doesNotContain(IGNORED_DOC.getKey()); assertThat(results).contains(MATCHING_DOC.getKey()); @@ -244,7 +246,8 @@ public void removeDocumentQuery() { Query query = query("coll").filter(filter("a", "==", "a")); ImmutableSortedMap results = - queryEngine.getDocumentsMatchingQuery(query); + queryEngine.getDocumentsMatchingQuery( + query, /* queryData= */ null, DocumentKey.emptyKeySet()); assertThat(results).doesNotContain(IGNORED_DOC.getKey()); assertThat(results).doesNotContain(MATCHING_DOC.getKey()); @@ -262,7 +265,8 @@ public void nestedQuery() { Query query = query("coll").filter(filter("a.a", "==", "a")); ImmutableSortedMap results = - queryEngine.getDocumentsMatchingQuery(query); + queryEngine.getDocumentsMatchingQuery( + query, /* queryData= */ null, DocumentKey.emptyKeySet()); assertThat(results).doesNotContain(ignoredDoc.getKey()); assertThat(results).contains(matchingDoc.getKey()); @@ -276,7 +280,8 @@ public void orderByQuery() { Query query = query("coll").orderBy(TestUtil.orderBy("a")); ImmutableSortedMap results = - queryEngine.getDocumentsMatchingQuery(query); + queryEngine.getDocumentsMatchingQuery( + query, /* queryData= */ null, DocumentKey.emptyKeySet()); assertThat(results).doesNotContain(IGNORED_DOC.getKey()); assertThat(results).contains(MATCHING_DOC.getKey()); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java index 4004759bf6f..cdb16bcc2ba 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java @@ -19,6 +19,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.deleteMutation; import static com.google.firebase.firestore.testutil.TestUtil.deletedDoc; import static com.google.firebase.firestore.testutil.TestUtil.doc; +import static com.google.firebase.firestore.testutil.TestUtil.filter; import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.noChangeEvent; @@ -40,6 +41,8 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assume.assumeFalse; +import static org.junit.Assume.assumeTrue; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -72,6 +75,7 @@ import java.util.Set; import javax.annotation.Nullable; import org.junit.After; +import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -86,15 +90,19 @@ * */ public abstract class LocalStoreTestCase { + private QueryEngine queryEngine; private Persistence localStorePersistence; private LocalStore localStore; private List batches; private @Nullable ImmutableSortedMap lastChanges; + private ImmutableSortedMap lastQueryResult; private int lastTargetId; AccumulatingStatsCollector statsCollector; + abstract QueryEngine getQueryEngine(); + abstract Persistence getPersistence(); abstract boolean garbageCollectorIsEager(); @@ -104,10 +112,12 @@ public void setUp() { statsCollector = new AccumulatingStatsCollector(); batches = new ArrayList<>(); lastChanges = null; + lastQueryResult = null; lastTargetId = 0; localStorePersistence = getPersistence(); - localStore = new LocalStore(localStorePersistence, User.UNAUTHENTICATED); + queryEngine = getQueryEngine(); + localStore = new LocalStore(localStorePersistence, queryEngine, User.UNAUTHENTICATED); localStore.start(); } @@ -136,6 +146,10 @@ private void notifyLocalViewChanges(LocalViewChanges changes) { localStore.notifyLocalViewChanges(asList(changes)); } + private void udpateViews(int targetId, boolean synced) { + notifyLocalViewChanges(viewChanges(targetId, synced, asList(), asList())); + } + private void acknowledgeMutation(long documentVersion, @Nullable Object transformResult) { MutationBatch batch = batches.remove(0); SnapshotVersion version = version(documentVersion); @@ -165,6 +179,11 @@ private int allocateQuery(Query query) { return queryData.getTargetId(); } + private void executeQuery(Query query) { + resetPersistenceStats(); + lastQueryResult = localStore.executeQuery(query); + } + private void releaseQuery(Query query) { localStore.releaseQuery(query); } @@ -212,6 +231,14 @@ private void assertNotContains(String keyPathString) { assertNull(actual); } + private void assertQueryReturned(String... keys) { + assertNotNull(lastQueryResult); + for (String key : keys) { + assertTrue("Expected query to return: " + key, lastQueryResult.containsKey(key(key))); + } + assertEquals(lastQueryResult.size(), keys.length); + } + /** * Asserts the expected numbers of mutation rows read by the MutationQueue since the last call to * `resetPersistenceStats()`. @@ -284,7 +311,7 @@ public void testHandlesSetMutationThenAckThenRelease() { allocateQuery(query); writeMutation(setMutation("foo/bar", map("foo", "bar"))); - notifyLocalViewChanges(viewChanges(2, asList("foo/bar"), emptyList())); + notifyLocalViewChanges(viewChanges(2, /* synced= */ false, asList("foo/bar"), emptyList())); assertChanged(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); assertContains(doc("foo/bar", 0, map("foo", "bar"), Document.DocumentState.LOCAL_MUTATIONS)); @@ -804,14 +831,16 @@ public void testPinsDocumentsInTheLocalView() { assertContains(doc("foo/bar", 1, map("foo", "bar"))); assertContains(doc("foo/baz", 0, map("foo", "baz"), Document.DocumentState.LOCAL_MUTATIONS)); - notifyLocalViewChanges(viewChanges(2, asList("foo/bar", "foo/baz"), emptyList())); + notifyLocalViewChanges( + viewChanges(2, /* synced= */ false, asList("foo/bar", "foo/baz"), emptyList())); applyRemoteEvent(updateRemoteEvent(doc("foo/bar", 1, map("foo", "bar")), none, two)); applyRemoteEvent(updateRemoteEvent(doc("foo/baz", 2, map("foo", "baz")), two, none)); acknowledgeMutation(2); assertContains(doc("foo/bar", 1, map("foo", "bar"))); assertContains(doc("foo/baz", 2, map("foo", "baz"))); - notifyLocalViewChanges(viewChanges(2, emptyList(), asList("foo/bar", "foo/baz"))); + notifyLocalViewChanges( + viewChanges(2, /* synced= */ false, emptyList(), asList("foo/bar", "foo/baz"))); releaseQuery(query); assertNotContains("foo/bar"); @@ -901,10 +930,7 @@ public void testReadsAllDocumentsForCollectionQueries() { @Test public void testPersistsResumeTokens() { - // This test only works in the absence of the EagerGarbageCollector. - if (garbageCollectorIsEager()) { - return; - } + assumeFalse(garbageCollectorIsEager()); Query query = query("foo/bar"); int targetId = allocateQuery(query); @@ -921,10 +947,7 @@ public void testPersistsResumeTokens() { @Test public void testDoesNotReplaceResumeTokenWithEmptyByteString() { - // This test only works in the absence of the EagerGarbageCollector. - if (garbageCollectorIsEager()) { - return; - } + assumeFalse(garbageCollectorIsEager()); Query query = query("foo/bar"); int targetId = allocateQuery(query); @@ -979,12 +1002,10 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() { @Test public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() { - if (garbageCollectorIsEager()) { - // Since this test doesn't start a listen, Eager GC removes the documents from the cache as - // soon as the mutation is applied. This creates a lot of special casing in this unit test but - // does not expand its test coverage. - return; - } + // Since this test doesn't start a listen, Eager GC removes the documents from the cache as + // soon as the mutation is applied. This creates a lot of special casing in this unit test but + // does not expand its test coverage. + assumeFalse(garbageCollectorIsEager()); writeMutation(setMutation("foo/bar", map("sum", 0))); assertContains(doc("foo/bar", 0, map("sum", 0), Document.DocumentState.LOCAL_MUTATIONS)); @@ -1007,6 +1028,208 @@ public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransfo assertChanged(doc("foo/bar", 2, map("sum", 3), Document.DocumentState.LOCAL_MUTATIONS)); } + @Test + public void testUsesTargetMappingToExecuteQueries() { + assumeFalse(garbageCollectorIsEager()); + assumeTrue(queryEngine instanceof IndexFreeQueryEngine); + + // This test verifies that once a target mapping has been written, only documents that match + // the query are read from the RemoteDocumentCache. + + Query query = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + int targetId = allocateQuery(query); + + writeMutation(setMutation("foo/a", map("matches", true))); + writeMutation(setMutation("foo/b", map("matches", true))); + writeMutation(setMutation("foo/ignored", map("matches", false))); + acknowledgeMutation(10); + acknowledgeMutation(10); + acknowledgeMutation(10); + + // Execute the query, but note that we read all existing documents from the RemoteDocumentCache + // since we do not yet have target mapping. + executeQuery(query); + assertRemoteDocumentsRead(3); + + // Issue a RemoteEvent to persist the target mapping. + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))), + asList(targetId), + emptyList())); + applyRemoteEvent(noChangeEvent(targetId, 10)); + udpateViews(targetId, /* synced= */ true); + + // Execute the query again, this time verifying that we only read the two documents that match + // the query. + executeQuery(query); + assertRemoteDocumentsRead(2); + assertQueryReturned("foo/a", "foo/b"); + } + + @Test + public void testLastLimboFreeSnapshotIsAdvancedDuringViewProcessing() { + assumeFalse(garbageCollectorIsEager()); + assumeTrue(queryEngine instanceof IndexFreeQueryEngine); + + // This test verifies that the `lastLimboFreeSnapshot` version for QueryData is advanced when + // we compute a limbo-free free view and that the mapping is persisted when we release a query. + + writeMutation(setMutation("foo/ignored", map("matches", false))); + acknowledgeMutation(10); + + Query query = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + int targetId = allocateQuery(query); + + // Mark the query as current. + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))), + asList(targetId), + emptyList())); + applyRemoteEvent(noChangeEvent(targetId, 10)); + + // At this point, we have not yet confirmed that the query is limbo free. + Assert.assertEquals( + SnapshotVersion.NONE, localStore.getQueryData(query).getLastLimboFreeSnapshotVersion()); + + // Update the view, but don't mark the view synced. + Assert.assertEquals( + SnapshotVersion.NONE, localStore.getQueryData(query).getLastLimboFreeSnapshotVersion()); + udpateViews(targetId, /* synced=*/ true); + + // The query is marked limbo-free only when we mark the view synced. + udpateViews(targetId, /* synced=*/ true); + Assert.assertNotEquals( + SnapshotVersion.NONE, localStore.getQueryData(query).getLastLimboFreeSnapshotVersion()); + + // The last limbo free snapshot version is persisted even if we release the query. + releaseQuery(query); + allocateQuery(query); + + // Verify that we only read the two documents that match the query. + executeQuery(query); + assertRemoteDocumentsRead(2); + assertQueryReturned("foo/a", "foo/b"); + } + + @Test + public void testQueriesIncludeLocallyModifiedDocuments() { + assumeFalse(garbageCollectorIsEager()); + + // This test verifies that queries that have a persisted TargetMapping include documents that + // were modified by local edits after the target mapping was written. + Query query = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + int targetId = allocateQuery(query); + + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))), + asList(targetId), + emptyList())); + applyRemoteEvent(noChangeEvent(targetId, 10)); + udpateViews(targetId, /* synced= */ true); + + // Execute the query based on the RemoteEvent. + executeQuery(query); + assertQueryReturned("foo/a", "foo/b"); + + // Write a document. + writeMutation(setMutation("foo/c", map("matches", true))); + + // Execute the query and make sure that the pending mutation is included in the result. + executeQuery(query); + assertQueryReturned("foo/a", "foo/b", "foo/c"); + + acknowledgeMutation(11); + + // Execute the query and make sure that the acknowledged mutation is included in the result. + executeQuery(query); + assertQueryReturned("foo/a", "foo/b", "foo/c"); + } + + @Test + public void testQueriesIncludeDocumentsFromOtherQueries() { + assumeFalse(garbageCollectorIsEager()); + + // This test verifies that queries that have a persisted TargetMapping include documents that + // were modified by other queries after the target mapping was written. + + Query filteredQuery = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + int targetId = allocateQuery(filteredQuery); + + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))), + asList(targetId), + emptyList())); + applyRemoteEvent(noChangeEvent(targetId, 10)); + udpateViews(targetId, /* synced=*/ true); + releaseQuery(filteredQuery); + + // Start another query and add more matching documents to the collection. + Query fullQuery = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + targetId = allocateQuery(filteredQuery); + applyRemoteEvent( + addedRemoteEvent( + asList( + doc("foo/a", 10, map("matches", true)), + doc("foo/b", 10, map("matches", true)), + doc("foo/c", 20, map("matches", true))), + asList(targetId), + emptyList())); + releaseQuery(fullQuery); + + // Run the original query again and ensure that both the original matches as well as all new + // matches are included in the result set. + allocateQuery(filteredQuery); + executeQuery(filteredQuery); + assertQueryReturned("foo/a", "foo/b", "foo/c"); + } + + @Test + public void testQueriesFilterDocumentsThatNoLongerMatch() { + assumeFalse(garbageCollectorIsEager()); + + // This test verifies that documents that once matched a query are post-filtered if they no + // longer match the query filter. + + // Add two document results for a simple filter query + Query filteredQuery = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + int targetId = allocateQuery(filteredQuery); + + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 10, map("matches", true))), + asList(targetId), + emptyList())); + applyRemoteEvent(noChangeEvent(targetId, 10)); + udpateViews(targetId, /* synced=*/ true); + releaseQuery(filteredQuery); + + // Modify one of the documents to no longer match while the filtered query is inactive. + Query fullQuery = + Query.atPath(ResourcePath.fromString("foo")).filter(filter("matches", "==", true)); + targetId = allocateQuery(filteredQuery); + applyRemoteEvent( + addedRemoteEvent( + asList(doc("foo/a", 10, map("matches", true)), doc("foo/b", 20, map("matches", false))), + asList(targetId), + emptyList())); + releaseQuery(fullQuery); + + // Re-run the filtered query and verify that the modified document is no longer returned. + allocateQuery(filteredQuery); + executeQuery(filteredQuery); + assertQueryReturned("foo/a"); + } + @Test public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransformMutation() { Query query = Query.atPath(ResourcePath.fromString("foo")); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLocalStoreTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLocalStoreTest.java index 5793626fe2f..4b9b05664b5 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLocalStoreTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/MemoryLocalStoreTest.java @@ -14,19 +14,38 @@ package com.google.firebase.firestore.local; +import java.util.Arrays; +import java.util.Collection; import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner; import org.robolectric.annotation.Config; -@RunWith(RobolectricTestRunner.class) +@RunWith(ParameterizedRobolectricTestRunner.class) @Config(manifest = Config.NONE) public class MemoryLocalStoreTest extends LocalStoreTestCase { + private QueryEngine queryEngine; + + @ParameterizedRobolectricTestRunner.Parameters(name = "QueryEngine = {0}") + public static Collection data() { + return Arrays.asList( + new Object[] {new SimpleQueryEngine()}, new Object[] {new IndexFreeQueryEngine()}); + } + + public MemoryLocalStoreTest(QueryEngine queryEngine) { + this.queryEngine = queryEngine; + } + @Override Persistence getPersistence() { return PersistenceTestHelpers.createEagerGCMemoryPersistence(statsCollector); } + @Override + QueryEngine getQueryEngine() { + return this.queryEngine; + } + @Override boolean garbageCollectorIsEager() { return true; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java index 5fc83ea808a..9f66aa72679 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteLocalStoreTest.java @@ -14,14 +14,33 @@ package com.google.firebase.firestore.local; +import java.util.Arrays; +import java.util.Collection; import org.junit.runner.RunWith; -import org.robolectric.RobolectricTestRunner; +import org.robolectric.ParameterizedRobolectricTestRunner; import org.robolectric.annotation.Config; -@RunWith(RobolectricTestRunner.class) +@RunWith(ParameterizedRobolectricTestRunner.class) @Config(manifest = Config.NONE) public class SQLiteLocalStoreTest extends LocalStoreTestCase { + private QueryEngine queryEngine; + + @ParameterizedRobolectricTestRunner.Parameters(name = "QueryEngine = {0}") + public static Collection data() { + return Arrays.asList( + new Object[] {new SimpleQueryEngine()}, new Object[] {new IndexFreeQueryEngine()}); + } + + public SQLiteLocalStoreTest(QueryEngine queryEngine) { + this.queryEngine = queryEngine; + } + + @Override + QueryEngine getQueryEngine() { + return this.queryEngine; + } + @Override Persistence getPersistence() { return PersistenceTestHelpers.createSQLitePersistence(statsCollector); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/QueryEvent.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/QueryEvent.java index 4f528afe78a..ddb63712e2a 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/QueryEvent.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/QueryEvent.java @@ -14,10 +14,10 @@ package com.google.firebase.firestore.spec; +import androidx.annotation.Nullable; import com.google.firebase.firestore.FirebaseFirestoreException; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.core.ViewSnapshot; -import javax.annotation.Nullable; /** Object that contains exactly one of either a view snapshot or an error for the given query. */ public class QueryEvent { diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java index e6c5f6f966a..b4816ba1adb 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java @@ -48,7 +48,9 @@ import com.google.firebase.firestore.local.LocalStore; import com.google.firebase.firestore.local.Persistence; import com.google.firebase.firestore.local.QueryData; +import com.google.firebase.firestore.local.QueryEngine; import com.google.firebase.firestore.local.QueryPurpose; +import com.google.firebase.firestore.local.SimpleQueryEngine; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; @@ -262,7 +264,9 @@ protected void specTearDown() throws Exception { */ private void initClient() { localPersistence = getPersistence(garbageCollectionEnabled); - LocalStore localStore = new LocalStore(localPersistence, currentUser); + // TODO(index-free): Update to index-free query engine when it becomes default. + QueryEngine queryEngine = new SimpleQueryEngine(); + LocalStore localStore = new LocalStore(localPersistence, queryEngine, currentUser); queue = new AsyncQueue(); diff --git a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java index 476e6d05f15..42a35a5c71e 100644 --- a/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java +++ b/firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java @@ -23,6 +23,7 @@ import androidx.annotation.NonNull; import com.fasterxml.jackson.core.type.TypeReference; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.android.gms.common.internal.Preconditions; import com.google.common.base.Charsets; import com.google.common.collect.Lists; import com.google.common.collect.Sets; @@ -395,8 +396,16 @@ public static RemoteEvent noChangeEvent(int targetId, int version, ByteString re public static RemoteEvent addedRemoteEvent( MaybeDocument doc, List updatedInTargets, List removedFromTargets) { - DocumentChange change = - new DocumentChange(updatedInTargets, removedFromTargets, doc.getKey(), doc); + return addedRemoteEvent(Collections.singletonList(doc), updatedInTargets, removedFromTargets); + } + + public static RemoteEvent addedRemoteEvent( + List docs, List updatedInTargets, List removedFromTargets) { + Preconditions.checkArgument(!docs.isEmpty(), "Cannot pass empty docs array"); + + ResourcePath collectionPath = docs.get(0).getKey().getPath().popLast(); + SnapshotVersion version = docs.get(0).getVersion(); + WatchChangeAggregator aggregator = new WatchChangeAggregator( new WatchChangeAggregator.TargetMetadataProvider() { @@ -407,11 +416,16 @@ public ImmutableSortedSet getRemoteKeysForTarget(int targetId) { @Override public QueryData getQueryDataForTarget(int targetId) { - return queryData(targetId, QueryPurpose.LISTEN, doc.getKey().toString()); + return queryData(targetId, QueryPurpose.LISTEN, collectionPath.toString()); } }); - aggregator.handleDocumentChange(change); - return aggregator.createRemoteEvent(doc.getVersion()); + + for (MaybeDocument doc : docs) { + DocumentChange change = + new DocumentChange(updatedInTargets, removedFromTargets, doc.getKey(), doc); + aggregator.handleDocumentChange(change); + } + return aggregator.createRemoteEvent(version); } public static RemoteEvent updateRemoteEvent( @@ -509,7 +523,7 @@ public static MutationResult mutationResult(long version) { } public static LocalViewChanges viewChanges( - int targetId, List addedKeys, List removedKeys) { + int targetId, boolean synced, List addedKeys, List removedKeys) { ImmutableSortedSet added = DocumentKey.emptyKeySet(); for (String keyPath : addedKeys) { added = added.insert(key(keyPath)); @@ -518,7 +532,7 @@ public static LocalViewChanges viewChanges( for (String keyPath : removedKeys) { removed = removed.insert(key(keyPath)); } - return new LocalViewChanges(targetId, false, added, removed); + return new LocalViewChanges(targetId, synced, added, removed); } /** Creates a resume token to match the given snapshot version. */ From 0df060a1ad9e18f94473c10718fcf52e85258f6d Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 21 Aug 2019 16:18:42 -0700 Subject: [PATCH 08/13] Typo --- .../src/proto/google/firebase/firestore/proto/target.proto | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto b/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto index e835cde4c1c..8bca7e4adfd 100644 --- a/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto +++ b/firebase-firestore/src/proto/google/firebase/firestore/proto/target.proto @@ -79,7 +79,7 @@ message Target { google.firestore.v1.Target.DocumentsTarget documents = 6; } - // Denotes the maximum snapshot version at which the associated querye view + // Denotes the maximum snapshot version at which the associated query view // contained no limbo documents. google.protobuf.Timestamp last_limbo_free_snapshot_version = 7; } From efc9fb7644d7fcba0d6466f818ad7de06766917f Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 21 Aug 2019 21:00:14 -0700 Subject: [PATCH 09/13] Index-free: Use different SQL for non-index free queries (#726) --- .../local/SQLiteRemoteDocumentCache.java | 93 +++++++++++-------- .../firestore/local/SQLiteSchemaTest.java | 13 ++- 2 files changed, 61 insertions(+), 45 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java index cf8ef713903..c1270ef0d91 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteRemoteDocumentCache.java @@ -155,48 +155,59 @@ public ImmutableSortedMap getAllDocumentsMatchingQuery( (ImmutableSortedMap[]) new ImmutableSortedMap[] {DocumentCollections.emptyDocumentMap()}; + SQLitePersistence.Query sqlQuery; + if (sinceReadTime.equals(SnapshotVersion.NONE)) { + sqlQuery = + db.query("SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ?") + .binding(prefixPath, prefixSuccessorPath); + } else { + // Execute an index-free query and filter by read time. This is safe since all document + // changes to queries that have a lastLimboFreeSnapshotVersion (`sinceReadTime`) have a read + // time set. + sqlQuery = + db.query( + "SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ?" + + "AND (read_time_seconds > ? OR (read_time_seconds = ? AND read_time_nanos > ?))") + .binding( + prefixPath, + prefixSuccessorPath, + readTime.getSeconds(), + readTime.getSeconds(), + readTime.getNanoseconds()); + } + int rowsProcessed = - db.query( - "SELECT path, contents FROM remote_documents WHERE path >= ? AND path < ? " - + "AND (read_time_seconds IS NULL OR read_time_seconds > ? " - + "OR (read_time_seconds = ? AND read_time_nanos > ?))") - .binding( - prefixPath, - prefixSuccessorPath, - readTime.getSeconds(), - readTime.getSeconds(), - readTime.getNanoseconds()) - .forEach( - row -> { - // TODO: Actually implement a single-collection query - // - // The query is actually returning any path that starts with the query path prefix - // which may include documents in subcollections. For example, a query on 'rooms' - // will return rooms/abc/messages/xyx but we shouldn't match it. Fix this by - // discarding rows with document keys more than one segment longer than the query - // path. - ResourcePath path = EncodedPath.decodeResourcePath(row.getString(0)); - if (path.length() != immediateChildrenPathLength) { - return; - } - - byte[] rawDocument = row.getBlob(1); - - // Since scheduling background tasks incurs overhead, we only dispatch to a - // background thread if there are still some documents remaining. - Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; - executor.execute( - () -> { - MaybeDocument maybeDoc = decodeMaybeDocument(rawDocument); - - if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) { - synchronized (SQLiteRemoteDocumentCache.this) { - matchingDocuments[0] = - matchingDocuments[0].insert(maybeDoc.getKey(), (Document) maybeDoc); - } - } - }); - }); + sqlQuery.forEach( + row -> { + // TODO: Actually implement a single-collection query + // + // The query is actually returning any path that starts with the query path prefix + // which may include documents in subcollections. For example, a query on 'rooms' + // will return rooms/abc/messages/xyx but we shouldn't match it. Fix this by + // discarding rows with document keys more than one segment longer than the query + // path. + ResourcePath path = EncodedPath.decodeResourcePath(row.getString(0)); + if (path.length() != immediateChildrenPathLength) { + return; + } + + byte[] rawDocument = row.getBlob(1); + + // Since scheduling background tasks incurs overhead, we only dispatch to a + // background thread if there are still some documents remaining. + Executor executor = row.isLast() ? Executors.DIRECT_EXECUTOR : backgroundQueue; + executor.execute( + () -> { + MaybeDocument maybeDoc = decodeMaybeDocument(rawDocument); + + if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) { + synchronized (SQLiteRemoteDocumentCache.this) { + matchingDocuments[0] = + matchingDocuments[0].insert(maybeDoc.getKey(), (Document) maybeDoc); + } + } + }); + }); statsCollector.recordRowsRead(STATS_TAG, rowsProcessed); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index e3fbbeb9c43..8b16cbe9440 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -423,12 +423,17 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { new Object[] {encode(path("coll/new")), 0, 3000, createDummyDocument("coll/new")}); SQLiteRemoteDocumentCache remoteDocumentCache = createRemoteDocumentCache(); + + // Verify that queries with SnapshotVersion.NONE return all results, regardless of whether the + // read time has been set. ImmutableSortedMap results = - remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(2)); + remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(0)); + assertResultsContain(results, "coll/existing", "coll/old", "coll/current", "coll/new"); - // Verify that queries return the documents that existed prior to the index-free migration, as - // well as any documents with a newer read time than the one passed in. - assertResultsContain(results, "coll/existing", "coll/new"); + // Queries that filter by read time only return documents that were written after the index-free + // migration. + results = remoteDocumentCache.getAllDocumentsMatchingQuery(query("coll"), version(2)); + assertResultsContain(results, "coll/new"); } private SQLiteRemoteDocumentCache createRemoteDocumentCache() { From 209285735903837d12d0af476132f6415edf9e95 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Thu, 22 Aug 2019 16:38:04 -0700 Subject: [PATCH 10/13] Update comment --- .../java/com/google/firebase/firestore/local/QueryEngine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java index d071316718b..a4477dc59bb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java @@ -24,7 +24,7 @@ /** * Represents a query engine capable of performing queries over the local document cache. You must - * call setQueryDataProvider() and setLocalDocumentsView() before using. + * call setLocalDocumentsView() before using. */ public interface QueryEngine { From e11ca4874436fefa56543d7daf781076f182e779 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Fri, 23 Aug 2019 14:22:59 -0700 Subject: [PATCH 11/13] Index-free: Drop last limbo free snapshot if re-upgraded (#734) --- .../firestore/local/SQLiteSchema.java | 53 ++++++++++++-- .../firestore/local/SQLiteSchemaTest.java | 73 +++++++++++++++++++ 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 97c09efc415..838aba94159 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.local; +import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; import android.content.ContentValues; @@ -26,7 +27,9 @@ import androidx.annotation.VisibleForTesting; import com.google.common.base.Preconditions; import com.google.firebase.firestore.model.ResourcePath; +import com.google.firebase.firestore.proto.Target; import com.google.firebase.firestore.util.Consumer; +import com.google.protobuf.InvalidProtocolBufferException; import java.util.ArrayList; import java.util.List; @@ -136,7 +139,16 @@ void runMigrations(int fromVersion, int toVersion) { } if (fromVersion < 9 && toVersion >= 9) { - addReadTime(); + if (!hasReadTime()) { + addReadTime(); + } else { + // Index-free queries rely on the fact that documents updated after a query's last limbo + // free snapshot version are persisted with their read-time. If a customer upgrades to + // schema version 9, downgrades and then upgrades again, some queries may have a last limbo + // free snapshot version despite the fact that not all updated document have an associated + // read time. + dropLastLimboFreeSnapshotVersion(); + } } /* @@ -363,14 +375,39 @@ private void addSequenceNumber() { } } + private boolean hasReadTime() { + boolean hasReadTimeSeconds = tableContainsColumn("remote_documents", "read_time_seconds"); + boolean hasReadTimeNanos = tableContainsColumn("remote_documents", "read_time_nanos"); + + hardAssert( + hasReadTimeSeconds == hasReadTimeNanos, + "Table contained just one of read_time_seconds or read_time_nanos"); + + return hasReadTimeSeconds && hasReadTimeNanos; + } + private void addReadTime() { - if (!tableContainsColumn("remote_documents", "read_time_seconds")) { - hardAssert( - !tableContainsColumn("remote_documents", "read_time_nanos"), - "Table contained read_time_nanos, but is missing read_time_seconds"); - db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER"); - db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER"); - } + db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER"); + db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER"); + } + + private void dropLastLimboFreeSnapshotVersion() { + new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets") + .forEach( + cursor -> { + int targetId = cursor.getInt(0); + byte[] targetProtoBytes = cursor.getBlob(1); + + try { + Target targetProto = Target.parseFrom(targetProtoBytes); + targetProto = targetProto.toBuilder().clearLastLimboFreeSnapshotVersion().build(); + db.execSQL( + "UPDATE targets SET target_proto = ? WHERE target_id = ?", + new Object[] {targetProto.toByteArray(), targetId}); + } catch (InvalidProtocolBufferException e) { + throw fail("Failed to decode Query data for target %s", targetId); + } + }); } /** diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java index 8b16cbe9440..495bb6f71f8 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteSchemaTest.java @@ -21,6 +21,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.path; import static com.google.firebase.firestore.testutil.TestUtil.query; import static com.google.firebase.firestore.testutil.TestUtil.version; +import static com.google.firebase.firestore.util.Assert.fail; import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -36,10 +37,13 @@ import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; import com.google.firebase.firestore.proto.MaybeDocument; +import com.google.firebase.firestore.proto.Target; import com.google.firebase.firestore.proto.WriteBatch; import com.google.firebase.firestore.remote.RemoteSerializer; import com.google.firestore.v1.Document; import com.google.firestore.v1.Write; +import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.Timestamp; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; @@ -436,6 +440,68 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() { assertResultsContain(results, "coll/new"); } + @Test + public void dropsLastLimboFreeSnapshotIfPreviouslyDowngraded() { + schema.runMigrations(0, 9); + + db.execSQL( + "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)", + new Object[] {1, "foo", createDummyQueryTargetWithLimboFreeVersion(1).toByteArray()}); + db.execSQL( + "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?, ?, ?)", + new Object[] {2, "bar", createDummyQueryTargetWithLimboFreeVersion(2).toByteArray()}); + db.execSQL( + "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)", + new Object[] {3, "baz", createDummyQueryTargetWithLimboFreeVersion(3).toByteArray()}); + + schema.runMigrations(0, 8); + schema.runMigrations(8, 9); + + int rowCount = + new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets") + .forEach( + cursor -> { + int targetId = cursor.getInt(0); + byte[] targetProtoBytes = cursor.getBlob(1); + + try { + Target targetProto = Target.parseFrom(targetProtoBytes); + assertEquals(targetId, targetProto.getTargetId()); + assertFalse(targetProto.hasLastLimboFreeSnapshotVersion()); + } catch (InvalidProtocolBufferException e) { + fail("Failed to decode Query data"); + } + }); + + assertEquals(3, rowCount); + } + + @Test + public void keepsLastLimboFreeSnapshotIfNotDowngraded() { + schema.runMigrations(0, 9); + + db.execSQL( + "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)", + new Object[] {1, "foo", createDummyQueryTargetWithLimboFreeVersion(1).toByteArray()}); + + // Make sure that we don't drop the lastLimboFreeSnapshotVersion if we are already on schema + // version 9. + schema.runMigrations(9, 9); + + new SQLitePersistence.Query(db, "SELECT target_proto FROM targets") + .forEach( + cursor -> { + byte[] targetProtoBytes = cursor.getBlob(0); + + try { + Target targetProto = Target.parseFrom(targetProtoBytes); + assertTrue(targetProto.hasLastLimboFreeSnapshotVersion()); + } catch (InvalidProtocolBufferException e) { + fail("Failed to decode Query data"); + } + }); + } + private SQLiteRemoteDocumentCache createRemoteDocumentCache() { DatabaseId databaseId = DatabaseId.forProject("foo"); LocalSerializer serializer = new LocalSerializer(new RemoteSerializer(databaseId)); @@ -460,6 +526,13 @@ private byte[] createDummyDocument(String name) { .toByteArray(); } + private Target createDummyQueryTargetWithLimboFreeVersion(int targetId) { + return Target.newBuilder() + .setTargetId(targetId) + .setLastLimboFreeSnapshotVersion(Timestamp.newBuilder().setSeconds(42)) + .build(); + } + private void assertResultsContain( ImmutableSortedMap actualResults, String... docs) { From eb702be19c10e73aa75529eb8eb44896e4917b63 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 27 Aug 2019 10:51:32 -0700 Subject: [PATCH 12/13] Index-Free: Only exclude limit queries if last document changed (#737) --- .../collection/ImmutableSortedMap.java | 8 -- .../firestore/local/IndexFreeQueryEngine.java | 104 ++++++++++-------- .../local/IndexFreeQueryEngineTest.java | 50 ++++++--- 3 files changed, 97 insertions(+), 65 deletions(-) diff --git a/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java b/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java index d7a648b9684..f62c973aa1d 100644 --- a/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java +++ b/firebase-database-collection/src/main/java/com/google/firebase/database/collection/ImmutableSortedMap.java @@ -56,14 +56,6 @@ public abstract class ImmutableSortedMap implements Iterable getComparator(); - public ImmutableSortedMap insertAll(ImmutableSortedMap source) { - ImmutableSortedMap result = this; - for (Map.Entry entry : source) { - result = result.insert(entry.getKey(), entry.getValue()); - } - return result; - } - @Override @SuppressWarnings("unchecked") public boolean equals(Object o) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java index a6b10572f35..5c2517e37c8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/IndexFreeQueryEngine.java @@ -21,10 +21,10 @@ import com.google.firebase.database.collection.ImmutableSortedSet; import com.google.firebase.firestore.core.Query; import com.google.firebase.firestore.model.Document; -import com.google.firebase.firestore.model.DocumentCollections; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.MaybeDocument; import com.google.firebase.firestore.model.SnapshotVersion; +import java.util.Collections; import java.util.Map; /** @@ -76,54 +76,83 @@ public ImmutableSortedMap getDocumentsMatchingQuery( return executeFullCollectionScan(query); } - ImmutableSortedMap result = - executeIndexFreeQuery(query, queryData, remoteKeys); + ImmutableSortedSet previousResults = getSortedPreviousResults(query, remoteKeys); - return result != null ? result : executeFullCollectionScan(query); + if (query.hasLimit() + && needsRefill(previousResults, remoteKeys, queryData.getLastLimboFreeSnapshotVersion())) { + return executeFullCollectionScan(query); + } + + // Retrieve all results for documents that were updated since the last limbo-document free + // remote snapshot. + ImmutableSortedMap updatedResults = + localDocumentsView.getDocumentsMatchingQuery( + query, queryData.getLastLimboFreeSnapshotVersion()); + for (Document result : previousResults) { + updatedResults = updatedResults.insert(result.getKey(), result); + } + + return updatedResults; } /** - * Attempts index-free query execution. Returns the set of query results on success, otherwise - * returns null. + * Returns the documents for the specified remote keys if they still match the query, sorted by + * the query's comparator. */ - private @Nullable ImmutableSortedMap executeIndexFreeQuery( - Query query, QueryData queryData, ImmutableSortedSet remoteKeys) { + private ImmutableSortedSet getSortedPreviousResults( + Query query, ImmutableSortedSet remoteKeys) { // Fetch the documents that matched the query at the last snapshot. ImmutableSortedMap previousResults = localDocumentsView.getDocuments(remoteKeys); - // Limit queries are not eligible for index-free query execution if any part of the result was - // modified after we received the last query snapshot. This makes sure that we re-populate the - // view with older documents that may sort before the modified document. - if (query.hasLimit() - && containsUpdatesSinceSnapshotVersion(previousResults, queryData.getSnapshotVersion())) { - return null; - } - - ImmutableSortedMap results = DocumentCollections.emptyDocumentMap(); - - // Re-apply the query filter since previously matching documents do not necessarily still - // match the query. + // Sort the documents and re-apply the query filter since previously matching documents do not + // necessarily still match the query. + ImmutableSortedSet results = + new ImmutableSortedSet<>(Collections.emptyList(), query.comparator()); for (Map.Entry entry : previousResults) { MaybeDocument maybeDoc = entry.getValue(); if (maybeDoc instanceof Document && query.matches((Document) maybeDoc)) { Document doc = (Document) maybeDoc; - results = results.insert(entry.getKey(), doc); - } else if (query.hasLimit()) { - // Limit queries with documents that no longer match need to be re-filled from cache. - return null; + results = results.insert(doc); } } + return results; + } - // Retrieve all results for documents that were updated since the last limbo-document free - // remote snapshot. - ImmutableSortedMap updatedResults = - localDocumentsView.getDocumentsMatchingQuery( - query, queryData.getLastLimboFreeSnapshotVersion()); + /** + * Determines if a limit query needs to be refilled from cache, making it ineligible for + * index-free execution. + * + * @param sortedPreviousResults The documents that matched the query when it was last + * synchronized, sorted by the query's comparator. + * @param remoteKeys The document keys that matched the query at the last snapshot. + * @param limboFreeSnapshotVersion The version of the snapshot when the query was last + * synchronized. + */ + private boolean needsRefill( + ImmutableSortedSet sortedPreviousResults, + ImmutableSortedSet remoteKeys, + SnapshotVersion limboFreeSnapshotVersion) { + // The query needs to be refilled if a previously matching document no longer matches. + if (remoteKeys.size() != sortedPreviousResults.size()) { + return true; + } - results = results.insertAll(updatedResults); + // We don't need to find a better match from cache if no documents matched the query. + if (sortedPreviousResults.isEmpty()) { + return false; + } - return results; + // Limit queries are not eligible for index-free query execution if there is a potential that an + // older document from cache now sorts before a document that was previously part of the limit. + // This, however, can only happen if the last document of the limit sorts lower than it did when + // the query was last synchronized. If a document that is not the limit boundary sorts + // differently, the boundary of the limit itself did not change and documents from cache will + // continue to be "rejected" by this boundary. Therefore, we can ignore any modifications that + // don't affect the last document. + Document lastDocumentInLimit = sortedPreviousResults.getMaxEntry(); + return lastDocumentInLimit.hasPendingWrites() + || lastDocumentInLimit.getVersion().compareTo(limboFreeSnapshotVersion) > 0; } @Override @@ -131,19 +160,6 @@ public void handleDocumentChange(MaybeDocument oldDocument, MaybeDocument newDoc // No indexes to update. } - private boolean containsUpdatesSinceSnapshotVersion( - ImmutableSortedMap previousResults, - SnapshotVersion sinceSnapshotVersion) { - for (Map.Entry doc : previousResults) { - if (doc.getValue().hasPendingWrites() - || doc.getValue().getVersion().compareTo(sinceSnapshotVersion) > 0) { - return true; - } - } - - return false; - } - private ImmutableSortedMap executeFullCollectionScan(Query query) { return localDocumentsView.getDocumentsMatchingQuery(query, SnapshotVersion.NONE); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java index 29e73f89a26..f21c3a6e821 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/IndexFreeQueryEngineTest.java @@ -17,6 +17,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.doc; import static com.google.firebase.firestore.testutil.TestUtil.docSet; import static com.google.firebase.firestore.testutil.TestUtil.filter; +import static com.google.firebase.firestore.testutil.TestUtil.key; import static com.google.firebase.firestore.testutil.TestUtil.map; import static com.google.firebase.firestore.testutil.TestUtil.orderBy; import static com.google.firebase.firestore.testutil.TestUtil.query; @@ -61,8 +62,6 @@ public class IndexFreeQueryEngineTest { doc("coll/a", 11, map("matches", true, "order", 1), Document.DocumentState.SYNCED); private static final Document MATCHING_DOC_B = doc("coll/b", 1, map("matches", true, "order", 2), Document.DocumentState.SYNCED); - private static final Document NON_MATCHING_DOC_B = - doc("coll/b", 1, map("matches", false, "order", 2), Document.DocumentState.SYNCED); private static final Document UPDATED_MATCHING_DOC_B = doc("coll/b", 11, map("matches", true, "order", 2), Document.DocumentState.SYNCED); @@ -102,13 +101,13 @@ public ImmutableSortedMap getDocumentsMatchingQuery( } /** Adds the provided documents to the query target mapping. */ - private void persistQueryMapping(Document... docs) { + private void persistQueryMapping(DocumentKey... documentKeys) { persistence.runTransaction( "persistQueryMapping", () -> { ImmutableSortedSet remoteKeys = DocumentKey.emptyKeySet(); - for (Document doc : docs) { - remoteKeys = remoteKeys.insert(doc.getKey()); + for (DocumentKey documentKey : documentKeys) { + remoteKeys = remoteKeys.insert(documentKey); } queryCache.addMatchingKeys(remoteKeys, TEST_TARGET_ID); }); @@ -162,7 +161,7 @@ public void usesTargetMappingForInitialView() throws Exception { QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); addDocument(MATCHING_DOC_A, MATCHING_DOC_B); - persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey()); DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, queryData)); assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs); @@ -174,7 +173,7 @@ public void filtersNonMatchingInitialResults() throws Exception { QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); addDocument(MATCHING_DOC_A, MATCHING_DOC_B); - persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey()); // Add a mutated document that is not yet part of query's set of remote keys. addDocument(PENDING_NON_MATCHING_DOC_A); @@ -189,7 +188,7 @@ public void includesChangesSinceInitialResults() throws Exception { QueryData originalQueryData = queryData(query, /* hasLimboFreeSnapshot= */ true); addDocument(MATCHING_DOC_A, MATCHING_DOC_B); - persistQueryMapping(MATCHING_DOC_A, MATCHING_DOC_B); + persistQueryMapping(MATCHING_DOC_A.getKey(), MATCHING_DOC_B.getKey()); DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, originalQueryData)); assertEquals(docSet(query.comparator(), MATCHING_DOC_A, MATCHING_DOC_B), docs); @@ -225,7 +224,7 @@ public void doesNotUseInitialResultsForLimitQueryWithDocumentRemoval() throws Ex // While the backend would never add DocA to the set of remote keys, this allows us to easily // simulate what would happen when a document no longer matches due to an out-of-band update. addDocument(NON_MATCHING_DOC_A); - persistQueryMapping(NON_MATCHING_DOC_A); + persistQueryMapping(NON_MATCHING_DOC_A.getKey()); addDocument(MATCHING_DOC_B); @@ -235,7 +234,8 @@ public void doesNotUseInitialResultsForLimitQueryWithDocumentRemoval() throws Ex } @Test - public void doesNotUseInitialResultsForLimitQueryWithPendingWrite() throws Exception { + public void doesNotUseInitialResultsForLimitQueryWhenLastDocumentHasPendingWrite() + throws Exception { Query query = query("coll") .filter(filter("matches", "==", true)) @@ -245,7 +245,7 @@ public void doesNotUseInitialResultsForLimitQueryWithPendingWrite() throws Excep // Add a query mapping for a document that matches, but that sorts below another document due to // a pending write. addDocument(PENDING_MATCHING_DOC_A); - persistQueryMapping(PENDING_MATCHING_DOC_A); + persistQueryMapping(PENDING_MATCHING_DOC_A.getKey()); QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); @@ -256,7 +256,7 @@ public void doesNotUseInitialResultsForLimitQueryWithPendingWrite() throws Excep } @Test - public void doesNotUseInitialResultsForLimitQueryWithDocumentThatHasBeenUpdatedOutOfBand() + public void doesNotUseInitialResultsForLimitQueryWhenLastDocumentHasBeenUpdatedOutOfBand() throws Exception { Query query = query("coll") @@ -267,7 +267,7 @@ public void doesNotUseInitialResultsForLimitQueryWithDocumentThatHasBeenUpdatedO // Add a query mapping for a document that matches, but that sorts below another document based // due to an update that the SDK received after the query's snapshot was persisted. addDocument(UDPATED_DOC_A); - persistQueryMapping(UDPATED_DOC_A); + persistQueryMapping(UDPATED_DOC_A.getKey()); QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); @@ -277,6 +277,30 @@ public void doesNotUseInitialResultsForLimitQueryWithDocumentThatHasBeenUpdatedO assertEquals(docSet(query.comparator(), MATCHING_DOC_B), docs); } + @Test + public void limitQueriesUseInitialResultsIfLastDocumentInLimitIsUnchanged() throws Exception { + Query query = query("coll").orderBy(orderBy("order")).limit(2); + + addDocument(doc("coll/a", 1, map("order", 1))); + addDocument(doc("coll/b", 1, map("order", 3))); + persistQueryMapping(key("coll/a"), key("coll/b")); + QueryData queryData = queryData(query, /* hasLimboFreeSnapshot= */ true); + + // Update "coll/a" but make sure it still sorts before "coll/b" + addDocument(doc("coll/a", 1, map("order", 2), Document.DocumentState.LOCAL_MUTATIONS)); + + // Since the last document in the limit didn't change (and hence we know that all documents + // written prior to query execution still sort after "coll/b"), we should use an Index-Free + // query. + DocumentSet docs = expectIndexFreeQuery(() -> runQuery(query, queryData)); + assertEquals( + docSet( + query.comparator(), + doc("coll/a", 1, map("order", 2), Document.DocumentState.LOCAL_MUTATIONS), + doc("coll/b", 1, map("order", 3))), + docs); + } + private QueryData queryData(Query query, boolean hasLimboFreeSnapshot) { return new QueryData( query, From e4aebae9a2c7851dad8b13e3efcf51845078cbf3 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Tue, 27 Aug 2019 12:58:59 -0700 Subject: [PATCH 13/13] Index-free: Remove stale TODO (#740) --- .../com/google/firebase/firestore/local/SQLiteSchema.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java index 838aba94159..132737445be 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteSchema.java @@ -48,13 +48,6 @@ class SQLiteSchema { /** * The version of the schema. Increase this by one for each migration added to runMigrations * below. - * - *

TODO(index-free): The migration to schema version 9 doesn't backfill `read_time` as this - * requires rewriting the RemoteDocumentCache. For index-free queries to efficiently handle - * existing documents, we still need to populate read_time for all existing entries, drop the - * RemoteDocumentCache or ask users to invoke `clearPersistence()` manually. If we decide to - * backfill or drop the contents of the RemoteDocumentCache, we need to perform an additional - * schema migration. */ static final int VERSION = 9;