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

Allow decorating a boom error #160

Closed
mariecl opened this issue Jul 2, 2017 · 15 comments
Assignees
Labels
Milestone

Comments

@mariecl
Copy link

@mariecl mariecl commented Jul 2, 2017

What are you trying to achieve or the steps to reproduce?

I am trying to wrap a Boom object (whether it was an error that got wrapped before, or an error created with Boom).

const Boom = require('boom');

const error = new Error('an error');
const modifiedError = Boom.wrap(error, 501, 'bad error'); // success
const wrappedError = Boom.wrap(modifiedError, 500, 'terrible error'); // failure

or

const Boom = require('boom');

const error = Boom.badData('an error');
const wrappedError = Boom.wrap(error, 500, 'terrible error'); // failure

What was the result you received?

In both cases:

/Users/redacted/node_modules/boom/node_modules/hoek/lib/index.js:736
    throw new Error(msgs.join(' ') || 'Unknown error');
    ^

Error: Cannot provide statusCode or message with boom error
    at Object.exports.assert (/Users/redacted/node_modules/boom/node_modules/hoek/lib/index.js:736:11)
    at Object.exports.wrap (/Users/redacted/node_modules/boom/lib/index.js:76:10)
    at Object.<anonymous> (/Users/redacted/tool/wrappedError.js:5:27)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:158:16)

What did you expect?

I expected no thrown error and Boom.wrap to return the original Boom object, as stated in the README:

error - the error object to wrap. If error is already a boom object, returns back the same object.

Context

  • node version: 8.1.0
  • boom version: 5.1.0
  • last boom version where it was working: 4.2.0
  • other boom version where it also fails: 5.0.0, 4.3.1, 4.3.0
@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jul 2, 2017

The error message is pretty self explanatory: "Cannot provide statusCode or message with boom error"

@hueniverse hueniverse closed this Jul 2, 2017
@hueniverse hueniverse added the non issue label Jul 2, 2017
@hueniverse hueniverse self-assigned this Jul 2, 2017
@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 3, 2017

I am sorry but I don't see how this behavior is consistent with the documentation, especially since the behavior changed and the documentation did not. If I can only get my original Boom object back when I don't specify a statusCode and a message, then the documentation should specify it instead of reading If error is already a boom object, returns back the same object.

In my opinion, this new behavior makes the wrap function much less useful as you can basically only use it on non-Boom objects without causing an exception. Doing Boom.wrap(error, statusCode, message) when you don't know whether the error is a Boom object (or you don't want to make the assumption) was a very convenient way of not writing if (error.isBoom) {} else {} everywhere while making sure the server would always respond with Boom objects. More specifically, since Boom.wrap already has two different behaviors depending on the type of error that's being wrapped, why not keep on embracing this principle fully in newer versions?

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jul 3, 2017

This was always how wrap() worked. Not sure what changed exactly. We never supported overriding one boom error with a new status code. You should never force a boom error into something else. If you want to do that, create a new boom error and pass the existing error as data.

@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 3, 2017

I am not expecting the Boom error to be over-written, I am expecting it get returned unchanged. But I want non-Boom errors to be wrapped. Again, that's only useful when some other function returns an error that can or can not be a Boom object.

const nonConsistentFunction = function(input, callback) {
  if (input === 'a') {
    return callback(new Error('an error'));
  }
  else {
    return callback(Boom.badData());
  }
}

nonConsistentFunction(value, (error, result) => {
   if (error) {
    return reply(Boom.wrap(error, 500, 'terrible error'));
  }
  // something else
}

In the past:

  • case input === 'a', the server would reply a 500,
  • case input !== 'a', the server would reply with the previously defined 422 Unprocessable Entity.

Now:

  • case input === 'a', the server still replies with 500,
  • case input !=='a', there's an uncaught exception.

What breaks the previous behavior is this commit: 544cc13db7c7fc99b43bbb8eb1ce2ce57f2c78db
Indeed, previously, Boom.wrap() would notice the supplied error is a Boom object, not care about other passed arguments, and return the error unchanged, as stated in the docs.
Now, Boom.wrap() finds it abnormal to be supplied with a Boom object AND a statusCode and a message.
Note that Boom.wrap(Boom.badData(), null, null) doesn't throw, as verified in the test case modified in this commit.

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jul 3, 2017

Past being what version?

@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 3, 2017

As far as I can tell, the last working version was 4.2.0.

@hueniverse hueniverse reopened this Jul 4, 2017
@Samueljoli

This comment has been minimized.

Copy link

@Samueljoli Samueljoli commented Jul 11, 2017

@mariecl Would you happen to be working on a fix?

Seems like it's a simple solution, the author of that commit should of used double bangs in order to coerce those values into Booleans like so...

Hoek.assert(!error.isBoom || (!!statusCode && !!message), 'Cannot provide statusCode or message with boom error');
@Nargonath

This comment has been minimized.

Copy link
Member

@Nargonath Nargonath commented Jul 11, 2017

@mariecl @Samueljoli If you happen not to have time to create the PR, I'll be happy to help.

@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 12, 2017

I had not understood that @hueniverse was willing to take a PR on this but happy to work on something today just in case.

@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 13, 2017

Done

@hueniverse

This comment has been minimized.

Copy link
Member

@hueniverse hueniverse commented Jul 18, 2017

I wasn't looking for a PR just yet... (I was on vacation).

There is no obvious right answer here other than another breaking change to clear this up or a secondary interface for your use case. I just need to pick one. I'll figure it out later today.

Simply ignoring the arguments is a non-starter. It was a bug that has some useful side effects for you.

@hueniverse hueniverse added request and removed non issue labels Jul 18, 2017
@hueniverse hueniverse added this to the 5.1.1 milestone Jul 18, 2017
@hueniverse hueniverse added feature and removed request labels Jul 18, 2017
@hueniverse hueniverse changed the title Boom.wrap a Boom object throws an error Allow decorating a boom error Jul 18, 2017
@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Jul 19, 2017

No worries. Thanks for considering this as a potential future enhancement and thanks for the good work!

@AJamesPhillips

This comment has been minimized.

Copy link

@AJamesPhillips AJamesPhillips commented Jul 30, 2017

@mariecl I'm sure you've already come up with a work around for your application. If you have a single common entry point into your application did you decide to mutate / monkeypatch Boom with something like .safeWrap = (...) => { if (error.isBoom) { ... } else { ... } }?

@AJamesPhillips

This comment has been minimized.

Copy link

@AJamesPhillips AJamesPhillips commented Jul 30, 2017

We should definitely put in a PR to fix the docs though, you want to do this @mariecl ? Otherwise I'm happy to.

@mariecl

This comment has been minimized.

Copy link
Author

@mariecl mariecl commented Aug 1, 2017

@AJamesPhillips I think the documentation was already updated with the new solution the maintainer came up with.

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