Test cleanup and better documentation.

MarkDuckworth · MarkDuckworth · commit 40d2b33ac712 · 2024-10-25T12:01:47.000-06:00
diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts
@@ -588,9 +588,9 @@ export class WatchChangeAggregator {
         if (targetState.current && targetIsDocumentTarget(targetData.target)) {
           // Document queries for document that don't exist can produce an empty
           // result set. To update our local cache, we synthesize a document
-          // delete if we have not previously received the document. This
-          // resolves the limbo state of the document, removing it from
-          // limboDocumentRefs.
+          // delete if we have not previously received the document for this
+          // target. This resolves the limbo state of the document, removing it
+          // from limboDocumentRefs.
           //
           // TODO(dimond): Ideally we would have an explicit lookup target
           // instead resulting in an explicit delete message and we could
diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts
@@ -1173,7 +1173,7 @@ describeSpec('Limbo Documents:', [], () => {
   );
 
   specTest(
-    'Fix #8474 - Limbo resolution for documentC is removed even if documentC updates occurred earlier in the global snapshot window',
+    'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred before documentDelete in the global snapshot window',
     [],
     () => {
       // onSnapshot(fullQuery)
@@ -1199,7 +1199,7 @@ describeSpec('Limbo Documents:', [], () => {
           })
 
           // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
-          .watchSends({ removed: [fullQuery] }, docC)
+          .watchRemovesDoc(docC.key, fullQuery)
           .watchCurrents(fullQuery, 'resume-token-1002')
           .watchSnapshots(1002)
           .expectLimboDocs(docC.key)
@@ -1221,19 +1221,19 @@ describeSpec('Limbo Documents:', [], () => {
           .watchAcks(filterQuery)
 
           // Watch responds to limboQueryC - docC was deleted
-          .watchSends({ affects: [limboQueryC] })
+          .watchDeletesDoc(docC.key, 1009, limboQueryC)
           .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)
+          .watchRemovesDoc(docA.key, filterQuery)
           .watchCurrents(filterQuery, 'resume-token-1004')
           .watchSends({ affects: [filterQuery] }, docC)
           .watchCurrents(filterQuery, 'resume-token-1005')
-          .watchSends({ removed: [filterQuery] }, docC)
+          .watchRemovesDoc(docC.key, filterQuery)
           .watchSends({ affects: [filterQuery] }, docA2)
           .watchCurrents(filterQuery, 'resume-token-1007')
 
@@ -1255,96 +1255,103 @@ describeSpec('Limbo Documents:', [], () => {
             removed: [docC],
             added: [docA2]
           })
+
+          // No more expected events
+          .watchSnapshots(1100, [])
       );
     }
   );
 
-  specTest('Fix #8474 - Watch sends deletedDoc on limbo resolution', [], () => {
-    // onSnapshot(fullQuery)
-    const fullQuery = query('collection');
+  specTest(
+    'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred in the global snapshot window and no document delete was received for the limbo resolution query',
+    [],
+    () => {
+      // onSnapshot(fullQuery)
+      const fullQuery = query('collection');
 
-    // getDocs(filterQuery)
-    const filterQuery = query('collection', filter('included', '==', true));
+      // 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 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 limboQueryC = newQueryForPath(docC.key.path);
+      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]
-        })
+      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
-        })
+          // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
+          .watchRemovesDoc(docC.key, fullQuery)
+          .watchCurrents(fullQuery, 'resume-token-1002')
+          .watchSnapshots(1002)
+          .expectLimboDocs(docC.key)
+          .expectEvents(fullQuery, {
+            fromCache: true
+          })
 
-        // User begins getDocs(filterQuery)
-        .userListensForGet(filterQuery)
+          // User begins getDocs(filterQuery)
+          .userListensForGet(filterQuery)
 
-        // getDocs(filterQuery) will not resolve on the snapshot from cache
-        .expectEvents(filterQuery, {
-          fromCache: true,
-          added: [docC]
-        })
+          // getDocs(filterQuery) will not resolve on the snapshot from cache
+          .expectEvents(filterQuery, {
+            fromCache: true,
+            added: [docC]
+          })
 
-        // Watch acks limbo and filter queries
-        .watchAcks(limboQueryC)
-        .watchAcks(filterQuery)
+          // 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])
+          // Watch currents the limbo query, but did not send a document delete.
+          // This is and unexpected code path, but something that is called
+          // out as possible in the watch change aggregator.
+          .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)
+          // 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')
+          // Rapid events of document update and delete caused by application
+          .watchRemovesDoc(docA.key, filterQuery)
+          .watchCurrents(filterQuery, 'resume-token-1004')
+          .watchSends({ affects: [filterQuery] }, docC)
+          .watchCurrents(filterQuery, 'resume-token-1005')
+          .watchRemovesDoc(docC.key, filterQuery)
+          .watchSends({ affects: [filterQuery] }, docA2)
+          .watchCurrents(filterQuery, 'resume-token-1007')
 
-        .watchSnapshots(1010, [fullQuery, limboQueryC])
+          .watchSnapshots(1010, [fullQuery, limboQueryC])
 
-        // All changes are current and we get a global snapshot
-        .watchSnapshots(1010, [])
+          // 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],
-          added: [docA2]
-        })
+          // 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]
+          })
 
-        // No more expected events
-        .watchSnapshots(1100, [])
-    );
-  });
+          // No more expected events
+          .watchSnapshots(1100, [])
+      );
+    }
+  );
 
   specTest(
     'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot',
@@ -1359,7 +1366,6 @@ describeSpec('Limbo Documents:', [], () => {
       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);
 
@@ -1374,7 +1380,7 @@ describeSpec('Limbo Documents:', [], () => {
           })
 
           // docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
-          .watchSends({ removed: [fullQuery] }, docC)
+          .watchRemovesDoc(docC.key, fullQuery)
           .watchCurrents(fullQuery, 'resume-token-1002')
           .watchSnapshots(1002)
           .expectLimboDocs(docC.key)
@@ -1398,11 +1404,11 @@ describeSpec('Limbo Documents:', [], () => {
           .expectLimboDocs(docC.key)
 
           // Rapid events of document update and delete caused by application
-          .watchSends({ removed: [filterQuery] }, docA)
+          .watchRemovesDoc(docA.key, filterQuery)
           .watchCurrents(filterQuery, 'resume-token-1004')
           .watchSends({ affects: [filterQuery] }, docC)
           .watchCurrents(filterQuery, 'resume-token-1005')
-          .watchSends({ removed: [filterQuery] }, docC)
+          .watchRemovesDoc(docC.key, filterQuery)
           .watchSends({ affects: [filterQuery] }, docA2)
           .watchCurrents(filterQuery, 'resume-token-1007')
 
@@ -1425,7 +1431,7 @@ describeSpec('Limbo Documents:', [], () => {
           .watchAcks(limboQueryC)
 
           // Watch responds to limboQueryC - docC was deleted
-          .watchSends({ affects: [limboQueryC] }, docCDeleted)
+          .watchDeletesDoc(docC.key, 1009, limboQueryC)
           .watchCurrents(limboQueryC, 'resume-token-1009')
           .watchSnapshots(1100, [limboQueryC])
 
diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts
@@ -51,7 +51,7 @@ import { forEach } from '../../../src/util/obj';
 import { ObjectMap } from '../../../src/util/obj_map';
 import { isNullOrUndefined } from '../../../src/util/types';
 import { firestore } from '../../util/api_helpers';
-import { TestSnapshotVersion } from '../../util/helpers';
+import { deletedDoc, TestSnapshotVersion } from '../../util/helpers';
 
 import { RpcError } from './spec_rpc_error';
 import {
@@ -830,6 +830,23 @@ export class SpecBuilder {
     return this;
   }
 
+  watchDeletesDoc(
+    key: DocumentKey,
+    version: TestSnapshotVersion,
+    ...targets: Query[]
+  ): this {
+    this.nextStep();
+    this.currentStep = {
+      watchEntity: {
+        doc: SpecBuilder.docToSpec(
+          deletedDoc(key.path.canonicalString(), version)
+        ),
+        removedTargets: targets.map(query => this.getTargetId(query))
+      }
+    };
+    return this;
+  }
+
   watchFilters(
     queries: Query[],
     docs: DocumentKey[] = [],
diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts
@@ -172,12 +172,19 @@ export function doc(
 }
 
 export function deletedDoc(
-  keyStr: string,
+  keyStrOrDocumentKey: string | DocumentKey,
   ver: TestSnapshotVersion
 ): MutableDocument {
-  return MutableDocument.newNoDocument(key(keyStr), version(ver)).setReadTime(
+  if (
+    keyStrOrDocumentKey instanceof String ||
+    typeof keyStrOrDocumentKey === 'string'
+  ) {
+    keyStrOrDocumentKey = key(keyStrOrDocumentKey as string);
+  }
+  return MutableDocument.newNoDocument(
+    keyStrOrDocumentKey,
     version(ver)
-  );
+  ).setReadTime(version(ver));
 }
 
 export function unknownDoc(
