diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index e168fb6fcdb..758eb0120bb 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -433,15 +433,18 @@ export class WatchChangeAggregator { // raise a snapshot with `isFromCache:true`. if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. - const applyResult = this.applyBloomFilter(watchChange, currentSize); + const bloomFilter = this.parseBloomFilter(watchChange); + const status = bloomFilter + ? this.applyBloomFilter(bloomFilter, watchChange, currentSize) + : BloomFilterApplicationStatus.Skipped; - if (applyResult.status !== BloomFilterApplicationStatus.Success) { + if (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 = - applyResult.status === BloomFilterApplicationStatus.FalsePositive + status === BloomFilterApplicationStatus.FalsePositive ? TargetPurpose.ExistenceFilterMismatchBloom : TargetPurpose.ExistenceFilterMismatch; this.pendingTargetResets = this.pendingTargetResets.insert( @@ -451,10 +454,11 @@ export class WatchChangeAggregator { } TestingHooks.instance?.notifyOnExistenceFilterMismatch( createExistenceFilterMismatchInfoForTestingHooks( - applyResult.status, - applyResult.bloomFilterMightContain ?? null, currentSize, - watchChange.existenceFilter + watchChange.existenceFilter, + this.metadataProvider.getDatabaseId(), + bloomFilter, + status ) ); } @@ -463,21 +467,15 @@ export class WatchChangeAggregator { } /** - * Apply bloom filter to remove the deleted documents, and return the - * application status. + * Parse the bloom filter from the "unchanged_names" field of an existence + * filter. */ - private applyBloomFilter( - watchChange: ExistenceFilterChange, - currentCount: number - ): { - status: BloomFilterApplicationStatus; - bloomFilterMightContain?: (documentPath: string) => boolean; - } { - const { unchangedNames, count: expectedCount } = - watchChange.existenceFilter; - + private parseBloomFilter( + watchChange: ExistenceFilterChange + ): BloomFilter | null { + const unchangedNames = watchChange.existenceFilter.unchangedNames; if (!unchangedNames || !unchangedNames.bits) { - return { status: BloomFilterApplicationStatus.Skipped }; + return null; } const { @@ -495,7 +493,7 @@ export class WatchChangeAggregator { err.message + '); ignoring the bloom filter and falling back to full re-query.' ); - return { status: BloomFilterApplicationStatus.Skipped }; + return null; } else { throw err; } @@ -511,31 +509,35 @@ export class WatchChangeAggregator { } else { logWarn('Applying bloom filter failed: ', err); } - return { status: BloomFilterApplicationStatus.Skipped }; + return null; } - 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 { status: BloomFilterApplicationStatus.Skipped }; + return null; } + return bloomFilter; + } + + /** + * Apply bloom filter to remove the deleted documents, and return the + * application status. + */ + private applyBloomFilter( + bloomFilter: BloomFilter, + watchChange: ExistenceFilterChange, + currentCount: number + ): BloomFilterApplicationStatus { + const expectedCount = watchChange.existenceFilter.count; + const removedDocumentCount = this.filterRemovedDocuments( - watchChange.targetId, - bloomFilterMightContain + bloomFilter, + watchChange.targetId ); - const status = - expectedCount === currentCount - removedDocumentCount - ? BloomFilterApplicationStatus.Success - : BloomFilterApplicationStatus.FalsePositive; - return { status, bloomFilterMightContain }; + return expectedCount === currentCount - removedDocumentCount + ? BloomFilterApplicationStatus.Success + : BloomFilterApplicationStatus.FalsePositive; } /** @@ -543,14 +545,20 @@ export class WatchChangeAggregator { * return number of documents removed. */ private filterRemovedDocuments( - targetId: number, - bloomFilterMightContain: (documentPath: string) => boolean + bloomFilter: BloomFilter, + targetId: number ): number { const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId); let removalCount = 0; existingKeys.forEach(key => { - if (!bloomFilterMightContain(key.path.canonicalString())) { + const databaseId = this.metadataProvider.getDatabaseId(); + const documentPath = + `projects/${databaseId.projectId}` + + `/databases/${databaseId.database}` + + `/documents/${key.path.canonicalString()}`; + + if (!bloomFilter.mightContain(documentPath)) { this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null); removalCount++; } @@ -835,27 +843,29 @@ function snapshotChangesMap(): SortedMap { } function createExistenceFilterMismatchInfoForTestingHooks( - status: BloomFilterApplicationStatus, - bloomFilterMightContain: null | ((documentPath: string) => boolean), localCacheCount: number, - existenceFilter: ExistenceFilter + existenceFilter: ExistenceFilter, + databaseId: DatabaseId, + bloomFilter: BloomFilter | null, + bloomFilterStatus: BloomFilterApplicationStatus ): TestingHooksExistenceFilterMismatchInfo { const result: TestingHooksExistenceFilterMismatchInfo = { localCacheCount, - existenceFilterCount: existenceFilter.count + existenceFilterCount: existenceFilter.count, + databaseId: databaseId.database, + projectId: databaseId.projectId }; const unchangedNames = existenceFilter.unchangedNames; if (unchangedNames) { result.bloomFilter = { - applied: status === BloomFilterApplicationStatus.Success, + applied: bloomFilterStatus === BloomFilterApplicationStatus.Success, hashCount: unchangedNames?.hashCount ?? 0, bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0, - padding: unchangedNames?.bits?.padding ?? 0 + padding: unchangedNames?.bits?.padding ?? 0, + mightContain: (value: string): boolean => + bloomFilter?.mightContain(value) ?? false }; - 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 191ab0bbbd0..abce89ac1c0 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -103,6 +103,18 @@ export interface ExistenceFilterMismatchInfo { */ existenceFilterCount: number; + /** + * The projectId used when checking documents for membership in the bloom + * filter. + */ + projectId: string; + + /** + * The databaseId used when checking documents for membership in the bloom + * filter. + */ + databaseId: string; + /** * Information about the bloom filter provided by Watch in the ExistenceFilter * message's `unchangedNames` field. If this property is omitted or undefined @@ -126,19 +138,11 @@ export interface ExistenceFilterMismatchInfo { 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. + * Tests the given string for membership in the bloom filter created from + * the existence filter; will be undefined if creating the bloom filter + * failed. */ - mightContain?(documentPath: string): boolean; + mightContain?: (value: string) => boolean; }; } diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 45d9fdc31e4..32f687386de 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2260,8 +2260,9 @@ apiDescribe('Queries', persistence => { // Delete one of the documents so that the next call to getDocs() will // experience an existence filter mismatch. Use a WriteBatch, rather // than deleteDoc(), to avoid affecting the local cache. + const documentToDelete = doc(coll, 'DocumentToDelete'); const writeBatchForDocumentDeletes = writeBatch(db); - writeBatchForDocumentDeletes.delete(doc(coll, 'DocumentToDelete')); + writeBatchForDocumentDeletes.delete(documentToDelete); await writeBatchForDocumentDeletes.commit(); // Wait for 10 seconds, during which Watch will stop tracking the query @@ -2278,7 +2279,7 @@ apiDescribe('Queries', persistence => { documentSnapshot => documentSnapshot.id ); const testDocIdsMinusDeletedDocId = testDocIds.filter( - documentId => documentId !== 'DocumentToDelete' + documentId => documentId !== documentToDelete.id ); expect(snapshot2DocumentIds, 'snapshot2DocumentIds').to.have.members( testDocIdsMinusDeletedDocId @@ -2309,20 +2310,17 @@ apiDescribe('Queries', persistence => { // 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` - ); + const isFalsePositive = bloomFilter.mightContain(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}`; + for (const testDoc of snapshot2.docs.map(snapshot => snapshot.ref)) { expect( - bloomFilter.mightContain!(testDocPath), - `bloomFilter.mightContain('${testDocPath}')` + bloomFilter.mightContain(testDoc), + `bloomFilter.mightContain('${testDoc.path}')` ).to.be.true; } }); diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 60064446560..3dec7d379d8 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,7 +15,10 @@ * limitations under the License. */ -import { _TestingHooks as TestingHooks } from './firebase_export'; +import { + DocumentReference, + _TestingHooks as TestingHooks +} from './firebase_export'; /** * Captures all existence filter mismatches in the Watch 'Listen' stream that @@ -30,9 +33,9 @@ export async function captureExistenceFilterMismatches( ): Promise<[ExistenceFilterMismatchInfo[], T]> { const results: ExistenceFilterMismatchInfo[] = []; const onExistenceFilterMismatchCallback = ( - info: ExistenceFilterMismatchInfo + info: ExistenceFilterMismatchInfoInternal ): void => { - results.push(info); + results.push(createExistenceFilterMismatchInfoFrom(info)); }; const unregister = @@ -51,8 +54,7 @@ export async function captureExistenceFilterMismatches( } /** - * Information about an existence filter mismatch, captured during an invocation - * of `captureExistenceFilterMismatches()`. + * A copy of `ExistenceFilterMismatchInfo` as defined in `testing_hooks.ts`. * * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()` * for the meaning of these values. @@ -61,6 +63,27 @@ export async function captureExistenceFilterMismatches( * 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. */ +interface ExistenceFilterMismatchInfoInternal { + localCacheCount: number; + existenceFilterCount: number; + projectId: string; + databaseId: string; + bloomFilter?: { + applied: boolean; + hashCount: number; + bitmapLength: number; + padding: number; + mightContain?: (value: string) => boolean; + }; +} + +/** + * Information about an existence filter mismatch, captured during an invocation + * of `captureExistenceFilterMismatches()`. + * + * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()` + * for the meaning of these values. + */ export interface ExistenceFilterMismatchInfo { localCacheCount: number; existenceFilterCount: number; @@ -69,6 +92,33 @@ export interface ExistenceFilterMismatchInfo { hashCount: number; bitmapLength: number; padding: number; - mightContain?(documentPath: string): boolean; + mightContain(documentRef: DocumentReference): boolean; }; } + +function createExistenceFilterMismatchInfoFrom( + internalInfo: ExistenceFilterMismatchInfoInternal +): ExistenceFilterMismatchInfo { + const info: ExistenceFilterMismatchInfo = { + localCacheCount: internalInfo.localCacheCount, + existenceFilterCount: internalInfo.existenceFilterCount + }; + + const internalBloomFilter = internalInfo.bloomFilter; + if (internalBloomFilter) { + info.bloomFilter = { + applied: internalBloomFilter.applied, + hashCount: internalBloomFilter.hashCount, + bitmapLength: internalBloomFilter.bitmapLength, + padding: internalBloomFilter.padding, + mightContain: (documentRef: DocumentReference): boolean => + internalBloomFilter.mightContain?.( + `projects/${internalInfo.projectId}` + + `/databases/${internalInfo.databaseId}` + + `/documents/${documentRef.path}` + ) ?? false + }; + } + + return info; +}