Merge ffb8f6f into 434d87e

Author: schmidt-sebastian

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<MutableDocumentMap> {
-    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);

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,51 +216,31 @@ export class LocalDocumentsView {
   ): PersistencePromise<DocumentMap> {
     // 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(
           transaction,
           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<MutableDocumentMap> {
-    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;
-      });
-  }
 }

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<MutableDocumentMap> {
-    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());

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<MutableDocumentMap>;
 
   /**
-   * 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<MutableDocumentMap>;
 

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;
           });
       },