Skip to content

Commit d73d97c

Browse files
dconeybedconeybe
authored
Update the integration test to verify that bloom filter averted full requery (#11440)
1 parent 665372e commit d73d97c

File tree

4 files changed

+220
-109
lines changed

4 files changed

+220
-109
lines changed

‎Firestore/Example/Tests/Integration/API/FIRQueryTests.mm‎

Lines changed: 126 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -1192,7 +1192,10 @@ - (void)testOrderByEquality {
11921192
matchesResult:@[ @"doc6", @"doc3" ]];
11931193
}
11941194

1195-
- (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes {
1195+
- (void)testResumingAQueryShouldUseBloomFilterToAvoidFullRequery {
1196+
using firebase::firestore::testutil::CaptureExistenceFilterMismatches;
1197+
using firebase::firestore::util::TestingHooks;
1198+
11961199
// Set this test to stop when the first failure occurs because some test assertion failures make
11971200
// the rest of the test not applicable or will even crash.
11981201
[self setContinueAfterFailure:NO];
@@ -1204,100 +1207,135 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes {
12041207
[testDocs setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(1000 + i)]];
12051208
}
12061209

1207-
// Create 100 documents in a new collection.
1208-
FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs];
1209-
1210-
// Run a query to populate the local cache with the 100 documents and a resume token.
1211-
FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef
1212-
source:FIRFirestoreSourceDefault];
1213-
XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value");
1214-
NSArray<FIRDocumentReference *> *createdDocuments =
1215-
FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1);
1216-
1217-
// Delete 50 of the 100 documents. Do this in a transaction, rather than
1218-
// [FIRDocumentReference deleteDocument], to avoid affecting the local cache.
1219-
NSSet<NSString *> *deletedDocumentIds;
1220-
{
1221-
NSMutableArray<NSString *> *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
1222-
XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"];
1223-
[collRef.firestore
1224-
runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
1225-
for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) {
1226-
FIRDocumentReference *documentToDelete = createdDocuments[i];
1227-
[transaction deleteDocument:documentToDelete];
1228-
[deletedDocumentIdsAccumulator addObject:documentToDelete.documentID];
1229-
}
1230-
return @"document deletion successful";
1231-
}
1232-
completion:^(id, NSError *) {
1233-
[expectation fulfill];
1234-
}];
1235-
[self awaitExpectation:expectation];
1236-
deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator];
1237-
}
1238-
XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size");
1239-
1240-
// Wait for 10 seconds, during which Watch will stop tracking the query and will send an existence
1241-
// filter rather than "delete" events when the query is resumed.
1242-
[NSThread sleepForTimeInterval:10.0f];
1243-
1244-
// Resume the query and save the resulting snapshot for verification.
1245-
// Use some internal testing hooks to "capture" the existence filter mismatches to verify them.
1246-
FIRQuerySnapshot *querySnapshot2;
1247-
std::vector<firebase::firestore::util::TestingHooks::ExistenceFilterMismatchInfo>
1248-
existence_filter_mismatches =
1249-
firebase::firestore::testutil::CaptureExistenceFilterMismatches([&] {
1250-
querySnapshot2 = [self readDocumentSetForRef:collRef source:FIRFirestoreSourceDefault];
1251-
});
1252-
1253-
// Verify that the snapshot from the resumed query contains the expected documents; that is,
1254-
// that it contains the 50 documents that were _not_ deleted.
1255-
// TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to
1256-
// send an existence filter. At the time of writing, the Firestore emulator fails to send an
1257-
// existence filter, resulting in the client including the deleted documents in the snapshot
1258-
// of the resumed query.
1259-
if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) {
1260-
NSSet<NSString *> *actualDocumentIds =
1261-
[NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)];
1262-
NSSet<NSString *> *expectedDocumentIds;
1210+
// Each iteration of the "while" loop below runs a single iteration of the test. The test will
1211+
// be run multiple times only if a bloom filter false positive occurs.
1212+
int attemptNumber = 0;
1213+
while (true) {
1214+
attemptNumber++;
1215+
1216+
// Create 100 documents in a new collection.
1217+
FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs];
1218+
1219+
// Run a query to populate the local cache with the 100 documents and a resume token.
1220+
FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef
1221+
source:FIRFirestoreSourceDefault];
1222+
XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value");
1223+
NSArray<FIRDocumentReference *> *createdDocuments =
1224+
FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1);
1225+
1226+
// Delete 50 of the 100 documents. Do this in a transaction, rather than
1227+
// [FIRDocumentReference deleteDocument], to avoid affecting the local cache.
1228+
NSSet<NSString *> *deletedDocumentIds;
12631229
{
1264-
NSMutableArray<NSString *> *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
1265-
for (FIRDocumentReference *documentRef in createdDocuments) {
1266-
if (![deletedDocumentIds containsObject:documentRef.documentID]) {
1267-
[expectedDocumentIdsAccumulator addObject:documentRef.documentID];
1230+
NSMutableArray<NSString *> *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
1231+
XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"];
1232+
[collRef.firestore
1233+
runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) {
1234+
for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) {
1235+
FIRDocumentReference *documentToDelete = createdDocuments[i];
1236+
[transaction deleteDocument:documentToDelete];
1237+
[deletedDocumentIdsAccumulator addObject:documentToDelete.documentID];
1238+
}
1239+
return @"document deletion successful";
1240+
}
1241+
completion:^(id, NSError *) {
1242+
[expectation fulfill];
1243+
}];
1244+
[self awaitExpectation:expectation];
1245+
deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator];
1246+
}
1247+
XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size");
1248+
1249+
// Wait for 10 seconds, during which Watch will stop tracking the query and will send an
1250+
// existence filter rather than "delete" events when the query is resumed.
1251+
[NSThread sleepForTimeInterval:10.0f];
1252+
1253+
// Resume the query and save the resulting snapshot for verification.
1254+
// Use some internal testing hooks to "capture" the existence filter mismatches to verify that
1255+
// Watch sent a bloom filter, and it was used to avert a full requery.
1256+
FIRQuerySnapshot *querySnapshot2;
1257+
std::vector<TestingHooks::ExistenceFilterMismatchInfo> existence_filter_mismatches =
1258+
CaptureExistenceFilterMismatches([&] {
1259+
querySnapshot2 = [self readDocumentSetForRef:collRef source:FIRFirestoreSourceDefault];
1260+
});
1261+
1262+
// Verify that the snapshot from the resumed query contains the expected documents; that is,
1263+
// that it contains the 50 documents that were _not_ deleted.
1264+
// TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to
1265+
// send an existence filter. At the time of writing, the Firestore emulator fails to send an
1266+
// existence filter, resulting in the client including the deleted documents in the snapshot
1267+
// of the resumed query.
1268+
if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) {
1269+
NSSet<NSString *> *actualDocumentIds =
1270+
[NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)];
1271+
NSSet<NSString *> *expectedDocumentIds;
1272+
{
1273+
NSMutableArray<NSString *> *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init];
1274+
for (FIRDocumentReference *documentRef in createdDocuments) {
1275+
if (![deletedDocumentIds containsObject:documentRef.documentID]) {
1276+
[expectedDocumentIdsAccumulator addObject:documentRef.documentID];
1277+
}
12681278
}
1279+
expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator];
1280+
}
1281+
if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) {
1282+
NSArray<NSString *> *unexpectedDocumentIds =
1283+
SortedStringsNotIn(actualDocumentIds, expectedDocumentIds);
1284+
NSArray<NSString *> *missingDocumentIds =
1285+
SortedStringsNotIn(expectedDocumentIds, actualDocumentIds);
1286+
XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): "
1287+
@"%lu unexpected and %lu missing; "
1288+
@"unexpected documents: %@; missing documents: %@",
1289+
(unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count,
1290+
(unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count,
1291+
[unexpectedDocumentIds componentsJoinedByString:@", "],
1292+
[missingDocumentIds componentsJoinedByString:@", "]);
12691293
}
1270-
expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator];
12711294
}
1272-
if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) {
1273-
NSArray<NSString *> *unexpectedDocumentIds =
1274-
SortedStringsNotIn(actualDocumentIds, expectedDocumentIds);
1275-
NSArray<NSString *> *missingDocumentIds =
1276-
SortedStringsNotIn(expectedDocumentIds, actualDocumentIds);
1277-
XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): "
1278-
@"%lu unexpected and %lu missing; "
1279-
@"unexpected documents: %@; missing documents: %@",
1280-
(unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count,
1281-
(unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count,
1282-
[unexpectedDocumentIds componentsJoinedByString:@", "],
1283-
[missingDocumentIds componentsJoinedByString:@", "]);
1295+
1296+
// Skip the verification of the existence filter mismatch when testing against the Firestore
1297+
// emulator because the Firestore emulator fails to to send an existence filter at all.
1298+
// TODO(b/270731363): Enable the verification of the existence filter mismatch once the
1299+
// Firestore emulator is fixed to send an existence filter.
1300+
if ([FSTIntegrationTestCase isRunningAgainstEmulator]) {
1301+
return;
12841302
}
1285-
}
12861303

1287-
// Skip the verification of the existence filter mismatch when testing against the Firestore
1288-
// emulator because the Firestore emulator fails to to send an existence filter at all.
1289-
// TODO(b/270731363): Enable the verification of the existence filter mismatch once the Firestore
1290-
// emulator is fixed to send an existence filter.
1291-
if ([FSTIntegrationTestCase isRunningAgainstEmulator]) {
1292-
return;
1293-
}
1304+
// Verify that Watch sent an existence filter with the correct counts when the query was
1305+
// resumed.
1306+
XCTAssertEqual(existence_filter_mismatches.size(), size_t{1},
1307+
@"Watch should have sent exactly 1 existence filter");
1308+
const TestingHooks::ExistenceFilterMismatchInfo &existenceFilterMismatchInfo =
1309+
existence_filter_mismatches[0];
1310+
XCTAssertEqual(existenceFilterMismatchInfo.local_cache_count, 100);
1311+
XCTAssertEqual(existenceFilterMismatchInfo.existence_filter_count, 50);
1312+
1313+
// Verify that Watch sent a valid bloom filter.
1314+
const absl::optional<TestingHooks::BloomFilterInfo> &bloom_filter =
1315+
existence_filter_mismatches[0].bloom_filter;
1316+
XCTAssertTrue(bloom_filter.has_value(),
1317+
"Watch should have included a bloom filter in the existence filter");
1318+
XCTAssertGreaterThan(bloom_filter->hash_count, 0);
1319+
XCTAssertGreaterThan(bloom_filter->bitmap_length, 0);
1320+
XCTAssertGreaterThan(bloom_filter->padding, 0);
1321+
XCTAssertLessThan(bloom_filter->padding, 8);
1322+
1323+
// Verify that the bloom filter was successfully used to avert a full requery. If a false
1324+
// positive occurred then retry the entire test. Although statistically rare, false positives
1325+
// are expected to happen occasionally. When a false positive _does_ happen, just retry the test
1326+
// with a different set of documents. If that retry _also_ experiences a false positive, then
1327+
// fail the test because that is so improbable that something must have gone wrong.
1328+
if (attemptNumber == 1 && !bloom_filter->applied) {
1329+
continue;
1330+
}
1331+
1332+
XCTAssertTrue(bloom_filter->applied,
1333+
@"The bloom filter should have been successfully applied with attemptNumber=%@",
1334+
@(attemptNumber));
12941335

1295-
// Verify that Watch sent an existence filter with the correct counts when the query was resumed.
1296-
XCTAssertEqual(existence_filter_mismatches.size(), size_t{1});
1297-
firebase::firestore::util::TestingHooks::ExistenceFilterMismatchInfo &info =
1298-
existence_filter_mismatches[0];
1299-
XCTAssertEqual(info.local_cache_count, 100);
1300-
XCTAssertEqual(info.existence_filter_count, 50);
1336+
// Break out of the test loop now that the test passes.
1337+
break;
1338+
}
13011339
}
13021340

13031341
@end

‎Firestore/core/src/remote/remote_event.cc‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,29 @@ std::vector<TargetId> WatchChangeAggregator::GetTargetIds(
212212
return result;
213213
}
214214

215+
namespace {
216+
217+
TestingHooks::ExistenceFilterMismatchInfo
218+
create_existence_filter_mismatch_info_for_testing_hooks(
219+
BloomFilterApplicationStatus status,
220+
int local_cache_count,
221+
const ExistenceFilterWatchChange& existence_filter) {
222+
absl::optional<TestingHooks::BloomFilterInfo> bloom_filter;
223+
if (existence_filter.filter().bloom_filter_parameters().has_value()) {
224+
const BloomFilterParameters& bloom_filter_parameters =
225+
existence_filter.filter().bloom_filter_parameters().value();
226+
bloom_filter = {status == BloomFilterApplicationStatus::kSuccess,
227+
bloom_filter_parameters.hash_count,
228+
static_cast<int>(bloom_filter_parameters.bitmap.size()),
229+
bloom_filter_parameters.padding};
230+
}
231+
232+
return {local_cache_count, existence_filter.filter().count(),
233+
std::move(bloom_filter)};
234+
}
235+
236+
} // namespace
237+
215238
void WatchChangeAggregator::HandleExistenceFilter(
216239
const ExistenceFilterWatchChange& existence_filter) {
217240
TargetId target_id = existence_filter.target_id();
@@ -255,7 +278,8 @@ void WatchChangeAggregator::HandleExistenceFilter(
255278
}
256279

257280
TestingHooks::GetInstance().NotifyOnExistenceFilterMismatch(
258-
{current_size, expected_count});
281+
create_existence_filter_mismatch_info_for_testing_hooks(
282+
status, current_size, existence_filter));
259283
}
260284
}
261285
}

‎Firestore/core/src/util/testing_hooks.h‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#include "Firestore/core/src/api/listener_registration.h"
2626
#include "Firestore/core/src/util/no_destructor.h"
27+
#include "absl/types/optional.h"
2728

2829
namespace firebase {
2930
namespace firestore {
@@ -39,6 +40,28 @@ class TestingHooks final {
3940
/** Returns the singleton instance of this class. */
4041
static TestingHooks& GetInstance();
4142

43+
/**
44+
* Information about the bloom filter provided by Watch in the ExistenceFilter
45+
* message's `unchangedNames` field.
46+
*/
47+
struct BloomFilterInfo {
48+
/**
49+
* Whether a full requery was averted by using the bloom filter. If false,
50+
* then something happened, such as a false positive, to prevent using the
51+
* bloom filter to avoid a full requery.
52+
*/
53+
bool applied = false;
54+
55+
/** The number of hash functions used in the bloom filter. */
56+
int hash_count = -1;
57+
58+
/** The number of bytes in the bloom filter's bitmask. */
59+
int bitmap_length = -1;
60+
61+
/** The number of bits of padding in the last byte of the bloom filter. */
62+
int padding = -1;
63+
};
64+
4265
/**
4366
* Information about an existence filter mismatch, as specified to callbacks
4467
* registered with `OnExistenceFilterMismatch()`.
@@ -52,6 +75,13 @@ class TestingHooks final {
5275
* specified in the `ExistenceFilter` message's `count` field.
5376
*/
5477
int existence_filter_count = -1;
78+
79+
/**
80+
* Information about the bloom filter provided by Watch in the
81+
* ExistenceFilter message's `unchangedNames` field. If empty, then that
82+
* means that Watch did _not_ provide a bloom filter.
83+
*/
84+
absl::optional<BloomFilterInfo> bloom_filter;
5585
};
5686

5787
using ExistenceFilterMismatchCallback =

0 commit comments

Comments
 (0)