diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index b9dd68d3908..e168fb6fcdb 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -433,15 +433,15 @@ export class WatchChangeAggregator { // raise a snapshot with `isFromCache:true`. if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. - const status = this.applyBloomFilter(watchChange, currentSize); + const applyResult = this.applyBloomFilter(watchChange, currentSize); - if (status !== BloomFilterApplicationStatus.Success) { + if (applyResult.status !== BloomFilterApplicationStatus.Success) { // If bloom filter application fails, we reset the mapping and // trigger re-run of the query. this.resetTarget(targetId); const purpose: TargetPurpose = - status === BloomFilterApplicationStatus.FalsePositive + applyResult.status === BloomFilterApplicationStatus.FalsePositive ? TargetPurpose.ExistenceFilterMismatchBloom : TargetPurpose.ExistenceFilterMismatch; this.pendingTargetResets = this.pendingTargetResets.insert( @@ -451,7 +451,8 @@ export class WatchChangeAggregator { } TestingHooks.instance?.notifyOnExistenceFilterMismatch( createExistenceFilterMismatchInfoForTestingHooks( - status, + applyResult.status, + applyResult.bloomFilterMightContain ?? null, currentSize, watchChange.existenceFilter ) @@ -468,12 +469,15 @@ export class WatchChangeAggregator { private applyBloomFilter( watchChange: ExistenceFilterChange, currentCount: number - ): BloomFilterApplicationStatus { + ): { + status: BloomFilterApplicationStatus; + bloomFilterMightContain?: (documentPath: string) => boolean; + } { const { unchangedNames, count: expectedCount } = watchChange.existenceFilter; if (!unchangedNames || !unchangedNames.bits) { - return BloomFilterApplicationStatus.Skipped; + return { status: BloomFilterApplicationStatus.Skipped }; } const { @@ -491,7 +495,7 @@ export class WatchChangeAggregator { err.message + '); ignoring the bloom filter and falling back to full re-query.' ); - return BloomFilterApplicationStatus.Skipped; + return { status: BloomFilterApplicationStatus.Skipped }; } else { throw err; } @@ -507,23 +511,31 @@ export class WatchChangeAggregator { } else { logWarn('Applying bloom filter failed: ', err); } - return BloomFilterApplicationStatus.Skipped; + return { status: BloomFilterApplicationStatus.Skipped }; } + const bloomFilterMightContain = (documentPath: string): boolean => { + const databaseId = this.metadataProvider.getDatabaseId(); + return bloomFilter.mightContain( + `projects/${databaseId.projectId}/databases/${databaseId.database}` + + `/documents/${documentPath}` + ); + }; + if (bloomFilter.bitCount === 0) { - return BloomFilterApplicationStatus.Skipped; + return { status: BloomFilterApplicationStatus.Skipped }; } const removedDocumentCount = this.filterRemovedDocuments( watchChange.targetId, - bloomFilter + bloomFilterMightContain ); - if (expectedCount !== currentCount - removedDocumentCount) { - return BloomFilterApplicationStatus.FalsePositive; - } - - return BloomFilterApplicationStatus.Success; + const status = + expectedCount === currentCount - removedDocumentCount + ? BloomFilterApplicationStatus.Success + : BloomFilterApplicationStatus.FalsePositive; + return { status, bloomFilterMightContain }; } /** @@ -532,18 +544,13 @@ export class WatchChangeAggregator { */ private filterRemovedDocuments( targetId: number, - bloomFilter: BloomFilter + bloomFilterMightContain: (documentPath: string) => boolean ): number { const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId); let removalCount = 0; existingKeys.forEach(key => { - const databaseId = this.metadataProvider.getDatabaseId(); - const documentPath = `projects/${databaseId.projectId}/databases/${ - databaseId.database - }/documents/${key.path.canonicalString()}`; - - if (!bloomFilter.mightContain(documentPath)) { + if (!bloomFilterMightContain(key.path.canonicalString())) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; } @@ -829,6 +836,7 @@ function snapshotChangesMap(): SortedMap { function createExistenceFilterMismatchInfoForTestingHooks( status: BloomFilterApplicationStatus, + bloomFilterMightContain: null | ((documentPath: string) => boolean), localCacheCount: number, existenceFilter: ExistenceFilter ): TestingHooksExistenceFilterMismatchInfo { @@ -845,6 +853,9 @@ function createExistenceFilterMismatchInfoForTestingHooks( bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, padding: unchangedNames?.bits?.padding ?? 0 }; + if (bloomFilterMightContain) { + result.bloomFilter.mightContain = bloomFilterMightContain; + } } return result; diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index d621cd42834..191ab0bbbd0 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -124,6 +124,21 @@ export interface ExistenceFilterMismatchInfo { /** The number of bits of padding in the last byte of the bloom filter. */ padding: number; + + /** + * Check if the given document path is contained in the bloom filter. + * + * The "path" of a document can be retrieved from the + * `DocumentReference.path` property. + * + * Note that due to the probabilistic nature of a bloom filter, it is + * possible that false positives may occur; that is, this function may + * return `true` even though the given string is not in the bloom filter. + * + * This property is "optional"; if it is undefined then parsing the bloom + * filter failed. + */ + mightContain?(documentPath: string): boolean; }; } diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 90d146344ab..e9a863bdb22 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2243,99 +2243,98 @@ apiDescribe('Queries', persistence => { return map; }, {} as { [key: string]: DocumentData }); - // Ensure that the local cache is configured to use LRU garbage - // collection (rather than eager garbage collection) so that the resume - // token and document data does not get prematurely evicted. + // Ensure that the local cache is configured to use LRU garbage collection + // (rather than eager garbage collection) so that the resume token and + // document data does not get prematurely evicted. const lruPersistence = persistence.toLruGc(); - return withRetry(async attemptNumber => { - return withTestCollection( - lruPersistence, - testDocs, - async (coll, db) => { - // Run a query to populate the local cache with documents that have - // names with complex Unicode characters. - const snapshot1 = await getDocs(coll); - const snapshot1DocumentIds = snapshot1.docs.map( - documentSnapshot => documentSnapshot.id - ); - expect( - snapshot1DocumentIds, - 'snapshot1DocumentIds' - ).to.have.members(testDocIds); - - // Delete one of the documents so that the next call to getDocs() will - // experience an existence filter mismatch. Do this deletion in a - // transaction, rather than using deleteDoc(), to avoid affecting the - // local cache. - await runTransaction(db, async txn => { - const snapshotOfDocumentToDelete = await txn.get( - doc(coll, 'DocumentToDelete') - ); - expect( - snapshotOfDocumentToDelete.exists(), - 'snapshotOfDocumentToDelete.exists()' - ).to.be.true; - txn.delete(snapshotOfDocumentToDelete.ref); - }); - - // 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 save the resulting snapshot for verification. - // Use some internal testing hooks to "capture" the existence filter - // mismatches. - const [existenceFilterMismatches, snapshot2] = - await captureExistenceFilterMismatches(() => getDocs(coll)); - const snapshot2DocumentIds = snapshot2.docs.map( - documentSnapshot => documentSnapshot.id - ); - const testDocIdsMinusDeletedDocId = testDocIds.filter( - documentId => documentId !== 'DocumentToDelete' - ); - expect( - snapshot2DocumentIds, - 'snapshot2DocumentIds' - ).to.have.members(testDocIdsMinusDeletedDocId); - - // Verify that Watch sent an existence filter with the correct counts. - expect( - existenceFilterMismatches, - 'existenceFilterMismatches' - ).to.have.length(1); - const { localCacheCount, existenceFilterCount, bloomFilter } = - existenceFilterMismatches[0]; - expect(localCacheCount, 'localCacheCount').to.equal( - testDocIds.length - ); - expect(existenceFilterCount, 'existenceFilterCount').to.equal( - testDocIds.length - 1 - ); + return withTestCollection(lruPersistence, testDocs, async (coll, db) => { + // Run a query to populate the local cache with documents that have + // names with complex Unicode characters. + const snapshot1 = await getDocs(coll); + const snapshot1DocumentIds = snapshot1.docs.map( + documentSnapshot => documentSnapshot.id + ); + expect(snapshot1DocumentIds, 'snapshot1DocumentIds').to.have.members( + testDocIds + ); - // 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'); - } - - // Verify that the bloom filter was successfully used to avert a full - // requery. If a false positive occurred, which is statistically rare, - // but technically possible, then retry the entire test. - if (attemptNumber === 1 && !bloomFilter.applied) { - throw new RetryError(); - } - - expect( - bloomFilter.applied, - `bloomFilter.applied with attemptNumber=${attemptNumber}` - ).to.be.true; - } + // Delete one of the documents so that the next call to getDocs() will + // experience an existence filter mismatch. Do this deletion in a + // transaction, rather than using deleteDoc(), to avoid affecting the + // local cache. + await runTransaction(db, async txn => { + const snapshotOfDocumentToDelete = await txn.get( + doc(coll, 'DocumentToDelete') + ); + expect( + snapshotOfDocumentToDelete.exists(), + 'snapshotOfDocumentToDelete.exists()' + ).to.be.true; + txn.delete(snapshotOfDocumentToDelete.ref); + }); + + // 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 save the resulting snapshot for verification. + // Use some internal testing hooks to "capture" the existence filter + // mismatches. + const [existenceFilterMismatches, snapshot2] = + await captureExistenceFilterMismatches(() => getDocs(coll)); + const snapshot2DocumentIds = snapshot2.docs.map( + documentSnapshot => documentSnapshot.id + ); + const testDocIdsMinusDeletedDocId = testDocIds.filter( + documentId => documentId !== 'DocumentToDelete' + ); + expect(snapshot2DocumentIds, 'snapshot2DocumentIds').to.have.members( + testDocIdsMinusDeletedDocId + ); + + // Verify that Watch sent an existence filter with the correct counts. + expect( + existenceFilterMismatches, + 'existenceFilterMismatches' + ).to.have.length(1); + const existenceFilterMismatch = existenceFilterMismatches[0]; + expect( + existenceFilterMismatch.localCacheCount, + 'localCacheCount' + ).to.equal(testDocIds.length); + expect( + existenceFilterMismatch.existenceFilterCount, + 'existenceFilterCount' + ).to.equal(testDocIds.length - 1); + + // Verify that we got a bloom filter from Watch. + const bloomFilter = existenceFilterMismatch.bloomFilter!; + expect(bloomFilter?.mightContain, 'bloomFilter.mightContain').to.not.be + .undefined; + + // The bloom filter application should statistically be successful + // almost every time; the _only_ time when it would _not_ be successful + // is if there is a false positive when testing for 'DocumentToDelete' + // in the bloom filter. So verify that the bloom filter application is + // successful, unless there was a false positive. + const isFalsePositive = bloomFilter.mightContain!( + `${coll.path}/DocumentToDelete` + ); + expect(bloomFilter.applied, 'bloomFilter.applied').to.equal( + !isFalsePositive ); + + // Verify that the bloom filter contains the document paths with complex + // Unicode characters. + for (const testDocId of testDocIdsMinusDeletedDocId) { + const testDocPath = `${coll.path}/${testDocId}`; + expect( + bloomFilter.mightContain!(testDocPath), + `bloomFilter.mightContain('${testDocPath}')` + ).to.be.true; + } }); } ).timeout('90s'); diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index de0b40f1914..60064446560 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -69,5 +69,6 @@ export interface ExistenceFilterMismatchInfo { hashCount: number; bitmapLength: number; padding: number; + mightContain?(documentPath: string): boolean; }; }