Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

UnWrap HttpErrors in pop middleware and return them. #138

Merged
merged 7 commits into from
Jan 16, 2017

Conversation

lumost
Copy link
Contributor

@lumost lumost commented Jan 15, 2017

The pop transaction middleware wraps all handlers in a connection.Transaction, which in turn wraps any errors the handler emits. The default ErrorHandler will then handle the error as a 500 and will not be able to mirror the requested status from a call such as c.Error(401, err)

@markbates
Copy link
Member

Thinking about this, I feel like it should be moved to where the error handling is done globally, so everything benefits, not just this middleware.

Thoughts?

@lumost
Copy link
Contributor Author

lumost commented Jan 15, 2017

I like the idea; one potential issue is that we will always swallow the error from the middleware, which could be a problem for folks rolling in their own middlewares. Do you think it would be worthwhile to create an additional MiddlewareError type to allow middlewares to override the HTTPError?

side note - I'll take a look at the build issue.

@lumost
Copy link
Contributor Author

lumost commented Jan 15, 2017

I did some digging on potential ways of handling the case where different middleware's in the chain would want to alter the final response. By far the simplest solution is to expose the HTTPError type at the package level and allow middlewares to return it if they need to alter the error response, optionally these middlewares can embed the original HTTPError in the msg of the response or add additional field to the error depending on the use case.

I've set it up as you suggested and will check whether the root error is a buffalo.HTTPError for all handlers and if it is we will use that status code.

@markbates
Copy link
Member

This looks good! Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants