diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index cc01b5fa977..d2a1e08d4c6 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2065,115 +2065,137 @@ apiDescribe('Queries', (persistence: boolean) => { }); it('resuming a query should use bloom filter to avoid full requery', async () => { - // Create 100 documents in a new collection. + // Prepare the names and contents of the 100 documents to create. const testDocs: { [key: string]: object } = {}; - for (let i = 1; i <= 100; i++) { - testDocs['doc' + i] = { key: i }; + for (let i = 0; i < 100; i++) { + testDocs['doc' + (1000 + i)] = { key: 42 }; } // 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. + // 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. + // 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); + const createdDocuments = snapshot1.docs.map(snapshot => snapshot.ref); - // Delete 50 of the 100 documents. Do this in a transaction, rather - // than deleteDoc(), to avoid affecting the local cache. + // Delete 50 of the 100 documents. Do this in a transaction, rather than + // deleteDoc(), to avoid affecting the local cache. + const deletedDocumentIds = new Set(); await runTransaction(db, async txn => { - for (let i = 1; i <= 50; i++) { - txn.delete(doc(coll, 'doc' + i)); + for (let i = 0; i < createdDocuments.length; i += 2) { + const documentToDelete = createdDocuments[i]; + txn.delete(documentToDelete); + deletedDocumentIds.add(documentToDelete.id); } }); - // 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. + // 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); - } - } - ); + // Resume the query and save the resulting snapshot for verification. + // Use some internal testing hooks to "capture" the existence filter + // mismatches to verify that Watch sent a bloom filter, and it was used to + // avert a full requery. + 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_ + // deleted. + // 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)) { + const actualDocumentIds = snapshot2.docs + .map(documentSnapshot => documentSnapshot.ref.id) + .sort(); + const expectedDocumentIds = createdDocuments + .filter(documentRef => !deletedDocumentIds.has(documentRef.id)) + .map(documentRef => documentRef.id) + .sort(); + expect(actualDocumentIds, 'snapshot2.docs').to.deep.equal( + 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. + // 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 '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. + // 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'); - } + // 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, bloomFilter } = + existenceFilterMismatches[0]; + expect(localCacheCount, 'localCacheCount').to.equal(100); + expect(existenceFilterCount, 'existenceFilterCount').to.equal(50); + + // Skip the verification of the bloom filter when testing against + // production because the bloom filter is only implemented in nightly. + // TODO(b/271949433) Remove this "if" block once the bloom filter logic is + // deployed to production. + if (TARGET_BACKEND !== 'nightly') { + 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 Watch sent a valid bloom filter. + 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); + + // Verify that the bloom filter was successfully used to avert a full + // requery. If a false positive occurred then retry the entire test. + // 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 with attemptNumber=${attemptNumber}` + ).to.be.true; return 'passed'; }; diff --git a/packages/firestore/test/integration/util/helpers.ts b/packages/firestore/test/integration/util/helpers.ts index b70a116e839..3e0f34ec302 100644 --- a/packages/firestore/test/integration/util/helpers.ts +++ b/packages/firestore/test/integration/util/helpers.ts @@ -32,7 +32,9 @@ import { PrivateSettings, SnapshotListenOptions, newTestFirestore, - newTestApp + newTestApp, + writeBatch, + WriteBatch } from './firebase_export'; import { ALT_PROJECT_ID, @@ -315,11 +317,34 @@ export function withTestCollectionSettings( const collectionId = 'test-collection-' + doc(collection(testDb, 'x')).id; const testCollection = collection(testDb, collectionId); const setupCollection = collection(setupDb, collectionId); - const sets: Array> = []; - Object.keys(docs).forEach(key => { - sets.push(setDoc(doc(setupCollection, key), docs[key])); - }); - return Promise.all(sets).then(() => fn(testCollection, testDb)); + + const writeBatchCommits: Array> = []; + let writeBatch_: WriteBatch | null = null; + let writeBatchSize = 0; + + for (const key of Object.keys(docs)) { + if (writeBatch_ === null) { + writeBatch_ = writeBatch(setupDb); + } + + writeBatch_.set(doc(setupCollection, key), docs[key]); + writeBatchSize++; + + // Write batches are capped at 500 writes. Use 400 just to be safe. + if (writeBatchSize === 400) { + writeBatchCommits.push(writeBatch_.commit()); + writeBatch_ = null; + writeBatchSize = 0; + } + } + + if (writeBatch_ !== null) { + writeBatchCommits.push(writeBatch_.commit()); + } + + return Promise.all(writeBatchCommits).then(() => + fn(testCollection, testDb) + ); } ); } diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index d66a19d7ce9..d5bda1be660 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -24,9 +24,9 @@ import { _TestingHooks as TestingHooks } from './firebase_export'; * callback all existence filter mismatches will be captured. * @return the captured existence filter mismatches. */ -export async function captureExistenceFilterMismatches( - callback: () => Promise -): Promise { +export async function captureExistenceFilterMismatches( + callback: () => Promise +): Promise<[ExistenceFilterMismatchInfo[], T]> { const results: ExistenceFilterMismatchInfo[] = []; const onExistenceFilterMismatchCallback = ( info: ExistenceFilterMismatchInfo @@ -39,13 +39,14 @@ export async function captureExistenceFilterMismatches( onExistenceFilterMismatchCallback ); + let callbackResult: T; try { - await callback(); + callbackResult = await callback(); } finally { unregister(); } - return results; + return [results, callbackResult]; } /**