diff --git a/packages/firestore/src/core/sync_engine_impl.ts b/packages/firestore/src/core/sync_engine_impl.ts index efe65df6439..60a36d0dcaf 100644 --- a/packages/firestore/src/core/sync_engine_impl.ts +++ b/packages/firestore/src/core/sync_engine_impl.ts @@ -71,7 +71,6 @@ import { primitiveComparator } from '../util/misc'; import { ObjectMap } from '../util/obj_map'; import { Deferred } from '../util/promise'; import { SortedMap } from '../util/sorted_map'; -import { SortedSet } from '../util/sorted_set'; import { BATCHID_UNKNOWN } from '../util/types'; import { @@ -639,7 +638,9 @@ export async function syncEngineRejectListen( const event = new RemoteEvent( SnapshotVersion.min(), /* targetChanges= */ new Map(), - /* targetMismatches= */ new SortedSet(primitiveComparator), + /* targetMismatches= */ new SortedMap( + primitiveComparator + ), documentUpdates, resolvedLimboDocuments ); diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index ae1760f5930..e56ffc3af04 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -601,7 +601,7 @@ export function localStoreApplyRemoteEventToLocalCache( let newTargetData = oldTargetData.withSequenceNumber( txn.currentSequenceNumber ); - if (remoteEvent.targetMismatches.has(targetId)) { + if (remoteEvent.targetMismatches.get(targetId) !== null) { newTargetData = newTargetData .withResumeToken( ByteString.EMPTY_BYTE_STRING, diff --git a/packages/firestore/src/local/target_data.ts b/packages/firestore/src/local/target_data.ts index 2893cbf342a..0128d1b2322 100644 --- a/packages/firestore/src/local/target_data.ts +++ b/packages/firestore/src/local/target_data.ts @@ -26,10 +26,17 @@ export const enum TargetPurpose { Listen, /** - * The query target was used to refill a query after an existence filter mismatch. + * The query target was used to refill a query after an existence filter + * mismatch. */ ExistenceFilterMismatch, + /** + * The query target was used if the query is the result of a false positive in + * the bloom filter. + */ + ExistenceFilterMismatchBloom, + /** The query target was used to resolve a limbo document. */ LimboResolution } diff --git a/packages/firestore/src/remote/remote_event.ts b/packages/firestore/src/remote/remote_event.ts index 5d48d32bb3f..49b2ef56a97 100644 --- a/packages/firestore/src/remote/remote_event.ts +++ b/packages/firestore/src/remote/remote_event.ts @@ -17,15 +17,16 @@ import { SnapshotVersion } from '../core/snapshot_version'; import { TargetId } from '../core/types'; +import { TargetPurpose } from '../local/target_data'; import { documentKeySet, DocumentKeySet, mutableDocumentMap, - MutableDocumentMap, - targetIdSet + MutableDocumentMap } from '../model/collections'; import { ByteString } from '../util/byte_string'; -import { SortedSet } from '../util/sorted_set'; +import { primitiveComparator } from '../util/misc'; +import { SortedMap } from '../util/sorted_map'; /** * An event from the RemoteStore. It is split into targetChanges (changes to the @@ -43,10 +44,11 @@ export class RemoteEvent { */ readonly targetChanges: Map, /** - * A set of targets that is known to be inconsistent. Listens for these - * targets should be re-established without resume tokens. + * A map of targets that is known to be inconsistent, and the purpose for + * re-listening. Listens for these targets should be re-established without + * resume tokens. */ - readonly targetMismatches: SortedSet, + readonly targetMismatches: SortedMap, /** * A set of which documents have changed or been deleted, along with the * doc's new values (if not deleted). @@ -82,7 +84,7 @@ export class RemoteEvent { return new RemoteEvent( SnapshotVersion.min(), targetChanges, - targetIdSet(), + new SortedMap(primitiveComparator), mutableDocumentMap(), documentKeySet() ); diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index 68196a50ad2..8cb361bfe09 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -24,7 +24,7 @@ import { localStoreGetNextMutationBatch } from '../local/local_store_impl'; import { isIndexedDbTransactionError } from '../local/simple_db'; -import { TargetData, TargetPurpose } from '../local/target_data'; +import { TargetData } from '../local/target_data'; import { MutationResult } from '../model/mutation'; import { MutationBatch, MutationBatchResult } from '../model/mutation_batch'; import { debugAssert, debugCast } from '../util/assert'; @@ -587,7 +587,7 @@ function raiseWatchSnapshot( // Re-establish listens for the targets that have been invalidated by // existence filter mismatches. - remoteEvent.targetMismatches.forEach(targetId => { + remoteEvent.targetMismatches.forEach((targetId, targetPurpose) => { const targetData = remoteStoreImpl.listenTargets.get(targetId); if (!targetData) { // A watched target might have been removed already. @@ -615,7 +615,7 @@ function raiseWatchSnapshot( const requestTargetData = new TargetData( targetData.target, targetId, - TargetPurpose.ExistenceFilterMismatch, + targetPurpose, targetData.sequenceNumber ); sendWatchRequest(remoteStoreImpl, requestTargetData); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 39a787a943d..1945ddbff4e 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -1008,7 +1008,7 @@ export function toListenRequestLabels( serializer: JsonProtoSerializer, targetData: TargetData ): ProtoApiClientObjectMap | null { - const value = toLabel(serializer, targetData.purpose); + const value = toLabel(targetData.purpose); if (value == null) { return null; } else { @@ -1018,15 +1018,14 @@ export function toListenRequestLabels( } } -function toLabel( - serializer: JsonProtoSerializer, - purpose: TargetPurpose -): string | null { +export function toLabel(purpose: TargetPurpose): string | null { switch (purpose) { case TargetPurpose.Listen: return null; case TargetPurpose.ExistenceFilterMismatch: return 'existence-filter-mismatch'; + case TargetPurpose.ExistenceFilterMismatchBloom: + return 'existence-filter-mismatch-bloom'; case TargetPurpose.LimboResolution: return 'limbo-document'; default: diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 82ef17213dd..dd201c4ec1f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -87,6 +87,11 @@ export const enum WatchTargetChangeState { Reset } +const enum BloomFilterApplicationStatus { + Success, + Skipped, + FalsePositive +} export class WatchTargetChange { constructor( /** What kind of change occurred to the watch target. */ @@ -280,11 +285,13 @@ export class WatchChangeAggregator { private pendingDocumentTargetMapping = documentTargetMap(); /** - * A list of targets with existence filter mismatches. These targets are + * A map of targets with existence filter mismatches. These targets are * known to be inconsistent and their listens needs to be re-established by * RemoteStore. */ - private pendingTargetResets = new SortedSet(primitiveComparator); + private pendingTargetResets = new SortedMap( + primitiveComparator + ); /** * Processes and adds the DocumentWatchChange to the current set of changes. @@ -422,31 +429,40 @@ export class WatchChangeAggregator { // raise a snapshot with `isFromCache:true`. if (currentSize !== expectedCount) { // Apply bloom filter to identify and mark removed documents. - const bloomFilterApplied = this.applyBloomFilter( - watchChange, - currentSize - ); - if (!bloomFilterApplied) { + const status = this.applyBloomFilter(watchChange, currentSize); + + if (status !== BloomFilterApplicationStatus.Success) { // If bloom filter application fails, we reset the mapping and // trigger re-run of the query. this.resetTarget(targetId); - this.pendingTargetResets = this.pendingTargetResets.add(targetId); + + const purpose: TargetPurpose = + status === BloomFilterApplicationStatus.FalsePositive + ? TargetPurpose.ExistenceFilterMismatchBloom + : TargetPurpose.ExistenceFilterMismatch; + this.pendingTargetResets = this.pendingTargetResets.insert( + targetId, + purpose + ); } } } } } - /** Returns whether a bloom filter removed the deleted documents successfully. */ + /** + * Apply bloom filter to remove the deleted documents, and return the + * application status. + */ private applyBloomFilter( watchChange: ExistenceFilterChange, currentCount: number - ): boolean { + ): BloomFilterApplicationStatus { const { unchangedNames, count: expectedCount } = watchChange.existenceFilter; if (!unchangedNames || !unchangedNames.bits) { - return false; + return BloomFilterApplicationStatus.Skipped; } const { @@ -464,7 +480,7 @@ export class WatchChangeAggregator { err.message + '); ignoring the bloom filter and falling back to full re-query.' ); - return false; + return BloomFilterApplicationStatus.Skipped; } else { throw err; } @@ -480,7 +496,11 @@ export class WatchChangeAggregator { } else { logWarn('Applying bloom filter failed: ', err); } - return false; + return BloomFilterApplicationStatus.Skipped; + } + + if (bloomFilter.bitCount === 0) { + return BloomFilterApplicationStatus.Skipped; } const removedDocumentCount = this.filterRemovedDocuments( @@ -488,7 +508,11 @@ export class WatchChangeAggregator { bloomFilter ); - return expectedCount === currentCount - removedDocumentCount; + if (expectedCount !== currentCount - removedDocumentCount) { + return BloomFilterApplicationStatus.FalsePositive; + } + + return BloomFilterApplicationStatus.Success; } /** @@ -599,7 +623,9 @@ export class WatchChangeAggregator { this.pendingDocumentUpdates = mutableDocumentMap(); this.pendingDocumentTargetMapping = documentTargetMap(); - this.pendingTargetResets = new SortedSet(primitiveComparator); + this.pendingTargetResets = new SortedMap( + primitiveComparator + ); return remoteEvent; } diff --git a/packages/firestore/test/integration/api/query.test.ts b/packages/firestore/test/integration/api/query.test.ts index 2438d8bdb6c..94e8221100f 100644 --- a/packages/firestore/test/integration/api/query.test.ts +++ b/packages/firestore/test/integration/api/query.test.ts @@ -2061,8 +2061,8 @@ 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 + // 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)( diff --git a/packages/firestore/test/unit/remote/remote_event.test.ts b/packages/firestore/test/unit/remote/remote_event.test.ts index 36646a5dba6..36596a8997c 100644 --- a/packages/firestore/test/unit/remote/remote_event.test.ts +++ b/packages/firestore/test/unit/remote/remote_event.test.ts @@ -431,6 +431,9 @@ describe('RemoteEvent', () => { expectTargetChangeEquals(event.targetChanges.get(1)!, expected); }); + // TODO(b/272564458): Add test cases for existence filter with bloom filter, + // one will skip the re-query, one will yield false positive result and clears + // target mapping. it('existence filters clears target mapping', () => { const targets = listens(1, 2); @@ -459,6 +462,9 @@ describe('RemoteEvent', () => { event = aggregator.createRemoteEvent(version(3)); expect(event.documentUpdates.size).to.equal(0); expect(event.targetMismatches.size).to.equal(1); + expect(event.targetMismatches.get(1)).to.equal( + TargetPurpose.ExistenceFilterMismatch + ); expect(event.targetChanges.size).to.equal(1); const expected = updateMapping( @@ -499,6 +505,9 @@ describe('RemoteEvent', () => { const event = aggregator.createRemoteEvent(version(3)); expect(event.documentUpdates.size).to.equal(1); expect(event.targetMismatches.size).to.equal(1); + expect(event.targetMismatches.get(1)).to.equal( + TargetPurpose.ExistenceFilterMismatch + ); expect(event.targetChanges.get(1)!.current).to.be.false; }); diff --git a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts index 334d47ce178..2b67ef8212e 100644 --- a/packages/firestore/test/unit/specs/existence_filter_spec.test.ts +++ b/packages/firestore/test/unit/specs/existence_filter_spec.test.ts @@ -16,6 +16,7 @@ */ import { newQueryForPath } from '../../../src/core/query'; +import { TargetPurpose } from '../../../src/local/target_data'; import { Code } from '../../../src/util/error'; import { deletedDoc, @@ -305,7 +306,11 @@ describeSpec('Existence Filters:', [], () => { // BloomFilter correctly identifies docC is deleted, but yields false // positive results for docB. Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatchBloom + }) ); } ); @@ -394,7 +399,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); } ); @@ -424,7 +433,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); } ); @@ -452,7 +465,11 @@ describeSpec('Existence Filters:', [], () => { .watchSnapshots(2000) // Re-run query is triggered. .expectEvents(query1, { fromCache: true }) - .expectActiveTargets({ query: query1, resumeToken: '' }) + .expectActiveTargets({ + query: query1, + resumeToken: '', + targetPurpose: TargetPurpose.ExistenceFilterMismatch + }) ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index a18e5742256..5cff73f45a3 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -28,6 +28,7 @@ import { import { canonifyTarget, Target, targetEquals } from '../../../src/core/target'; import { TargetIdGenerator } from '../../../src/core/target_id_generator'; import { TargetId } from '../../../src/core/types'; +import { TargetPurpose } from '../../../src/local/target_data'; import { Document } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; import { FieldIndex } from '../../../src/model/field_index'; @@ -77,6 +78,7 @@ export interface ActiveTargetSpec { resumeToken?: string; readTime?: TestSnapshotVersion; expectedCount?: number; + targetPurpose?: TargetPurpose; } export interface ActiveTargetMap { @@ -538,18 +540,26 @@ export class SpecBuilder { resumeToken?: string; readTime?: TestSnapshotVersion; expectedCount?: number; + targetPurpose?: TargetPurpose; }> ): this { this.assertStep('Active target expectation requires previous step'); const currentStep = this.currentStep!; this.clientState.activeTargets = {}; - targets.forEach(({ query, resumeToken, readTime, expectedCount }) => { - this.addQueryToActiveTargets(this.getTargetId(query), query, { - resumeToken, - readTime, - expectedCount - }); - }); + targets.forEach( + ({ query, resumeToken, readTime, expectedCount, targetPurpose }) => { + this.addQueryToActiveTargets( + this.getTargetId(query), + query, + { + resumeToken, + readTime, + expectedCount + }, + targetPurpose + ); + } + ); currentStep.expectedState = currentStep.expectedState || {}; currentStep.expectedState.activeTargets = { ...this.activeTargets }; return this; @@ -1093,7 +1103,8 @@ export class SpecBuilder { private addQueryToActiveTargets( targetId: number, query: Query, - resume?: ResumeSpec + resume?: ResumeSpec, + targetPurpose?: TargetPurpose ): void { if (!(resume?.resumeToken || resume?.readTime) && resume?.expectedCount) { fail('Expected count is present without a resume token or read time.'); @@ -1111,14 +1122,16 @@ export class SpecBuilder { queries: [SpecBuilder.queryToSpec(query), ...activeQueries], resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } else { this.activeTargets[targetId] = { queries: activeQueries, resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } } else { @@ -1126,7 +1139,8 @@ export class SpecBuilder { queries: [SpecBuilder.queryToSpec(query)], resumeToken: resume?.resumeToken || '', readTime: resume?.readTime, - expectedCount: resume?.expectedCount + expectedCount: resume?.expectedCount, + targetPurpose }; } } diff --git a/packages/firestore/test/unit/specs/spec_test_components.ts b/packages/firestore/test/unit/specs/spec_test_components.ts index 1436efa3fcd..c51bc1a461b 100644 --- a/packages/firestore/test/unit/specs/spec_test_components.ts +++ b/packages/firestore/test/unit/specs/spec_test_components.ts @@ -53,6 +53,7 @@ import { Mutation } from '../../../src/model/mutation'; import { encodeBase64 } from '../../../src/platform/base64'; import { newSerializer } from '../../../src/platform/serializer'; import * as api from '../../../src/protos/firestore_proto_api'; +import { ApiClientObjectMap } from '../../../src/protos/firestore_proto_api'; import { Connection, Stream } from '../../../src/remote/connection'; import { Datastore, newDatastore } from '../../../src/remote/datastore'; import { WriteRequest } from '../../../src/remote/persistent_stream'; @@ -259,7 +260,12 @@ export class MockConnection implements Connection { * Tracks the currently active watch targets as detected by the mock watch * stream, as a mapping from target ID to query Target. */ - activeTargets: { [targetId: number]: api.Target } = {}; + activeTargets: { + [targetId: number]: { + target: api.Target; + labels?: ApiClientObjectMap; + }; + } = {}; /** A Deferred that is resolved once watch opens. */ watchOpen = new Deferred(); @@ -398,7 +404,10 @@ export class MockConnection implements Connection { ++this.watchStreamRequestCount; if (request.addTarget) { const targetId = request.addTarget.targetId!; - this.activeTargets[targetId] = request.addTarget; + this.activeTargets[targetId] = { + target: request.addTarget, + labels: request.labels + }; } else if (request.removeTarget) { delete this.activeTargets[request.removeTarget]; } else { diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 137f51d6d54..230297146ed 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -105,6 +105,7 @@ import { import { mapCodeFromRpcCode } from '../../../src/remote/rpc_error'; import { JsonProtoSerializer, + toLabel, toMutation, toTarget, toVersion @@ -1095,7 +1096,17 @@ abstract class TestRunner { undefined, 'Expected active target not found: ' + JSON.stringify(expected) ); - const actualTarget = actualTargets[targetId]; + const { target: actualTarget, labels } = actualTargets[targetId]; + + if (expected.targetPurpose) { + debugAssert( + labels !== undefined, + "Actual listen request doesn't have a 'goog-listen-tags'" + ); + expect(labels['goog-listen-tags']).to.equal( + toLabel(expected.targetPurpose) + ); + } // TODO(mcg): populate the purpose of the target once it's possible to // encode that in the spec tests. For now, hard-code that it's a listen