From ef6f88001bb485b0a5e792dcb6ee43410fed86e9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 12:23:59 -0400 Subject: [PATCH] Firestore: query.test.ts: improve the test that resumes a query with existence filter to actually validate the existence filter. This builds upon the test added in https://github.com/firebase/firebase-js-sdk/pull/7134 --- integration/firestore/gulpfile.js | 1 + packages/firestore/src/api.ts | 1 + packages/firestore/src/remote/watch_change.ts | 5 + packages/firestore/src/util/testing_hooks.ts | 116 ++++++++++++++++++ .../test/integration/api/query.test.ts | 37 +++++- .../integration/util/testing_hooks_util.ts | 67 ++++++++++ 6 files changed, 226 insertions(+), 1 deletion(-) 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/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' ], diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 807e5dcd647..f06bf1bc568 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -211,3 +211,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 ab80ef0b962..d6bd3cc19a3 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -34,6 +34,7 @@ import { logDebug } 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 { ExistenceFilter } from './existence_filter'; import { RemoteEvent, TargetChange } from './remote_event'; @@ -414,6 +415,10 @@ export class WatchChangeAggregator { // snapshot with `isFromCache:true`. this.resetTarget(targetId); this.pendingTargetResets = this.pendingTargetResets.add(targetId); + TestingHooks.instance?.notifyOnExistenceFilterMismatch({ + localCacheCount: currentSize, + existenceFilterCount: watchChange.existenceFilter.count + }); } } } diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts new file mode 100644 index 00000000000..b778dcca500 --- /dev/null +++ b/packages/firestore/src/util/testing_hooks.ts @@ -0,0 +1,116 @@ +/** + * @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. + */ + +/** + * 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 the global singleton instance of this class: + * 1. The `instance` property, which returns null if the global singleton + * 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 this method if the instance is + * needed to, for example, register a callback. + * + * @internal + */ +export class TestingHooks { + private readonly onExistenceFilterMismatchCallbacks = new Map< + Symbol, + ExistenceFilterMismatchCallback + >(); + + 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. + * + * The relative order in which callbacks are notified is unspecified; do not + * 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. + * + * @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: ExistenceFilterMismatchCallback + ): () => void { + const key = Symbol(); + this.onExistenceFilterMismatchCallbacks.set(key, callback); + return () => this.onExistenceFilterMismatchCallbacks.delete(key); + } + + /** + * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. + * @param info Information about the existence filter mismatch. + */ + notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { + this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info)); + } +} + +/** + * 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. */ + localCacheCount: number; + + /** + * The number of documents that matched the query on the server, as specified + * in the ExistenceFilter message's `count` field. + */ + existenceFilterCount: number; +} + +/** + * The signature of callbacks registered with + * `TestingUtils.onExistenceFilterMismatch()`. + */ +export type ExistenceFilterMismatchCallback = ( + info: ExistenceFilterMismatchInfo +) => 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 cd401b28d68..ccc4db7b8a7 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -66,6 +66,7 @@ import { withTestDb } from '../util/helpers'; import { USE_EMULATOR } from '../util/settings'; +import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; apiDescribe('Queries', (persistence: boolean) => { addEqualityMatcher(); @@ -2092,7 +2093,10 @@ apiDescribe('Queries', (persistence: boolean) => { await new Promise(resolve => setTimeout(resolve, 10000)); // Resume the query and save the resulting snapshot for verification. - const snapshot2 = await getDocs(coll); + // Use some internal testing hooks to "capture" the existence filter + // mismatches to verify them. + const [existenceFilterMismatches, snapshot2] = + await captureExistenceFilterMismatches(() => getDocs(coll)); // Verify that the snapshot from the resumed query contains the expected // documents; that is, that it contains the 50 documents that were _not_ @@ -2114,6 +2118,37 @@ apiDescribe('Queries', (persistence: boolean) => { expectedDocumentIds ); } + + // 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. + // TODO(b/272754156) Re-write this test using a snapshot listener instead + // of calls to getDocs() and remove this check for disabled persistence. + if (!persistence) { + return; + } + + // Skip the verification of the existence filter mismatch when testing + // against the Firestore emulator because the Firestore emulator fails to + // to send an existence filter at all. + // TODO(b/270731363): Enable the verification of the existence filter + // mismatch once the Firestore emulator is fixed to send an existence + // filter. + if (USE_EMULATOR) { + return; + } + + // Verify that Watch sent an existence filter with the correct counts when + // the query was resumed. + expect( + existenceFilterMismatches, + 'existenceFilterMismatches' + ).to.have.length(1); + const { localCacheCount, existenceFilterCount } = + existenceFilterMismatches[0]; + expect(localCacheCount, 'localCacheCount').to.equal(100); + expect(existenceFilterCount, 'existenceFilterCount').to.equal(50); }); }).timeout('90s'); }); 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..2581fa681d2 --- /dev/null +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -0,0 +1,67 @@ +/** + * @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 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 and the result of awaiting + * the given callback. + */ +export async function captureExistenceFilterMismatches( + callback: () => Promise +): Promise<[ExistenceFilterMismatchInfo[], T]> { + const results: ExistenceFilterMismatchInfo[] = []; + const onExistenceFilterMismatchCallback = ( + info: ExistenceFilterMismatchInfo + ): void => { + results.push(info); + }; + + const unregister = + TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( + onExistenceFilterMismatchCallback + ); + + let callbackResult: T; + try { + callbackResult = await callback(); + } finally { + unregister(); + } + + return [results, callbackResult]; +} + +/** + * Information about an existence filter mismatch, captured during an invocation + * of `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 { + localCacheCount: number; + existenceFilterCount: number; +}