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

Boom.badGateway( string, Boom.anything() ) #152

Closed
jmonster opened this issue Apr 18, 2017 · 10 comments
Assignees
Labels
bug
Milestone

Comments

@jmonster
Copy link

@jmonster jmonster commented Apr 18, 2017

Preface: Our use case may not be your use case; it's understood you may disagree with this concern. That said...

We're using promises in our codebase. So imagine a scenario like the following...

wreckAsPromised.get('https://...')
  .then((response) => { ... })
  .catch((err) => {
    // this is where things go badly vvv
    throw Boom.badGateway('Unable to reach some endpoint', err);
  });

Our server reports all errors that we reply() with to Rollbar. So, it's important for us to

  1. return a decent human message like 'Unable to reach the API'
  2. capture actual error data so we may be debug/fix issues

But this code fails because of this behavior:

I expect changing this will be very controversial but wanted to highlight the concern regardless.


cc @tevanoff

@arb

This comment has been minimized.

Copy link
Contributor

@arb arb commented Apr 19, 2017

Did you have a proposed solution or did you just want to start a conversation about possible solutions?

@tevanoff

This comment has been minimized.

Copy link

@tevanoff tevanoff commented Apr 19, 2017

@arb after looking at this a bit more, it looks like the root of the problem is this: https://github.com/hapijs/boom/blob/master/lib/index.js#L392

We haven't tested it out thoroughly, but if we change that to if (data instanceof Error && !data.isBoom) { it fixes the problem we're seeing since it avoids trying to wrap data when it's a Boom instance and just sets it on the created Boom instance as we were expecting.

We can test it out a bit more and open a PR if that sounds like a viable solution to you. Happy to hear if you have thoughts on a better solution too.

Thanks!

@camfraser

This comment has been minimized.

Copy link

@camfraser camfraser commented Apr 22, 2017

@tevanoff alternatively, you could adjust your call to Boom.badGateway to wrap the rejected data in an error. This is a technique I use regularly in our promise-based repos.

wreckAsPromised.get('https://...')
  .then((response) => { ... })
  .catch((err) => {
    // this is where things go badly vvv
    throw Boom.badGateway('Unable to reach some endpoint', new Error(err));
  });
@jmonster

This comment has been minimized.

Copy link
Author

@jmonster jmonster commented May 10, 2017

I appreciate the suggestions, but our underlying goal is to capture the original error's stack trace. The Boom docs specify that arbitrary data can be passed in as the second parameter yet there exists some data that it doesn't like (i.e. anything with isBoom: true).

So I think we have either a bug in the code or a bug in the docs?

@arb is there any reason not to apply @tevanoff's suggested fix at a minimum?

@WesTyler

This comment has been minimized.

Copy link

@WesTyler WesTyler commented May 15, 2017

So is the problem coming from the case when the err that gets caught is itself a Boom error object? Or do TypeErrors and other Node.js Error objects also cause a problem?

@tevanoff

This comment has been minimized.

Copy link

@tevanoff tevanoff commented May 17, 2017

@WesTyler yeah, it's when the err is a Boom and we try to pass that in as the data attribute on another Boom object. But we started looking into it a bit more and it only seems to break when using the 5xx error helpers.

For example, this is the case that started breaking for us:

error = Boom.badImplementation('badImpl');
Boom.badGateway('badGateway', error);
> Uncaught Error: Cannot provide statusCode or message with boom error

However, doing the same thing using create does work:

error = Boom.badImplementation('badImpl');
e = Boom.create(502, 'badGateway', error);
e.data
> Error: badImpl

The 4xx error helpers also seem to work fine:

error = Boom.badImplementation('badImpl');
e = Boom.notFound('notFound', error);
e.data
> Error: badImpl

I'm not sure if there's a reason for the different behaviors between Boom.create() and Boom.badGateway() and the like, but we started using Boom.create() since it works as we'd expect when passing an error (that may be a Boom) in as a the data attribute.

Any thoughts on why the 5xx error helpers seem to go through a different path? Thanks!

@AJamesPhillips

This comment has been minimized.

Copy link

@AJamesPhillips AJamesPhillips commented May 17, 2017

@tevanoff that is an interesting question but also I'm curious what the use case is for changing the type of the error? If it's a 500, 501, or 502 why not just return that?

@jmonster

This comment has been minimized.

Copy link
Author

@jmonster jmonster commented May 17, 2017

Our API makes calls to another API. When that service returns a 5XX it is incorrect for our server to propagate the response; our server did not error, it received an error from an upstream service. We want to reply with a 502 in that scenario -- for example.

As of wreck v12, 4XX responses are also considered errors. We do generally want to propagate those statuses, but we want to return meaningful semantics to our clients -- e.g. "Unable to fetch favorites" -- rather than the message from one of many API calls that are involved in identifying favorites.

We also don't want to return low-level details or stack traces; instead, we want simply human readable messages but we also want those low level details in our err object because we have a plugin icecreambar that will propagate low-level error data to Rollbar from any reply(err) call

@WesTyler

This comment has been minimized.

Copy link

@WesTyler WesTyler commented May 17, 2017

This is my own personal speculation and not fact, so take it with a hefty handful of salt.

I think the discrepancy between 4xx and 5xx behavior is that for 5xx server errors you typically are not trying to expose details to the requesting client. Additionally, for server errors, you are usually catching JS errors. I would expect 4xx responses from outside APIs to be handled specifically in an error block prior to landing in an ImplementationError catch.

Another opinionated thought is that when you receive a 4xx error from an outside API, is that really a server error on your part? To me, a 502 indicates connectivity errors or some other 5xx error from the downstream API.

@hueniverse hueniverse self-assigned this May 25, 2017
@hueniverse hueniverse added the bug label May 25, 2017
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented May 25, 2017

If you are passing a boom object into another, you clear mean to use it as data, not as the foundation for a new error. This is a bug.

@hueniverse hueniverse added this to the 4.3.2 milestone May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.