Relies on statusCode to figure out whether a request failed

wvteijlingen / Spine

A Swift library for working with JSON:API APIs. It supports mapping to custom model classes, fetching, advanced querying, linking and persisting.

MIT License

266 stars 109 forks source link

Relies on statusCode to figure out whether a request failed #28

Closed kurko closed 9 years ago

kurko commented 9 years ago

This PR stops relying on NSURLSession.error and relies on the status code to figure whether a request failed or not. The reason is because in my app, .error is nil even when status code is 400. That was preventing .onFailure to ever be triggered.

Running the tests after the change, nothing broke, so I suppose everything's fine. Plus, my app is now working :D

wvteijlingen commented 9 years ago

You are absolutely right on the need to handle non-success status codes.

The if let networkError = networkError should still be in the Operation, because that creates a networkError error when an error in the network occurs. This is different from an error returned by the API.

I'm not sure we need the change in Networking though. Those if's are just for logging purposes, the logic being: If a network error occurred an error is logged, otherwise if a success status code is returned info is logged, otherwise a warning is logged.

kurko commented 9 years ago

Just to confirm I understand what you're saying,

you don't think Networking needs to be touched: I agree
Operations should still keep a check for network error. (sorry, I didn't know that was strictly for network errors)
you agree on relying on status codes, but do you agree with this PR? Would you do it differently?

wvteijlingen commented 9 years ago

I agree with this PR, we should add a status code check to the Operation like you did. We should add an if let to statusCode since it can be nil in case of no response. Also I think it might be better to check for the 2xx success range instead of the 400-500 error range. Something along the lines of:

if let statusCode = statusCode where 200 ... 299 ~= statusCode {
    // Success
} else {
    // No success
}

Also, because this is needed in all operations, it's probably best to extract this. Maybe put it in the HTTPClient itself, or maybe in a separate function.

kurko commented 9 years ago

:+1: I'll do it as soon as I get back to Spine. Been learning Realm lately.

kurko commented 9 years ago

I don't know why this is conflicting. I rebased... gonna open another PR. I changed the things you suggested.

EDIT: silly me. The new PR is https://github.com/wvteijlingen/Spine/pull/28 and it fixes what you suggested.