From e4f358c986ae780a2daf7c27770dbe4d1d975c77 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 00:46:17 -0400 Subject: [PATCH 1/5] FIRQueryTests.mm: add a test for resuming a query with existence filter --- .../Tests/Integration/API/FIRQueryTests.mm | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index f311a140de0..e4470b92232 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -22,6 +22,16 @@ #import "Firestore/Example/Tests/Util/FSTHelpers.h" #import "Firestore/Example/Tests/Util/FSTIntegrationTestCase.h" +namespace { + +NSArray *SortedStringsNotIn(NSSet *set, NSSet *remove) { + NSMutableSet *mutableSet = [NSMutableSet setWithSet:set]; + [mutableSet minusSet:remove]; + return [mutableSet.allObjects sortedArrayUsingSelector:@selector(caseInsensitiveCompare:)]; +} + +} // namespace + @interface FIRQueryTests : FSTIntegrationTestCase @end @@ -1180,4 +1190,101 @@ - (void)testOrderByEquality { matchesResult:@[ @"doc6", @"doc3" ]]; } +- (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { + // Prepare the names and contents of the 100 documents to create. + NSMutableDictionary *> *testData = + [[NSMutableDictionary alloc] init]; + for (int i = 0; i < 100; i++) { + [testData setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(i)]]; + } + + // Create 100 documents in a new collection. + FIRCollectionReference *collRef = [self collectionRefWithDocuments:testData]; + + // Run a query to populate the local cache with the 100 documents and a resume token. + NSArray *createdDocuments; + { + FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + NSMutableArray *createdDocumentsAccumulator = + [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in querySnapshot1.documents) { + [createdDocumentsAccumulator addObject:documentSnapshot.reference]; + } + createdDocuments = [createdDocumentsAccumulator copy]; + } + XCTAssertEqual(createdDocuments.count, 100, @"createdDocuments has the wrong size"); + + // Delete 50 of the 100 documents. Do this in a transaction, rather than + // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. + NSSet *deletedDocumentIds; + { + NSMutableArray *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; + XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; + [collRef.firestore + runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) { + for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { + FIRDocumentReference *documentToDelete = createdDocuments[i]; + [transaction deleteDocument:documentToDelete]; + [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID]; + } + return @"document deletion successful"; + } + completion:^(id, NSError *) { + [expectation fulfill]; + }]; + [self awaitExpectation:expectation]; + deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator]; + } + XCTAssertEqual(deletedDocumentIds.count, 50, @"deletedDocumentIds has the wrong size"); + + // 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. + [NSThread sleepForTimeInterval:10.0f]; + + // Resume the query and save the resulting snapshot for verification. + FIRQuerySnapshot *querySnapshot2 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + + // Verify that the snapshot from the resumed query contains the expected documents; that is, + // that it contains the 50 documents that were _not_ deleted. + // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to + // send an existence filter. At the time of writing, the Firestore emulator fails to send an + // existence filter, resulting in the client including the deleted documents in the snapshot + // of the resumed query. + if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) { + NSSet *actualDocumentIds; + { + NSMutableArray *actualDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in querySnapshot2.documents) { + [actualDocumentIdsAccumulator addObject:documentSnapshot.documentID]; + } + actualDocumentIds = [NSSet setWithArray:actualDocumentIdsAccumulator]; + } + NSSet *expectedDocumentIds; + { + NSMutableArray *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; + for (FIRDocumentReference *documentRef in createdDocuments) { + if (![deletedDocumentIds containsObject:documentRef.documentID]) { + [expectedDocumentIdsAccumulator addObject:documentRef.documentID]; + } + } + expectedDocumentIds = [NSSet setWithArray:expectedDocumentIdsAccumulator]; + } + if (![actualDocumentIds isEqualToSet:expectedDocumentIds]) { + NSArray *unexpectedDocumentIds = + SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); + NSArray *missingDocumentIds = + SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); + XCTFail(@"The snapshot contained %@ documents (expected %@): %@ unexpected and %@ missing; " + @"unexpected documents: %@; missing documents: %@", + @(actualDocumentIds.count), @(expectedDocumentIds.count), + @(unexpectedDocumentIds.count), @(missingDocumentIds.count), + [unexpectedDocumentIds componentsJoinedByString:@", "], + [missingDocumentIds componentsJoinedByString:@", "]); + } + } +} + @end From b8a9c80909cc3d4e1ba2a2dba948e4eff206d710 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 00:52:36 -0400 Subject: [PATCH 2/5] FSTIntegrationTestCase.mm: use a WriteBatch to create documents, for efficiency --- .../Tests/Util/FSTIntegrationTestCase.mm | 42 ++++++++++++++++++- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm index bbff7d14abe..439eee6a833 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm @@ -25,6 +25,7 @@ #import #import #import +#import #include #include @@ -366,11 +367,48 @@ - (FIRCollectionReference *)collectionRefWithDocuments: - (void)writeAllDocuments:(NSDictionary *> *)documents toCollection:(FIRCollectionReference *)collection { + NSMutableArray *commits = [[NSMutableArray alloc] init]; + __block FIRWriteBatch *writeBatch = nil; + __block int writeBatchSize = 0; + [documents enumerateKeysAndObjectsUsingBlock:^(NSString *key, NSDictionary *value, BOOL *) { - FIRDocumentReference *ref = [collection documentWithPath:key]; - [self writeDocumentRef:ref data:value]; + if (writeBatch == nil) { + writeBatch = [collection.firestore batch]; + } + + [writeBatch setData:value forDocument:[collection documentWithPath:key]]; + writeBatchSize++; + + // Write batches are capped at 500 writes. Use 400 just to be safe. + if (writeBatchSize == 400) { + XCTestExpectation *commitExpectation = [self expectationWithDescription:@"WriteBatch commit"]; + [writeBatch commitWithCompletion:^(NSError *_Nullable error) { + [commitExpectation fulfill]; + if (error != nil) { + XCTFail(@"WriteBatch commit failed: %@", error); + } + }]; + [commits addObject:commitExpectation]; + writeBatch = nil; + writeBatchSize = 0; + } }]; + + if (writeBatch != nil) { + XCTestExpectation *commitExpectation = [self expectationWithDescription:@"WriteBatch commit"]; + [writeBatch commitWithCompletion:^(NSError *_Nullable error) { + [commitExpectation fulfill]; + if (error != nil) { + XCTFail(@"WriteBatch commit failed: %@", error); + } + }]; + [commits addObject:commitExpectation]; + } + + for (XCTestExpectation *commitExpectation in commits) { + [self awaitExpectation:commitExpectation]; + } } - (void)readerAndWriterOnDocumentRef:(void (^)(FIRDocumentReference *readerRef, From aed88292444ab3a7c5023527b669136e19f34dd1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 10:46:11 -0400 Subject: [PATCH 3/5] FIRQueryTests.mm: fix signed/unsigned integer conversion warnings --- .../Example/Tests/Integration/API/FIRQueryTests.mm | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index e4470b92232..786ad0d155a 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1213,7 +1213,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { } createdDocuments = [createdDocumentsAccumulator copy]; } - XCTAssertEqual(createdDocuments.count, 100, @"createdDocuments has the wrong size"); + XCTAssertEqual(createdDocuments.count, 100u, @"createdDocuments has the wrong size"); // Delete 50 of the 100 documents. Do this in a transaction, rather than // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. @@ -1237,7 +1237,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { [self awaitExpectation:expectation]; deletedDocumentIds = [NSSet setWithArray:deletedDocumentIdsAccumulator]; } - XCTAssertEqual(deletedDocumentIds.count, 50, @"deletedDocumentIds has the wrong size"); + XCTAssertEqual(deletedDocumentIds.count, 50u, @"deletedDocumentIds has the wrong size"); // 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. @@ -1277,10 +1277,11 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); NSArray *missingDocumentIds = SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); - XCTFail(@"The snapshot contained %@ documents (expected %@): %@ unexpected and %@ missing; " + XCTFail(@"The snapshot contained %lu documents (expected %lu): " + @"%lu unexpected and %lu missing; " @"unexpected documents: %@; missing documents: %@", - @(actualDocumentIds.count), @(expectedDocumentIds.count), - @(unexpectedDocumentIds.count), @(missingDocumentIds.count), + (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count, + (unsigned long)unexpectedDocumentIds.count, (unsigned long)missingDocumentIds.count, [unexpectedDocumentIds componentsJoinedByString:@", "], [missingDocumentIds componentsJoinedByString:@", "]); } From 1d5bed4413b615ade95105bbfd1cfe30b5a77a0d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 22 Mar 2023 12:03:14 -0400 Subject: [PATCH 4/5] FIRQueryTests.mm: fix unused variable warning of NSError** --- Firestore/Example/Tests/Integration/API/FIRQueryTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index 786ad0d155a..fab5db32ba9 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1223,7 +1223,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; [collRef.firestore - runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **error) { + runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) { for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { FIRDocumentReference *documentToDelete = createdDocuments[i]; [transaction deleteDocument:documentToDelete]; From b4fd14ec38c45e18dee927f2018c0f3413422b9e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 10:54:34 -0400 Subject: [PATCH 5/5] some small tweaks --- .../Tests/Integration/API/FIRQueryTests.mm | 38 ++++++------------- .../Tests/Util/FSTIntegrationTestCase.h | 3 ++ .../Tests/Util/FSTIntegrationTestCase.mm | 10 +++++ 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm index fab5db32ba9..93791bae165 100644 --- a/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm +++ b/Firestore/Example/Tests/Integration/API/FIRQueryTests.mm @@ -1192,39 +1192,31 @@ - (void)testOrderByEquality { - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { // Prepare the names and contents of the 100 documents to create. - NSMutableDictionary *> *testData = + NSMutableDictionary *> *testDocs = [[NSMutableDictionary alloc] init]; for (int i = 0; i < 100; i++) { - [testData setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(i)]]; + [testDocs setValue:@{@"key" : @42} forKey:[NSString stringWithFormat:@"doc%@", @(1000 + i)]]; } // Create 100 documents in a new collection. - FIRCollectionReference *collRef = [self collectionRefWithDocuments:testData]; + FIRCollectionReference *collRef = [self collectionRefWithDocuments:testDocs]; // Run a query to populate the local cache with the 100 documents and a resume token. - NSArray *createdDocuments; - { - FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef - source:FIRFirestoreSourceDefault]; - NSMutableArray *createdDocumentsAccumulator = - [[NSMutableArray alloc] init]; - for (FIRDocumentSnapshot *documentSnapshot in querySnapshot1.documents) { - [createdDocumentsAccumulator addObject:documentSnapshot.reference]; - } - createdDocuments = [createdDocumentsAccumulator copy]; - } - XCTAssertEqual(createdDocuments.count, 100u, @"createdDocuments has the wrong size"); + FIRQuerySnapshot *querySnapshot1 = [self readDocumentSetForRef:collRef + source:FIRFirestoreSourceDefault]; + XCTAssertEqual(querySnapshot1.count, 100, @"querySnapshot1.count has an unexpected value"); + NSArray *createdDocuments = + FIRDocumentReferenceArrayFromQuerySnapshot(querySnapshot1); // Delete 50 of the 100 documents. Do this in a transaction, rather than // [FIRDocumentReference deleteDocument], to avoid affecting the local cache. NSSet *deletedDocumentIds; { NSMutableArray *deletedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; - deletedDocumentIds = [deletedDocumentIdsAccumulator copy]; XCTestExpectation *expectation = [self expectationWithDescription:@"DeleteTransaction"]; [collRef.firestore runTransactionWithBlock:^id _Nullable(FIRTransaction *transaction, NSError **) { - for (NSUInteger i = 0; i < createdDocuments.count; i += 2) { + for (decltype(createdDocuments.count) i = 0; i < createdDocuments.count; i += 2) { FIRDocumentReference *documentToDelete = createdDocuments[i]; [transaction deleteDocument:documentToDelete]; [deletedDocumentIdsAccumulator addObject:documentToDelete.documentID]; @@ -1254,14 +1246,8 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { // existence filter, resulting in the client including the deleted documents in the snapshot // of the resumed query. if (!([FSTIntegrationTestCase isRunningAgainstEmulator] && querySnapshot2.count == 100)) { - NSSet *actualDocumentIds; - { - NSMutableArray *actualDocumentIdsAccumulator = [[NSMutableArray alloc] init]; - for (FIRDocumentSnapshot *documentSnapshot in querySnapshot2.documents) { - [actualDocumentIdsAccumulator addObject:documentSnapshot.documentID]; - } - actualDocumentIds = [NSSet setWithArray:actualDocumentIdsAccumulator]; - } + NSSet *actualDocumentIds = + [NSSet setWithArray:FIRQuerySnapshotGetIDs(querySnapshot2)]; NSSet *expectedDocumentIds; { NSMutableArray *expectedDocumentIdsAccumulator = [[NSMutableArray alloc] init]; @@ -1277,7 +1263,7 @@ - (void)testResumingAQueryShouldUseExistenceFilterToDetectDeletes { SortedStringsNotIn(actualDocumentIds, expectedDocumentIds); NSArray *missingDocumentIds = SortedStringsNotIn(expectedDocumentIds, actualDocumentIds); - XCTFail(@"The snapshot contained %lu documents (expected %lu): " + XCTFail(@"querySnapshot2 contained %lu documents (expected %lu): " @"%lu unexpected and %lu missing; " @"unexpected documents: %@; missing documents: %@", (unsigned long)actualDocumentIds.count, (unsigned long)expectedDocumentIds.count, diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h index 6b4ee5fb356..c33f3f5bc97 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.h @@ -142,6 +142,9 @@ NSArray *FIRQuerySnapshotGetIDs(FIRQuerySnapshot *docs); * in order of { type, doc title, doc data }. */ NSArray *> *FIRQuerySnapshotGetDocChangesData(FIRQuerySnapshot *docs); +/** Gets the FIRDocumentReference objects from a FIRQuerySnapshot and returns them. */ +NSArray *FIRDocumentReferenceArrayFromQuerySnapshot(FIRQuerySnapshot *); + #if __cplusplus } // extern "C" #endif diff --git a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm index d89e6881ce6..d2db81f869b 100644 --- a/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm +++ b/Firestore/Example/Tests/Util/FSTIntegrationTestCase.mm @@ -600,6 +600,16 @@ - (void)waitUntil:(BOOL (^)())predicate { return result; } +extern "C" NSArray *FIRDocumentReferenceArrayFromQuerySnapshot( + FIRQuerySnapshot *docs) { + NSMutableArray *documentReferenceAccumulator = + [[NSMutableArray alloc] init]; + for (FIRDocumentSnapshot *documentSnapshot in docs.documents) { + [documentReferenceAccumulator addObject:documentSnapshot.reference]; + } + return [documentReferenceAccumulator copy]; +} + @end NS_ASSUME_NONNULL_END