From c0295212e9639cf3950d9ec7215c69b48775ecd8 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Sun, 21 Oct 2018 14:38:16 -0400 Subject: [PATCH 1/3] Wrap diagnostics notification in collection flag check. Some of the diagnostics notifications were missed and not covered by the data collection flag. --- Example/Core/Tests/FIRAppTest.m | 27 +++++++++++++++++++++-- Firebase/Core/FIRApp.m | 38 ++++++++++++++++++++------------- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/Example/Core/Tests/FIRAppTest.m b/Example/Core/Tests/FIRAppTest.m index 13bc9210817..cea06d6a202 100644 --- a/Example/Core/Tests/FIRAppTest.m +++ b/Example/Core/Tests/FIRAppTest.m @@ -690,8 +690,6 @@ - (void)testGlobalDataCollectionClearedAfterDelete { } - (void)testGlobalDataCollectionNoDiagnosticsSent { - [FIRApp configure]; - // Add an observer for the diagnostics notification - both with and without an object to ensure it // catches it either way. Currently no object is sent, but in the future that could change. [self.notificationCenter addMockObserver:self.observerMock @@ -707,6 +705,9 @@ - (void)testGlobalDataCollectionNoDiagnosticsSent { OCMStub([self.appClassMock readDataCollectionSwitchFromUserDefaultsForApp:OCMOCK_ANY]) .andReturn(@NO); + // Ensure configure doesn't fire a notification. + [FIRApp configure]; + NSError *error = [NSError errorWithDomain:@"com.firebase" code:42 userInfo:nil]; [[FIRApp defaultApp] sendLogsWithServiceName:@"Service" version:@"Version" error:error]; @@ -715,6 +716,28 @@ - (void)testGlobalDataCollectionNoDiagnosticsSent { OCMVerifyAll(self.observerMock); } +- (void)testGlobalDataCollectionNoDiagnosticsSentForNoOptions { + // Add an observer for the diagnostics notification - both with and without an object to ensure it + // catches it either way. Currently no object is sent, but in the future that could change. + [self.notificationCenter addMockObserver:self.observerMock + name:kFIRAppDiagnosticsNotification + object:nil]; + [self.notificationCenter addMockObserver:self.observerMock + name:kFIRAppDiagnosticsNotification + object:OCMOCK_ANY]; + + OCMStub([self.appClassMock isDataCollectionDefaultEnabled]).andReturn(@NO); + + // Ensure there is no FIROptions instance, and call configure which should throw an exception and + // attempt to log a diagnostics notification. + self.optionsInstanceMock = nil; + XCTAssertThrows([FIRApp configure]); + + // The observer mock is strict and will raise an exception when an unexpected notification is + // received. + OCMVerifyAll(self.observerMock); +} + #pragma mark - Analytics Flag Tests - (void)testAnalyticsSetByGlobalDataCollectionSwitch { diff --git a/Firebase/Core/FIRApp.m b/Firebase/Core/FIRApp.m index 2edcb078963..adc760e15ce 100644 --- a/Firebase/Core/FIRApp.m +++ b/Firebase/Core/FIRApp.m @@ -107,13 +107,19 @@ @implementation FIRApp + (void)configure { FIROptions *options = [FIROptions defaultOptions]; if (!options) { - [[NSNotificationCenter defaultCenter] - postNotificationName:kFIRAppDiagnosticsNotification - object:nil - userInfo:@{ - kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), - kFIRAppDiagnosticsErrorKey : [FIRApp errorForMissingOptions] - }]; + // Read the Info.plist to see if the flag is set. At this point we can't check any user defaults + // since the app isn't configured at all, so only rely on the Info.plist value. + NSNumber *collectionEnabledPlistValue = [[self class] readDataCollectionSwitchFromPlist]; + if (!collectionEnabledPlistValue || [collectionEnabledPlistValue boolValue]) { + [[NSNotificationCenter defaultCenter] + postNotificationName:kFIRAppDiagnosticsNotification + object:nil + userInfo:@{ + kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), + kFIRAppDiagnosticsErrorKey : [FIRApp errorForMissingOptions] + }]; + } + [NSException raise:kFirebaseCoreErrorDomain format: @"`[FIRApp configure];` (`FirebaseApp.configure()` in Swift) could not find " @@ -274,13 +280,15 @@ + (void)addAppToAppDictionary:(FIRApp *)app { } if ([app configureCore]) { sAllApps[app.name] = app; - [[NSNotificationCenter defaultCenter] - postNotificationName:kFIRAppDiagnosticsNotification - object:nil - userInfo:@{ - kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), - kFIRAppDiagnosticsFIRAppKey : app - }]; + if ([app isDataCollectionDefaultEnabled]) { + [[NSNotificationCenter defaultCenter] + postNotificationName:kFIRAppDiagnosticsNotification + object:nil + userInfo:@{ + kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), + kFIRAppDiagnosticsFIRAppKey : app + }]; + } } else { [NSException raise:kFirebaseCoreErrorDomain format: @@ -317,7 +325,7 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh withCallback:(FIRTokenCallback - (BOOL)configureCore { [self checkExpectedBundleID]; if (![self isAppIDValid]) { - if (_options.usingOptionsFromDefaultPlist) { + if (_options.usingOptionsFromDefaultPlist && [self isDataCollectionDefaultEnabled]) { [[NSNotificationCenter defaultCenter] postNotificationName:kFIRAppDiagnosticsNotification object:nil From ce9637e9cf1eb3646ed016057cae9f8de11c9704 Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Mon, 22 Oct 2018 14:28:46 -0400 Subject: [PATCH 2/3] Remove redundant notification call, move Core diagnostics API call. --- Example/Core/Tests/FIRAppTest.m | 7 ++----- Firebase/Core/FIRApp.m | 19 ++++++++++--------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/Example/Core/Tests/FIRAppTest.m b/Example/Core/Tests/FIRAppTest.m index cea06d6a202..1dec09b71d0 100644 --- a/Example/Core/Tests/FIRAppTest.m +++ b/Example/Core/Tests/FIRAppTest.m @@ -717,11 +717,8 @@ - (void)testGlobalDataCollectionNoDiagnosticsSent { } - (void)testGlobalDataCollectionNoDiagnosticsSentForNoOptions { - // Add an observer for the diagnostics notification - both with and without an object to ensure it - // catches it either way. Currently no object is sent, but in the future that could change. - [self.notificationCenter addMockObserver:self.observerMock - name:kFIRAppDiagnosticsNotification - object:nil]; + // Add an observer for the diagnostics notification. Currently no object is sent, but in the + // future that could change. [self.notificationCenter addMockObserver:self.observerMock name:kFIRAppDiagnosticsNotification object:OCMOCK_ANY]; diff --git a/Firebase/Core/FIRApp.m b/Firebase/Core/FIRApp.m index adc760e15ce..2920d4c737d 100644 --- a/Firebase/Core/FIRApp.m +++ b/Firebase/Core/FIRApp.m @@ -280,15 +280,6 @@ + (void)addAppToAppDictionary:(FIRApp *)app { } if ([app configureCore]) { sAllApps[app.name] = app; - if ([app isDataCollectionDefaultEnabled]) { - [[NSNotificationCenter defaultCenter] - postNotificationName:kFIRAppDiagnosticsNotification - object:nil - userInfo:@{ - kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), - kFIRAppDiagnosticsFIRAppKey : app - }]; - } } else { [NSException raise:kFirebaseCoreErrorDomain format: @@ -337,6 +328,16 @@ - (BOOL)configureCore { return NO; } + if ([self isDataCollectionDefaultEnabled]) { + [[NSNotificationCenter defaultCenter] + postNotificationName:kFIRAppDiagnosticsNotification + object:nil + userInfo:@{ + kFIRAppDiagnosticsConfigurationTypeKey : @(FIRConfigTypeCore), + kFIRAppDiagnosticsFIRAppKey : self + }]; + } + #if TARGET_OS_IOS // Initialize the Analytics once there is a valid options under default app. Analytics should // always initialize first by itself before the other SDKs. From e6d45ab421d6b72e9c3e05e254a865a496812faf Mon Sep 17 00:00:00 2001 From: Ryan Wilson Date: Tue, 23 Oct 2018 11:25:10 -0400 Subject: [PATCH 3/3] Removed configure with no options test. --- Example/Core/Tests/FIRAppTest.m | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/Example/Core/Tests/FIRAppTest.m b/Example/Core/Tests/FIRAppTest.m index 1dec09b71d0..7b8b29dfb9f 100644 --- a/Example/Core/Tests/FIRAppTest.m +++ b/Example/Core/Tests/FIRAppTest.m @@ -716,25 +716,6 @@ - (void)testGlobalDataCollectionNoDiagnosticsSent { OCMVerifyAll(self.observerMock); } -- (void)testGlobalDataCollectionNoDiagnosticsSentForNoOptions { - // Add an observer for the diagnostics notification. Currently no object is sent, but in the - // future that could change. - [self.notificationCenter addMockObserver:self.observerMock - name:kFIRAppDiagnosticsNotification - object:OCMOCK_ANY]; - - OCMStub([self.appClassMock isDataCollectionDefaultEnabled]).andReturn(@NO); - - // Ensure there is no FIROptions instance, and call configure which should throw an exception and - // attempt to log a diagnostics notification. - self.optionsInstanceMock = nil; - XCTAssertThrows([FIRApp configure]); - - // The observer mock is strict and will raise an exception when an unexpected notification is - // received. - OCMVerifyAll(self.observerMock); -} - #pragma mark - Analytics Flag Tests - (void)testAnalyticsSetByGlobalDataCollectionSwitch {