From 86870f97e0a7df0fc0076a387b6ee15dd4dc6c92 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Mar 2023 22:32:18 -0500 Subject: [PATCH 01/30] update the test case to verify that the existence filter contains a bloom filter without a false positive --- packages/firestore/src/api.ts | 1 + packages/firestore/src/remote/watch_change.ts | 6 + packages/firestore/src/util/testing_hooks.ts | 88 ++++++++++++++ .../test/integration/api/query.test.ts | 71 +++++++++-- .../test/integration/util/settings.ts | 4 +- .../integration/util/testing_hooks_util.ts | 114 ++++++++++++++++++ 6 files changed, 269 insertions(+), 15 deletions(-) create mode 100644 packages/firestore/src/util/testing_hooks.ts create mode 100644 packages/firestore/test/integration/util/testing_hooks_util.ts diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 93da0b1ee08..42e039d1057 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -196,3 +196,4 @@ 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 } from './util/testing_hooks'; diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 82ef17213dd..34e925a12a9 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -37,6 +37,7 @@ import { logDebug, logWarn } from '../util/log'; import { primitiveComparator } from '../util/misc'; import { SortedMap } from '../util/sorted_map'; import { SortedSet } from '../util/sorted_set'; +import { TestingHooks } from '../util/testing_hooks'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; @@ -432,6 +433,11 @@ export class WatchChangeAggregator { this.resetTarget(targetId); this.pendingTargetResets = this.pendingTargetResets.add(targetId); } + TestingHooks.instance?.notifyOnExistenceFilterMismatch({ + actualCount: currentSize, + change: watchChange, + bloomFilterApplied: bloomFilterApplied + }); } } } diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts new file mode 100644 index 00000000000..ded724a6bc7 --- /dev/null +++ b/packages/firestore/src/util/testing_hooks.ts @@ -0,0 +1,88 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +let gTestingHooksSingletonInstance: TestingHooks | null = null; + +/** + * Manages "testing hooks", hooks into the internals of the SDK to verify + * internal state and events during integration tests. Do not use this class + * except for testing purposes. + * + * There are two ways to retrieve an instance of this class: + * 1. The `instance` property, which returns null if the global singleton + * instance has not been created. + * 2. The `getOrCreateInstance()` method, which creates the global singleton + * instance if it has not been created. + * + * Use the former method if the caller should "do nothing" there are no testing + * hooks registered. Use the latter if the instance is needed to, for example, + * register a testing hook. + */ +export class TestingHooks { + private readonly onExistenceFilterMismatchCallbacks = new Array< + (arg: unknown) => void + >(); + + private constructor() {} + + /** + * Returns the singleton instance of this class, or null if it has not been + * initialized. + */ + static get instance(): TestingHooks | null { + return gTestingHooksSingletonInstance; + } + + /** + * Returns the singleton instance of this class, creating it if is has never + * been created before. + */ + static getOrCreateInstance(): TestingHooks { + if (gTestingHooksSingletonInstance === null) { + gTestingHooksSingletonInstance = new TestingHooks(); + } + return gTestingHooksSingletonInstance; + } + + /** + * Registers a callback to be notified when an existence filter mismatch + * occurs in the Watch listen stream. + * @param callback the callback to invoke upon existence filter mismatch. + * @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: (arg: unknown) => void): () => void { + this.onExistenceFilterMismatchCallbacks.push(callback); + + let removed = false; + return () => { + if (!removed) { + const index = this.onExistenceFilterMismatchCallbacks.indexOf(callback); + this.onExistenceFilterMismatchCallbacks.splice(index, 1); + removed = true; + } + }; + } + + /** + * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. + */ + notifyOnExistenceFilterMismatch(arg: unknown): void { + this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(arg)); + } +} diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 2438d8bdb6c..fd30eb2ff1e 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -65,7 +65,8 @@ import { withTestCollection, withTestDb } from '../util/helpers'; -import { USE_EMULATOR } from '../util/settings'; +import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; +import { USE_EMULATOR, TARGET_BACKEND } from '../util/settings'; apiDescribe('Queries', (persistence: boolean) => { addEqualityMatcher(); @@ -2061,34 +2062,78 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - // TODO(Mila): Skip the test when using emulator as there is a bug related to - // sending existence filter in response: b/270731363. Remove the condition - // here once the bug is resolved. - // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR ? it.skip : it)( - 'resuming a query should remove deleted documents indicated by existence filter', + // TODO(b/270731363): Re-enable this test to run against the Firestore + // emulator once the bug where an existence filter fails to be sent when a + // query is resumed is fixed. + (USE_EMULATOR || !persistence ? it.skip : it.only)( + 'resuming a query should use bloom filter to avoid full requery', () => { + // TODO(dconeybe) Add a loop to retry 2 or 3 times if the bloom filter + // experienced a false positive. Since there is a non-zero chance of a + // false positive, and the chance of experiencing two in a row is + // negligible, retrying 2 or 3 times should result in basically zero + // false positives. + + // Create 100 documents in a new collection. const testDocs: { [key: string]: object } = {}; for (let i = 1; i <= 100; i++) { testDocs['doc' + i] = { key: i }; } + return withTestCollection(persistence, testDocs, async (coll, db) => { + // 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); - // Delete 50 docs in transaction so that it doesn't affect local cache. + + // Delete 50 of the 100 documents. Do this in a transaction, rather than + // deleteDoc(), to avoid affecting the local cache. await runTransaction(db, async txn => { for (let i = 1; i <= 50; i++) { txn.delete(doc(coll, 'doc' + i)); } }); - // Wait 10 seconds, during which Watch will stop tracking the query - // and will send an existence filter rather than "delete" events. + + // Wait for 10 seconds, during which Watch will stop tracking the query + // and will send an existence filter rather than "delete" events when + // the query is resumed. await new Promise(resolve => setTimeout(resolve, 10000)); - const snapshot2 = await getDocs(coll); - expect(snapshot2.size).to.equal(50); + + // Resume the query and expect to get a snapshot with the 50 remaining + // documents. Use some internal testing hooks to "capture" the existence + // filter mismatches to later verify that Watch sent a bloom filter and + // it was used to void the full requery. + const existenceFilterMismatches = + await captureExistenceFilterMismatches(async () => { + const snapshot2 = await getDocs(coll); + expect(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 was + // successfully used to avoid a full requery. + // TODO(b/NNNNNNNN) Replace this "if" condition with !USE_EMULATOR once + // the feature has been deployed to production. Note that there are no + // plans to add bloom filter support to 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(bloomFilter.applied).to.equal(true); + 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); + } }); } - ).timeout('20s'); + ).timeout('30s'); }); function verifyDocumentChange( diff --git a/packages/firestore/test/integration/util/settings.ts b/packages/firestore/test/integration/util/settings.ts index 521ed30870f..07a02ff367d 100644 --- a/packages/firestore/test/integration/util/settings.ts +++ b/packages/firestore/test/integration/util/settings.ts @@ -25,7 +25,7 @@ import { PrivateSettings } from './firebase_export'; // eslint-disable-next-line @typescript-eslint/no-explicit-any declare const __karma__: any; -enum TargetBackend { +export enum TargetBackend { EMULATOR = 'emulator', QA = 'qa', NIGHTLY = 'nightly', @@ -35,7 +35,7 @@ enum TargetBackend { // eslint-disable-next-line @typescript-eslint/no-require-imports const PROJECT_CONFIG = require('../../../../../config/project.json'); -const TARGET_BACKEND: TargetBackend = getTargetBackend(); +export const TARGET_BACKEND: TargetBackend = getTargetBackend(); export const USE_EMULATOR: boolean = TARGET_BACKEND === TargetBackend.EMULATOR; diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts new file mode 100644 index 00000000000..40bf232a019 --- /dev/null +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -0,0 +1,114 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { _TestingHooks as TestingHooks } from './firebase_export'; + +/** + * Captures all existence filter mismatches that occur during the execution of + * the given code block. + * @param callback The callback to invoke, during whose invocation the existence + * filter mismatches will be captured. + * @return the captured existence filter mismatches. + */ +export async function captureExistenceFilterMismatches( + callback: () => Promise +): Promise> { + const results: Array = []; + const callbackWrapper = (arg: unknown) => + results.push( + existenceFilterMismatchInfoFromRaw(arg as RawExistenceFilterMismatchInfo) + ); + const unregister = + TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( + callbackWrapper + ); + try { + await callback(); + } finally { + unregister(); + } + return results; +} + +/** + * The shape of the object specified to `onExistenceFilterMismatch` callbacks. + */ +interface RawExistenceFilterMismatchInfo { + actualCount: number; + bloomFilterApplied: boolean; + change: { + targetId: number; + existenceFilter: { + count: number; + unchangedNames?: { + bits?: { + bitmap?: string | Uint8Array; + padding?: number; + }; + hashCount?: number; + }; + }; + }; +} + +/** + * Information about an existence filter mismatch. + */ +export interface ExistenceFilterMismatchInfo { + actualCount: number; + expectedCount: number; + bloomFilter?: ExistenceFilterBloomFilter; +} + +/** + * Information about a bloom filter in an existence filter. + */ +export interface ExistenceFilterBloomFilter { + applied: boolean; + hashCount: number; + bitmapLength: number; + padding: number; +} + +function existenceFilterMismatchInfoFromRaw( + raw: RawExistenceFilterMismatchInfo +): ExistenceFilterMismatchInfo { + const result: ExistenceFilterMismatchInfo = { + actualCount: raw.actualCount, + expectedCount: raw.change.existenceFilter.count + }; + const bloomFilter = bloomFilterFromRawExistenceFilterMismatchInfo(raw); + if (bloomFilter) { + result.bloomFilter = bloomFilter; + } + return result; +} + +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 + }; +} From f75534f37720ae974260a56411bf7ae3dbb9a5e8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Mar 2023 22:56:45 -0500 Subject: [PATCH 02/30] add retry --- .../test/integration/api/query.test.ts | 139 +++++++++++------- .../test/integration/util/helpers.ts | 20 +-- 2 files changed, 92 insertions(+), 67 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index fd30eb2ff1e..d81d4c907bf 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2067,73 +2067,98 @@ apiDescribe('Queries', (persistence: boolean) => { // query is resumed is fixed. (USE_EMULATOR || !persistence ? it.skip : it.only)( 'resuming a query should use bloom filter to avoid full requery', - () => { - // TODO(dconeybe) Add a loop to retry 2 or 3 times if the bloom filter - // experienced a false positive. Since there is a non-zero chance of a - // false positive, and the chance of experiencing two in a row is - // negligible, retrying 2 or 3 times should result in basically zero - // false positives. - + async () => { // Create 100 documents in a new collection. const testDocs: { [key: string]: object } = {}; for (let i = 1; i <= 100; i++) { testDocs['doc' + i] = { key: i }; } - return withTestCollection(persistence, testDocs, async (coll, db) => { - // 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); - - // Delete 50 of the 100 documents. Do this in a transaction, rather than - // deleteDoc(), to avoid affecting the local cache. - await runTransaction(db, async txn => { - for (let i = 1; i <= 50; i++) { - txn.delete(doc(coll, 'doc' + i)); + let attemptNumber = 0; + while (true) { + attemptNumber++; + const bloomFilterApplied = await withTestCollection( + persistence, + testDocs, + async (coll, db) => { + // 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); + + // Delete 50 of the 100 documents. Do this in a transaction, rather + // than deleteDoc(), to avoid affecting the local cache. + await runTransaction(db, async txn => { + for (let i = 1; i <= 50; i++) { + txn.delete(doc(coll, 'doc' + i)); + } + }); + + // Wait for 10 seconds, during which Watch will stop tracking the + // query and will send an existence filter rather than "delete" events + // when the query is resumed. + await new Promise(resolve => setTimeout(resolve, 10000)); + + // Resume the query and expect to get a snapshot with the 50 remaining + // documents. Use some internal testing hooks to "capture" the + // existence filter mismatches to later verify that Watch sent a bloom + // filter, and it was used to void the full requery. + const existenceFilterMismatches = + await captureExistenceFilterMismatches(async () => { + const snapshot2 = await getDocs(coll); + expect(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 was + // successfully used to avoid a full requery. + // TODO(b/NNNNNNNN) 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. + 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'); + } + + // 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 false; + } 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); + } + + return true; } - }); + ); - // Wait for 10 seconds, during which Watch will stop tracking the query - // and will send an existence filter rather than "delete" events when - // the query is resumed. - await new Promise(resolve => setTimeout(resolve, 10000)); - - // Resume the query and expect to get a snapshot with the 50 remaining - // documents. Use some internal testing hooks to "capture" the existence - // filter mismatches to later verify that Watch sent a bloom filter and - // it was used to void the full requery. - const existenceFilterMismatches = - await captureExistenceFilterMismatches(async () => { - const snapshot2 = await getDocs(coll); - expect(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 was - // successfully used to avoid a full requery. - // TODO(b/NNNNNNNN) Replace this "if" condition with !USE_EMULATOR once - // the feature has been deployed to production. Note that there are no - // plans to add bloom filter support to 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(bloomFilter.applied).to.equal(true); - 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); + if (bloomFilterApplied) { + break; } - }); + } } - ).timeout('30s'); + ).timeout('90s'); }); function verifyDocumentChange( diff --git a/packages/firestore/test/integration/util/helpers.ts b/packages/firestore/test/integration/util/helpers.ts index 79dbacaafa0..b70a116e839 100644 --- a/packages/firestore/test/integration/util/helpers.ts +++ b/packages/firestore/test/integration/util/helpers.ts @@ -170,13 +170,13 @@ export function withTestDbs( fn ); } -export async function withTestDbsSettings( +export async function withTestDbsSettings( persistence: boolean, projectId: string, settings: PrivateSettings, numDbs: number, - fn: (db: Firestore[]) => Promise -): Promise { + fn: (db: Firestore[]) => Promise +): Promise { if (numDbs === 0) { throw new Error("Can't test with no databases"); } @@ -192,7 +192,7 @@ export async function withTestDbsSettings( } try { - await fn(dbs); + return await fn(dbs); } finally { for (const db of dbs) { await terminate(db); @@ -282,11 +282,11 @@ export function withTestDocAndInitialData( }); } -export function withTestCollection( +export function withTestCollection( persistence: boolean, docs: { [key: string]: DocumentData }, - fn: (collection: CollectionReference, db: Firestore) => Promise -): Promise { + fn: (collection: CollectionReference, db: Firestore) => Promise +): Promise { return withTestCollectionSettings(persistence, DEFAULT_SETTINGS, docs, fn); } @@ -299,12 +299,12 @@ export function withEmptyTestCollection( // TODO(mikelehen): Once we wipe the database between tests, we can probably // return the same collection every time. -export function withTestCollectionSettings( +export function withTestCollectionSettings( persistence: boolean, settings: PrivateSettings, docs: { [key: string]: DocumentData }, - fn: (collection: CollectionReference, db: Firestore) => Promise -): Promise { + fn: (collection: CollectionReference, db: Firestore) => Promise +): Promise { return withTestDbsSettings( persistence, DEFAULT_PROJECT_ID, From dc422283e6b7c49772ead0afa811a8ba28d36f31 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 6 Mar 2023 23:05:17 -0500 Subject: [PATCH 03/30] fix lint errors --- packages/firestore/src/remote/watch_change.ts | 2 +- packages/firestore/src/util/testing_hooks.ts | 4 ++-- packages/firestore/test/integration/api/query.test.ts | 3 ++- .../firestore/test/integration/util/testing_hooks_util.ts | 4 ++-- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 34e925a12a9..f5037535e54 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -436,7 +436,7 @@ export class WatchChangeAggregator { TestingHooks.instance?.notifyOnExistenceFilterMismatch({ actualCount: currentSize, change: watchChange, - bloomFilterApplied: bloomFilterApplied + bloomFilterApplied }); } } diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index ded724a6bc7..f7b44ba6055 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -33,9 +33,9 @@ let gTestingHooksSingletonInstance: TestingHooks | null = null; * register a testing hook. */ export class TestingHooks { - private readonly onExistenceFilterMismatchCallbacks = new Array< + private readonly onExistenceFilterMismatchCallbacks: Array< (arg: unknown) => void - >(); + > = []; private constructor() {} diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index d81d4c907bf..0cd25204195 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2065,7 +2065,8 @@ apiDescribe('Queries', (persistence: boolean) => { // TODO(b/270731363): Re-enable this test to run against the Firestore // emulator once the bug where an existence filter fails to be sent when a // query is resumed is fixed. - (USE_EMULATOR || !persistence ? it.skip : it.only)( + // eslint-disable-next-line no-restricted-properties + (USE_EMULATOR || !persistence ? it.skip : it)( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 40bf232a019..2b8bc8f541e 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -26,8 +26,8 @@ import { _TestingHooks as TestingHooks } from './firebase_export'; */ export async function captureExistenceFilterMismatches( callback: () => Promise -): Promise> { - const results: Array = []; +): Promise { + const results: ExistenceFilterMismatchInfo[] = []; const callbackWrapper = (arg: unknown) => results.push( existenceFilterMismatchInfoFromRaw(arg as RawExistenceFilterMismatchInfo) From c1bff43a8277a96fa7fb92ac68cbdd42dc0d83bf Mon Sep 17 00:00:00 2001 From: dconeybe Date: Tue, 7 Mar 2023 04:23:42 +0000 Subject: [PATCH 04/30] Update API reports --- common/api-review/firestore.api.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 077e0e4426f..c67fc78ecf5 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -556,6 +556,14 @@ export type TaskState = 'Error' | 'Running' | 'Success'; // @public export function terminate(firestore: Firestore): Promise; +// @public +export class _TestingHooks { + static getOrCreateInstance(): _TestingHooks; + static get instance(): _TestingHooks | null; + notifyOnExistenceFilterMismatch(arg: unknown): void; + onExistenceFilterMismatch(callback: (arg: unknown) => void): () => void; + } + // @public export class Timestamp { constructor( From 17535491cc024b78fe99bafe8220984cc4a7baf3 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 00:14:17 -0500 Subject: [PATCH 05/30] testing_hooks.ts: add @internal tag to TestingHooks class so it doesn't appear in common/api-review/firestore.api.md. Also do a tiny bit of cosmetic changes. --- common/api-review/firestore.api.md | 8 -------- packages/firestore/src/util/testing_hooks.ts | 11 +++++++++-- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index c67fc78ecf5..077e0e4426f 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -556,14 +556,6 @@ export type TaskState = 'Error' | 'Running' | 'Success'; // @public export function terminate(firestore: Firestore): Promise; -// @public -export class _TestingHooks { - static getOrCreateInstance(): _TestingHooks; - static get instance(): _TestingHooks | null; - notifyOnExistenceFilterMismatch(arg: unknown): void; - onExistenceFilterMismatch(callback: (arg: unknown) => void): () => void; - } - // @public export class Timestamp { constructor( diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index f7b44ba6055..3de3ff04531 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -15,8 +15,6 @@ * limitations under the License. */ -let gTestingHooksSingletonInstance: TestingHooks | null = null; - /** * Manages "testing hooks", hooks into the internals of the SDK to verify * internal state and events during integration tests. Do not use this class @@ -31,6 +29,8 @@ let gTestingHooksSingletonInstance: TestingHooks | null = null; * Use the former method if the caller should "do nothing" there are no testing * hooks registered. Use the latter if the instance is needed to, for example, * register a testing hook. + * + * @internal */ export class TestingHooks { private readonly onExistenceFilterMismatchCallbacks: Array< @@ -81,8 +81,15 @@ export class TestingHooks { /** * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. + * @param arg the argument to specify to the callbacks; the type of this + * argument is intentionally declared as `unknown` to discourage casual use; + * the specific use of this callback in tests knows the structure of the + * given argument and will use it accordingly. */ notifyOnExistenceFilterMismatch(arg: unknown): void { this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(arg)); } } + +/** The global singleton instance of `TestingHooks`. */ +let gTestingHooksSingletonInstance: TestingHooks | null = null; From 50418efa3672272d972f1a693b63ddfc6e3e4521 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 00:40:51 -0500 Subject: [PATCH 06/30] Add docs --- packages/firestore/src/util/testing_hooks.ts | 46 +++++++++---------- .../integration/util/testing_hooks_util.ts | 45 ++++++++++++------ 2 files changed, 54 insertions(+), 37 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 3de3ff04531..03a2bcb9ae0 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -20,20 +20,20 @@ * internal state and events during integration tests. Do not use this class * except for testing purposes. * - * There are two ways to retrieve an instance of this class: + * There are two ways to retrieve the global singleton instance of this class: * 1. The `instance` property, which returns null if the global singleton - * instance has not been created. + * instance has not been created. Use this property if the caller should + * "do nothing" if there are no testing hooks registered, such as when + * delivering an event to notify registered callbacks. * 2. The `getOrCreateInstance()` method, which creates the global singleton - * instance if it has not been created. - * - * Use the former method if the caller should "do nothing" there are no testing - * hooks registered. Use the latter if the instance is needed to, for example, - * register a testing hook. + * instance if it has not been created. Use this method if the instance is + * needed to, for example, register a callback. * * @internal */ export class TestingHooks { - private readonly onExistenceFilterMismatchCallbacks: Array< + private readonly onExistenceFilterMismatchCallbacks: Map< + Symbol, (arg: unknown) => void > = []; @@ -61,30 +61,30 @@ export class TestingHooks { /** * Registers a callback to be notified when an existence filter mismatch * occurs in the Watch listen stream. - * @param callback the callback to invoke upon existence filter mismatch. + * + * 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. The + * type of the argument to this callback is intentionally declared as + * `unknown`, rather than something more specific, to discourage its use + * unless you really, really know what you are doing. If you know what you are + * doing then you know the type and how to interpret it. + * * @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: (arg: unknown) => void): () => void { - this.onExistenceFilterMismatchCallbacks.push(callback); - - let removed = false; - return () => { - if (!removed) { - const index = this.onExistenceFilterMismatchCallbacks.indexOf(callback); - this.onExistenceFilterMismatchCallbacks.splice(index, 1); - removed = true; - } - }; + const key = Symbol(); + this.onExistenceFilterMismatchCallbacks.set(key, callback); + return () => this.onExistenceFilterMismatchCallbacks.delete(key); } /** * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. - * @param arg the argument to specify to the callbacks; the type of this - * argument is intentionally declared as `unknown` to discourage casual use; - * the specific use of this callback in tests knows the structure of the - * given argument and will use it accordingly. + * @param arg the argument to specify to the callbacks; see the documentation + * for `onExistenceFilterMismatch()` for details. */ notifyOnExistenceFilterMismatch(arg: unknown): void { this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(arg)); diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 2b8bc8f541e..5b074a9318b 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -18,10 +18,10 @@ import { _TestingHooks as TestingHooks } from './firebase_export'; /** - * Captures all existence filter mismatches that occur during the execution of - * the given code block. - * @param callback The callback to invoke, during whose invocation the existence - * filter mismatches will be captured. + * Captures all existence filter mismatches in the Watch 'Listen' stream that + * occur during the execution of the given code block. + * @param callback The callback to invoke; during the invocation of this + * callback all existence filter mismatches will be captured. * @return the captured existence filter mismatches. */ export async function captureExistenceFilterMismatches( @@ -32,20 +32,24 @@ export async function captureExistenceFilterMismatches( results.push( existenceFilterMismatchInfoFromRaw(arg as RawExistenceFilterMismatchInfo) ); + const unregister = TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( callbackWrapper ); + try { await callback(); } finally { unregister(); } + return results; } /** - * The shape of the object specified to `onExistenceFilterMismatch` callbacks. + * The shape of the object specified to + * `TestingUtils.onExistenceFilterMismatch()` callbacks. */ interface RawExistenceFilterMismatchInfo { actualCount: number; @@ -66,38 +70,51 @@ interface RawExistenceFilterMismatchInfo { } /** - * Information about an existence filter mismatch. + * Information about an existence filter mismatch, capturing during an + * invocation of `captureExistenceFilterMismatches()`. */ 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; - bloomFilter?: ExistenceFilterBloomFilter; + /** 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 able to be 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 { - const result: ExistenceFilterMismatchInfo = { + return { actualCount: raw.actualCount, - expectedCount: raw.change.existenceFilter.count + expectedCount: raw.change.existenceFilter.count, + bloomFilter: bloomFilterFromRawExistenceFilterMismatchInfo(raw) }; - const bloomFilter = bloomFilterFromRawExistenceFilterMismatchInfo(raw); - if (bloomFilter) { - result.bloomFilter = bloomFilter; - } - return result; } +/** + * Creates an `ExistenceFilterBloomFilter` object from the raw object given + * by `TestingUtils`, returning null if the given object does not defined a + * bloom filter. + */ function bloomFilterFromRawExistenceFilterMismatchInfo( raw: RawExistenceFilterMismatchInfo ): null | ExistenceFilterBloomFilter { From 01b3bf0123a181e39e37cb635b5886230dad9924 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 00:45:13 -0500 Subject: [PATCH 07/30] fix lint errors --- packages/firestore/test/integration/api/query.test.ts | 2 +- .../firestore/test/integration/util/testing_hooks_util.ts | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 0cd25204195..28327257a76 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -65,8 +65,8 @@ import { withTestCollection, withTestDb } from '../util/helpers'; -import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; import { USE_EMULATOR, TARGET_BACKEND } from '../util/settings'; +import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; apiDescribe('Queries', (persistence: boolean) => { addEqualityMatcher(); diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 5b074a9318b..fc4599e0ce4 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -28,10 +28,12 @@ export async function captureExistenceFilterMismatches( callback: () => Promise ): Promise { const results: ExistenceFilterMismatchInfo[] = []; - const callbackWrapper = (arg: unknown) => - results.push( - existenceFilterMismatchInfoFromRaw(arg as RawExistenceFilterMismatchInfo) + const callbackWrapper = (arg: unknown): void => { + const existenceFilterMismatchInfo = existenceFilterMismatchInfoFromRaw( + arg as RawExistenceFilterMismatchInfo ); + results.push(existenceFilterMismatchInfo); + }; const unregister = TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( From 384608cf39ebc17772ac49e79dd32bb5cea5e9d6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 01:10:15 -0500 Subject: [PATCH 08/30] cleanup --- packages/firestore/src/util/testing_hooks.ts | 4 +-- .../test/integration/api/query.test.ts | 30 ++++++++++--------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 03a2bcb9ae0..77cb08194f8 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -32,10 +32,10 @@ * @internal */ export class TestingHooks { - private readonly onExistenceFilterMismatchCallbacks: Map< + private readonly onExistenceFilterMismatchCallbacks = new Map< Symbol, (arg: unknown) => void - > = []; + >(); private constructor() {} diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 28327257a76..787a8607c61 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2077,13 +2077,14 @@ apiDescribe('Queries', (persistence: boolean) => { let attemptNumber = 0; while (true) { + type IterationResult = 'retry' | 'passed'; attemptNumber++; - const bloomFilterApplied = await withTestCollection( + const iterationResult = await withTestCollection( persistence, testDocs, async (coll, db) => { - // Run a query to populate the local cache with the 100 documents and - // a resume token. + // 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); @@ -2096,14 +2097,14 @@ apiDescribe('Queries', (persistence: boolean) => { }); // Wait for 10 seconds, during which Watch will stop tracking the - // query and will send an existence filter rather than "delete" events - // when the query is resumed. + // query and will send an existence filter rather than "delete" + // events when the query is resumed. await new Promise(resolve => setTimeout(resolve, 10000)); - // Resume the query and expect to get a snapshot with the 50 remaining - // documents. Use some internal testing hooks to "capture" the - // existence filter mismatches to later verify that Watch sent a bloom - // filter, and it was used to void the full requery. + // Resume the query and expect to get a snapshot with the 50 + // remaining documents. Use some internal testing hooks to "capture" + // the existence filter mismatches to later verify that Watch sent a + // bloom filter, and it was used to avert a full requery. const existenceFilterMismatches = await captureExistenceFilterMismatches(async () => { const snapshot2 = await getDocs(coll); @@ -2111,8 +2112,8 @@ apiDescribe('Queries', (persistence: boolean) => { }); // Verify that upon resuming the query that Watch sent an existence - // filter that included a bloom filter, and that that bloom filter was - // successfully used to avoid a full requery. + // filter that included a bloom filter, and that that bloom filter + // was successfully used to avoid a full requery. // TODO(b/NNNNNNNN) 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 @@ -2134,7 +2135,7 @@ apiDescribe('Queries', (persistence: boolean) => { // fail if the retry _also_ experiences a false positive. if (!bloomFilter.applied) { if (attemptNumber < 2) { - return false; + return 'retry'; } else { expect.fail( 'bloom filter false positive occurred ' + @@ -2150,11 +2151,12 @@ apiDescribe('Queries', (persistence: boolean) => { expect(bloomFilter.padding).to.be.below(8); } - return true; + return 'passed'; } ); - if (bloomFilterApplied) { + // Break out of the retry loop if the test passed. + if (iterationResult === 'passed') { break; } } From 67305eaf724cd629e0e8ba2dee60f8e071151b9d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 01:11:30 -0500 Subject: [PATCH 09/30] add a reference to b/271949433 in a comment --- packages/firestore/test/integration/api/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 787a8607c61..92919cd68ab 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2114,7 +2114,7 @@ apiDescribe('Queries', (persistence: boolean) => { // Verify that upon resuming the query that Watch sent an existence // filter that included a bloom filter, and that that bloom filter // was successfully used to avoid a full requery. - // TODO(b/NNNNNNNN) Replace this "if" condition with !USE_EMULATOR + // 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. From f9e070757c0bbc4378f96c397c8c0b5937e6a300 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 01:13:11 -0500 Subject: [PATCH 10/30] get rid of unnecssary 'type IterationResult' declaration --- packages/firestore/test/integration/api/query.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 92919cd68ab..e77f377e6d3 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2077,9 +2077,8 @@ apiDescribe('Queries', (persistence: boolean) => { let attemptNumber = 0; while (true) { - type IterationResult = 'retry' | 'passed'; attemptNumber++; - const iterationResult = await withTestCollection( + const iterationResult = await withTestCollection<'retry' | 'passed'>( persistence, testDocs, async (coll, db) => { From 9bfdcc2440890e7411db04b81d7a4026d24ee032 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 10:08:41 -0500 Subject: [PATCH 11/30] integration/firestore/gulpfile.js: add testing_hooks_util.ts to the list of copied files This may fix the following error: ``` [tsl] ERROR in /home/runner/work/firebase-js-sdk/firebase-js-sdk/integration/firestore/temp/test/integration/api/query.test.ts(76,50) TS2307: Cannot find module '../util/testing_hooks_util' or its corresponding type declarations. ``` https://github.com/firebase/firebase-js-sdk/actions/runs/4351236611/jobs/7602696515 --- integration/firestore/gulpfile.js | 1 + 1 file changed, 1 insertion(+) diff --git a/integration/firestore/gulpfile.js b/integration/firestore/gulpfile.js index 6e1cf0490c3..80c5b955504 100644 --- a/integration/firestore/gulpfile.js +++ b/integration/firestore/gulpfile.js @@ -45,6 +45,7 @@ function copyTests() { testBase + '/integration/util/events_accumulator.ts', testBase + '/integration/util/helpers.ts', testBase + '/integration/util/settings.ts', + testBase + '/integration/util/testing_hooks_util.ts', testBase + '/util/equality_matcher.ts', testBase + '/util/promise.ts' ], From 15d47bc1fc16a35213313f546bb403bc6bff4687 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 10:50:37 -0500 Subject: [PATCH 12/30] testing_hooks_util.ts: REVERT ME: add _logWarn() to existenceFilterMismatchInfoFromRaw() to help debug this error: ``` TypeError: Cannot read properties of undefined (reading 'existenceFilter') ``` https://github.com/firebase/firebase-js-sdk/actions/runs/4355588983/jobs/7612412525 --- .../firestore/test/integration/util/testing_hooks_util.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index fc4599e0ce4..b74932f8723 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { _TestingHooks as TestingHooks } from './firebase_export'; +import { _TestingHooks as TestingHooks, _logWarn } from './firebase_export'; /** * Captures all existence filter mismatches in the Watch 'Listen' stream that @@ -105,6 +105,10 @@ export interface ExistenceFilterBloomFilter { function existenceFilterMismatchInfoFromRaw( raw: RawExistenceFilterMismatchInfo ): ExistenceFilterMismatchInfo { + _logWarn( + 'zzyzx existenceFilterMismatchInfoFromRaw() start; raw:', + JSON.stringify(raw, null, 2) + ); return { actualCount: raw.actualCount, expectedCount: raw.change.existenceFilter.count, From 739f13d6b7384f2cbd4ce7378229c192a4bcb60c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 13:57:57 -0500 Subject: [PATCH 13/30] fix mangling of the property names of the argument to 'onExistenceFilterMismatch()' callbacks --- packages/firestore/src/api.ts | 5 ++- packages/firestore/src/util/testing_hooks.ts | 41 ++++++++++++++----- .../integration/util/testing_hooks_util.ts | 32 ++++----------- 3 files changed, 41 insertions(+), 37 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 42e039d1057..def86b1a6bb 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -196,4 +196,7 @@ 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 } from './util/testing_hooks'; +export { + TestingHooks as _TestingHooks, + ExistenceFilterMismatchInfo as _TestingUtilsExistenceFilterMismatchInfo +} from './util/testing_hooks'; diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 77cb08194f8..d97d6f135a5 100644 --- 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, - (arg: unknown) => void + (info: ExistenceFilterMismatchInfo) => void >(); private constructor() {} @@ -65,17 +65,15 @@ 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. The - * type of the argument to this callback is intentionally declared as - * `unknown`, rather than something more specific, to discourage its use - * unless you really, really know what you are doing. If you know what you are - * doing then you know the type and how to interpret it. + * @param callback the callback to invoke upon existence filter mismatch. * * @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: (arg: unknown) => void): () => void { + onExistenceFilterMismatch( + callback: (info: ExistenceFilterMismatchInfo) => void + ): () => void { const key = Symbol(); this.onExistenceFilterMismatchCallbacks.set(key, callback); return () => this.onExistenceFilterMismatchCallbacks.delete(key); @@ -83,13 +81,34 @@ export class TestingHooks { /** * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. - * @param arg the argument to specify to the callbacks; see the documentation - * for `onExistenceFilterMismatch()` for details. + * @param info the argument to specify to the callbacks. */ - notifyOnExistenceFilterMismatch(arg: unknown): void { - this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(arg)); + notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { + this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info)); } } +/** + * The shape of the object specified to + * `TestingUtils.onExistenceFilterMismatch()` callbacks. + */ +export interface ExistenceFilterMismatchInfo { + actualCount: number; + bloomFilterApplied: boolean; + change: { + targetId: number; + existenceFilter: { + count: number; + unchangedNames?: { + bits?: { + bitmap?: string | Uint8Array; + padding?: number; + }; + hashCount?: number; + }; + }; + }; +} + /** The global singleton instance of `TestingHooks`. */ let gTestingHooksSingletonInstance: TestingHooks | null = null; diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index b74932f8723..1e3a3d99287 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,7 +15,11 @@ * limitations under the License. */ -import { _TestingHooks as TestingHooks, _logWarn } from './firebase_export'; +import { + _TestingHooks as TestingHooks, + _TestingUtilsExistenceFilterMismatchInfo as RawExistenceFilterMismatchInfo, + _logWarn +} from './firebase_export'; /** * Captures all existence filter mismatches in the Watch 'Listen' stream that @@ -49,28 +53,6 @@ export async function captureExistenceFilterMismatches( return results; } -/** - * The shape of the object specified to - * `TestingUtils.onExistenceFilterMismatch()` callbacks. - */ -interface RawExistenceFilterMismatchInfo { - actualCount: number; - bloomFilterApplied: boolean; - change: { - targetId: number; - existenceFilter: { - count: number; - unchangedNames?: { - bits?: { - bitmap?: string | Uint8Array; - padding?: number; - }; - hashCount?: number; - }; - }; - }; -} - /** * Information about an existence filter mismatch, capturing during an * invocation of `captureExistenceFilterMismatches()`. @@ -88,7 +70,7 @@ export interface ExistenceFilterMismatchInfo { * Information about a bloom filter in an existence filter. */ export interface ExistenceFilterBloomFilter { - /** Whether the bloom filter was able to be used to avert a full requery. */ + /** 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; @@ -118,7 +100,7 @@ function existenceFilterMismatchInfoFromRaw( /** * Creates an `ExistenceFilterBloomFilter` object from the raw object given - * by `TestingUtils`, returning null if the given object does not defined a + * by `TestingUtils`, returning null if the given object does not define a * bloom filter. */ function bloomFilterFromRawExistenceFilterMismatchInfo( From af53f1a748cf824ec4d06b8fe9d80a2a18ec0d39 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 7 Mar 2023 14:41:38 -0500 Subject: [PATCH 14/30] testing_hooks.ts: add @internal tag to ExistenceFilterMismatchInfo --- packages/firestore/src/util/testing_hooks.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index d97d6f135a5..bd4a46f4f83 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -91,6 +91,8 @@ export class TestingHooks { /** * The shape of the object specified to * `TestingUtils.onExistenceFilterMismatch()` callbacks. + * + * @internal */ export interface ExistenceFilterMismatchInfo { actualCount: number; From 85ed7ab0ca024b7aa5f1b7edb60d0a13d31df0b3 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 11:16:40 -0500 Subject: [PATCH 15/30] avoid using objects for parameters since their names can get mangled and/or excluded from minified code --- packages/firestore/src/api.ts | 6 +- packages/firestore/src/remote/watch_change.ts | 32 ++++++- packages/firestore/src/util/testing_hooks.ts | 81 +++++++++++----- .../test/integration/api/query.test.ts | 75 ++++++++------- .../integration/util/testing_hooks_util.ts | 96 ++++++------------- 5 files changed, 155 insertions(+), 135 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index def86b1a6bb..7b622a5af47 100644 --- 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 index f5037535e54..a8f1dbd3487 100644 --- 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> { function snapshotChangesMap(): SortedMap { return new SortedMap(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 index bd4a46f4f83..9a0ec0a3cb0 100644 --- 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 index e77f377e6d3..bba71e43e6f 100644 --- 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 index 1e3a3d99287..b1da1341fb5 100644 --- 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 ): Promise { 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; } From 0b9aaf13d660007d8db8362309f30edd4266d109 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 13:38:53 -0500 Subject: [PATCH 16/30] try hand-creating the ExistenceFilterMismatchInfo to see if it survives minification --- packages/firestore/src/api.ts | 1 - packages/firestore/src/remote/watch_change.ts | 32 ++++--- packages/firestore/src/util/testing_hooks.ts | 95 +++++++++---------- .../test/integration/api/query.test.ts | 45 +++++---- .../integration/util/testing_hooks_util.ts | 35 +++---- 5 files changed, 96 insertions(+), 112 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 7b622a5af47..42e039d1057 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -197,4 +197,3 @@ 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 } 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 index a8f1dbd3487..4ea63589da9 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -37,7 +37,10 @@ import { logDebug, logWarn } from '../util/log'; import { primitiveComparator } from '../util/misc'; import { SortedMap } from '../util/sorted_map'; import { SortedSet } from '../util/sorted_set'; -import { TestingHooks } from '../util/testing_hooks'; +import { + ExistenceFilterMismatchInfo, + TestingHooks +} from '../util/testing_hooks'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; @@ -801,19 +804,22 @@ function notifyTestingHooksOnExistenceFilterMismatch( actualCount: number, existenceFilter: ExistenceFilter ): void { - const expectedCount = existenceFilter.count; + const existenceFilterMismatchInfo: ExistenceFilterMismatchInfo = { + actualCount, + 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; + if (unchangedNames) { + existenceFilterMismatchInfo.bloomFilter = { + applied: bloomFilterApplied, + hashCount: unchangedNames?.hashCount ?? 0, + bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, + padding: unchangedNames?.bits?.padding ?? 0 + }; + } + TestingHooks.instance?.notifyOnExistenceFilterMismatch( - actualCount, - expectedCount, - bloomFilterSentFromWatch, - bloomFilterApplied, - bloomFilterHashCount, - bloomFilterBitmapLength, - bloomFilterPadding + existenceFilterMismatchInfo ); } diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 9a0ec0a3cb0..c3c351be150 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -65,9 +65,7 @@ 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; see - * the documentation for `notifyOnExistenceFilterMismatch()` for the meaning - * of these arguments. + * @param callback the callback to invoke upon existence filter mismatch. * * @return a function that, when called, unregisters the given callback; only * the first invocation of the returned function does anything; all subsequent @@ -83,64 +81,57 @@ export class TestingHooks { /** * Invokes all currently-registered `onExistenceFilterMismatch` 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. + * @param info Information about the existence filter mismatch. */ - 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 - ); - }); + notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { + this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info)); } } /** * The signature of callbacks registered with * `TestingUtils.onExistenceFilterMismatch()`. - * - * @internal + */ +export interface ExistenceFilterMismatchInfo { + /** The number of documents that matched the query in the local cache. */ + actualCount: number; + + /** + * The number of documents that matched the query on the server, as specified + * in the ExistenceFilter message's `count` field. + */ + expectedCount: number; + + /** + * Information about the bloom filter provided by Watch in the ExistenceFilter + * message's `unchangedNames` field. If this property is omitted or undefined + * then that means that Watch did _not_ provide a bloom filter. + */ + bloomFilter?: { + /** + * Whether a full requery was averted by using the bloom filter. If false, + * then something happened, such as a false positive, to prevent using the + * bloom filter to avoid 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's bitmask. */ + bitmapLength: number; + + /** The number of bits of padding in the last byte of the bloom filter. */ + padding: number; + }; +} + +/** + * The signature of callbacks registered with + * `TestingUtils.onExistenceFilterMismatch()`. */ export type ExistenceFilterMismatchCallback = ( - actualCount: number, - expectedCount: number, - bloomFilterSentFromWatch: boolean, - bloomFilterApplied: boolean, - bloomFilterHashCount: number, - bloomFilterBitmapLength: number, - bloomFilterPadding: number + info: ExistenceFilterMismatchInfo ) => void; /** The global singleton instance of `TestingHooks`. */ diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index bba71e43e6f..c7f36542274 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2122,20 +2122,29 @@ apiDescribe('Queries', (persistence: boolean) => { existenceFilterMismatches, 'existenceFilterMismatches' ).to.have.length(1); - const { - actualCount, - expectedCount, - bloomFilterSentFromWatch, - bloomFilterApplied, - bloomFilterHashCount, - bloomFilterBitmapLength, - bloomFilterPadding - } = existenceFilterMismatches[0]; + const { actualCount, expectedCount, bloomFilter } = + existenceFilterMismatches[0]; expect(actualCount, 'actualCount').to.equal(100); expect(expectedCount, 'expectedCount').to.equal(50); - expect(bloomFilterSentFromWatch, 'bloomFilterSentFromWatch').to.be - .true; + if (!bloomFilter) { + expect.fail( + 'The existence filter should have specified ' + + 'a bloom filter in its `unchanged_names` field.' + ); + throw new Error('should never get here'); + } + + expect( + bloomFilter.hashCount, + 'bloomFilter.hashCount' + ).to.be.above(0); + expect( + bloomFilter.bitmapLength, + 'bloomFilter.bitmapLength' + ).to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8); // Retry the entire test if a bloom filter false positive occurs. // Although statistically rare, false positives are expected to @@ -2143,20 +2152,10 @@ apiDescribe('Queries', (persistence: boolean) => { // 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) { + if (attemptNumber > 1 && !bloomFilter.applied) { return 'retry'; } - - 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); + expect(bloomFilter.applied, 'bloomFilter.applied').to.be.true; } return 'passed'; diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index b1da1341fb5..911da358518 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { _TestingHooks as TestingHooks } from './firebase_export'; +import { _TestingHooks as TestingHooks, _logWarn } from './firebase_export'; /** * Captures all existence filter mismatches in the Watch 'Listen' stream that @@ -29,23 +29,11 @@ export async function captureExistenceFilterMismatches( ): Promise { const results: 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 - }); + info: ExistenceFilterMismatchInfo + ) => { + _logWarn('zzyzx onExistenceFilterMismatchCallback', info); + results.push(info); + }; const unregister = TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( @@ -71,9 +59,10 @@ export async function captureExistenceFilterMismatches( export interface ExistenceFilterMismatchInfo { actualCount: number; expectedCount: number; - bloomFilterSentFromWatch: boolean; - bloomFilterApplied: boolean; - bloomFilterHashCount: number; - bloomFilterBitmapLength: number; - bloomFilterPadding: number; + bloomFilter?: { + applied: boolean; + hashCount: number; + bitmapLength: number; + padding: number; + }; } From 266a7226e969f7fd18d4bb324bb6f23051c11d03 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 13:46:57 -0500 Subject: [PATCH 17/30] testing_hooks_util.ts: fix lint error --- packages/firestore/test/integration/util/testing_hooks_util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 911da358518..f273a40adf0 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -30,7 +30,7 @@ export async function captureExistenceFilterMismatches( const results: ExistenceFilterMismatchInfo[] = []; const onExistenceFilterMismatchCallback = ( info: ExistenceFilterMismatchInfo - ) => { + ): void => { _logWarn('zzyzx onExistenceFilterMismatchCallback', info); results.push(info); }; From c1437bc4dcf41307022b1ac82a023b62b47eac1e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 16:18:06 -0500 Subject: [PATCH 18/30] firestore-compat: fix typing error in fields.test.ts: TS2339: Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445 --- packages/firestore-compat/test/fields.test.ts | 2 +- packages/firestore-compat/test/util/firebase_export.ts | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/firestore-compat/test/fields.test.ts b/packages/firestore-compat/test/fields.test.ts index 6c72572d82d..c98b09fb865 100644 --- a/packages/firestore-compat/test/fields.test.ts +++ b/packages/firestore-compat/test/fields.test.ts @@ -387,7 +387,7 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { }); apiDescribe('`undefined` properties', (persistence: boolean) => { - const settings = { ...DEFAULT_SETTINGS }; + const settings: firebaseExport.Settings = { ...DEFAULT_SETTINGS }; settings.ignoreUndefinedProperties = true; it('are ignored in set()', () => { diff --git a/packages/firestore-compat/test/util/firebase_export.ts b/packages/firestore-compat/test/util/firebase_export.ts index acf33bda732..a8a8de31385 100644 --- a/packages/firestore-compat/test/util/firebase_export.ts +++ b/packages/firestore-compat/test/util/firebase_export.ts @@ -23,7 +23,7 @@ import firebase from '@firebase/app-compat'; import { FirebaseApp } from '@firebase/app-types'; import { GeoPoint, Timestamp } from '@firebase/firestore'; -import * as firestore from '@firebase/firestore-types'; +import { FirebaseFirestore, Settings } from '@firebase/firestore-types'; import { Blob } from '../../src/api/blob'; import { @@ -45,8 +45,8 @@ let appCount = 0; export function newTestFirestore( projectId: string, nameOrApp?: string | FirebaseApp, - settings?: firestore.Settings -): firestore.FirebaseFirestore { + settings?: Settings +): FirebaseFirestore { if (nameOrApp === undefined) { nameOrApp = 'test-app-' + appCount++; } @@ -78,5 +78,6 @@ export { Blob, GeoPoint, DocumentReference, - QueryDocumentSnapshot + QueryDocumentSnapshot, + Settings }; From 24fbb799dcad8e1e308211d600e3e0ab7e0e2804 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 18:42:36 -0500 Subject: [PATCH 19/30] Revert "firestore-compat: fix typing error in fields.test.ts: TS2339: Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }'" This reverts commit c1437bc4dcf41307022b1ac82a023b62b47eac1e. Importing @firestore/types didn't work in CI... ugh. --- packages/firestore-compat/test/fields.test.ts | 2 +- packages/firestore-compat/test/util/firebase_export.ts | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/firestore-compat/test/fields.test.ts b/packages/firestore-compat/test/fields.test.ts index c98b09fb865..6c72572d82d 100644 --- a/packages/firestore-compat/test/fields.test.ts +++ b/packages/firestore-compat/test/fields.test.ts @@ -387,7 +387,7 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { }); apiDescribe('`undefined` properties', (persistence: boolean) => { - const settings: firebaseExport.Settings = { ...DEFAULT_SETTINGS }; + const settings = { ...DEFAULT_SETTINGS }; settings.ignoreUndefinedProperties = true; it('are ignored in set()', () => { diff --git a/packages/firestore-compat/test/util/firebase_export.ts b/packages/firestore-compat/test/util/firebase_export.ts index a8a8de31385..acf33bda732 100644 --- a/packages/firestore-compat/test/util/firebase_export.ts +++ b/packages/firestore-compat/test/util/firebase_export.ts @@ -23,7 +23,7 @@ import firebase from '@firebase/app-compat'; import { FirebaseApp } from '@firebase/app-types'; import { GeoPoint, Timestamp } from '@firebase/firestore'; -import { FirebaseFirestore, Settings } from '@firebase/firestore-types'; +import * as firestore from '@firebase/firestore-types'; import { Blob } from '../../src/api/blob'; import { @@ -45,8 +45,8 @@ let appCount = 0; export function newTestFirestore( projectId: string, nameOrApp?: string | FirebaseApp, - settings?: Settings -): FirebaseFirestore { + settings?: firestore.Settings +): firestore.FirebaseFirestore { if (nameOrApp === undefined) { nameOrApp = 'test-app-' + appCount++; } @@ -78,6 +78,5 @@ export { Blob, GeoPoint, DocumentReference, - QueryDocumentSnapshot, - Settings + QueryDocumentSnapshot }; From f9adb549099ab26666ccacc9a1a77aaf2beaca74 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 8 Mar 2023 16:18:06 -0500 Subject: [PATCH 20/30] firestore-compat: *really* fix typing error in fields.test.ts: TS2339: Property 'ignoreUndefinedProperties' does not exist on type '{ host: string; ssl: boolean; }' See https://github.com/firebase/firebase-js-sdk/actions/runs/4367447223/jobs/7638769445 --- packages/firestore-compat/test/fields.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/firestore-compat/test/fields.test.ts b/packages/firestore-compat/test/fields.test.ts index 6c72572d82d..7620482c4ea 100644 --- a/packages/firestore-compat/test/fields.test.ts +++ b/packages/firestore-compat/test/fields.test.ts @@ -387,8 +387,7 @@ apiDescribe('Timestamp Fields in snapshots', (persistence: boolean) => { }); apiDescribe('`undefined` properties', (persistence: boolean) => { - const settings = { ...DEFAULT_SETTINGS }; - settings.ignoreUndefinedProperties = true; + const settings = { ...DEFAULT_SETTINGS, ignoreUndefinedProperties: true }; it('are ignored in set()', () => { return withTestDocAndSettings(persistence, settings, async doc => { From def79d16f0101703bca4c89b166658d18ed6bf43 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 12:32:39 -0500 Subject: [PATCH 21/30] minor cleanups from self code review --- packages/firestore/src/util/testing_hooks.ts | 7 ++--- .../test/integration/api/query.test.ts | 26 +++++++++++++++---- .../integration/util/testing_hooks_util.ts | 9 ++++--- 3 files changed, 31 insertions(+), 11 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index c3c351be150..aa26ffda2b0 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -63,7 +63,8 @@ export class TestingHooks { * occurs in the Watch listen stream. * * The relative order in which callbacks are notified is unspecified; do not - * rely on any particular ordering. + * rely on any particular ordering. If a given callback is registered multiple + * times then it will be notified multiple times, once per registration. * * @param callback the callback to invoke upon existence filter mismatch. * @@ -89,8 +90,8 @@ export class TestingHooks { } /** - * The signature of callbacks registered with - * `TestingUtils.onExistenceFilterMismatch()`. + * Information about an existence filter mismatch, as specified to callbacks + * registered with `TestingUtils.onExistenceFilterMismatch()`. */ export interface ExistenceFilterMismatchInfo { /** The number of documents that matched the query in the local cache. */ diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index c7f36542274..ae8eacd368f 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2066,7 +2066,7 @@ apiDescribe('Queries', (persistence: boolean) => { // emulator once the bug where an existence filter fails to be sent when a // query is resumed is fixed. // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR || !persistence ? it.skip : it)( + (USE_EMULATOR ? it.skip : it.only)( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. @@ -2110,13 +2110,29 @@ apiDescribe('Queries', (persistence: boolean) => { expect(snapshot2.size, 'snapshot2.size').to.equal(50); }); + // Skip the verification of the existence filter mismatch when + // persistence is disabled because without persistence there is no + // resume token specified in the subsequent call to getDocs(), and, + // therefore, Watch will _not_ send an existence filter. + if (! persistence) { + return 'passed'; + } + + // Skip the verification of the existence filter mismatch when + // testing against the Firestore emulator because the Firestore + // emulator does not include the `unchanged_names` bloom filter when + // it sends ExistenceFilter messages. Some day the emulator _may_ + // implement this logic, at which time this short-circuit can be + // removed. + if (USE_EMULATOR) { + return 'passed'; + } + // Verify that upon resuming the query that Watch sent an existence // 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*. + // TODO(b/271949433) Remove this check for "nightly" once the bloom + // filter logic is deployed to production, circa May 2023. if (TARGET_BACKEND === 'nightly') { expect( existenceFilterMismatches, diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index f273a40adf0..22211f28c7b 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { _TestingHooks as TestingHooks, _logWarn } from './firebase_export'; +import { _TestingHooks as TestingHooks } from './firebase_export'; /** * Captures all existence filter mismatches in the Watch 'Listen' stream that @@ -31,9 +31,8 @@ export async function captureExistenceFilterMismatches( const onExistenceFilterMismatchCallback = ( info: ExistenceFilterMismatchInfo ): void => { - _logWarn('zzyzx onExistenceFilterMismatchCallback', info); results.push(info); - }; + } const unregister = TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( @@ -55,6 +54,10 @@ export async function captureExistenceFilterMismatches( * * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()` * for the meaning of these values. + * + * TODO: Delete this "interface" definition and instead use the one from + * testing_hooks.ts. I tried to do this but couldn't figure out how to get it to + * work in a way that survived bundling and minification. */ export interface ExistenceFilterMismatchInfo { actualCount: number; From f5415f517c8f51fa5259ae58c04399721e68d16c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 12:34:06 -0500 Subject: [PATCH 22/30] yarn prettier --- packages/firestore/test/integration/api/query.test.ts | 2 +- packages/firestore/test/integration/util/testing_hooks_util.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index ae8eacd368f..86b117e1014 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2114,7 +2114,7 @@ apiDescribe('Queries', (persistence: boolean) => { // persistence is disabled because without persistence there is no // resume token specified in the subsequent call to getDocs(), and, // therefore, Watch will _not_ send an existence filter. - if (! persistence) { + if (!persistence) { return 'passed'; } diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 22211f28c7b..1b7125786cf 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -32,7 +32,7 @@ export async function captureExistenceFilterMismatches( info: ExistenceFilterMismatchInfo ): void => { results.push(info); - } + }; const unregister = TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( From 6bc7b3b80be352160aacb61ab1fd525c47a9d0d8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 12:42:38 -0500 Subject: [PATCH 23/30] watch_change.ts minor cleanup --- packages/firestore/src/remote/watch_change.ts | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 4ea63589da9..dd646962c88 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -38,7 +38,7 @@ import { primitiveComparator } from '../util/misc'; import { SortedMap } from '../util/sorted_map'; import { SortedSet } from '../util/sorted_set'; import { - ExistenceFilterMismatchInfo, + ExistenceFilterMismatchInfo as TestingHooksExistenceFilterMismatchInfo, TestingHooks } from '../util/testing_hooks'; @@ -436,10 +436,12 @@ export class WatchChangeAggregator { this.resetTarget(targetId); this.pendingTargetResets = this.pendingTargetResets.add(targetId); } - notifyTestingHooksOnExistenceFilterMismatch( - bloomFilterApplied, - currentSize, - watchChange.existenceFilter + TestingHooks.instance?.notifyOnExistenceFilterMismatch( + createExistenceFilterMismatchInfoForTestingHooks( + bloomFilterApplied, + currentSize, + watchChange.existenceFilter + ) ); } } @@ -799,19 +801,19 @@ function snapshotChangesMap(): SortedMap { return new SortedMap(DocumentKey.comparator); } -function notifyTestingHooksOnExistenceFilterMismatch( +function createExistenceFilterMismatchInfoForTestingHooks( bloomFilterApplied: boolean, actualCount: number, existenceFilter: ExistenceFilter -): void { - const existenceFilterMismatchInfo: ExistenceFilterMismatchInfo = { +): TestingHooksExistenceFilterMismatchInfo { + const result: TestingHooksExistenceFilterMismatchInfo = { actualCount, expectedCount: existenceFilter.count }; const unchangedNames = existenceFilter.unchangedNames; if (unchangedNames) { - existenceFilterMismatchInfo.bloomFilter = { + result.bloomFilter = { applied: bloomFilterApplied, hashCount: unchangedNames?.hashCount ?? 0, bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, @@ -819,7 +821,5 @@ function notifyTestingHooksOnExistenceFilterMismatch( }; } - TestingHooks.instance?.notifyOnExistenceFilterMismatch( - existenceFilterMismatchInfo - ); + return result; } From bad0ca9665290765c94cac0d3c70d9bd53953e30 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 12:49:15 -0500 Subject: [PATCH 24/30] query.test.ts: remove it.only() --- packages/firestore/test/integration/api/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 86b117e1014..47679b4551f 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2066,7 +2066,7 @@ apiDescribe('Queries', (persistence: boolean) => { // emulator once the bug where an existence filter fails to be sent when a // query is resumed is fixed. // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR ? it.skip : it.only)( + (USE_EMULATOR ? it.skip : it)( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. From 9871614df2026378c060ee48e8d284240302d02b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 13:16:19 -0500 Subject: [PATCH 25/30] refactor the test a bit --- .../test/integration/api/query.test.ts | 204 +++++++++--------- 1 file changed, 105 insertions(+), 99 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 47679b4551f..b1918594b37 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -26,6 +26,7 @@ import { Bytes, collection, collectionGroup, + CollectionReference, deleteDoc, disableNetwork, doc, @@ -35,6 +36,7 @@ import { enableNetwork, endAt, endBefore, + Firestore, GeoPoint, getDocs, getDocsFromCache, @@ -2066,7 +2068,7 @@ apiDescribe('Queries', (persistence: boolean) => { // emulator once the bug where an existence filter fails to be sent when a // query is resumed is fixed. // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR ? it.skip : it)( + (USE_EMULATOR ? it.skip : it.only)( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. @@ -2075,110 +2077,114 @@ apiDescribe('Queries', (persistence: boolean) => { testDocs['doc' + i] = { key: i }; } + // The function that runs a single iteration of the test. + // Below this definition, there is a "while" loop that calls this + // function potentially multiple times. + const runTestIteration = async ( + coll: CollectionReference, + db: Firestore + ): Promise<'retry' | 'passed'> => { + // Run a query to populate the local cache with the 100 documents + // and a resume token. + const snapshot1 = await getDocs(coll); + 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. + await runTransaction(db, async txn => { + for (let i = 1; i <= 50; i++) { + txn.delete(doc(coll, 'doc' + i)); + } + }); + + // Wait for 10 seconds, during which Watch will stop tracking the + // query and will send an existence filter rather than "delete" + // events when the query is resumed. + await new Promise(resolve => setTimeout(resolve, 10000)); + + // Resume the query and expect to get a snapshot with the 50 + // remaining documents. Use some internal testing hooks to "capture" + // the existence filter mismatches to later verify that Watch sent a + // bloom filter, and it was used to avert a full requery. + const existenceFilterMismatches = + await captureExistenceFilterMismatches(async () => { + const snapshot2 = await getDocs(coll); + expect(snapshot2.size, 'snapshot2.size').to.equal(50); + }); + + // Skip the verification of the existence filter mismatch when + // persistence is disabled because without persistence there is no + // resume token specified in the subsequent call to getDocs(), and, + // therefore, Watch will _not_ send an existence filter. + if (!persistence) { + return 'passed'; + } + + // Skip the verification of the existence filter mismatch when + // testing against the Firestore emulator because the Firestore + // emulator does not include the `unchanged_names` bloom filter when + // it sends ExistenceFilter messages. Some day the emulator _may_ + // implement this logic, at which time this short-circuit can be + // removed. + if (USE_EMULATOR) { + return 'passed'; + } + + // Verify that upon resuming the query that Watch sent an existence + // filter that included a bloom filter, and that the bloom filter + // was successfully used to avoid a full requery. + // TODO(b/271949433) Remove this check for "nightly" once the bloom + // filter logic is deployed to production, circa May 2023. + if (TARGET_BACKEND === 'nightly') { + expect( + existenceFilterMismatches, + 'existenceFilterMismatches' + ).to.have.length(1); + const { actualCount, expectedCount, bloomFilter } = + existenceFilterMismatches[0]; + + expect(actualCount, 'actualCount').to.equal(100); + expect(expectedCount, 'expectedCount').to.equal(50); + if (!bloomFilter) { + expect.fail( + 'The existence filter should have specified ' + + 'a bloom filter in its `unchanged_names` field.' + ); + throw new Error('should never get here'); + } + + expect(bloomFilter.hashCount, 'bloomFilter.hashCount').to.be.above(0); + expect( + bloomFilter.bitmapLength, + 'bloomFilter.bitmapLength' + ).to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8); + + // 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 && !bloomFilter.applied) { + return 'retry'; + } + expect(bloomFilter.applied, 'bloomFilter.applied').to.be.true; + } + + return 'passed'; + }; + + // Run the test let attemptNumber = 0; while (true) { attemptNumber++; - const iterationResult = await withTestCollection<'retry' | 'passed'>( + const iterationResult = await withTestCollection( persistence, testDocs, - async (coll, db) => { - // Run a query to populate the local cache with the 100 documents - // and a resume token. - const snapshot1 = await getDocs(coll); - 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. - await runTransaction(db, async txn => { - for (let i = 1; i <= 50; i++) { - txn.delete(doc(coll, 'doc' + i)); - } - }); - - // Wait for 10 seconds, during which Watch will stop tracking the - // query and will send an existence filter rather than "delete" - // events when the query is resumed. - await new Promise(resolve => setTimeout(resolve, 10000)); - - // Resume the query and expect to get a snapshot with the 50 - // remaining documents. Use some internal testing hooks to "capture" - // the existence filter mismatches to later verify that Watch sent a - // bloom filter, and it was used to avert a full requery. - const existenceFilterMismatches = - await captureExistenceFilterMismatches(async () => { - const snapshot2 = await getDocs(coll); - expect(snapshot2.size, 'snapshot2.size').to.equal(50); - }); - - // Skip the verification of the existence filter mismatch when - // persistence is disabled because without persistence there is no - // resume token specified in the subsequent call to getDocs(), and, - // therefore, Watch will _not_ send an existence filter. - if (!persistence) { - return 'passed'; - } - - // Skip the verification of the existence filter mismatch when - // testing against the Firestore emulator because the Firestore - // emulator does not include the `unchanged_names` bloom filter when - // it sends ExistenceFilter messages. Some day the emulator _may_ - // implement this logic, at which time this short-circuit can be - // removed. - if (USE_EMULATOR) { - return 'passed'; - } - - // Verify that upon resuming the query that Watch sent an existence - // filter that included a bloom filter, and that the bloom filter - // was successfully used to avoid a full requery. - // TODO(b/271949433) Remove this check for "nightly" once the bloom - // filter logic is deployed to production, circa May 2023. - if (TARGET_BACKEND === 'nightly') { - expect( - existenceFilterMismatches, - 'existenceFilterMismatches' - ).to.have.length(1); - const { actualCount, expectedCount, bloomFilter } = - existenceFilterMismatches[0]; - - expect(actualCount, 'actualCount').to.equal(100); - expect(expectedCount, 'expectedCount').to.equal(50); - if (!bloomFilter) { - expect.fail( - 'The existence filter should have specified ' + - 'a bloom filter in its `unchanged_names` field.' - ); - throw new Error('should never get here'); - } - - expect( - bloomFilter.hashCount, - 'bloomFilter.hashCount' - ).to.be.above(0); - expect( - bloomFilter.bitmapLength, - 'bloomFilter.bitmapLength' - ).to.be.above(0); - expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0); - expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8); - - // 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 && !bloomFilter.applied) { - return 'retry'; - } - expect(bloomFilter.applied, 'bloomFilter.applied').to.be.true; - } - - return 'passed'; - } + runTestIteration ); - - // Break out of the retry loop if the test passed. if (iterationResult === 'passed') { break; } From 61a9cd72a0880986beb36cc4629e44b4070091d8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 13:30:07 -0500 Subject: [PATCH 26/30] query.test.ts: it.only -> it (oops) --- packages/firestore/test/integration/api/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index b1918594b37..5d0c4605bce 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2068,7 +2068,7 @@ apiDescribe('Queries', (persistence: boolean) => { // emulator once the bug where an existence filter fails to be sent when a // query is resumed is fixed. // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR ? it.skip : it.only)( + (USE_EMULATOR ? it.skip : it)( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. From 2acf945cf96c18598a4037ee49416768aa55efb3 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 15:53:43 -0500 Subject: [PATCH 27/30] rename actualCount/expectedCount to localCacheCount/existenceFilterCount to be less confusing, since I've already confused myself with this naming --- packages/firestore/src/remote/watch_change.ts | 6 +++--- packages/firestore/src/util/testing_hooks.ts | 4 ++-- packages/firestore/test/integration/api/query.test.ts | 6 +++--- .../firestore/test/integration/util/testing_hooks_util.ts | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index dd646962c88..3565d8eefeb 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -803,12 +803,12 @@ function snapshotChangesMap(): SortedMap { function createExistenceFilterMismatchInfoForTestingHooks( bloomFilterApplied: boolean, - actualCount: number, + localCacheCount: number, existenceFilter: ExistenceFilter ): TestingHooksExistenceFilterMismatchInfo { const result: TestingHooksExistenceFilterMismatchInfo = { - actualCount, - expectedCount: existenceFilter.count + localCacheCount, + existenceFilterCount: existenceFilter.count }; const unchangedNames = existenceFilter.unchangedNames; diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index aa26ffda2b0..d621cd42834 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -95,13 +95,13 @@ export class TestingHooks { */ export interface ExistenceFilterMismatchInfo { /** The number of documents that matched the query in the local cache. */ - actualCount: number; + localCacheCount: number; /** * The number of documents that matched the query on the server, as specified * in the ExistenceFilter message's `count` field. */ - expectedCount: number; + existenceFilterCount: number; /** * Information about the bloom filter provided by Watch in the ExistenceFilter diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 5d0c4605bce..26d06b1eed5 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2140,11 +2140,11 @@ apiDescribe('Queries', (persistence: boolean) => { existenceFilterMismatches, 'existenceFilterMismatches' ).to.have.length(1); - const { actualCount, expectedCount, bloomFilter } = + const { localCacheCount, existenceFilterCount, bloomFilter } = existenceFilterMismatches[0]; - expect(actualCount, 'actualCount').to.equal(100); - expect(expectedCount, 'expectedCount').to.equal(50); + expect(localCacheCount, 'localCacheCount').to.equal(100); + expect(existenceFilterCount, 'existenceFilterCount').to.equal(50); if (!bloomFilter) { expect.fail( 'The existence filter should have specified ' + diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 1b7125786cf..d66a19d7ce9 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -60,8 +60,8 @@ export async function captureExistenceFilterMismatches( * work in a way that survived bundling and minification. */ export interface ExistenceFilterMismatchInfo { - actualCount: number; - expectedCount: number; + localCacheCount: number; + existenceFilterCount: number; bloomFilter?: { applied: boolean; hashCount: number; From daf049ccd56f9de05b3f7a2dcdac1bc3553ded16 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 15:56:00 -0500 Subject: [PATCH 28/30] disable the annoying 'check changeset' check, which is broken when targeting non-master branches. This is a cherry-pick of https://github.com/firebase/firebase-js-sdk/pull/7110 --- .github/workflows/check-changeset.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/check-changeset.yml b/.github/workflows/check-changeset.yml index d6ea0b15d63..1e709c65dd2 100644 --- a/.github/workflows/check-changeset.yml +++ b/.github/workflows/check-changeset.yml @@ -2,8 +2,8 @@ name: Check Changeset on: pull_request: - branches-ignore: - - release + branches: + - master env: GITHUB_PULL_REQUEST_HEAD_SHA: ${{ github.event.pull_request.head.sha }} From 677b9e717442e7512f40be6bf312270a48fb84d0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 20:51:27 -0500 Subject: [PATCH 29/30] address review feedback --- packages/firestore/src/remote/watch_change.ts | 6 +++--- .../firestore/test/integration/api/query.test.ts | 15 +++++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 4d7506501a1..b9dd68d3908 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -451,7 +451,7 @@ export class WatchChangeAggregator { } TestingHooks.instance?.notifyOnExistenceFilterMismatch( createExistenceFilterMismatchInfoForTestingHooks( - bloomFilterApplied, + status, currentSize, watchChange.existenceFilter ) @@ -828,7 +828,7 @@ function snapshotChangesMap(): SortedMap { } function createExistenceFilterMismatchInfoForTestingHooks( - bloomFilterApplied: boolean, + status: BloomFilterApplicationStatus, localCacheCount: number, existenceFilter: ExistenceFilter ): TestingHooksExistenceFilterMismatchInfo { @@ -840,7 +840,7 @@ function createExistenceFilterMismatchInfoForTestingHooks( const unchangedNames = existenceFilter.unchangedNames; if (unchangedNames) { result.bloomFilter = { - applied: bloomFilterApplied, + applied: status === BloomFilterApplicationStatus.Success, hashCount: unchangedNames?.hashCount ?? 0, bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, padding: unchangedNames?.bits?.padding ?? 0 diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 26d06b1eed5..d65e24e0f05 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2064,11 +2064,7 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - // TODO(b/270731363): Re-enable this test to run against the Firestore - // emulator once the bug where an existence filter fails to be sent when a - // query is resumed is fixed. - // eslint-disable-next-line no-restricted-properties - (USE_EMULATOR ? it.skip : it)( + it( 'resuming a query should use bloom filter to avoid full requery', async () => { // Create 100 documents in a new collection. @@ -2109,7 +2105,14 @@ apiDescribe('Queries', (persistence: boolean) => { const existenceFilterMismatches = await captureExistenceFilterMismatches(async () => { const snapshot2 = await getDocs(coll); - expect(snapshot2.size, 'snapshot2.size').to.equal(50); + // TODO(b/270731363): Remove the "if" condition below once the + // Firestore Emulator is fixed to send an existence filter. At the + // time of writing, the Firestore emulator fails to send an + // existence filter, resulting in the client including the deleted + // documents in the snapshot of the resumed query. + if (!(USE_EMULATOR && snapshot2.size === 100)) { + expect(snapshot2.size, 'snapshot2.size').to.equal(50); + } }); // Skip the verification of the existence filter mismatch when From acff0f42a612101c6f99144e0dc69017b4a76446 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 9 Mar 2023 20:55:55 -0500 Subject: [PATCH 30/30] yarn prettier --- .../test/integration/api/query.test.ts | 234 +++++++++--------- 1 file changed, 116 insertions(+), 118 deletions(-) diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index d65e24e0f05..cc01b5fa977 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2064,136 +2064,134 @@ apiDescribe('Queries', (persistence: boolean) => { }); }); - it( - 'resuming a query should use bloom filter to avoid full requery', - async () => { - // Create 100 documents in a new collection. - const testDocs: { [key: string]: object } = {}; - for (let i = 1; i <= 100; i++) { - testDocs['doc' + i] = { key: i }; - } + it('resuming a query should use bloom filter to avoid full requery', async () => { + // Create 100 documents in a new collection. + const testDocs: { [key: string]: object } = {}; + for (let i = 1; i <= 100; i++) { + testDocs['doc' + i] = { key: i }; + } - // The function that runs a single iteration of the test. - // Below this definition, there is a "while" loop that calls this - // function potentially multiple times. - const runTestIteration = async ( - coll: CollectionReference, - db: Firestore - ): Promise<'retry' | 'passed'> => { - // Run a query to populate the local cache with the 100 documents - // and a resume token. - const snapshot1 = await getDocs(coll); - expect(snapshot1.size, 'snapshot1.size').to.equal(100); + // The function that runs a single iteration of the test. + // Below this definition, there is a "while" loop that calls this + // function potentially multiple times. + const runTestIteration = async ( + coll: CollectionReference, + db: Firestore + ): Promise<'retry' | 'passed'> => { + // Run a query to populate the local cache with the 100 documents + // and a resume token. + const snapshot1 = await getDocs(coll); + 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. + await runTransaction(db, async txn => { + for (let i = 1; i <= 50; i++) { + txn.delete(doc(coll, 'doc' + i)); + } + }); - // Delete 50 of the 100 documents. Do this in a transaction, rather - // than deleteDoc(), to avoid affecting the local cache. - await runTransaction(db, async txn => { - for (let i = 1; i <= 50; i++) { - txn.delete(doc(coll, 'doc' + i)); + // Wait for 10 seconds, during which Watch will stop tracking the + // query and will send an existence filter rather than "delete" + // events when the query is resumed. + await new Promise(resolve => setTimeout(resolve, 10000)); + + // Resume the query and expect to get a snapshot with the 50 + // remaining documents. Use some internal testing hooks to "capture" + // the existence filter mismatches to later verify that Watch sent a + // bloom filter, and it was used to avert a full requery. + const existenceFilterMismatches = await captureExistenceFilterMismatches( + async () => { + const snapshot2 = await getDocs(coll); + // TODO(b/270731363): Remove the "if" condition below once the + // Firestore Emulator is fixed to send an existence filter. At the + // time of writing, the Firestore emulator fails to send an + // existence filter, resulting in the client including the deleted + // documents in the snapshot of the resumed query. + if (!(USE_EMULATOR && snapshot2.size === 100)) { + expect(snapshot2.size, 'snapshot2.size').to.equal(50); } - }); - - // Wait for 10 seconds, during which Watch will stop tracking the - // query and will send an existence filter rather than "delete" - // events when the query is resumed. - await new Promise(resolve => setTimeout(resolve, 10000)); - - // Resume the query and expect to get a snapshot with the 50 - // remaining documents. Use some internal testing hooks to "capture" - // the existence filter mismatches to later verify that Watch sent a - // bloom filter, and it was used to avert a full requery. - const existenceFilterMismatches = - await captureExistenceFilterMismatches(async () => { - const snapshot2 = await getDocs(coll); - // TODO(b/270731363): Remove the "if" condition below once the - // Firestore Emulator is fixed to send an existence filter. At the - // time of writing, the Firestore emulator fails to send an - // existence filter, resulting in the client including the deleted - // documents in the snapshot of the resumed query. - if (!(USE_EMULATOR && snapshot2.size === 100)) { - expect(snapshot2.size, 'snapshot2.size').to.equal(50); - } - }); - - // Skip the verification of the existence filter mismatch when - // persistence is disabled because without persistence there is no - // resume token specified in the subsequent call to getDocs(), and, - // therefore, Watch will _not_ send an existence filter. - if (!persistence) { - return 'passed'; } + ); - // Skip the verification of the existence filter mismatch when - // testing against the Firestore emulator because the Firestore - // emulator does not include the `unchanged_names` bloom filter when - // it sends ExistenceFilter messages. Some day the emulator _may_ - // implement this logic, at which time this short-circuit can be - // removed. - if (USE_EMULATOR) { - return 'passed'; - } + // Skip the verification of the existence filter mismatch when + // persistence is disabled because without persistence there is no + // resume token specified in the subsequent call to getDocs(), and, + // therefore, Watch will _not_ send an existence filter. + if (!persistence) { + return 'passed'; + } - // Verify that upon resuming the query that Watch sent an existence - // filter that included a bloom filter, and that the bloom filter - // was successfully used to avoid a full requery. - // TODO(b/271949433) Remove this check for "nightly" once the bloom - // filter logic is deployed to production, circa May 2023. - if (TARGET_BACKEND === 'nightly') { - expect( - existenceFilterMismatches, - 'existenceFilterMismatches' - ).to.have.length(1); - const { localCacheCount, existenceFilterCount, bloomFilter } = - existenceFilterMismatches[0]; - - expect(localCacheCount, 'localCacheCount').to.equal(100); - expect(existenceFilterCount, 'existenceFilterCount').to.equal(50); - if (!bloomFilter) { - expect.fail( - 'The existence filter should have specified ' + - 'a bloom filter in its `unchanged_names` field.' - ); - throw new Error('should never get here'); - } + // Skip the verification of the existence filter mismatch when + // testing against the Firestore emulator because the Firestore + // emulator does not include the `unchanged_names` bloom filter when + // it sends ExistenceFilter messages. Some day the emulator _may_ + // implement this logic, at which time this short-circuit can be + // removed. + if (USE_EMULATOR) { + return 'passed'; + } - expect(bloomFilter.hashCount, 'bloomFilter.hashCount').to.be.above(0); - expect( - bloomFilter.bitmapLength, - 'bloomFilter.bitmapLength' - ).to.be.above(0); - expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0); - expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8); - - // 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 && !bloomFilter.applied) { - return 'retry'; - } - expect(bloomFilter.applied, 'bloomFilter.applied').to.be.true; + // Verify that upon resuming the query that Watch sent an existence + // filter that included a bloom filter, and that the bloom filter + // was successfully used to avoid a full requery. + // TODO(b/271949433) Remove this check for "nightly" once the bloom + // filter logic is deployed to production, circa May 2023. + if (TARGET_BACKEND === 'nightly') { + expect( + existenceFilterMismatches, + 'existenceFilterMismatches' + ).to.have.length(1); + const { localCacheCount, existenceFilterCount, bloomFilter } = + existenceFilterMismatches[0]; + + expect(localCacheCount, 'localCacheCount').to.equal(100); + expect(existenceFilterCount, 'existenceFilterCount').to.equal(50); + if (!bloomFilter) { + expect.fail( + 'The existence filter should have specified ' + + 'a bloom filter in its `unchanged_names` field.' + ); + throw new Error('should never get here'); } - return 'passed'; - }; - - // Run the test - let attemptNumber = 0; - while (true) { - attemptNumber++; - const iterationResult = await withTestCollection( - persistence, - testDocs, - runTestIteration - ); - if (iterationResult === 'passed') { - break; + expect(bloomFilter.hashCount, 'bloomFilter.hashCount').to.be.above(0); + expect( + bloomFilter.bitmapLength, + 'bloomFilter.bitmapLength' + ).to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.above(0); + expect(bloomFilter.padding, 'bloomFilterPadding').to.be.below(8); + + // 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 && !bloomFilter.applied) { + return 'retry'; } + expect(bloomFilter.applied, 'bloomFilter.applied').to.be.true; + } + + return 'passed'; + }; + + // Run the test + let attemptNumber = 0; + while (true) { + attemptNumber++; + const iterationResult = await withTestCollection( + persistence, + testDocs, + runTestIteration + ); + if (iterationResult === 'passed') { + break; } } - ).timeout('90s'); + }).timeout('90s'); }); function verifyDocumentChange(