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

4XX is now an error #170

Closed
jmonster opened this issue Apr 4, 2017 · 7 comments
Closed

4XX is now an error #170

jmonster opened this issue Apr 4, 2017 · 7 comments
Assignees
Labels
Milestone

Comments

@jmonster
Copy link
Contributor

@jmonster jmonster commented Apr 4, 2017

The recent v12 release has redefined 4XX responses as errors. Is this an intentional redefinition? Historically http clients treat actual network errors as errors since a 4XX is a valid server generated response. It can have a meaningful payload and be considered a normal expected result.

I'm using wreck promisified with Bluebird and it took awhile to figure out that { Error: Response Error: null... was due to this change.

@geek geek added the non issue label Apr 4, 2017
@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Apr 4, 2017

@jmonster this is intentional #165

The response isn't short-circuited, so you can still get the full response and not error.

@geek geek closed this Apr 4, 2017
@jmonster

This comment has been minimized.

Copy link
Contributor Author

@jmonster jmonster commented Apr 4, 2017

👍 It'd probably be a good idea to mention that an error is anything > 399 in the README especially since it just changed / is ambiguous

@csrl

This comment has been minimized.

Copy link

@csrl csrl commented Aug 10, 2017

Are you open to having an option "treat4xxAsError" or something? When using Wreck to interface to REST apis, handling 4xx response is part of the application/business logic. But now we have to special case our error handling which used to be only for non-business logic errors - network problems, or configuration problems, or remote service not available problems - and split out the 4xx errors and insert them back into the normal business logic code flow.

@geek

This comment has been minimized.

Copy link
Member

@geek geek commented Aug 11, 2017

This documentation fix is in with #175... closing

@csrl thanks for raising this. Can you explain more about how you are handling the 400 errors... I'm not sure I understand the case where you insert them back into the business logic code flow. Maybe a code snippet would help clarify things. Thanks.

@geek geek closed this Aug 11, 2017
@geek geek self-assigned this Aug 11, 2017
@geek geek added this to the 12.2.0 milestone Aug 11, 2017
@jmonster

This comment has been minimized.

Copy link
Contributor Author

@jmonster jmonster commented Aug 11, 2017

We've moved on from this with our own work aroundw but FWIW had the exact same experience. 4XX statuses aren't errors and we had to completely rework our error handling to handle business logic and then rework all of our error reporting that used to be effortless..

Oh well ¯_(ツ)_/¯

@csrl

This comment has been minimized.

Copy link

@csrl csrl commented Aug 24, 2017

@geek what I've gone ahead and done, is wrapped wreck callbacks with this shim code:

const cbWrap = function (callback) {
  return function (err, response, payload) {
    if (err && err.data && err.data.isResponseError) {
      response = err.data.response;
      err = null;
    }
    callback(err, response, payload);
  }
}

It is used something like:

Wreck.get(Url, cbWrap(function (err, response, payload) {
  if (err) {
    return callerCallback(err);
  }
 // If we didn't wrap Wreck, then this turns into this ugliness:
// if (err && err.data && err.data.isResponseError && err.data.response.statusCode === 404)
  if (response.statusCode === 404) {
    // 404s are ok, ignore
    return callerCallback(null);
  }
  // do other things ...
});

Anyway, I've also moved on and worked around the issue now. But it would be nice if the response was always provided in the callback 'response' parameter whether it is considered an error or not, and then provide a config option to not treat 4xx as an error.

@tlivings

This comment has been minimized.

Copy link
Contributor

@tlivings tlivings commented Nov 29, 2017

If a response came back it is not an error. A 4xx means the input from client was wrong. A 5xx means that service did not respond in an expect fashion (including that it was unavailable). But the fact is a response was returned... a error did not occur in the request.

I think it’s a bad idea to treat any non-OK http response as an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.