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.

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

HTTPClient -> AFHTTPSessionManager | Response body on error? #1397

Closed
devmod opened this issue Sep 30, 2013 · 61 comments
Closed

HTTPClient -> AFHTTPSessionManager | Response body on error? #1397

devmod opened this issue Sep 30, 2013 · 61 comments

Comments

@devmod
Copy link

@devmod devmod commented Sep 30, 2013

I used to retrieve the json response body with HTTPClient using [(AFJSONRequestOperation*)operation responseJSON]

After migrating to 2.0, I have not found a way to do so. Am I missing something?

Thanks

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Sep 30, 2013

Related to #1390?

@oleksandr
Copy link

@oleksandr oleksandr commented Sep 30, 2013

Yes, indeed! Although I can't agree that storing the data under the NSLocalizedRecoverySuggestionErrorKey is the most obvious thing...

@dcaunt
Copy link
Contributor

@dcaunt dcaunt commented Sep 30, 2013

Should probably use a new key for this, but using the userInfo dictionary looks good to me. The only other way to access the data at present is via the userInfo for the task finish notification.

@oleksandr
Copy link

@oleksandr oleksandr commented Oct 1, 2013

It is possible to access data via userInfo for the "AFNetworkingTaskDidFinishNotification" notification. The problem is that notification is delivered after the failure:^(NSURLSessionDataTask *task, NSError *error) is executed :-(

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Oct 1, 2013

@oleksandr to add, one of the points of blocks is to be able to deal with all situations involving an invocation at the point of invocation. Listening for a notification, even if it was fired before failure, disrupts this model. We're dealing with the failure in the failure block, we just don't have all the information needed to deal with the failure available in this block at present.

@oleksandr
Copy link

@oleksandr oleksandr commented Oct 1, 2013

@gfiumara I agree. I've just mentioned this to highlight that either handling all the machinery in the notifications or adding missing data in the block. Mixing these two things won't work because of the async nature of the both.

@devmod
Copy link
Author

@devmod devmod commented Oct 7, 2013

@gfiumara yup that's exactly my thinking.

@bokojo
Copy link

@bokojo bokojo commented Oct 9, 2013

This would be such an easy fix to make. The responseObject is available and fully parsed in AFHTTPSessionManager's http methods (POST,GET,etc..). All it would require is changing the block signature of the failure block to include all three: the NSURLSessionDataTask_, NSError_, and the id responseObject.

I would like to see that for clarity, rather than stuffing it into the userInfo dict of the Error under a semi-obscure key.

@akashkamboj
Copy link

@akashkamboj akashkamboj commented Oct 9, 2013

@bokojo I agree, it will be much cleaner approach.

@dcaunt
Copy link
Contributor

@dcaunt dcaunt commented Oct 9, 2013

Changing the block signature would break backwards compatibility, so I can't see that happening.

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Oct 9, 2013

@bokojo That is cleaner, and was done in #1226, but not accepted. Shoving into NSError's userInfo is not ideal IMHO, but is how it was done in 1.x.

@bokojo
Copy link

@bokojo bokojo commented Oct 9, 2013

How can there be any backwards compatibility in the AFHTTPSessionManager? It's a non-existant object in prior versions. The methods themselves are non-existant?

@dcaunt
Copy link
Contributor

@dcaunt dcaunt commented Oct 9, 2013

@bokojo I meant compatibility between version 2.0 of AFNetworking and any subsequent release containing your proposed change (2.0.1 I guess). See semver.org.

@bokojo
Copy link

@bokojo bokojo commented Oct 9, 2013

Yeah, I suppose ideally this would've been considered in the RC stage of 2.0.

I'll take it anywhere I can get it. If not in the failure parameters, then in userInfo would be fine.

@akashkamboj
Copy link

@akashkamboj akashkamboj commented Oct 9, 2013

I don't see NSLocalizedRecoverySuggestionErrorKey in userInfo either... is it in 2.0.0 or in master branch only?

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Oct 9, 2013

@akashkamboj I believe it to be a regression, so I added it back in pull request #1390.

@mattt
Copy link
Contributor

@mattt mattt commented Oct 11, 2013

5bcac2f adds a responseObject property to AFHTTPRequestOperation, which creates parity to the responseJSON property of AFJSONRequestOperation.

@mattt mattt closed this Oct 11, 2013
@bokojo
Copy link

@bokojo bokojo commented Oct 11, 2013

Does this solve our problem?

Where can I gain access to this operation from the provided NSURLSessionDataTask* in the failure block? I see no exposure to the underlying operations. Maybe I'm just missing it?

@oleksandr
Copy link

@oleksandr oleksandr commented Oct 11, 2013

Clear as a mud to me :-) A possible solution for this could be (as suggested by @mattt somewhere in the issues) to extend AFJSONResponseSerializer and include the responseObject under the project's custom key into the NSError's userInfo.

@devmod
Copy link
Author

@devmod devmod commented Oct 11, 2013

@bokojo wondering too why this issue got closed

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Oct 11, 2013

You can do something like this for JSON, which is (I believe) what @mattt suggested in my pull request #1390 (comment). With the below, the data parameter will end up in the NSError parameter's userInfo property of the failure block of AFHTTPSessionManager convenience methods.

JSONResponseSerializerWithData.h:

#import "AFURLResponseSerialization.h"

/// NSError userInfo key that will contain response data
static NSString * const JSONResponseSerializerWithDataKey = @"JSONResponseSerializerWithDataKey";

@interface JSONResponseSerializerWithData : AFJSONResponseSerializer
@end

JSONResponseSerializerWithData.m:

#import "JSONResponseSerializerWithData.h"

@implementation JSONResponseSerializerWithData

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
    if (![self validateResponse:(NSHTTPURLResponse *)response data:data error:error]) {
        if (*error != nil) {
            NSMutableDictionary *userInfo = [(*error).userInfo mutableCopy];
            userInfo[JSONResponseSerializerWithDataKey] = data;
            NSError *newError = [NSError errorWithDomain:(*error).domain code:(*error).code userInfo:userInfo];
            (*error) = newError;
        }

        return (nil);
    }

    return ([super responseObjectForResponse:response data:data error:error]);
}

You probably have a custom SessionManager somewhere, you just need to set the responseSerializer property on it.
CustomSessionManager.m:

+ (instancetype)sharedManager
{
    static CustomSharedManager *manager = nil;
    static dispatch_once_t onceToken;
    dispatch_once(&onceToken, ^{
        manager = [[CustomSharedManager alloc] initWithBaseURL:<#url#>];

        // *** Use our custom response serializer ***
        manager.responseSerializer = [JSONResponseSerializerWithData serializer];
    });

    return (manager);
}
@bokojo
Copy link

@bokojo bokojo commented Oct 11, 2013

Thanks for posting that code.

Certainly options are available for solving the problem in my current project: custom serializer; overriding the convenience methods in my custom session manager; favoring notifications over response blocks; forking AFNetworking, breaking backwards compatibility, and changing the failure block parameters...

At least for myself, I'm pushing the issue on here because it warrants a solution in AFNetworking. It's an unreasonable oversight to assume the server will never return useful information just because it returned a failing status code. This is a convenience library. It should not be inconvenient to want a basic core functionality.

That being said, my thanks and congratulations to @matt for maintaining one of a very few cocoa convenience libraries that I will allow in a project I'm working on. I hope to continue using it!

@oleksandr
Copy link

@oleksandr oleksandr commented Oct 11, 2013

@bokojo 👍

@akashkamboj
Copy link

@akashkamboj akashkamboj commented Oct 11, 2013

@gfiumara Thanks for sharing the code. I was in middle of doing more or less like same after seeing mattt's response on your pull request. Your code helped a lot. but i have a few concerns:

  1. What if we fill JSONResponseSerializerWithDataKey with JSON instead of NSData? (Is it recommended or not, since there could be response without JSON. And also what's the best way to fill with JSON)
  2. What if we change NSLocalizedDescriptionKey, NSLocalizedFailureReasonErrorKey values based on what kind of error we got. So that things like UIAlertView+AFNetworking can be used without any code changes.
@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Oct 11, 2013

@akashkamboj I can see filling JSONResponseSerializerWithDataKey with JSON if the web service you are using returns a JSON response with an error message (just add the result of [NSJSONSerialization JSONObjectWithData:options:error:] to the userInfo property instead of data). That would save a step in your failure blocks.

I haven't been using UIAlertView+AFNetworking, but I guess you'd be okay doing that too? The code I posted isn't doing anything except adding a new unique key to the NSError's userInfo property. It's copying everything else populated from validateResponse:data:error:, so none of that should change.

The nice thing about this serializer subclass is that you can make it do more-or-less exactly what you want for different web services.

@akashkamboj
Copy link

@akashkamboj akashkamboj commented Oct 14, 2013

Well after struggling a lot with in an app which use Oauth2 tokens, Refresh tokens. I reached to a conclusion that AFJSONResponseSerializer needs a lot of refactoring or you have to rewrite almost complete ResponseSerializer.

@gfiumara I went to the direction as you mentioned above, by overriding the

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error

but reached to a conclusion that it's not the best way to customize it. Because validateResponse is called twice in this case. Instead if I modify the validateResponse method, it would be a bit better. But then I have to write the validateResponse code completely. What do you think should I go for modifying the validateResponse instead or not?

@Hackmodford
Copy link

@Hackmodford Hackmodford commented Oct 15, 2013

I really liked @gfiumara answer. However I went one step further and made it return an actual string instead of the NSData (it works better for my scenario) I thought I would share the code for others.

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
    if (![self validateResponse:(NSHTTPURLResponse *)response data:data error:error]) {
        if (*error != nil) {
            NSMutableDictionary *userInfo = [(*error).userInfo mutableCopy];
            userInfo[JSONResponseSerializerWithDataKey] = [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding];
            NSError *newError = [NSError errorWithDomain:(*error).domain code:(*error).code userInfo:userInfo];
            (*error) = newError;
        }

        return (nil);
    }

    return ([super responseObjectForResponse:response data:data error:error]);
}

After doing that to retrieve the error message it's as simple as this.

} failure:^(NSURLSessionDataTask *task, NSError *error) {

        NSLog(@"%@",[error.userInfo objectForKey:@"JSONResponseSerializerWithDataKey"]);        
    }];
@Hackmodford
Copy link

@Hackmodford Hackmodford commented Oct 15, 2013

@aehlke
Copy link

@aehlke aehlke commented Jan 3, 2014

Also you mention that you need to update your example to handle a nil responseBody but I don't see why. Won't it still be fine if the key gets set to nil in your original code?

@bokojo
Copy link

@bokojo bokojo commented Jan 3, 2014

Cant have a nil value in a dictionary.

On Jan 2, 2014, at 6:40 PM, Alex Ehlke notifications@github.com wrote:

Also you mention that you need to update your example to handle a nil responseBody but I don't see why. Won't it still be fine if the key gets set to nil in your original code?


Reply to this email directly or view it on GitHub.

@Hackmodford
Copy link

@Hackmodford Hackmodford commented Jan 3, 2014

I've decided to take this one step further and add the response code to the user info dictionary.

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
    id JSONObject = [super responseObjectForResponse:response data:data error:error]; // may mutate `error`

        if (*error != nil) {
            NSMutableDictionary *userInfo = [(*error).userInfo mutableCopy];
            [userInfo setValue:[[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding] forKey:JSONResponseSerializerWithDataKey];
            [userInfo setValue:[response valueForKey:JSONResponseSerializerWithBodyKey] forKey:JSONResponseSerializerWithBodyKey];
            NSError *newError = [NSError errorWithDomain:(*error).domain code:(*error).code userInfo:userInfo];
            (*error) = newError;
        }

    return JSONObject;
}
@Hackmodford
Copy link

@Hackmodford Hackmodford commented Jan 3, 2014

I decided to create my own repo for this subclass, you can find it here. https://github.com/Hackmodford/HMFJSONResponseSerializerWithData

@aehlke
Copy link

@aehlke aehlke commented Jan 3, 2014

You can already get that from the NSURLSessionDataTask, @Hackmodford

@Hackmodford
Copy link

@Hackmodford Hackmodford commented Jan 4, 2014

@aehlke If I remember correctly it was contained in a string like, "blah blah balh (401) blah blah blah". Instead of using nsstring's substring method I wanted a simpler way to get the statusCode.

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Jan 5, 2014

Since this is getting some traffic again, I wrote a blog post about it.

@nikuph
Copy link

@nikuph nikuph commented Jan 15, 2014

@gfiumara why don't you simply use the already parsed JSONObject?

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
  id JSONObject = [super responseObjectForResponse:response data:data error:error];
  if (*error != nil) {
    if (JSONObject != nil) {
      NSMutableDictionary *userInfo = [(*error).userInfo mutableCopy];
      userInfo[JSONResponseSerializerWithDataKey] = JSONObject;
      NSError *newError = [NSError errorWithDomain:(*error).domain code:(*error).code userInfo:userInfo];
      (*error) = newError;
    }
  }

  return (JSONObject);
}
@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Jan 15, 2014

@nikuph you can do whatever you want for your own implementation. In my case, I already had error handler code that expected an NSData. Other people just want to print an NSString because their web service provides them a string. That line is going to be pretty specific to whatever you're dealing with I'd imagine.

@sternhenri
Copy link

@sternhenri sternhenri commented Apr 16, 2014

So this may be a silly question, but with the sole purpose of getting back the HTTP status code from the response, can someone walk me through the downside of doing this, after having checked the task.code (and that it is -1011):

failure:^(NSURLSessionDataTask *task, NSError *error) {
        NSHTTPURLResponse *response = (NSHTTPURLResponse *)task.response;
        int statusCode = response.statusCode;

(from http://stackoverflow.com/questions/19096556/afnetworking-2-0-afhttpsessionmanager-how-to-get-status-code-and-response-json).

Apologies for the overly general question, I'm simply loathe to subclass the serializer if I can avoid it!

Thanks!

@rromanchuk
Copy link

@rromanchuk rromanchuk commented Apr 16, 2014

@sternhenri yeah, it's actually a lot more complicated and @Hackmodford is not helping at all, as he doesn't understand your question (and i'm sure understand that basics of http status codes). To pass back the status code after error response you need to do something like this

#import "JSONResponseSerializerWithData.h"

@implementation JSONResponseSerializerWithData

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
    if (![self validateResponse:(NSHTTPURLResponse *)response data:data error:error]) {
        if (*error != nil) {
            NSMutableDictionary *userInfo = [(*error).userInfo mutableCopy];
            NSInteger statusCode = [[[*error userInfo] objectForKey:@"AFNetworkingOperationFailingURLResponseErrorKey"] statusCode];
            if (statusCode == 401) {
                dispatch_async(dispatch_get_main_queue(),^{
                    [[NSNotificationCenter defaultCenter] postNotificationName:@"unauthorized_request" object:nil];
                    NSLog(@"UNAUTHORIZED!!!");
                });
            }

            userInfo[JSONResponseSerializerWithDataKey] = data;
            NSError *newError = [NSError errorWithDomain:(*error).domain code:(*error).code userInfo:userInfo];
            (*error) = newError;
        }

        return (nil);
    }

    return ([super responseObjectForResponse:response data:data error:error]);
}

@end
@sternhenri
Copy link

@sternhenri sternhenri commented Apr 16, 2014

@rromanchuk thanks a lot for the great snippet! I simply wonder in what ways this approach is superior than the two-liner I put above for someone simply looking to extract the HTTP Status Code from the response to an AFHTTPSessionManager request.

The code from my API is all I need to do the iOS error post-processing in the failure block so I'm wondering if I've misunderstood something (likely) given that I'm not sure why I would need to implement the code you outlined above...?

@rromanchuk
Copy link

@rromanchuk rromanchuk commented Apr 16, 2014

@sternhenri it's a pain in the ass, but you're always going to get a -1011 on error (even if server is passing 5xx) By far the worst part of afnetworking 2, it's like pulling teeth.

@tewha
Copy link
Contributor

@tewha tewha commented Apr 16, 2014

You could write a method in a category on NSError pretty easily. Return the HTTP status code if the domain is AFNetworkingErrorDomain and the code is NSURLErrorBadServerResponse; otherwise, return 0.

Whoops. Beg pardon. I don't remember if the object is available in userInfo. If it isn't, never mind.

@sternhenri
Copy link

@sternhenri sternhenri commented Apr 17, 2014

@rromanchuk I gotcha, though the actual status code (not the -1011) is also part of user info. Thanks so much for all the replies!

@tewha It's available in the Task in any case so it can be done through there! Thanks!

@wprater
Copy link

@wprater wprater commented May 15, 2014

Why can't you just set the container of the NSError pointer to have a new NSError with your userInfo keys set? This way, we can still call super and not worry about AFNetworking API changes. Something like:

- (id)responseObjectForResponse:(NSURLResponse *)response
                           data:(NSData *)data
                          error:(NSError *__autoreleasing *)error
{
    id *responseObject = [super responseObjectForResponse:response
                                                     data:data
                                                    error:error];

    if (error) {
        NSMutableDictionary.new *userInfo;
        [userInfo addEntriesFromDictionary:error.userInfo];
        [userInfo setValue:responseObject.message forKey:NSLocalizedDescriptionKey];
        [userInfo setValue:responseObject.code forKey:NSLocalizedFailureReasonErrorKey];

        *error = [NSError errorWithDomain:error.domain code:error.code userInfo:userInfo];        
    }

    return responseObject;
}
@TomSwift
Copy link

@TomSwift TomSwift commented May 17, 2014

Sorry if I'm missing the point. But why not treat HTTP status codes that you want to process as successes instead of failures? E.g., if you want to process the response for a 404, add 404 to the acceptableStatusCodes set owned by the responseSerializer and be done with it. Look for the 404 in the success block and act accordingly. I think an argument can be made that if you intend to process a returned response then the request was successful.

@onmyway133
Copy link

@onmyway133 onmyway133 commented Jul 23, 2014

@gfiumara
The method responseObjectForResponse:data:error is called in

- (id)responseObject {
    [self.lock lock];
    if (!_responseObject && [self isFinished] && !self.error) {
        NSError *error = nil;
        self.responseObject = [self.responseSerializer responseObjectForResponse:self.response data:self.responseData error:&error];
        if (error) {
            self.responseSerializationError = error;
        }
    }
    [self.lock unlock];

    return _responseObject;
}

Please note the !self.error check, how can your override method be called in the failure case?

@gfiumara
Copy link
Contributor

@gfiumara gfiumara commented Jul 23, 2014

@onmyway133 That method is off of AFHTTPRequestOperation and directly picking off the response. The solution above overrides the responseSerializer from a SessionManager. The solution presented in the comments here wouldn't rely on calling that method.

@Jeehut
Copy link

@Jeehut Jeehut commented Aug 4, 2014

Anyone know how to solve this issue using Swift?

I've tried this here with no luck (the responseObjectForResponse(...) doesn't get called at all):

let ResponseBodyKey = "ResponseBody"

class FeedbackJSONResponseSerializer: AFJSONResponseSerializer {
    override func responseObjectForResponse(response: NSURLResponse!, data: NSData!, error: NSErrorPointer) -> AnyObject! {

        var validationError: NSError?
        if !self.validateResponse(response as NSHTTPURLResponse!, data: data, error: &validationError) {

            if let userInfo: NSDictionary = error.memory?.userInfo {
                var mutableUserInfo: NSMutableDictionary! = userInfo.mutableCopy() as NSMutableDictionary!

                mutableUserInfo[ResponseBodyKey] = NSString(data: data, encoding: NSUTF8StringEncoding)

                if let errorMemory: NSError = error.memory {
                    error.memory = NSError.errorWithDomain(errorMemory.domain, code: errorMemory.code, userInfo: mutableUserInfo.copy() as NSDictionary!)
                }
            }

            return nil
        }

        return super.responseObjectForResponse(response, data: data, error: error);
    }
}

And AFHTTPSessionManager subclass:

private let _SharedAPIConnector = APIConnector()

class APIConnector: AFHTTPSessionManager {

    private let apiUrl: NSURL = NSURL(string: "https://api.someservice.com/")

    class var sharedInstance: APIConnector { return _APIConnector }

    private init() {
        super.init(baseURL: self.apiUrl, sessionConfiguration: nil)

        self.responseSerializer = FeedbackJSONResponseSerializer()
    }
}

Btw: I've come across this discussion the second time now and just still can't understand, why there is no solution for this by default in AFNetworking. This is such a basic feature of every HTTP client! This is weird, and "backwards compatibility" doesn't make it better. This should be in there somewhere. Added as an option, if not else possible.

@chrishulbert
Copy link

@chrishulbert chrishulbert commented Sep 10, 2014

I've been wrestling with this a bit lately. I originally came up with a solution involving swizzling, but later settled on a AFHTTPSessionManager subclass. It wraps completion handlers with a shim that inject the response body into the nserror's userInfo.

If you're interested, you can read it here: http://www.splinter.com.au/2014/09/10/afnetworking-error-bodies/

I believe you could adapt my solution to Swift without much trouble, as it's fairly straightforward.

@Jeehut
Copy link

@Jeehut Jeehut commented Sep 14, 2014

Thanks for the tip @chrishulbert but I switched to Alamofire which doesn't have this problem although I needed to implement something similar to the session manager on my own.

@chrishulbert
Copy link

@chrishulbert chrishulbert commented Sep 14, 2014

Isn't Alamofire swift-only?

On Sun, Sep 14, 2014 at 11:04 PM, Dschee notifications@github.com wrote:

Thanks for the tip @chrishulbert https://github.com/chrishulbert but I
switched to Alamofire which doesn't have this problem although I needed to
implement something similar to the session manager on my own.


Reply to this email directly or view it on GitHub
#1397 (comment)
.

@Jeehut
Copy link

@Jeehut Jeehut commented Sep 15, 2014

It's written in Swift which doesn't mean it's not usable in ObjC either. Apple has build a working bridging system in order to make Swuft and ObjC code live and wirk well together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.