Add custom exception handlers

[krzyzak]

With this code, you can handle your exceptions with custom logic (method or anything callable) – the only constraint is that it must return an array with status code and message.

This is still work in progress, just wanted to show my approach and current progress. I’ll update docs once everything else will be settled.

I’m still in doubt if if wouldn’t be more convinient to change one more function: configuration.excepton_code. At the moment it checks a hash for a key with exact match, which isn’t very flexible for me. I’d love to have something like this in my controller:

handle_exception ApiReadTimeout => :api_read_timeout
handle_exception OtherException => :some_other_handler
handle_exception Exception => :generic_exception # this one catches everything else

To achieve this, I’d have to specify all possible exception classes, which of course is not an option.
Thus, I’d suggest doing something like this:

def exception_code(exception)
  @handled_exceptions.find{|ex| exception.is_a?(ex) }.to_a.last || DEFAULT_ERROR_CODE
end

What do you thing about such approach?

[jodosha]

@krzyzak Thanks for this PR.

My idea for feature is to give more power to developers to handle exceptions with an alternative logic from Lotus::Action. If we force both the method signature def handle_my_exception(exception) and the returning value [code, message] we are limiting the scope of the custom handler. It will become just a way to decide which HTTP code and body to return.

What I'd really want achieve is a command-query separation and make the handler a void method.

def _handle_exception(exception)
  raise unless configuration.handle_exceptions
  handler = configuration.handled_exceptions[exception.class]

  code = if handler.respond_to?(:to_proc)
    handler.to_proc.call(self, exception)
  else
    configuration.exception_code(exception.class)
  end

  halt code
end

and then..

def halt(code = nil)
  status(*Http::Status.for_code(code)) unless code.nil?
  throw :halt
end

and having a custom handler like:

def handle_my_exception(exception)
  self.status = 501
  self.body   = 'Please go away!'
end

# ...or

def handle_my_exception(exception)
  status 501, 'Please go away!'
end

# ...or

def handle_my_exception(exception)
  abort "Can't manage this exception: #{ e.message }"
end

What do you think?

[krzyzak]

I love this approach!
Definitely better than mine.

How about change in exception_code ?

[krzyzak]

@jodosha I started playing aroud with that code, and I found that it would be impossible to use procs at the moment (because status method is protected) – I’m not sure how important is to have ability to handle exceptions via procs, so I can either remove such ability (and then just public_send handler, instead of calling it), or make status method public. Which options do you think is better?

[jodosha]

@krzyzak I was doing the same and going to suggest the same thing: only accept integers or symbols for direct status code assigment or custom exception handling via concrete method.

Also, we should use method(handler).call(exception) instead of to_proc, so that exception handlers can be private methods, and also remove self as method argument.

[jodosha]

@krzyzak I like the idea of the exceptions hierarchy, but we should rethink a little bit the implementation:

def _handle_exception(exception)
  raise unless configuration.handle_exceptions
  handler = configuration.handled_exceptions[exception.class] # 1

  code = if handler.respond_to?(:to_proc)
    handler.to_proc.call(self, exception)
  else
    configuration.exception_code(exception.class)  # 2
  end

  halt code
end

Lines #1 and #2 are accessing the same data structure, which doesn't make the code DRY. What if we introduce something like exception_handler(exception)?

# throwable.rb
def _handle_exception(exception)
  raise unless configuration.handle_exceptions

  halt(
    method(
      configuration.exception_handler(exception)
    ).call(exception)
  )
end

def default_exception_handler(exception)
  # override me!
end

# configuration.rb

# deprecate exception_code and add the following method:
def exception_handler(exception)
  # Here we can also implement the concept of hierarchy with @krzyzak `is_a?` suggestion
  handled_exceptions.fetch(exception.class) { :default_exception_handler }
end

This is just a spike, and I don't know if it can work, but it reflects better the new concept of exception handler and avoids conditional statements. What do you think?

[krzyzak]

@jodosha I played around with that new approach, and (mostly) it works.
There was one issue though – when you specify exception handler with explicit status code, everything blows up, because sth. like 404 clearly isn’t valid method name (thus, we can’t use it in _handle_exception method).

Because of that, I removed ability to specify exception handling with explicit status code (eg. handle_exception NotImplementedError => 501), instead of that, I introduced Throwable::Handlers module, which defines handlers for all status codes (so you’d define that exception handler as handle_exception NotImplementedError => :not_implemented).

I know that changes behaviour of handle_exception method, and probably we’ll have to support old way for some time – it should be fairly easy though.

[krzyzak]

@jodosha As a recap, I’ll write short summarisation of our conversation on chat.

It’s just not worth to introduce so many new methods to solve so simple problem (and it might cause confusion, if someone would like to implement method like ok, not_found on his own), thus, I removed Throwable::Handlers module, and restored previous behaviour.

There was also one issue that I've been using protected handled_exceptions method outside of configurable scope – so I could make it either public (but I don’t think that should be public), or use exception_code. However, I have to change its name to reflect current behaviour, so it’s exception_handler now. (It also cleaned up _handle_exception a bit).

If you like current changes, then the only thing left is to update documentation.