diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index 25b7429a13f..d790b162a59 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { isCollectionGroupQuery, Query, queryMatches } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, @@ -245,30 +244,26 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { }); } - getDocumentsMatchingQuery( + getAll( transaction: PersistenceTransaction, - query: Query, + collection: ResourcePath, sinceReadTime: SnapshotVersion ): PersistencePromise { - debugAssert( - !isCollectionGroupQuery(query), - 'CollectionGroup queries should be handled in LocalDocumentsView' - ); let results = mutableDocumentMap(); - const immediateChildrenPathLength = query.path.length + 1; + const immediateChildrenPathLength = collection.length + 1; const iterationOptions: IterateOptions = {}; if (sinceReadTime.isEqual(SnapshotVersion.min())) { // Documents are ordered by key, so we can use a prefix scan to narrow // down the documents we need to match the query against. - const startKey = query.path.toArray(); + const startKey = collection.toArray(); iterationOptions.range = IDBKeyRange.lowerBound(startKey); } 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. - const collectionKey = query.path.toArray(); + const collectionKey = collection.toArray(); const readTimeKey = toDbTimestampKey(sinceReadTime); iterationOptions.range = IDBKeyRange.lowerBound( [collectionKey, readTimeKey], @@ -292,10 +287,10 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { DocumentKey.fromSegments(key), dbRemoteDoc ); - if (!query.path.isPrefixOf(document.key.path)) { - control.done(); - } else if (queryMatches(query, document)) { + if (collection.isPrefixOf(document.key.path)) { results = results.insert(document.key, document); + } else { + control.done(); } }) .next(() => results); diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 0ec26989c42..cf60ca6dbf1 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -25,14 +25,13 @@ import { import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, - documentKeySet, DocumentMap, documentMap, MutableDocumentMap } from '../model/collections'; import { Document, MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; -import { mutationApplyToLocalView, PatchMutation } from '../model/mutation'; +import { mutationApplyToLocalView } from '../model/mutation'; import { MutationBatch } from '../model/mutation_batch'; import { ResourcePath } from '../model/path'; import { debugAssert } from '../util/assert'; @@ -217,9 +216,8 @@ export class LocalDocumentsView { ): PersistencePromise { // Query the remote documents and overlay mutations. let results: MutableDocumentMap; - let mutationBatches: MutationBatch[]; return this.remoteDocumentCache - .getDocumentsMatchingQuery(transaction, query, sinceReadTime) + .getAll(transaction, query.path, sinceReadTime) .next(queryResults => { results = queryResults; return this.mutationQueue.getAllMutationBatchesAffectingQuery( @@ -227,41 +225,22 @@ export class LocalDocumentsView { query ); }) - .next(matchingMutationBatches => { - mutationBatches = matchingMutationBatches; - // It is possible that a PatchMutation can make a document match a query, even if - // the version in the RemoteDocumentCache is not a match yet (waiting for server - // to ack). To handle this, we find all document keys affected by the PatchMutations - // that are not in `result` yet, and back fill them via `remoteDocumentCache.getEntries`, - // otherwise those `PatchMutations` will be ignored because no base document can be found, - // and lead to missing result for the query. - return this.addMissingBaseDocuments( - transaction, - mutationBatches, - results - ).next(mergedDocuments => { - results = mergedDocuments; - - for (const batch of mutationBatches) { - for (const mutation of batch.mutations) { - const key = mutation.key; - let document = results.get(key); - if (document == null) { - // Create invalid document to apply mutations on top of - document = MutableDocument.newInvalidDocument(key); - results = results.insert(key, document); - } - mutationApplyToLocalView( - mutation, - document, - batch.localWriteTime - ); - if (!document.isFoundDocument()) { - results = results.remove(key); - } + .next(mutationBatches => { + for (const batch of mutationBatches) { + for (const mutation of batch.mutations) { + const key = mutation.key; + let document = results.get(key); + if (document == null) { + // Create invalid document to apply mutations on top of + document = MutableDocument.newInvalidDocument(key); + results = results.insert(key, document); + } + mutationApplyToLocalView(mutation, document, batch.localWriteTime); + if (!document.isFoundDocument()) { + results = results.remove(key); } } - }); + } }) .next(() => { // Finally, filter out any documents that don't actually match @@ -275,35 +254,4 @@ export class LocalDocumentsView { return results as DocumentMap; }); } - - private addMissingBaseDocuments( - transaction: PersistenceTransaction, - matchingMutationBatches: MutationBatch[], - existingDocuments: MutableDocumentMap - ): PersistencePromise { - let missingBaseDocEntriesForPatching = documentKeySet(); - for (const batch of matchingMutationBatches) { - for (const mutation of batch.mutations) { - if ( - mutation instanceof PatchMutation && - existingDocuments.get(mutation.key) === null - ) { - missingBaseDocEntriesForPatching = - missingBaseDocEntriesForPatching.add(mutation.key); - } - } - } - - let mergedDocuments = existingDocuments; - return this.remoteDocumentCache - .getEntries(transaction, missingBaseDocEntriesForPatching) - .next(missingBaseDocs => { - missingBaseDocs.forEach((key, doc) => { - if (doc.isFoundDocument()) { - mergedDocuments = mergedDocuments.insert(key, doc); - } - }); - return mergedDocuments; - }); - } } diff --git a/packages/firestore/src/local/memory_remote_document_cache.ts b/packages/firestore/src/local/memory_remote_document_cache.ts index d305c91fd51..b8be4f866f6 100644 --- a/packages/firestore/src/local/memory_remote_document_cache.ts +++ b/packages/firestore/src/local/memory_remote_document_cache.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { isCollectionGroupQuery, Query, queryMatches } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, @@ -24,6 +23,7 @@ import { } from '../model/collections'; import { Document, MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; +import { ResourcePath } from '../model/path'; import { debugAssert } from '../util/assert'; import { SortedMap } from '../util/sorted_map'; @@ -154,33 +154,30 @@ class MemoryRemoteDocumentCacheImpl implements MemoryRemoteDocumentCache { return PersistencePromise.resolve(results); } - getDocumentsMatchingQuery( + getAll( transaction: PersistenceTransaction, - query: Query, + collectionPath: ResourcePath, sinceReadTime: SnapshotVersion ): PersistencePromise { - debugAssert( - !isCollectionGroupQuery(query), - 'CollectionGroup queries should be handled in LocalDocumentsView' - ); let results = mutableDocumentMap(); // Documents are ordered by key, so we can use a prefix scan to narrow down // the documents we need to match the query against. - const prefix = new DocumentKey(query.path.child('')); + const prefix = new DocumentKey(collectionPath.child('')); const iterator = this.docs.getIteratorFrom(prefix); while (iterator.hasNext()) { const { key, value: { document } } = iterator.getNext(); - if (!query.path.isPrefixOf(key.path)) { + if (!collectionPath.isPrefixOf(key.path)) { break; } - if (document.readTime.compareTo(sinceReadTime) <= 0) { + if (key.path.length > collectionPath.length + 1) { + // Exclude entries from subcollections. continue; } - if (!queryMatches(query, document)) { + if (document.readTime.compareTo(sinceReadTime) <= 0) { continue; } results = results.insert(document.key, document.mutableCopy()); diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index 961d86a0067..5a8933348af 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -15,11 +15,11 @@ * limitations under the License. */ -import { Query } from '../core/query'; import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, MutableDocumentMap } from '../model/collections'; import { MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; +import { ResourcePath } from '../model/path'; import { IndexManager } from './index_manager'; import { PersistencePromise } from './persistence_promise'; @@ -62,21 +62,16 @@ export interface RemoteDocumentCache { ): PersistencePromise; /** - * Executes a query against the cached Document entries. + * Returns the documents from the provided collection. * - * Implementations may return extra documents if convenient. The results - * should be re-filtered by the consumer before presenting them to the user. - * - * Cached NoDocument entries have no bearing on query results. - * - * @param query - The query to match documents against. + * @param collection - The collection to read. * @param sinceReadTime - If not set to SnapshotVersion.min(), return only * documents that have been read since this snapshot version (exclusive). * @returns The set of matching documents. */ - getDocumentsMatchingQuery( + getAll( transaction: PersistenceTransaction, - query: Query, + collection: ResourcePath, sinceReadTime: SnapshotVersion ): PersistencePromise; diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index c3614aa91cc..08e7b662da5 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -36,7 +36,7 @@ export class CountingQueryEngine extends QueryEngine { * `getAllMutationBatchesAffectingQuery()` API (since the last call to * `resetCounts()`) */ - mutationsReadByQuery = 0; + mutationsReadByCollection = 0; /** * The number of mutations returned by the MutationQueue's @@ -48,9 +48,9 @@ export class CountingQueryEngine extends QueryEngine { /** * The number of documents returned by the RemoteDocumentCache's - * `getDocumentsMatchingQuery()` API (since the last call to `resetCounts()`) + * `getAll()` API (since the last call to `resetCounts()`) */ - documentsReadByQuery = 0; + documentsReadByCollection = 0; /** * The number of documents returned by the RemoteDocumentCache's `getEntry()` @@ -59,9 +59,9 @@ export class CountingQueryEngine extends QueryEngine { documentsReadByKey = 0; resetCounts(): void { - this.mutationsReadByQuery = 0; + this.mutationsReadByCollection = 0; this.mutationsReadByKey = 0; - this.documentsReadByQuery = 0; + this.documentsReadByCollection = 0; this.documentsReadByKey = 0; } @@ -96,11 +96,11 @@ export class CountingQueryEngine extends QueryEngine { setIndexManager: (indexManager: IndexManager) => { subject.setIndexManager(indexManager); }, - getDocumentsMatchingQuery: (transaction, query, sinceReadTime) => { + getAll: (transaction, collectionGroup, sinceReadTime) => { return subject - .getDocumentsMatchingQuery(transaction, query, sinceReadTime) + .getAll(transaction, collectionGroup, sinceReadTime) .next(result => { - this.documentsReadByQuery += result.size; + this.documentsReadByCollection += result.size; return result; }); }, @@ -154,7 +154,7 @@ export class CountingQueryEngine extends QueryEngine { return subject .getAllMutationBatchesAffectingQuery(transaction, query) .next(result => { - this.mutationsReadByQuery += result.length; + this.mutationsReadByCollection += result.length; return result; }); }, diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 58b1cba3ade..da8228d7b9d 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -330,25 +330,25 @@ class LocalStoreTester { * the MutationQueue and the RemoteDocumentCache. * * @param expectedCount.mutationsByQuery - The number of mutations read by - * executing a query against the MutationQueue. + * executing a collection scan against the MutationQueue. * @param expectedCount.mutationsByKey - The number of mutations read by * document key lookups. * @param expectedCount.documentsByQuery - The number of mutations read by - * executing a query against the RemoteDocumentCache. + * executing a collection scan against the RemoteDocumentCache. * @param expectedCount.documentsByKey - The number of documents read by * document key lookups. */ toHaveRead(expectedCount: { - mutationsByQuery?: number; + mutationsByCollection?: number; mutationsByKey?: number; - documentsByQuery?: number; + documentsByCollection?: number; documentsByKey?: number; }): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { - if (expectedCount.mutationsByQuery !== undefined) { - expect(this.queryEngine.mutationsReadByQuery).to.be.eq( - expectedCount.mutationsByQuery, - 'Mutations read (by query)' + if (expectedCount.mutationsByCollection !== undefined) { + expect(this.queryEngine.mutationsReadByCollection).to.be.eq( + expectedCount.mutationsByCollection, + 'Mutations read (by collection)' ); } if (expectedCount.mutationsByKey !== undefined) { @@ -357,10 +357,10 @@ class LocalStoreTester { 'Mutations read (by key)' ); } - if (expectedCount.documentsByQuery !== undefined) { - expect(this.queryEngine.documentsReadByQuery).to.be.eq( - expectedCount.documentsByQuery, - 'Remote documents read (by query)' + if (expectedCount.documentsByCollection !== undefined) { + expect(this.queryEngine.documentsReadByCollection).to.be.eq( + expectedCount.documentsByCollection, + 'Remote documents read (by collection)' ); } if (expectedCount.documentsByKey !== undefined) { @@ -1209,7 +1209,7 @@ function genericLocalStoreTests( doc('foo/baz', 20, { matches: true }), doc('foo/bonk', 0, { matches: true }).setHasLocalMutations() ) - .toHaveRead({ documentsByQuery: 2, mutationsByQuery: 1 }) + .toHaveRead({ documentsByCollection: 2, mutationsByCollection: 1 }) .finish(); }); @@ -1806,7 +1806,7 @@ function genericLocalStoreTests( // Execute the query, but note that we read all existing documents // from the RemoteDocumentCache since we do not yet have target // mapping. - .toHaveRead({ documentsByQuery: 2 }) + .toHaveRead({ documentsByCollection: 3 }) .after( docAddedRemoteEvent( [ @@ -1826,7 +1826,7 @@ function genericLocalStoreTests( ) .after(localViewChanges(2, /* fromCache= */ false, {})) .afterExecutingQuery(query1) - .toHaveRead({ documentsByKey: 2, documentsByQuery: 0 }) + .toHaveRead({ documentsByKey: 2, documentsByCollection: 0 }) .toReturnChanged( doc('foo/a', 10, { matches: true }), doc('foo/b', 10, { matches: true }) @@ -1935,7 +1935,7 @@ function genericLocalStoreTests( // Re-run the query as a collection scan .afterExecutingQuery(query1) .toReturnChanged(doc('foo/a', 10, { matches: true })) - .toHaveRead({ documentsByQuery: 1 }) + .toHaveRead({ documentsByCollection: 1 }) .finish() ); } diff --git a/packages/firestore/test/unit/local/remote_document_cache.test.ts b/packages/firestore/test/unit/local/remote_document_cache.test.ts index b10e57b9dda..d69c0d588e2 100644 --- a/packages/firestore/test/unit/local/remote_document_cache.test.ts +++ b/packages/firestore/test/unit/local/remote_document_cache.test.ts @@ -29,7 +29,7 @@ import { expectEqual, field, key, - query, + path, removedDoc, version, wrap @@ -360,9 +360,7 @@ function genericRemoteDocumentCacheTests( return cache.removeEntry(key(DOC_PATH)); }); - it('can get documents matching query', async () => { - // TODO(mikelehen): This just verifies that we do a prefix scan against the - // query path. We'll need more tests once we add index support. + it('can get all documents from collection', async () => { await cache.addEntries([ doc('a/1', VERSION, DOC_DATA), doc('b/1', VERSION, DOC_DATA), @@ -371,19 +369,14 @@ function genericRemoteDocumentCacheTests( doc('c/1', VERSION, DOC_DATA) ]); - const query1 = query('b'); - const matchingDocs = await cache.getDocumentsMatchingQuery( - query1, - SnapshotVersion.min() - ); - + const matchingDocs = await cache.getAll(path('b'), SnapshotVersion.min()); assertMatches( [doc('b/1', VERSION, DOC_DATA), doc('b/2', VERSION, DOC_DATA)], matchingDocs ); }); - it('can get documents matching query by read time', async () => { + it('can get all documents since read time', async () => { await cache.addEntries([ doc('b/old', 1, DOC_DATA).setReadTime(version(11)) ]); @@ -394,21 +387,19 @@ function genericRemoteDocumentCacheTests( doc('b/new', 3, DOC_DATA).setReadTime(version(13)) ]); - const query1 = query('b'); - const matchingDocs = await cache.getDocumentsMatchingQuery( - query1, + const matchingDocs = await cache.getAll( + path('b'), /* sinceReadTime= */ version(12) ); assertMatches([doc('b/new', 3, DOC_DATA)], matchingDocs); }); - it('query matching uses read time rather than update time', async () => { + it('getAll() uses read time rather than update time', async () => { await cache.addEntries([doc('b/old', 1, DOC_DATA).setReadTime(version(2))]); await cache.addEntries([doc('b/new', 2, DOC_DATA).setReadTime(version(1))]); - const query1 = query('b'); - const matchingDocs = await cache.getDocumentsMatchingQuery( - query1, + const matchingDocs = await cache.getAll( + path('b'), /* sinceReadTime= */ version(1) ); assertMatches([doc('b/old', 1, DOC_DATA)], matchingDocs); @@ -442,9 +433,8 @@ function genericRemoteDocumentCacheTests( verifyOldValue(document); document.data.set(field('state'), wrap('new')); - const query1 = query('coll'); document = await cache - .getDocumentsMatchingQuery(query1, SnapshotVersion.min()) + .getAll(path('coll'), SnapshotVersion.min()) .then(m => m.get(key('coll/doc'))!); verifyOldValue(document); diff --git a/packages/firestore/test/unit/local/test_remote_document_cache.ts b/packages/firestore/test/unit/local/test_remote_document_cache.ts index 9a46561e9a7..ec0b3c09ec0 100644 --- a/packages/firestore/test/unit/local/test_remote_document_cache.ts +++ b/packages/firestore/test/unit/local/test_remote_document_cache.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { Query } from '../../../src/core/query'; import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { IndexManager } from '../../../src/local/index_manager'; import { remoteDocumentCacheGetNewDocumentChanges } from '../../../src/local/indexeddb_remote_document_cache'; @@ -29,6 +28,7 @@ import { } from '../../../src/model/collections'; import { Document, MutableDocument } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; +import { ResourcePath } from '../../../src/model/path'; /** * A wrapper around a RemoteDocumentCache that automatically creates a @@ -109,16 +109,14 @@ export class TestRemoteDocumentCache { }); } - getDocumentsMatchingQuery( - query: Query, + getAll( + collection: ResourcePath, sinceReadTime: SnapshotVersion ): Promise { return this.persistence.runTransaction( 'getDocumentsMatchingQuery', 'readonly', - txn => { - return this.cache.getDocumentsMatchingQuery(txn, query, sinceReadTime); - } + txn => this.cache.getAll(txn, collection, sinceReadTime) ); }