Skip to content

Commit

Permalink
Update test 'bloom filter should correctly encode special unicode cha…
Browse files Browse the repository at this point in the history 
…racters' in query.test.ts (#7413)
  • Loading branch information
@dconeybe
dconeybe committed Jul 6, 2023
1 parent 684bb4e commit c29156a
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 111 deletions.
55 changes: 33 additions & 22 deletions packages/firestore/src/remote/watch_change.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -451,7 +451,8 @@ export class WatchChangeAggregator {
}
TestingHooks.instance?.notifyOnExistenceFilterMismatch(
createExistenceFilterMismatchInfoForTestingHooks(
status,
applyResult.status,
applyResult.bloomFilterMightContain ?? null,
currentSize,
watchChange.existenceFilter
)
Expand All @@ -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 {
Expand All @@ -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;
}
Expand All @@ -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 };
}

/**
Expand All @@ -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++;
}
Expand Down Expand Up @@ -829,6 +836,7 @@ function snapshotChangesMap(): SortedMap<DocumentKey, ChangeType> {

function createExistenceFilterMismatchInfoForTestingHooks(
status: BloomFilterApplicationStatus,
bloomFilterMightContain: null | ((documentPath: string) => boolean),
localCacheCount: number,
existenceFilter: ExistenceFilter
): TestingHooksExistenceFilterMismatchInfo {
Expand All @@ -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;
Expand Down
15 changes: 15 additions & 0 deletions packages/firestore/src/util/testing_hooks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}

Expand Down
177 changes: 88 additions & 89 deletions packages/firestore/test/integration/api/query.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/test/integration/util/testing_hooks_util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,5 +69,6 @@ export interface ExistenceFilterMismatchInfo {
hashCount: number;
bitmapLength: number;
padding: number;
mightContain?(documentPath: string): boolean;
};
}

0 comments on commit c29156a

Please sign in to comment.