Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Improves error handling and message for FBRequestConnections to endpo…
…ints that return images

Summary:
Fixes caching bug, and checks for known unsupported mime types, in order to improve error
handling if an app points to a graph API that returns an image

Test Plan: Whitebox tested using a modified profile picker sample

Reviewers: clang, mmarucheck, gregschechte

Reviewed By: clang

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

Task ID: 1011836
  • Loading branch information
onebit committed Jun 14, 2012
1 parent ffc88e1 commit 2c529ae
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 13 deletions.
4 changes: 4 additions & 0 deletions src/FBError.h
Expand Up @@ -68,6 +68,10 @@ typedef enum FBErrorCode {

/// Non-success HTTP status code was returned from the operation.
FBErrorHTTPError,

/// An endpoint that returns a binary response was used with FBRequestConnection;
/// endpoints that return image/jpg, etc. should be accessed using NSURLRequest
FBErrorNonTextMimeTypeReturned,

} FBErrorCode;

Expand Down
40 changes: 30 additions & 10 deletions src/FBRequestConnection.m
Expand Up @@ -187,7 +187,8 @@ - (NSError *)errorFromResult:(id)idResult;
- (NSError *)errorWithCode:(FBErrorCode)code
statusCode:(int)statusCode
parsedJSONResponse:(id)response
innerError:(NSError *)innerError;
innerError:(NSError*)innerError
message:(NSString*)message;

- (NSError *)checkConnectionError:(NSError *)innerError
statusCode:(int)statusCode
Expand Down Expand Up @@ -427,9 +428,9 @@ - (void)startWithCacheIdentity:(NSString*)cacheIdentity
[deprecatedDelegate requestLoading:self.deprecatedRequest];
}

FBURLConnection *connection = [[FBURLConnection alloc]
initWithRequest:request
completionHandler:handler];
FBURLConnection *connection = [[FBURLConnection alloc] initWithRequest:request
skipRoundTripIfCached:NO
completionHandler:handler];
self.connection = connection;
[connection release];
} else {
Expand Down Expand Up @@ -843,6 +844,15 @@ - (void)completeWithResponse:(NSURLResponse *)response
response);
self.urlResponse = (NSHTTPURLResponse *)response;
statusCode = self.urlResponse.statusCode;

if (!error && [response.MIMEType hasPrefix:@"image"]) {
error = [self errorWithCode:FBErrorNonTextMimeTypeReturned
statusCode:0
parsedJSONResponse:nil
innerError:nil
message:@"Response is a non-text MIME type; endpoints that return images and other "
@"binary data should be fetched using NSURLRequest and NSURLConnection"];
}
} else {
// the cached case is always successful, from an http perspective
statusCode = 200;
Expand Down Expand Up @@ -871,7 +881,8 @@ - (void)completeWithResponse:(NSURLResponse *)response
error = [self errorWithCode:FBErrorProtocolMismatch
statusCode:statusCode
parsedJSONResponse:results
innerError:nil];
innerError:nil
message:nil];
}
}

Expand Down Expand Up @@ -970,7 +981,8 @@ - (NSArray *)parseJSONResponse:(NSData *)data
*error = [self errorWithCode:FBErrorProtocolMismatch
statusCode:statusCode
parsedJSONResponse:results
innerError:nil];
innerError:nil
message:nil];
}

[responseUTF8 release];
Expand Down Expand Up @@ -1098,7 +1110,8 @@ - (NSError *)errorFromResult:(id)idResult
return [self errorWithCode:FBErrorRequestConnectionApi
statusCode:200
parsedJSONResponse:idResult
innerError:nil];
innerError:nil
message:nil];
}

NSNumber *code = [dictionary valueForKey:@"code"];
Expand All @@ -1115,10 +1128,11 @@ - (NSError *)errorFromResult:(id)idResult
- (NSError *)errorWithCode:(FBErrorCode)code
statusCode:(int)statusCode
parsedJSONResponse:(id)response
innerError:(NSError*)innerError {
innerError:(NSError*)innerError
message:(NSString*)message {
NSMutableDictionary *userInfo = [[[NSMutableDictionary alloc] init] autorelease];
[userInfo setObject:[NSNumber numberWithInt:statusCode] forKey:FBErrorHTTPStatusCodeKey];

if (response) {
[userInfo setObject:response forKey:FBErrorParsedJSONResponseKey];
}
Expand All @@ -1127,6 +1141,11 @@ - (NSError *)errorWithCode:(FBErrorCode)code
[userInfo setObject:innerError forKey:FBErrorInnerErrorKey];
}

if (message) {
[userInfo setObject:message
forKey:NSLocalizedDescriptionKey];
}

NSError *error = [[[NSError alloc]
initWithDomain:FBiOSSDKDomain
code:code
Expand All @@ -1151,7 +1170,8 @@ - (NSError *)checkConnectionError:(NSError *)innerError
result = [self errorWithCode:FBErrorHTTPError
statusCode:statusCode
parsedJSONResponse:response
innerError:innerError];
innerError:innerError
message:nil];
}
return result;
}
Expand Down
1 change: 1 addition & 0 deletions src/FBURLConnection.h
Expand Up @@ -28,6 +28,7 @@ typedef void (^FBURLConnectionHandler)(FBURLConnection *connection,
completionHandler:(FBURLConnectionHandler)handler;

- (FBURLConnection *)initWithRequest:(NSURLRequest *)request
skipRoundTripIfCached:(BOOL)skipRoundtripIfCached
completionHandler:(FBURLConnectionHandler)handler;

- (void)cancel;
Expand Down
14 changes: 11 additions & 3 deletions src/FBURLConnection.m
Expand Up @@ -31,6 +31,7 @@ @interface FBURLConnection ()
@property (nonatomic, retain) NSURLResponse *response;
@property (nonatomic) unsigned long requestStartTime;
@property (nonatomic, readonly) NSUInteger loggerSerialNumber;
@property (nonatomic) BOOL skipRoundtripIfCached;

- (BOOL)isCDNURL:(NSURL *)url;

Expand All @@ -49,6 +50,7 @@ @implementation FBURLConnection
@synthesize loggerSerialNumber = _loggerSerialNumber;
@synthesize requestStartTime = _requestStartTime;
@synthesize response = _response;
@synthesize skipRoundtripIfCached = _skipRoundtripIfCached;

#pragma mark - Lifecycle

Expand All @@ -66,16 +68,22 @@ - (FBURLConnection *)initWithURL:(NSURL *)url
completionHandler:(FBURLConnectionHandler)handler
{
NSURLRequest *request = [[[NSURLRequest alloc] initWithURL:url] autorelease];
return [self initWithRequest:request completionHandler:handler];
return [self initWithRequest:request
skipRoundTripIfCached:YES
completionHandler:handler];
}

- (FBURLConnection *)initWithRequest:(NSURLRequest *)request
skipRoundTripIfCached:(BOOL)skipRoundtripIfCached
completionHandler:(FBURLConnectionHandler)handler
{
if (self = [super init]) {
self.skipRoundtripIfCached = skipRoundtripIfCached;

// Check if this url is cached
NSURL* url = request.URL;
NSData* cachedData = [[FBDataDiskCache sharedCache] dataForURL:url];
NSData* cachedData = skipRoundtripIfCached ? [[FBDataDiskCache sharedCache] dataForURL:url] : nil;

if (cachedData) {
[FBLogger singleShotLogEntry:FBLogBehaviorFBURLConnections
formatString:@"FBUrlConnection: <#%d>. Cached response %d kB\n",
Expand Down Expand Up @@ -223,7 +231,7 @@ -(NSURLRequest *)connection:(NSURLConnection *)connection
willSendRequest:(NSURLRequest *)request
redirectResponse:(NSURLResponse *)redirectResponse
{
if (redirectResponse) {
if (redirectResponse && self.skipRoundtripIfCached) {
NSURL* redirectURL = request.URL;

// Check for cache and short-circuit
Expand Down

0 comments on commit 2c529ae

Please sign in to comment.