Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Convenience option to return error on 4xx/5xx status codes? #606

Closed
aseemk opened this Issue · 15 comments

4 participants

Aseem Kishore Eric Schoffstall Mikeal Rogers Paul Walker
Aseem Kishore

I often want to treat a 4xx or 5xx response code from an API as an error case, so I often end up having to check that manually.

Would you be open to adding a boolean option to automatically treat 4xx/5xx responses as errors? E.g. Kenneth Reitz's requests library (for Python) has a convenience raise_for_status API like this (though it's a method there):

http://docs.python-requests.org/en/latest/user/quickstart/#response-status-codes

The default would/should be to maintain the current behavior of not erroring on 4xx/5xx status.

If you're open to it, the implementation is simple; the only question is what name it should have. =)

Some ideas are errorByStatus, expectSuccess, inspectStatus, but I'm sure others can come up with better ideas!

Thanks again for the great library. Cheers.

P.S. Usage example:

request.get({
  url: url,
  expectSuccess: true
}, callback);

And implementation snippet:

if (opts.expectSuccess && resp.statusCode >= 400) {
  return self.emit('error');
}
Eric Schoffstall

+1

Mikeal Rogers
Owner

needs a better option name, but i like it, i do this all too often.

Aseem Kishore

Great. Any ideas or suggestions for name, @mikeal?

Eric Schoffstall

Couple of questions to stimulate conversation:

  1. What would the behavior for this be? Does the body become the error message? Or is the error message just the statusCode?
  2. Should it respect {json:true} for errors and attempt to parse the body?
  3. Configuration for which statusCodes are considered errors?
  4. Should this behavior be the default?

As for the name - how about something like checkStatusCode?

Aseem Kishore
  1. No harm in including the body -- it might be helpful -- but status code for sure. How about: new Error(response.statusCode + ' response: ' + response.body)?

  2. For simplicity, I'd assume no. Request doesn't do any fancy errors today -- everything is just a standard Error object with only a string message -- so we wouldn't do anything special with the body anyway, other than stringify it back to include it in the message.

  3. It'd be simplest to just make the option a boolean and consider the standard 4xx/5xx to be an error. If it turns out people have legitimate use cases for customizing that, it's not hard to overload the option (like json) to support that.

  4. I assume no, since it'd be a serious breaking change.

All of these are of course ultimately @mikeal's call. =)

I like checkStatusCode! Also cool w/ a shorter checkStatus.

Paul Walker
  1. Body is a must.
  2. There are plenty of json apis that include a json body in non 2xx responses. I don't see a reason not to respect the json option here or at least examine the content-type header (if json: true) and decide appropriately.
  3. the auto editing isn't letting me leave out a 3, lol, no opinion.
  4. IMHO, at some point in this modules future life all 400+ responses should all be considered errors by default.
Aseem Kishore

@paulwalker: what would you do w/ a JSON-decoded body other than include it in the string error message?

Mikeal Rogers
Owner

this patch is going to get really complicated if it's trying to determine whether or not streaming is enabled and deferring the error until "end".

this error should get emitted in a "response" event handler and not include the body. if you want the body then just handle the error in the body callback like you do now.

Mikeal Rogers
Owner

oh, and this feature will never be enabled by default :)

Paul Walker

why not at least include the response as a property of the error? are you saying the status would not even be available?

Mikeal Rogers
Owner
Paul Walker

ok, so the whole response would be available correct? as long as i can get at the response body somehow from a general error handler i am happy. for example:

async.waterfall([
  function(callback) {
    request.get({ url: url, json: true }, callback);
  },
  function(r, json, callback) {
    request.get({ url: anotherUrl, json: true }, callback);
  }], ...
  function(err, result) {
    // want access to response body here somehow
  });
Eric Schoffstall

@paulwalker - He is saying the body would not be available on streaming requests but for your case where you pass the callback it would be.

Mikeal Rogers
Owner

i wouldn't assume that you can actually get the body if there is an error.

this is the problem with considering higher order failures an "error" at the same layer as socket errors. there is abort and teardown logic that gets fired on error.

Mikeal Rogers
Owner

we're past the point of talking about this, without a PR we can't really move forward.

Mikeal Rogers mikeal closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.