Only update mapping when query read time is updated.

Author: wu-hui

packages/firestore/src/core/bundle.ts

@@ -28,14 +28,19 @@ import * as bundleProto from '../protos/firestore_bundle_proto';
 import * as api from '../protos/firestore_proto_api';
 import { DocumentKey } from '../model/document_key';
 import { MaybeDocument, NoDocument } from '../model/document';
-import { debugAssert } from '../util/assert';
+import { debugAssert, debugCast } from '../util/assert';
 import {
   applyBundleDocuments,
+  getQueryDocumentMapping,
   LocalStore,
   saveNamedQuery
 } from '../local/local_store';
 import { SizedBundleElement } from '../util/bundle_reader';
-import { MaybeDocumentMap } from '../model/collections';
+import {
+  documentKeySet,
+  DocumentKeySet,
+  MaybeDocumentMap
+} from '../model/collections';
 import { BundleMetadata } from '../protos/firestore_bundle_proto';
 
 /**
@@ -218,17 +223,20 @@ export class BundleLoader {
       'Bundled documents ends with a document metadata and missing document.'
     );
 
-    const result = await applyBundleDocuments(this.localStore, this.documents);
+    const changedDocuments = await applyBundleDocuments(
+      this.localStore,
+      this.documents
+    );
 
+    const queryDocumentMap = getQueryDocumentMapping(
+      this.localStore,
+      this.documents
+    );
     for (const q of this.queries) {
-      await saveNamedQuery(
-        this.localStore,
-        q,
-        result.queryDocumentMap.get(q.name!)
-      );
+      await saveNamedQuery(this.localStore, q, queryDocumentMap.get(q.name!));
     }
 
     this.progress.taskState = 'Success';
-    return new BundleLoadResult({ ...this.progress }, result.changedDocuments);
+    return new BundleLoadResult({ ...this.progress }, changedDocuments);
   }
 }

packages/firestore/src/local/local_store.ts

@@ -1249,9 +1249,28 @@ export async function ignoreIfPrimaryLeaseLoss(
   }
 }
 
-export interface ApplyBundleDocumentsResult {
-  changedDocuments: MaybeDocumentMap;
-  queryDocumentMap: Map<string, DocumentKeySet>;
+export function getQueryDocumentMapping(
+  localStore: LocalStore,
+  documents: BundledDocuments
+): Map<string, DocumentKeySet> {
+  const localStoreImpl = debugCast(localStore, LocalStoreImpl);
+  const queryDocumentMap = new Map<string, DocumentKeySet>();
+  const bundleConverter = new BundleConverter(localStoreImpl.serializer);
+  for (const bundleDoc of documents) {
+    if (bundleDoc.metadata.queries) {
+      const documentKey = bundleConverter.toDocumentKey(
+        bundleDoc.metadata.name!
+      );
+      for (const queryName of bundleDoc.metadata.queries) {
+        const documentKeys = (
+          queryDocumentMap.get(queryName) || documentKeySet()
+        ).add(documentKey);
+        queryDocumentMap.set(queryName, documentKeys);
+      }
+    }
+  }
+
+  return queryDocumentMap;
 }
 
 /**
@@ -1264,12 +1283,11 @@ export interface ApplyBundleDocumentsResult {
 export function applyBundleDocuments(
   localStore: LocalStore,
   documents: BundledDocuments
-): Promise<ApplyBundleDocumentsResult> {
+): Promise<MaybeDocumentMap> {
   const localStoreImpl = debugCast(localStore, LocalStoreImpl);
   const bundleConverter = new BundleConverter(localStoreImpl.serializer);
   let documentMap = maybeDocumentMap();
   let versionMap = documentVersionMap();
-  const queryDocumentMap: Map<string, DocumentKeySet> = new Map();
   for (const bundleDoc of documents) {
     const documentKey = bundleConverter.toDocumentKey(bundleDoc.metadata.name!);
     documentMap = documentMap.insert(
@@ -1280,14 +1298,6 @@ export function applyBundleDocuments(
       documentKey,
       bundleConverter.toSnapshotVersion(bundleDoc.metadata.readTime!)
     );
-    if (bundleDoc.metadata.queries) {
-      for (const queryName of bundleDoc.metadata.queries) {
-        const documentKeys = (
-          queryDocumentMap.get(queryName) || documentKeySet()
-        ).add(documentKey);
-        queryDocumentMap.set(queryName, documentKeys);
-      }
-    }
   }
 
   const documentBuffer = localStoreImpl.remoteDocuments.newChangeBuffer({
@@ -1316,10 +1326,7 @@ export function applyBundleDocuments(
           );
         })
         .next(changedDocuments => {
-          return PersistencePromise.resolve({
-            changedDocuments,
-            queryDocumentMap
-          });
+          return PersistencePromise.resolve(changedDocuments);
         });
     }
   );
@@ -1410,32 +1417,40 @@ export async function saveNamedQuery(
     'readwrite',
     transaction => {
       // Update allocated target's read time, if the bundle's read time is newer.
-      let updateReadTime = PersistencePromise.resolve();
+      let updateReadTime = PersistencePromise.resolve(false);
       const readTime = fromVersion(query.readTime!);
       if (allocated.snapshotVersion.compareTo(readTime) < 0) {
         const newTargetData = allocated.withResumeToken(
           ByteString.EMPTY_BYTE_STRING,
           readTime
         );
-        updateReadTime = localStoreImpl.targetCache.updateTargetData(
-          transaction,
-          newTargetData
-        );
+        updateReadTime = localStoreImpl.targetCache
+          .updateTargetData(transaction, newTargetData)
+          .next(
+            () => true,
+            () => false
+          );
         localStoreImpl.targetDataByTarget = localStoreImpl.targetDataByTarget.insert(
           newTargetData.targetId,
           newTargetData
         );
       }
       return updateReadTime
+        .next(updated => {
+          if (updated) {
+            return localStoreImpl.targetCache
+              .removeMatchingKeysForTargetId(transaction, allocated.targetId)
+              .next(() =>
+                localStoreImpl.targetCache.addMatchingKeys(
+                  transaction,
+                  documents,
+                  allocated.targetId
+                )
+              );
+          }
+        })
         .next(() =>
           localStoreImpl.bundleCache.saveNamedQuery(transaction, query)
-        )
-        .next(() =>
-          localStoreImpl.targetCache.addMatchingKeys(
-            transaction,
-            documents,
-            allocated.targetId
-          )
         );
     }
   );

packages/firestore/src/protos/firestore_bundle_proto.ts

@@ -56,7 +56,7 @@ export interface BundledDocumentMetadata {
   exists?: boolean | null;
 
   /** The names of the queries in this bundle that this document matches to. */
-  queries?: string[] | null;
+  queries?: string[];
 }
 
 /** Properties of a BundleMetadata. */

packages/firestore/test/unit/local/local_store.test.ts

@@ -35,7 +35,6 @@ import { IndexFreeQueryEngine } from '../../../src/local/index_free_query_engine
 import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence';
 import {
   applyBundleDocuments,
-  ApplyBundleDocumentsResult,
   getNamedQuery,
   hasNewerBundle,
   LocalStore,
@@ -116,7 +115,6 @@ export interface LocalStoreComponents {
 class LocalStoreTester {
   private promiseChain: Promise<void> = Promise.resolve();
   private lastChanges: MaybeDocumentMap | null = null;
-  private lastBundleQueryDocumentMap: Map<string, DocumentKeySet> | null = null;
   private lastTargetId: TargetId | null = null;
   private batches: MutationBatch[] = [];
 
@@ -129,7 +127,6 @@ class LocalStoreTester {
   private prepareNextStep(): void {
     this.promiseChain = this.promiseChain.then(() => {
       this.lastChanges = null;
-      this.lastBundleQueryDocumentMap = null;
       this.lastTargetId = null;
       this.queryEngine.resetCounts();
     });
@@ -193,9 +190,8 @@ class LocalStoreTester {
 
     this.promiseChain = this.promiseChain
       .then(() => applyBundleDocuments(this.localStore, documents))
-      .then((result: ApplyBundleDocumentsResult) => {
-        this.lastChanges = result.changedDocuments;
-        this.lastBundleQueryDocumentMap = result.queryDocumentMap;
+      .then(result => {
+        this.lastChanges = result;
       });
     return this;
   }
@@ -382,25 +378,6 @@ class LocalStoreTester {
     return this;
   }
 
-  toReturnQueryDocumentMap(
-    expected: Map<string, DocumentKeySet>
-  ): LocalStoreTester {
-    this.promiseChain = this.promiseChain.then(() => {
-      debugAssert(
-        !!this.lastBundleQueryDocumentMap,
-        'Expecting query document map to be returned from applying bundled documents'
-      );
-      for (const k of Array.from(this.lastBundleQueryDocumentMap.keys())) {
-        expect(expected.has(k)).to.be.true;
-        expect(
-          this.lastBundleQueryDocumentMap.get(k)!.isEqual(expected.get(k)!)
-        ).to.be.true;
-      }
-      this.lastBundleQueryDocumentMap = null;
-    });
-    return this;
-  }
-
   toReturnRemoved(...keyStrings: string[]): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       debugAssert(
@@ -1758,7 +1735,7 @@ function genericLocalStoreTests(
       .finish();
   });
 
-  it('handles loading named queries allocates targets and updates target document mapping', async () => {
+  it('loading named queries allocates targets and updates target document mapping', async () => {
     const expectedQueryDocumentMap = new Map([
       ['query-1', documentKeySet(key('foo1/bar'))],
       ['query-2', documentKeySet(key('foo2/bar'))]
@@ -1777,7 +1754,6 @@ function genericLocalStoreTests(
         doc('foo1/bar', 1, { sum: 1337 }),
         doc('foo2/bar', 2, { sum: 42 })
       )
-      .toReturnQueryDocumentMap(expectedQueryDocumentMap)
       .toContain(doc('foo1/bar', 1, { sum: 1337 }))
       .toContain(doc('foo2/bar', 2, { sum: 42 }))
       .after(