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 }} 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 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 dd201c4ec1f..b9dd68d3908 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -37,6 +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 { + ExistenceFilterMismatchInfo as TestingHooksExistenceFilterMismatchInfo, + TestingHooks +} from '../util/testing_hooks'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; @@ -445,6 +449,13 @@ export class WatchChangeAggregator { purpose ); } + TestingHooks.instance?.notifyOnExistenceFilterMismatch( + createExistenceFilterMismatchInfoForTestingHooks( + status, + currentSize, + watchChange.existenceFilter + ) + ); } } } @@ -815,3 +826,26 @@ function documentTargetMap(): SortedMap> { function snapshotChangesMap(): SortedMap { return new SortedMap(DocumentKey.comparator); } + +function createExistenceFilterMismatchInfoForTestingHooks( + status: BloomFilterApplicationStatus, + localCacheCount: number, + existenceFilter: ExistenceFilter +): TestingHooksExistenceFilterMismatchInfo { + const result: TestingHooksExistenceFilterMismatchInfo = { + localCacheCount, + existenceFilterCount: existenceFilter.count + }; + + const unchangedNames = existenceFilter.unchangedNames; + if (unchangedNames) { + result.bloomFilter = { + applied: status === BloomFilterApplicationStatus.Success, + hashCount: unchangedNames?.hashCount ?? 0, + bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, + padding: unchangedNames?.bits?.padding ?? 0 + }; + } + + return result; +} diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts new file mode 100644 index 00000000000..d621cd42834 --- /dev/null +++ b/packages/firestore/src/util/testing_hooks.ts @@ -0,0 +1,139 @@ +/** + * @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; + + /** + * 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 = ( + 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 94e8221100f..cc01b5fa977 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, @@ -65,7 +67,8 @@ import { withTestCollection, withTestDb } from '../util/helpers'; -import { USE_EMULATOR } from '../util/settings'; +import { USE_EMULATOR, TARGET_BACKEND } from '../util/settings'; +import { captureExistenceFilterMismatches } from '../util/testing_hooks_util'; apiDescribe('Queries', (persistence: boolean) => { addEqualityMatcher(); @@ -2061,34 +2064,134 @@ 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', - () => { - const testDocs: { [key: string]: object } = {}; - for (let i = 1; i <= 100; i++) { - testDocs['doc' + i] = { key: i }; - } - return withTestCollection(persistence, testDocs, async (coll, db) => { - const snapshot1 = await getDocs(coll); - expect(snapshot1.size).to.equal(100); - // Delete 50 docs in transaction so that it doesn't affect 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. - await new Promise(resolve => setTimeout(resolve, 10000)); - const snapshot2 = await getDocs(coll); - expect(snapshot2.size).to.equal(50); + 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); + + // 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); + } + } + ); + + // 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 { 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'); + } + + 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('20s'); + }).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, 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..d66a19d7ce9 --- /dev/null +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -0,0 +1,71 @@ +/** + * @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. + */ +export async function captureExistenceFilterMismatches( + callback: () => Promise +): Promise { + const results: ExistenceFilterMismatchInfo[] = []; + const onExistenceFilterMismatchCallback = ( + info: ExistenceFilterMismatchInfo + ): void => { + results.push(info); + }; + + const unregister = + TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( + onExistenceFilterMismatchCallback + ); + + try { + await callback(); + } finally { + unregister(); + } + + return results; +} + +/** + * Information about an existence filter mismatch, capturing 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; + bloomFilter?: { + applied: boolean; + hashCount: number; + bitmapLength: number; + padding: number; + }; +}