From d3b77b114e7aaca3ef8317e6010cfedb0493ad94 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Fri, 16 Feb 2018 12:23:50 -0500 Subject: [PATCH 1/5] Delete stale Firestore instances after FIRApp is deleted. --- .../Example/Tests/API/FIRFirestoreTests.mm | 59 +++++++++++++++++++ Firestore/Source/API/FIRFirestore.mm | 31 +++++++++- 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 Firestore/Example/Tests/API/FIRFirestoreTests.mm diff --git a/Firestore/Example/Tests/API/FIRFirestoreTests.mm b/Firestore/Example/Tests/API/FIRFirestoreTests.mm new file mode 100644 index 00000000000..485ddb7557a --- /dev/null +++ b/Firestore/Example/Tests/API/FIRFirestoreTests.mm @@ -0,0 +1,59 @@ +/* + * Copyright 2017 Google + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#import +#import +#import + +#import + +@interface FIRFirestoreTests : XCTestCase +@end + +@implementation FIRFirestoreTests + +- (void)testDeleteApp { + // Create a FIRApp for testing. + NSString *appName = @"custom_app_name"; + FIROptions *options = + [[FIROptions alloc] initWithGoogleAppID:@"1:123:ios:123ab" GCMSenderID:@"gcm_sender_id"]; + options.projectID = @"project_id"; + [FIRApp configureWithName:appName options:options]; + + // Ensure the app is set appropriately. + FIRApp *app = [FIRApp appNamed:appName]; + FIRFirestore *firestore = [FIRFirestore firestoreForApp:app]; + XCTAssertEqualObjects(firestore.app, app); + + XCTestExpectation *defaultAppDeletedExpectation = + [self expectationWithDescription: + @"Deleting the default app should invalidate the default " + @"Firestore instance."]; + [app deleteApp:^(BOOL success) { + // Recreate the FIRApp with the same name, fetch a new Firestore instance and make sure it's + // different than the other one. + [FIRApp configureWithName:appName options:options]; + FIRApp *newApp = [FIRApp appNamed:appName]; + FIRFirestore *newInstance = [FIRFirestore firestoreForApp:newApp]; + XCTAssertNotEqualObjects(newInstance, firestore); + + [defaultAppDeletedExpectation fulfill]; + }]; + + [self waitForExpectations:@[ defaultAppDeletedExpectation ] timeout:200]; +} + +@end diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm index 5e978cc828b..286e2c4bd2c 100644 --- a/Firestore/Source/API/FIRFirestore.mm +++ b/Firestore/Source/API/FIRFirestore.mm @@ -16,7 +16,7 @@ #import "FIRFirestore.h" -#import +#import #import #import @@ -80,6 +80,35 @@ @implementation FIRFirestore { return instances; } ++ (void)load { + NSNotificationCenter *center = [NSNotificationCenter defaultCenter]; + [center addObserverForName:kFIRAppDeleteNotification + object:nil + queue:nil + usingBlock:^(NSNotification *_Nonnull note) { + NSString *appName = note.userInfo[kFIRAppNameKey]; + if (appName == nil) return; + + NSMutableDictionary *instances = [self instances]; + @synchronized(instances) { + // Since the key for instances isn't just the app name, iterate over all the + // keys to get the one(s) we have to delete. There could be multiple in case + // the user calls firestoreForApp:database:. + NSMutableArray *keysToDelete = [[NSMutableArray alloc] init]; + for (NSString *key in instances.allKeys) { + if ([key hasPrefix:appName]) { + [keysToDelete addObject:key]; + } + } + + // Loop through the keys found and delete them from the stored instances. + for (NSString *key in keysToDelete) { + [instances removeObjectForKey:key]; + } + } + }]; +} + + (instancetype)firestore { FIRApp *app = [FIRApp defaultApp]; if (!app) { From 4e84ae8781664f14537b247f733092ee181a55e7 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Fri, 16 Feb 2018 12:25:32 -0500 Subject: [PATCH 2/5] What year is it? --- Firestore/Example/Tests/API/FIRFirestoreTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/API/FIRFirestoreTests.mm b/Firestore/Example/Tests/API/FIRFirestoreTests.mm index 485ddb7557a..8d8b0d0af66 100644 --- a/Firestore/Example/Tests/API/FIRFirestoreTests.mm +++ b/Firestore/Example/Tests/API/FIRFirestoreTests.mm @@ -1,5 +1,5 @@ /* - * Copyright 2017 Google + * Copyright 2018 Google * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 5e2a31f628ee23eb800bb6f04bb1e73ad02b3a55 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Fri, 16 Feb 2018 13:16:10 -0500 Subject: [PATCH 3/5] Address comments, fix prefix issue --- Firestore/Source/API/FIRFirestore.mm | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Firestore/Source/API/FIRFirestore.mm b/Firestore/Source/API/FIRFirestore.mm index 286e2c4bd2c..510065bdb5c 100644 --- a/Firestore/Source/API/FIRFirestore.mm +++ b/Firestore/Source/API/FIRFirestore.mm @@ -80,7 +80,7 @@ @implementation FIRFirestore { return instances; } -+ (void)load { ++ (void)initialize { NSNotificationCenter *center = [NSNotificationCenter defaultCenter]; [center addObserverForName:kFIRAppDeleteNotification object:nil @@ -95,8 +95,9 @@ + (void)load { // keys to get the one(s) we have to delete. There could be multiple in case // the user calls firestoreForApp:database:. NSMutableArray *keysToDelete = [[NSMutableArray alloc] init]; + NSString *keyPrefix = [NSString stringWithFormat:@"%@|", appName]; for (NSString *key in instances.allKeys) { - if ([key hasPrefix:appName]) { + if ([key hasPrefix:keyPrefix]) { [keysToDelete addObject:key]; } } @@ -138,6 +139,9 @@ + (instancetype)firestoreForApp:(FIRApp *)app database:(NSString *)database { "database", util::WrapNSStringNoCopy(DatabaseId::kDefaultDatabaseId)); } + + // Note: If the key format changes, please change the code that detects FIRApps being deleted + // contained in +initialize. It checks for the app's name followed by a | character. NSString *key = [NSString stringWithFormat:@"%@|%@", app.name, database]; NSMutableDictionary *instances = self.instances; From 73b28720712e06366b1ea23f3aad97d1b17b8fd2 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Fri, 16 Feb 2018 13:19:02 -0500 Subject: [PATCH 4/5] Reduce timeout to reasonable amount. --- Firestore/Example/Tests/API/FIRFirestoreTests.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firestore/Example/Tests/API/FIRFirestoreTests.mm b/Firestore/Example/Tests/API/FIRFirestoreTests.mm index 8d8b0d0af66..33ea97c3eae 100644 --- a/Firestore/Example/Tests/API/FIRFirestoreTests.mm +++ b/Firestore/Example/Tests/API/FIRFirestoreTests.mm @@ -53,7 +53,7 @@ - (void)testDeleteApp { [defaultAppDeletedExpectation fulfill]; }]; - [self waitForExpectations:@[ defaultAppDeletedExpectation ] timeout:200]; + [self waitForExpectations:@[ defaultAppDeletedExpectation ] timeout:2]; } @end From ffb595d168f8035681057187c971b16af4dd4c13 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Fri, 16 Feb 2018 14:34:57 -0500 Subject: [PATCH 5/5] Verify instances are still returned properly. --- Firestore/Example/Tests/API/FIRFirestoreTests.mm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Firestore/Example/Tests/API/FIRFirestoreTests.mm b/Firestore/Example/Tests/API/FIRFirestoreTests.mm index 33ea97c3eae..4daf35abc84 100644 --- a/Firestore/Example/Tests/API/FIRFirestoreTests.mm +++ b/Firestore/Example/Tests/API/FIRFirestoreTests.mm @@ -38,6 +38,9 @@ - (void)testDeleteApp { FIRFirestore *firestore = [FIRFirestore firestoreForApp:app]; XCTAssertEqualObjects(firestore.app, app); + // Ensure that firestoreForApp returns the same instance. + XCTAssertEqualObjects(firestore, [FIRFirestore firestoreForApp:app]); + XCTestExpectation *defaultAppDeletedExpectation = [self expectationWithDescription: @"Deleting the default app should invalidate the default "