Throw an error if the fetch response returns an error (irrespective o… (#4438)

dmandar · web-flow · commit f47ce4bbf335 · 2019-12-04T17:07:43.000-08:00
* Throw an error if the fetch response returns an error (irrespective of status code).

* Remove duplicate metadata set call.

* Add comment.
diff --git a/FirebaseRemoteConfig/Sources/RCNFetch.m b/FirebaseRemoteConfig/Sources/RCNFetch.m
@@ -358,7 +358,7 @@ - (void)fetchWithUserProperties:(NSDictionary *)userProperties
       NSInteger statusCode = [((NSHTTPURLResponse *)response) statusCode];
 
       if (error || (statusCode != kRCNFetchResponseHTTPStatusCodeOK)) {
-        // Update failure fetch time and database.
+        // Update metadata about fetch failure.
         [strongSelf->_settings updateMetadataWithFetchSuccessStatus:NO];
         if (error) {
           if (strongSelf->_settings.lastFetchStatus == FIRRemoteConfigFetchStatusSuccess) {
@@ -393,27 +393,25 @@ - (void)fetchWithUserProperties:(NSDictionary *)userProperties
               return [strongSelf reportCompletionOnHandler:completionHandler
                                                 withStatus:strongSelf->_settings.lastFetchStatus
                                                  withError:error];
-            } else {
-              // Return back the received error.
-              // Must set lastFetchStatus before setting Fetch Error.
-              strongSelf->_settings.lastFetchStatus = FIRRemoteConfigFetchStatusFailure;
-              strongSelf->_settings.lastFetchError = FIRRemoteConfigErrorInternalError;
-              NSDictionary<NSErrorUserInfoKey, id> *userInfo = @{
-                NSLocalizedDescriptionKey :
-                    (error ? [error localizedDescription]
-                           : [NSString stringWithFormat:@"Internal Error. Status code: %ld",
-                                                        (long)statusCode])
-              };
-              return [strongSelf
-                  reportCompletionOnHandler:completionHandler
-                                 withStatus:FIRRemoteConfigFetchStatusFailure
-                                  withError:[NSError
-                                                errorWithDomain:FIRRemoteConfigErrorDomain
-                                                           code:FIRRemoteConfigErrorInternalError
-                                                       userInfo:userInfo]];
             }
-          }
-        }
+          }  // Response error code 429, 500, 503
+        }    // StatusCode != kRCNFetchResponseHTTPStatusCodeOK
+        // Return back the received error.
+        // Must set lastFetchStatus before setting Fetch Error.
+        strongSelf->_settings.lastFetchStatus = FIRRemoteConfigFetchStatusFailure;
+        strongSelf->_settings.lastFetchError = FIRRemoteConfigErrorInternalError;
+        NSDictionary<NSErrorUserInfoKey, id> *userInfo = @{
+          NSLocalizedDescriptionKey :
+              (error ? [error localizedDescription]
+                     : [NSString
+                           stringWithFormat:@"Internal Error. Status code: %ld", (long)statusCode])
+        };
+        return [strongSelf
+            reportCompletionOnHandler:completionHandler
+                           withStatus:FIRRemoteConfigFetchStatusFailure
+                            withError:[NSError errorWithDomain:FIRRemoteConfigErrorDomain
+                                                          code:FIRRemoteConfigErrorInternalError
+                                                      userInfo:userInfo]];
       }
 
       // Fetch was successful. Check if we have data.
diff --git a/FirebaseRemoteConfig/Tests/Sample/RemoteConfigSampleApp/ViewController.m b/FirebaseRemoteConfig/Tests/Sample/RemoteConfigSampleApp/ViewController.m
@@ -159,39 +159,39 @@ - (IBAction)fetchButtonPressed:(id)sender {
 - (IBAction)fetchAndActivateButtonPressed:(id)sender {
   // fetchConfig api callback, this is triggered when client receives response from server
   ViewController *__weak weakSelf = self;
-  FIRRemoteConfigFetchAndActivateCompletion fetchAndActivateCompletion =
-      ^(FIRRemoteConfigFetchAndActivateStatus status, NSError *error) {
-        ViewController *strongSelf = weakSelf;
-        if (!strongSelf) {
-          return;
-        }
-        NSMutableString *output = @"Fetch and activate status :";
-        if (status == FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote) {
-          [output appendString:@"Success from remote fetch."];
-        } else if (status == FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData) {
-          [output appendString:@"Success using pre-fetched data."];
-        } else if (status == FIRRemoteConfigFetchAndActivateStatusError) {
-          output appendString
-              : [NSString stringWithFormat:@"Failure: %@", [error localizedDescription]];
-        }
+  FIRRemoteConfigFetchAndActivateCompletion fetchAndActivateCompletion = ^(
+      FIRRemoteConfigFetchAndActivateStatus status, NSError *error) {
+    ViewController *strongSelf = weakSelf;
+    if (!strongSelf) {
+      return;
+    }
+    NSMutableString *output = [@"Fetch and activate status :" mutableCopy];
+    if (status == FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote) {
+      [output appendString:@"Success from remote fetch."];
+    } else if (status == FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData) {
+      [output appendString:@"Success using pre-fetched data."];
+    } else if (status == FIRRemoteConfigFetchAndActivateStatusError) {
+      [output
+          appendString:[NSString stringWithFormat:@"Failure: %@", [error localizedDescription]]];
+    }
 
-        if (error) {
-          [output appendFormat:@"%@\n", error];
-        }
-        if (status == FIRRemoteConfigFetchAndActivateStatusError) {
-          [output appendString:[NSString stringWithFormat:@"Fetch And Activate Error :%@.\n",
-                                                          [strongSelf errorString:error.code]]];
-          if (error.code == FIRRemoteConfigErrorThrottled) {
-            [output appendString:[NSString stringWithFormat:@"Throttled.\n"]];
-          }
-        }
-        // activate status
-        [[FRCLog sharedInstance] logToConsole:output];
-        if (status == FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote ||
-            status == FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData) {
-          [strongSelf printResult:[[NSMutableString alloc] init]];
-        }
-      };
+    if (error) {
+      [output appendFormat:@"%@\n", error];
+    }
+    if (status == FIRRemoteConfigFetchAndActivateStatusError) {
+      [output appendString:[NSString stringWithFormat:@"Fetch And Activate Error :%@.\n",
+                                                      [strongSelf errorString:error.code]]];
+      if (error.code == FIRRemoteConfigErrorThrottled) {
+        [output appendString:[NSString stringWithFormat:@"Throttled.\n"]];
+      }
+    }
+    // activate status
+    [[FRCLog sharedInstance] logToConsole:output];
+    if (status == FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote ||
+        status == FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData) {
+      [strongSelf printResult:[[NSMutableString alloc] init]];
+    }
+  };
 
   // fetchConfig api call
   [self.RCInstances[self.currentNamespace][self.FIRAppName]
diff --git a/FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m b/FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m
@@ -558,6 +558,129 @@ - (void)testFetchConfigsFailed {
                                }];
 }
 
+// TODO(mandard): Break up test with helper methods.
+- (void)testFetchConfigsFailedErrorNoNetwork {
+  // Override the setup values to return back an error status.
+  RCNConfigContent *configContent = [[RCNConfigContent alloc] initWithDBManager:_DBManager];
+  // Populate the default, second app, second namespace instances.
+  for (int i = 0; i < RCNTestRCNumTotalInstances; i++) {
+    NSString *currentAppName = nil;
+    FIROptions *currentOptions = nil;
+    NSString *currentNamespace = nil;
+    switch (i) {
+      case RCNTestRCInstanceSecondNamespace:
+        currentAppName = RCNTestsDefaultFIRAppName;
+        currentOptions = [self firstAppOptions];
+        currentNamespace = RCNTestsPerfNamespace;
+        break;
+      case RCNTestRCInstanceSecondApp:
+        currentAppName = RCNTestsSecondFIRAppName;
+        currentOptions = [self secondAppOptions];
+        currentNamespace = FIRNamespaceGoogleMobilePlatform;
+        break;
+      case RCNTestRCInstanceDefault:
+      default:
+        currentAppName = RCNTestsDefaultFIRAppName;
+        currentOptions = [self firstAppOptions];
+        currentNamespace = RCNTestsFIRNamespace;
+        break;
+    }
+    NSString *fullyQualifiedNamespace =
+        [NSString stringWithFormat:@"%@:%@", currentNamespace, currentAppName];
+    RCNUserDefaultsManager *userDefaultsManager =
+        [[RCNUserDefaultsManager alloc] initWithAppName:currentAppName
+                                               bundleID:[NSBundle mainBundle].bundleIdentifier
+                                              namespace:fullyQualifiedNamespace];
+    userDefaultsManager.lastFetchTime = 0;
+
+    FIRRemoteConfig *config =
+        OCMPartialMock([[FIRRemoteConfig alloc] initWithAppName:currentAppName
+                                                     FIROptions:currentOptions
+                                                      namespace:currentNamespace
+                                                      DBManager:_DBManager
+                                                  configContent:configContent
+                                                      analytics:nil]);
+
+    _configInstances[i] = config;
+    RCNConfigSettings *settings =
+        [[RCNConfigSettings alloc] initWithDatabaseManager:_DBManager
+                                                 namespace:fullyQualifiedNamespace
+                                           firebaseAppName:currentAppName
+                                               googleAppID:currentOptions.googleAppID];
+    dispatch_queue_t queue = dispatch_queue_create(
+        [[NSString stringWithFormat:@"testqueue: %d", i] cStringUsingEncoding:NSUTF8StringEncoding],
+        DISPATCH_QUEUE_SERIAL);
+    _configFetch[i] = OCMPartialMock([[RCNConfigFetch alloc] initWithContent:configContent
+                                                                   DBManager:_DBManager
+                                                                    settings:settings
+                                                                   analytics:nil
+                                                                  experiment:nil
+                                                                       queue:queue
+                                                                   namespace:fullyQualifiedNamespace
+                                                                     options:currentOptions]);
+
+    OCMStub([_configFetch[i] fetchConfigWithExpirationDuration:43200 completionHandler:OCMOCK_ANY])
+        .andDo(^(NSInvocation *invocation) {
+          void (^handler)(FIRRemoteConfigFetchStatus status, NSError *_Nullable error) = nil;
+          // void (^handler)(FIRRemoteConfigFetchCompletion);
+          [invocation getArgument:&handler atIndex:3];
+          [_configFetch[i] fetchWithUserProperties:[[NSDictionary alloc] init]
+                                 completionHandler:handler];
+        });
+
+    _response[i] = @{};
+
+    _responseData[i] = [NSJSONSerialization dataWithJSONObject:_response[i] options:0 error:nil];
+
+    // A no network error is accompanied with an HTTP status code of 0.
+    _URLResponse[i] =
+        [[NSHTTPURLResponse alloc] initWithURL:[NSURL URLWithString:@"https://firebase.com"]
+                                    statusCode:0
+                                   HTTPVersion:nil
+                                  headerFields:@{@"etag" : @"etag1"}];
+
+    id completionBlock =
+        [OCMArg invokeBlockWithArgs:_responseData[i], _URLResponse[i], [NSNull null], nil];
+
+    OCMExpect([_configFetch[i] URLSessionDataTaskWithContent:[OCMArg any]
+                                           completionHandler:completionBlock])
+        .andReturn(nil);
+    [_configInstances[i] updateWithNewInstancesForConfigFetch:_configFetch[i]
+                                                configContent:configContent
+                                               configSettings:settings
+                                             configExperiment:nil];
+  }
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+  // Make the fetch calls for all instances.
+  NSMutableArray<XCTestExpectation *> *expectations =
+      [[NSMutableArray alloc] initWithCapacity:RCNTestRCNumTotalInstances];
+
+  for (int i = 0; i < RCNTestRCNumTotalInstances; i++) {
+    expectations[i] = [self
+        expectationWithDescription:
+            [NSString stringWithFormat:@"Test enumerating configs successfully - instance %d", i]];
+    XCTAssertEqual(_configInstances[i].lastFetchStatus, FIRRemoteConfigFetchStatusNoFetchYet);
+    FIRRemoteConfigFetchCompletion fetchCompletion =
+        ^void(FIRRemoteConfigFetchStatus status, NSError *error) {
+          XCTAssertEqual(_configInstances[i].lastFetchStatus, FIRRemoteConfigFetchStatusFailure);
+          XCTAssertFalse([_configInstances[i] activateFetched]);
+          XCTAssertNotNil(error);
+          // No such key, still return a static value.
+          FIRRemoteConfigValue *value = _configInstances[RCNTestRCInstanceDefault][@"key1"];
+          XCTAssertEqual((int)value.source, (int)FIRRemoteConfigSourceStatic);
+          XCTAssertEqualObjects(value.stringValue, @"");
+          XCTAssertEqual(value.boolValue, NO);
+          [expectations[i] fulfill];
+        };
+    [_configInstances[i] fetchWithExpirationDuration:43200 completionHandler:fetchCompletion];
+  }
+  [self waitForExpectationsWithTimeout:_expectationTimeout
+                               handler:^(NSError *error) {
+                                 XCTAssertNil(error);
+                               }];
+}
+
 // Activate should return false if a fetch response returns 200 with NO_CHANGE as the response body.
 - (void)testActivateOnFetchNoChangeStatus {
   // Override the setup values to return back an error status.
