hapijs / boom

HTTP-friendly error objects
Other
2.94k stars 192 forks source link

Expose Boom.unauthorized as a way to wrap another error #43

Closed phated closed 9 years ago

phated commented 9 years ago

I am trying to augment some custom errors with properties from Boom.unauthorized (mainly the header) and am finding that you can't easily wrap a custom error with it. I am currently just generating a new unauthorized error with the properties I want and then using extend to attach the properties. I was hoping there would be a cleaner way to do this, like Boom.wrap does for the generic stuff.

arb commented 9 years ago

Can you give an example of how you'd like it to work or how you would use it? Even the code you described would be helpful.

arb commented 9 years ago

And updates @phated?

phated commented 9 years ago

Sorry, I've been on vacation. Let me see if I can put something together.

phated commented 9 years ago

Custom error using Boom.wrap - RecordNotFoundError

function RecordNotFoundError(message){
    Error.call(this);

    this.message = message;

    boom.wrap(this, 404);
}

util.inherits(RecordNotFoundError, Error);

Custom error for unauthorized (currently) - AccountLockedError

function AccountLockedError(message){
    Error.call(this);

    var unauth = boom.unauthorized('Account Locked', 'bearer');

    Object.assign(this, unauth);

    this.message = message || unauth.message;
}

util.inherits(AccountLockedError, Error);

It would be nice if Boom.wrap could be used for the Boom.unauthorized example to avoid the Object.assign, etc. The reason I didn't use Boom.wrap(this, 401) is because the headers are important to pass along.

Let me know if any of that doesn't make sense.

arb commented 9 years ago

I guess I'm not understanding what utility you are getting from this. It seems like you just want boom.unauthorized? Couldn't you just create that, add or change what you wanted, and return it?

I mean, currently, wrap doesn't do a whole lot in this context, just adds the boilerplate Boom properties and looks up the HTTP message by the status code.

I guess we could add boom.wrapUnauthorized or add a check in wrap to do more than it does currently.

phated commented 9 years ago

Why would I want to manually handle the boom boilerplate when wrap does that in a more future-compatible way? I prefer subclassed error constructors over modifying a plain error object.

I'd be fine with either of those 2 suggestions. It just seems weird that unauthorized is the only method that has custom functionality that wrap can't add to another error.

hueniverse commented 9 years ago

The original design was to include any such error as extra data inside the 401 error, not as its base. What are you trying to add?

phated commented 9 years ago

I am trying to have the headers attached to my custom error constructor

hueniverse commented 9 years ago

I get that. But why? The 401 response should really be just the header. It feels like you are exposing your internal logic through the error instead of putting that internal state elsewhere and exposing a pure 401 error.

phated commented 9 years ago

@hueniverse This stems out of https://github.com/hapijs/hapi/issues/2326. I would prefer to intercept my application-specific errors in a plugin that registers onPostHandler and then does the conversion to send boom errors to the client, but that causes an internalError to be logged. The only way around that currently is to make our application errors "inherit" from boom errors to have the properties they need to respond with. I wouldn't really care about this issue if the other one was solved, but if I need to work around it, then this would be helpful.

lock[bot] commented 4 years ago

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.