Fix for 8474

Author: MarkDuckworth

packages/firestore/src/remote/watch_change.ts

@@ -291,6 +291,7 @@ export class WatchChangeAggregator {
 
   /** Keeps track of the documents to update since the last raised snapshot. */
   private pendingDocumentUpdates = mutableDocumentMap();
+  private pendingDocumentUpdatesByTarget = documentTargetMap();
 
   /** A mapping of document keys to their set of target IDs. */
   private pendingDocumentTargetMapping = documentTargetMap();
@@ -596,7 +597,7 @@ export class WatchChangeAggregator {
           // remove this special logic.
           const key = new DocumentKey(targetData.target.path);
           if (
-            this.pendingDocumentUpdates.get(key) === null &&
+            !this.ensureDocumentUpdateByTarget(key).has(targetId) &&
             !this.targetContainsDocument(targetId, key)
           ) {
             this.removeDocumentFromTarget(
@@ -655,6 +656,7 @@ export class WatchChangeAggregator {
     );
 
     this.pendingDocumentUpdates = mutableDocumentMap();
+    this.pendingDocumentUpdatesByTarget = documentTargetMap();
     this.pendingDocumentTargetMapping = documentTargetMap();
     this.pendingTargetResets = new SortedMap<TargetId, TargetPurpose>(
       primitiveComparator
@@ -685,6 +687,11 @@ export class WatchChangeAggregator {
       document
     );
 
+    this.pendingDocumentUpdatesByTarget =
+      this.pendingDocumentUpdatesByTarget.insert(
+        document.key,
+        this.ensureDocumentUpdateByTarget(document.key).add(targetId));
+
     this.pendingDocumentTargetMapping =
       this.pendingDocumentTargetMapping.insert(
         document.key,
@@ -724,6 +731,12 @@ export class WatchChangeAggregator {
         this.ensureDocumentTargetMapping(key).delete(targetId)
       );
 
+    this.pendingDocumentTargetMapping =
+      this.pendingDocumentTargetMapping.insert(
+        key,
+        this.ensureDocumentTargetMapping(key).add(targetId)
+      );
+
     if (updatedDocument) {
       this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
         key,
@@ -782,6 +795,18 @@ export class WatchChangeAggregator {
     return targetMapping;
   }
 
+  private ensureDocumentUpdateByTarget(key: DocumentKey): SortedSet<TargetId> {
+    let targetMapping = this.pendingDocumentUpdatesByTarget.get(key);
+
+    if (!targetMapping) {
+      targetMapping = new SortedSet<TargetId>(primitiveComparator);
+      this.pendingDocumentUpdatesByTarget =
+        this.pendingDocumentUpdatesByTarget.insert(key, targetMapping);
+    }
+
+    return targetMapping;
+  }
+
   /**
    * Verifies that the user is still interested in this target (by calling
    * `getTargetDataForTarget()`) and that we are not waiting for pending ADDs

packages/firestore/test/unit/specs/collection_spec.test.ts

@@ -15,15 +15,15 @@
  * limitations under the License.
  */
 
-import {doc, filter, query} from '../../util/helpers';
+import {newQueryForPath} from "../../../src/core/query";
+import {deletedDoc, doc, filter, query} from '../../util/helpers';
 
 import { describeSpec, specTest } from './describe_spec';
 import { spec } from './spec_builder';
-import {newQueryForPath} from "../../../src/core/query";
 
 describeSpec('Collections:', [], () => {
 
-  specTest('8474', ['exclusive', 'durable-persistence'], () => {
+  specTest('8474', [], () => {
 
     // onSnapshot(fullQuery)
     const fullQuery = query('collection');
@@ -90,36 +90,104 @@ describeSpec('Collections:', [], () => {
       // All changes are current and we get a global snapshot
       .watchSnapshots(1010, [])
 
-      // UNEXPECTED docC is still in limbo
-      .expectLimboDocs(docC.key)
+      // Now docC is out of limbo
+      .expectLimboDocs()
+      .expectEvents(fullQuery, {
+        fromCache: false,
+        modified: [docA2],
+        removed: [docC]
+      })
+      // Now getDocs(filterQuery) can be resolved
+      .expectEvents(filterQuery, {
+        fromCache: false,
+        removed: [docC],
+        added: [docA2]
+      });
+  });
+  specTest('8474-deleted', [], () => {
 
-      // This causes snapshots for fullQuery and filterQuery
-      // to be raised as from cache
+    // onSnapshot(fullQuery)
+    const fullQuery = query('collection');
+
+    // getDocs(filterQuery)
+    const filterQuery = query('collection', filter('included', '==', true));
+
+    const docA = doc('collection/a', 1000, {key: 'a', included: false});
+    const docA2 = doc('collection/a', 1007, {key: 'a', included: true});
+    const docC = doc('collection/c', 1002, {key: 'c', included: true});
+    const docCDeleted = deletedDoc('collection/c', 1009);
+
+    const limboQueryC = newQueryForPath(docC.key.path);
+
+    return spec()
+      // onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
+      .userListens(fullQuery)
+      .watchAcksFull(fullQuery, 1001, docA, docC)
+      .expectEvents(fullQuery, {
+        fromCache: false,
+        added: [docA, docC]
+      })
+
+      // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
+      .watchSends({removed: [fullQuery]}, docC)
+      .watchCurrents(fullQuery, 'resume-token-1002')
+      .watchSnapshots(1002)
+      .expectLimboDocs(docC.key)
       .expectEvents(fullQuery, {
         fromCache: true,
-        modified: [docA2]
       })
 
-      // getDocs(filterQuery) will not be resolved with this snapsho
-      // because it is from cache
+      // User begins getDocs(filterQuery)
+      .userListensForGet(filterQuery)
+
+      // getDocs(filterQuery) will not resolve on the snapshot from cache
       .expectEvents(filterQuery, {
         fromCache: true,
-        added: [docA2]
+        added: [docC]
       })
 
-      // 45-seconds later on heartbeat/global-snapshot
-      .watchSnapshots(1100, [])
+      // Watch acks limbo and filter queries
+      .watchAcks(limboQueryC)
+      .watchAcks(filterQuery)
+
+      // Watch responds to limboQueryC - docC was deleted
+      .watchSends({affects: [limboQueryC]}, docCDeleted)
+      .watchCurrents(limboQueryC, 'resume-token-1009')
+      .watchSnapshots(1009, [limboQueryC, fullQuery])
+
+      // However, docC is still in limbo because there has not been a global snapshot
+      .expectLimboDocs(docC.key)
+
+      // Rapid events of document update and delete caused by application
+      .watchSends({removed: [filterQuery]}, docA)
+      .watchCurrents(filterQuery, 'resume-token-1004')
+      .watchSends({affects: [filterQuery]}, docC)
+      .watchCurrents(filterQuery, 'resume-token-1005')
+      .watchSends({removed: [filterQuery]}, docC)
+      .watchSends({affects: [filterQuery]}, docA2)
+      .watchCurrents(filterQuery, 'resume-token-1007')
+
+      .watchSnapshots(1010, [fullQuery, limboQueryC])
+
+      // All changes are current and we get a global snapshot
+      .watchSnapshots(1010, [])
+
       // Now docC is out of limbo
       .expectLimboDocs()
       .expectEvents(fullQuery, {
         fromCache: false,
+        modified: [docA2],
         removed: [docC]
       })
       // Now getDocs(filterQuery) can be resolved
       .expectEvents(filterQuery, {
         fromCache: false,
-        removed: [docC]
-      });
+        removed: [docC],
+        added: [docA2]
+      })
+
+    // No more expected events
+    .watchSnapshots(1100, []);
   });
 
   specTest('Events are raised after watch ack', [], () => {