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

Add ability to add handlers for raised exceptions #688

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

syeopite
Copy link

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.

require "kemal"

class NewException < Exception
end

get "/" do | env |
  raise NewException.new()
end

error NewException do | env |
  "An error occured!"
end

Kemal.run

Alternate Designs

Benefits

This provides for a cleaner and easier experience for defining custom errors. See #622.

Possible Drawbacks

@@ -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)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@robacarp
Copy link

This is perfect!

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach won't work with inheritance.

Copy link
Author

@syeopite syeopite Sep 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by inheritance in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling .has_key? will check for the exception class only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it should be something like Kemal.config.error_handlers.any?{ |key, _| ex.class <= key } in order to match subclasses of key as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does identify whether the exception inherits from a known exception but shouldn't this also take into account the order of inheritance? For example

class GrandParentException < Exception
end

class ParentException < GrandParentException
end

class ChildException < ParentException
end

error Exception do
  "Generic" 
end

error GrandParentException do
  "Grandparent exception"
end

error ParentException do
  "Parent exception"
end

get "/" do
  raise ChildException.new()
end

I'll expect that the raised error in the above logic be caught by the handler for ParentException.

Is there any way of identify the order of inheritance at runtime?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you could sort classes by their inheritance relationship.

I don't think that would be a good idea though. Just following the order of declaration is probably best. rescue works the same way: the first matching branch wins, even if a later type would be a closer fit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. In that case I think this should achieve the desired behavior

Comment on lines +14 to +15
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: This special case with has_key? seems unnecessary because this is already covered with ex.class <= key.
It should be safe to remove this partial duplication.

Suggested change
# 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)

Comment on lines 14 to +15
FILTER_HANDLERS = [] of HTTP::Handler
ERROR_HANDLERS = {} of Int32 => HTTP::Server::Context, Exception -> String
ERROR_HANDLERS = {} of (Int32 | Exception.class) => HTTP::Server::Context, Exception -> String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: I'm wondering if it makes sense to mash up Int32 and Exception.class keys in a single hash.
Might be better to have two separate collections. They're used completely independent of each other. There's no need to expose both under the same name Kemal.error_handlers.
Maybe we could introduce a new name Kemal.exception_handlers for this feature?

handler_to_use = override_handler_used
end

if !Kemal.config.error_handlers.empty? && Kemal.config.error_handlers.has_key?(handler_to_use)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

performance: The combination of h.has_key?(key) and later h[key] is inefficient because it requires two hash lookups for the same key. Instead, you could directly query the value with h[key]? and use that as a condition. A nil return value would indicate the key does not exist.
Even better, you could already retrieve the handler while matching the exception (use each instead of each_key and you get the value for free) and pass it directly to this method. That removes two unnecessary hash lookups.

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

Successfully merging this pull request may close these issues.

Allow handling errors with error handlers
4 participants