Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from Dec 5, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6381ab7
Throw an error if the fetch response returns an error (irrespective o…
dmandar Dec 4, 2019
82a32fb
Remove duplicate metadata set call.
dmandar Dec 4, 2019
d3cf64b
Add comment.
dmandar Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 19 additions & 21 deletions FirebaseRemoteConfig/Sources/RCNFetch.m
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down
64 changes: 32 additions & 32 deletions FirebaseRemoteConfig/Tests/Sample/RemoteConfigSampleApp/ViewController.m
View file
Open in desktop
Expand Up @@ -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]
Expand Down
123 changes: 123 additions & 0 deletions FirebaseRemoteConfig/Tests/Unit/RCNRemoteConfigTest.m
View file
Open in desktop
Expand Up @@ -558,6 +558,129 @@ - (void)testFetchConfigsFailed {
}];
}

// TODO(mandard): Break up test with helper methods.
- (void)testFetchConfigsFailedErrorNoNetwork {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I wonder if it is too big effort to break this test to several methods, e.g. refactor all mocks setup to a separate method that can be called from the for-loop (and may be potentially used by other tests). Also, separate methods for configuring individual mocks may improve readability and code reuse.

Feel free to consider it as a separate improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sg. Other tests have the mocks initialized in the setup section. It is only the testFetchConfigsFailed and now this test that need a different response code back. But yes, makes sense to refactor..will make that a separate change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a todo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

// 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.
Expand Down