avoid using objects for parameters since their names can get mangled and/or excluded from minified code

dconeybe · dconeybe · commit 85ed7ab0ca02 · 2023-03-08T12:07:37.000-05:00
diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts
@@ -196,7 +196,5 @@ export type { ByteString as _ByteString } from './util/byte_string';
 export { logWarn as _logWarn } from './util/log';
 export { EmptyAuthCredentialsProvider as _EmptyAuthCredentialsProvider } from './api/credentials';
 export { EmptyAppCheckTokenProvider as _EmptyAppCheckTokenProvider } from './api/credentials';
-export {
-  TestingHooks as _TestingHooks,
-  ExistenceFilterMismatchInfo as _TestingUtilsExistenceFilterMismatchInfo
-} from './util/testing_hooks';
+export { TestingHooks as _TestingHooks } from './util/testing_hooks';
+export { ExistenceFilterMismatchCallback as _TestingHooksExistenceFilterMismatchCallback } from './util/testing_hooks';
diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts
@@ -433,11 +433,11 @@ export class WatchChangeAggregator {
             this.resetTarget(targetId);
             this.pendingTargetResets = this.pendingTargetResets.add(targetId);
           }
-          TestingHooks.instance?.notifyOnExistenceFilterMismatch({
-            actualCount: currentSize,
-            change: watchChange,
-            bloomFilterApplied
-          });
+          notifyTestingHooksOnExistenceFilterMismatch(
+            bloomFilterApplied,
+            currentSize,
+            watchChange.existenceFilter
+          );
         }
       }
     }
@@ -795,3 +795,25 @@ function documentTargetMap(): SortedMap<DocumentKey, SortedSet<TargetId>> {
 function snapshotChangesMap(): SortedMap<DocumentKey, ChangeType> {
   return new SortedMap<DocumentKey, ChangeType>(DocumentKey.comparator);
 }
+
+function notifyTestingHooksOnExistenceFilterMismatch(
+  bloomFilterApplied: boolean,
+  actualCount: number,
+  existenceFilter: ExistenceFilter
+): void {
+  const expectedCount = existenceFilter.count;
+  const unchangedNames = existenceFilter.unchangedNames;
+  const bloomFilterSentFromWatch = !!unchangedNames;
+  const bloomFilterHashCount = unchangedNames?.hashCount ?? 0;
+  const bloomFilterBitmapLength = unchangedNames?.bits?.bitmap?.length ?? 0;
+  const bloomFilterPadding = unchangedNames?.bits?.padding ?? 0;
+  TestingHooks.instance?.notifyOnExistenceFilterMismatch(
+    actualCount,
+    expectedCount,
+    bloomFilterSentFromWatch,
+    bloomFilterApplied,
+    bloomFilterHashCount,
+    bloomFilterBitmapLength,
+    bloomFilterPadding
+  );
+}
diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts
@@ -34,7 +34,7 @@
 export class TestingHooks {
   private readonly onExistenceFilterMismatchCallbacks = new Map<
     Symbol,
-    (info: ExistenceFilterMismatchInfo) => void
+    ExistenceFilterMismatchCallback
   >();
 
   private constructor() {}
@@ -65,14 +65,16 @@ export class TestingHooks {
    * The relative order in which callbacks are notified is unspecified; do not
    * rely on any particular ordering.
    *
-   * @param callback the callback to invoke upon existence filter mismatch.
+   * @param callback the callback to invoke upon existence filter mismatch; see
+   * the documentation for `notifyOnExistenceFilterMismatch()` for the meaning
+   * of these arguments.
    *
    * @return a function that, when called, unregisters the given callback; only
    * the first invocation of the returned function does anything; all subsequent
    * invocations do nothing.
    */
   onExistenceFilterMismatch(
-    callback: (info: ExistenceFilterMismatchInfo) => void
+    callback: ExistenceFilterMismatchCallback
   ): () => void {
     const key = Symbol();
     this.onExistenceFilterMismatchCallbacks.set(key, callback);
@@ -81,36 +83,65 @@ export class TestingHooks {
 
   /**
    * Invokes all currently-registered `onExistenceFilterMismatch` callbacks.
-   * @param info the argument to specify to the callbacks.
+   * @param bloomFilterApplied true if a full requery was averted because the
+   * limbo documents were successfully deduced using the bloom filter, or false
+   * if a full requery had to be performed, such as due to a false positive in
+   * the bloom filter.
+   * @param actualCount the number of documents that matched the query in the
+   * local cache.
+   * @param expectedCount the number of documents that matched the query on the
+   * server, as specified in the ExistenceFilter message's `count` field.
+   * @param bloomFilterSentFromWatch whether the ExistenceFilter message
+   * included the `unchangedNames` bloom filter. If false, the remaining
+   * arguments are not applicable and may have any values.
+   * @param bloomFilterApplied whether a full requery was averted by using the
+   * bloom filter; if false, then a false positive occurred, requiring a full
+   * requery to deduce which documents should go into limbo.
+   * @param bloomFilterHashCount the number of hash functions used in the bloom
+   * filter.
+   * @param bloomFilterBitmapLength the number of bytes used by the bloom
+   * filter.
+   * @param bloomFilterPadding the number of bits of padding in the last byte
+   * of the bloom filter.
    */
-  notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void {
-    this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info));
+  notifyOnExistenceFilterMismatch(
+    actualCount: number,
+    expectedCount: number,
+    bloomFilterSentFromWatch: boolean,
+    bloomFilterApplied: boolean,
+    bloomFilterHashCount: number,
+    bloomFilterBitmapLength: number,
+    bloomFilterPadding: number
+  ): void {
+    this.onExistenceFilterMismatchCallbacks.forEach(callback => {
+      callback(
+        actualCount,
+        expectedCount,
+        bloomFilterSentFromWatch,
+        bloomFilterApplied,
+        bloomFilterHashCount,
+        bloomFilterBitmapLength,
+        bloomFilterPadding
+      );
+    });
   }
 }
 
 /**
- * The shape of the object specified to
- * `TestingUtils.onExistenceFilterMismatch()` callbacks.
+ * The signature of callbacks registered with
+ * `TestingUtils.onExistenceFilterMismatch()`.
  *
  * @internal
  */
-export interface ExistenceFilterMismatchInfo {
-  actualCount: number;
-  bloomFilterApplied: boolean;
-  change: {
-    targetId: number;
-    existenceFilter: {
-      count: number;
-      unchangedNames?: {
-        bits?: {
-          bitmap?: string | Uint8Array;
-          padding?: number;
-        };
-        hashCount?: number;
-      };
-    };
-  };
-}
+export type ExistenceFilterMismatchCallback = (
+  actualCount: number,
+  expectedCount: number,
+  bloomFilterSentFromWatch: boolean,
+  bloomFilterApplied: boolean,
+  bloomFilterHashCount: number,
+  bloomFilterBitmapLength: number,
+  bloomFilterPadding: number
+) => void;
 
 /** The global singleton instance of `TestingHooks`. */
 let gTestingHooksSingletonInstance: TestingHooks | null = null;
diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts
@@ -2085,7 +2085,7 @@ apiDescribe('Queries', (persistence: boolean) => {
             // Run a query to populate the local cache with the 100 documents
             // and a resume token.
             const snapshot1 = await getDocs(coll);
-            expect(snapshot1.size).to.equal(100);
+            expect(snapshot1.size, 'snapshot1.size').to.equal(100);
 
             // Delete 50 of the 100 documents. Do this in a transaction, rather
             // than deleteDoc(), to avoid affecting the local cache.
@@ -2107,47 +2107,56 @@ apiDescribe('Queries', (persistence: boolean) => {
             const existenceFilterMismatches =
               await captureExistenceFilterMismatches(async () => {
                 const snapshot2 = await getDocs(coll);
-                expect(snapshot2.size).to.equal(50);
+                expect(snapshot2.size, 'snapshot2.size').to.equal(50);
               });
 
             // Verify that upon resuming the query that Watch sent an existence
-            // filter that included a bloom filter, and that that bloom filter
+            // filter that included a bloom filter, and that the bloom filter
             // was successfully used to avoid a full requery.
             // TODO(b/271949433) Replace this "if" condition with !USE_EMULATOR
             // once the feature has been deployed to production. Note that there
-            // are no plans to implement the bloom filter in the existence filter
-            // responses sent from the Firestore emulator.
+            // are no plans to implement the bloom filter in the existence
+            // filter responses sent from the Firestore *emulator*.
             if (TARGET_BACKEND === 'nightly') {
-              expect(existenceFilterMismatches).to.have.length(1);
-              const existenceFilterMismatch = existenceFilterMismatches[0];
-              expect(existenceFilterMismatch.actualCount).to.equal(100);
-              expect(existenceFilterMismatch.expectedCount).to.equal(50);
-              const bloomFilter = existenceFilterMismatch.bloomFilter;
-              if (!bloomFilter) {
-                expect.fail('existence filter should have had a bloom filter');
-                throw new Error('should never get here');
+              expect(
+                existenceFilterMismatches,
+                'existenceFilterMismatches'
+              ).to.have.length(1);
+              const {
+                actualCount,
+                expectedCount,
+                bloomFilterSentFromWatch,
+                bloomFilterApplied,
+                bloomFilterHashCount,
+                bloomFilterBitmapLength,
+                bloomFilterPadding
+              } = existenceFilterMismatches[0];
+
+              expect(actualCount, 'actualCount').to.equal(100);
+              expect(expectedCount, 'expectedCount').to.equal(50);
+              expect(bloomFilterSentFromWatch, 'bloomFilterSentFromWatch').to.be
+                .true;
+
+              // Retry the entire test if a bloom filter false positive occurs.
+              // Although statistically rare, false positives are expected to
+              // happen occasionally. When a false positive _does_ happen, just
+              // retry the test with a different set of documents. If that retry
+              // _also_ experiences a false positive, then fail the test because
+              // that is so improbable that something must have gone wrong.
+              if (attemptNumber > 1 && !bloomFilterApplied) {
+                return 'retry';
               }
 
-              // Although statistically rare, it _is_ possible to get a legitimate
-              // false positive when checking the bloom filter. If this happens,
-              // then retry the test with a different set of documents, and only
-              // fail if the retry _also_ experiences a false positive.
-              if (!bloomFilter.applied) {
-                if (attemptNumber < 2) {
-                  return 'retry';
-                } else {
-                  expect.fail(
-                    'bloom filter false positive occurred ' +
-                      'multiple times; this is statistically ' +
-                      'improbable and should be investigated.'
-                  );
-                }
-              }
-
-              expect(bloomFilter.bitmapLength).to.be.above(0);
-              expect(bloomFilter.hashCount).to.be.above(0);
-              expect(bloomFilter.padding).to.be.above(0);
-              expect(bloomFilter.padding).to.be.below(8);
+              expect(bloomFilterApplied, 'bloomFilterApplied').to.be.true;
+              expect(bloomFilterHashCount, 'bloomFilterHashCount').to.be.above(
+                0
+              );
+              expect(
+                bloomFilterBitmapLength,
+                'bloomFilterBitmapLength'
+              ).to.be.above(0);
+              expect(bloomFilterPadding, 'bloomFilterPadding').to.be.above(0);
+              expect(bloomFilterPadding, 'bloomFilterPadding').to.be.below(8);
             }
 
             return 'passed';
diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts
@@ -15,11 +15,7 @@
  * limitations under the License.
  */
 
-import {
-  _TestingHooks as TestingHooks,
-  _TestingUtilsExistenceFilterMismatchInfo as RawExistenceFilterMismatchInfo,
-  _logWarn
-} from './firebase_export';
+import { _TestingHooks as TestingHooks } from './firebase_export';
 
 /**
  * Captures all existence filter mismatches in the Watch 'Listen' stream that
@@ -32,16 +28,28 @@ export async function captureExistenceFilterMismatches(
   callback: () => Promise<void>
 ): Promise<ExistenceFilterMismatchInfo[]> {
   const results: ExistenceFilterMismatchInfo[] = [];
-  const callbackWrapper = (arg: unknown): void => {
-    const existenceFilterMismatchInfo = existenceFilterMismatchInfoFromRaw(
-      arg as RawExistenceFilterMismatchInfo
-    );
-    results.push(existenceFilterMismatchInfo);
-  };
+  const onExistenceFilterMismatchCallback = (
+    actualCount: number,
+    expectedCount: number,
+    bloomFilterSentFromWatch: boolean,
+    bloomFilterApplied: boolean,
+    bloomFilterHashCount: number,
+    bloomFilterBitmapLength: number,
+    bloomFilterPadding: number
+  ) =>
+    results.push({
+      actualCount,
+      expectedCount,
+      bloomFilterSentFromWatch,
+      bloomFilterApplied,
+      bloomFilterHashCount,
+      bloomFilterBitmapLength,
+      bloomFilterPadding
+    });
 
   const unregister =
     TestingHooks.getOrCreateInstance().onExistenceFilterMismatch(
-      callbackWrapper
+      onExistenceFilterMismatchCallback
     );
 
   try {
@@ -56,64 +64,16 @@ export async function captureExistenceFilterMismatches(
 /**
  * Information about an existence filter mismatch, capturing during an
  * invocation of `captureExistenceFilterMismatches()`.
+ *
+ * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()`
+ * for the meaning of these values.
  */
 export interface ExistenceFilterMismatchInfo {
-  /** The number of documents that match the query in the local cache. */
   actualCount: number;
-  /** The number of documents that matched the query on the server. */
   expectedCount: number;
-  /** The bloom filter provided by the server, or null if not provided. */
-  bloomFilter: ExistenceFilterBloomFilter | null;
-}
-
-/**
- * Information about a bloom filter in an existence filter.
- */
-export interface ExistenceFilterBloomFilter {
-  /** Whether the bloom filter was used to avert a full requery. */
-  applied: boolean;
-  /** The number of hash functions used in the bloom filter. */
-  hashCount: number;
-  /** The number of bytes in the bloom filter. */
-  bitmapLength: number;
-  /** The number of bits of "padding" in the last byte of the bloom filter. */
-  padding: number;
-}
-
-/**
- * Creates a `ExistenceFilterMismatchInfo` object from the raw object given
- * by `TestingUtils`.
- */
-function existenceFilterMismatchInfoFromRaw(
-  raw: RawExistenceFilterMismatchInfo
-): ExistenceFilterMismatchInfo {
-  _logWarn(
-    'zzyzx existenceFilterMismatchInfoFromRaw() start; raw:',
-    JSON.stringify(raw, null, 2)
-  );
-  return {
-    actualCount: raw.actualCount,
-    expectedCount: raw.change.existenceFilter.count,
-    bloomFilter: bloomFilterFromRawExistenceFilterMismatchInfo(raw)
-  };
-}
-
-/**
- * Creates an `ExistenceFilterBloomFilter` object from the raw object given
- * by `TestingUtils`, returning null if the given object does not define a
- * bloom filter.
- */
-function bloomFilterFromRawExistenceFilterMismatchInfo(
-  raw: RawExistenceFilterMismatchInfo
-): null | ExistenceFilterBloomFilter {
-  const unchangedNames = raw.change.existenceFilter?.unchangedNames;
-  if (!unchangedNames) {
-    return null;
-  }
-  return {
-    applied: raw.bloomFilterApplied,
-    hashCount: unchangedNames.hashCount ?? 0,
-    bitmapLength: unchangedNames.bits?.bitmap?.length ?? 0,
-    padding: unchangedNames.bits?.padding ?? 0
-  };
+  bloomFilterSentFromWatch: boolean;
+  bloomFilterApplied: boolean;
+  bloomFilterHashCount: number;
+  bloomFilterBitmapLength: number;
+  bloomFilterPadding: number;
 }
