Allow decorating a boom error

[mariecl]

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]

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

[mariecl]

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 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]

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]

Past being what version?

[mariecl]

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

[samueljoli]

@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]

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

[mariecl]

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]

Done ✅

[hueniverse]

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.

[mariecl]

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

[AJamesPhillips]

@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]

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

[mariecl]

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

[lock]

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.