Skip to content

Commit

Permalink
Improves developer model for error handling in FBRequestConnection
Browse files Browse the repository at this point in the history
Summary:
This changes requires a rational; here goes. The fix is that now the userInfo of the
error object works the same whether or not the request is a batch. This significantly
improves understandability, as well as makes it more feasible to write apps that dynamically
choose to batch or not. That's the good news. There are actually two reasons to make this
change now:
1) The previously mentioned improvment to the developer model
2) Changing this aspect of the developer model later would be a particularly painful
class of breaking change, that would likely introduce difficult to find/repro bugs in apps

The tradeoff is that the elegant fix to make improve the developer model is also a deep
change that would be too risky given that we are about to move these bits to production. So
we defer that fix until we have a time-window to stablize the change, and we make a less
elegant, but more stable fix here -- at a skin-deep point in the code.

Test Plan: ran unit tests, ran impacted samples

Reviewers: gregschechte, clang, mmarucheck

Reviewed By: mmarucheck

CC: msdkexp@, platform-diffs@lists

Differential Revision: https://phabricator.fb.com/D538988

Task ID: 1256476
  • Loading branch information
Jason Clark committed Aug 3, 2012
1 parent bf79200 commit 4a4f8ca
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 4 deletions.
Expand Up @@ -171,9 +171,7 @@ - (void)postAction:(NSString *)actionPath

// see if we can improve on it with an error message from the server
id json = [error.userInfo objectForKey:FBErrorParsedJSONResponseKey];
if ([json isKindOfClass:[NSArray class]] &&
(json = [json objectAtIndex:0]) &&
[json isKindOfClass:[NSDictionary class]] &&
if ([json isKindOfClass:[NSDictionary class]] &&
(json = [json objectForKey:@"body"]) &&
[json isKindOfClass:[NSDictionary class]] &&
(json = [json objectForKey:@"error"]) &&
Expand Down
3 changes: 2 additions & 1 deletion src/FBError.h
Expand Up @@ -26,7 +26,8 @@ extern NSString *const FacebookSDKDomain;
/// The key for an inner NSError.
extern NSString *const FBErrorInnerErrorKey;

/// The key for parsed JSON response from the server.
/// The key for parsed JSON response from the server. In case of a batch,
/// includes the JSON for a single FBRequest.
extern NSString *const FBErrorParsedJSONResponseKey;

/// The key for HTTP status code.
Expand Down
22 changes: 22 additions & 0 deletions src/FBRequestConnection.m
Expand Up @@ -1168,6 +1168,28 @@ - (void)completeWithResults:(NSArray *)results
}

if (metadata.completionHandler) {
// task #1256476: in the current implementation, FBErrorParsedJSONResponseKey has two
// semantics; both of which are used by the implementation; the right fix is to break the meaning into
// two throughout, and surface both in the public API; the following fix is a lower risk and also
// less correct solution that improves the public API surface for this release
// Unpack FBErrorParsedJSONResponseKey array if present
id parsedResponse;
if ((parsedResponse = itemError.userInfo) && // do we have an error with userInfo
(parsedResponse = [parsedResponse objectForKey:FBErrorParsedJSONResponseKey]) && // response present?
([parsedResponse isKindOfClass:[NSArray class]])) { // array?
id newValue = nil;
// if we successfully spelunk this far, then we don't want to return FBErrorParsedJSONResponseKey as is
// but if there is an empty array here, then we are better off nil-ing the key
if ([parsedResponse count]) {
newValue = [parsedResponse objectAtIndex:0];
}
itemError = [self errorWithCode:itemError.code
statusCode:[[itemError.userInfo objectForKey:FBErrorHTTPStatusCodeKey] intValue]
parsedJSONResponse:newValue
innerError:[itemError.userInfo objectForKey:FBErrorInnerErrorKey]
message:[itemError.userInfo objectForKey:NSLocalizedDescriptionKey]];
}

metadata.completionHandler(self, body, itemError);
}
}
Expand Down

0 comments on commit 4a4f8ca

Please sign in to comment.