Add ability to add handlers for raised exceptions #688
Conversation
src/kemal/exception_handler.cr
Outdated
| @@ -11,12 +11,26 @@ module Kemal | |||
| rescue ex : Kemal::Exceptions::CustomException | |||
| call_exception_with_status_code(context, ex, context.response.status_code) | |||
| rescue ex : Exception | |||
| # Use error handler defined for the current exception if it exists | |||
| return call_exception_with_exception(context, ex, 500) if Kemal.config.error_handlers.has_key?(ex.class) | |||
There was a problem hiding this comment.
This line is added above the log function in order for the specs to pass.
A logger is needed for #log to be able to successfully run which the tests in exception_handler_spec.cr doesn't appear to have defined.
This shouldn't be a problem in the real world case as a logger would always exist afaik.
Please let me know if this should be rectified in some other way.
|
This is perfect! |
Co-authored-by: Johannes Müller <straightshoota@gmail.com>
syeopite
force-pushed
the
handle-custom-errors
branch
from
e7f441a to
bf19e27
Compare
`ex.class <= key` below should already handle the exact match ```crystal Kemal.config.error_handlers.each_key do |key| if ex.class <= key end end ```
Prior to this commit, handlers for exceptions and handlers for HTTP status codes were stored in a single collection. However, the two are used completely independently from each other and should instead be stored in two separate collections.
|
|
||
| # Adds an error handler for the given exception | ||
| def add_exception_handler(exception : Exception.class, &handler : HTTP::Server::Context, Exception -> _) | ||
| EXCEPTION_HANDLERS[exception] = ->(context : HTTP::Server::Context, error : Exception) { handler.call(context, error).to_s } |
There was a problem hiding this comment.
I believe this can be added as given, avoiding an additional closure.
| EXCEPTION_HANDLERS[exception] = ->(context : HTTP::Server::Context, error : Exception) { handler.call(context, error).to_s } | |
| EXCEPTION_HANDLERS[exception] = handler |
There was a problem hiding this comment.
Yeah I agree requiring String output type should be preferred in general. This might have some UX consequences though...
@syeopite What's the motivation for an unspecified block output type?
There was a problem hiding this comment.
LGTM @syeopite, thanks a lot 👍 Waiting for the review of @straight-shoota ⌛
| def add_error_handler(status_code : Int32, &handler : HTTP::Server::Context, Exception -> _) | ||
| ERROR_HANDLERS[status_code] = ->(context : HTTP::Server::Context, error : Exception) { handler.call(context, error).to_s } | ||
| end | ||
|
|
||
| # Returns tje defined error handlers for exceptions |
There was a problem hiding this comment.
typo:
| # Returns tje defined error handlers for exceptions | |
| # Returns the defined error handlers for exceptions |
|
|
||
| # Adds an error handler for the given exception | ||
| def add_exception_handler(exception : Exception.class, &handler : HTTP::Server::Context, Exception -> _) | ||
| EXCEPTION_HANDLERS[exception] = ->(context : HTTP::Server::Context, error : Exception) { handler.call(context, error).to_s } |
There was a problem hiding this comment.
Yeah I agree requiring String output type should be preferred in general. This might have some UX consequences though...
@syeopite What's the motivation for an unspecified block output type?
Closes #622
Description of the Change
This PR adds the ability for users to define error handlers that are used when a specific exception is raised.
Alternate Designs
Benefits
This provides for a cleaner and easier experience for defining custom errors. See #622.
Possible Drawbacks