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 {