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

Introduce ActionDispatch::DebugExceptions interceptors #23868

Merged
merged 1 commit into from Apr 20, 2018

Conversation

Projects
None yet
7 participants
@gsamokovarov
Contributor

gsamokovarov commented Feb 24, 2016

Plugins interacting with the exceptions caught and displayed by
ActionDispatch::DebugExceptions currently have to monkey patch it to get
the much needed exception for their calculation.

With DebugExceptions.register_interceptor, plugin authors can hook into
DebugExceptions and process the exception, before being rendered. They
can store it into the request and process it on the way back of the
middleware chain execution or act on it straight in the interceptor.

The interceptors can be plain blocks, procs, lambdas or any object that
responds to #call.

You can see web-console implemented through the interceptor API here.
Other plugin authors can benefit from this API as well. Most of the error
reporting plugins can use this.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 24, 2016

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

@maclover7

View changes

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb Outdated
backtrace_cleaner = request.get_header('action_dispatch.backtrace_cleaner')
wrapper = ExceptionWrapper.new(backtrace_cleaner, exception)
@interceptors.each do |interceptor|

This comment has been minimized.

@maclover7

maclover7 Feb 24, 2016

Member

Can we get rid of the transitory @interceptors, and just call self.class.interceptors here?

This comment has been minimized.

@gsamokovarov

gsamokovarov Feb 24, 2016

Contributor

Injected it, so I can test it more easily – no need to keep track of the global state during the test run.

This comment has been minimized.

@maclover7

maclover7 Feb 24, 2016

Member

:shrug: I just think it would make sense to use the direct variable we will iterating on, instead of creating a shadow variable, that we won't even be changing.

@maclover7

View changes

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb Outdated
raise exception unless request.show_exceptions?
render_exception(request, exception)
end
private
def invoke_interceptors(request, exception)

This comment has been minimized.

@maclover7

maclover7 Feb 24, 2016

Member

We could call this method from inside render_exception, or should set the backtrace_cleaner and wrapper variables to be instance variables, so we don't have to create the same thing multiple times 😬

This comment has been minimized.

@gsamokovarov

gsamokovarov Feb 24, 2016

Contributor

I tried something similar, let me see what I can do.

@nateberkopec

This comment has been minimized.

Contributor

nateberkopec commented Feb 25, 2016

As the maintainer of an exception notifier (sentry-raven) I am wholeheartedly for this PR.

@gsamokovarov

View changes

actionpack/lib/action_dispatch/middleware/debug_exceptions.rb Outdated
raise exception unless request.show_exceptions?
render_exception(request, exception)
render_exception(request, exception, wrapper)

This comment has been minimized.

@gsamokovarov

gsamokovarov Feb 27, 2016

Contributor

Actually, this breaks the render_exception interface, and plugin authors already depend on it. I'm going with the duplicated code approach. Yeah, I like DRY code, but sometimes the duplication is so much better than breaking interfaces or leaking abstractions.

@tgxworld

This comment has been minimized.

Contributor

tgxworld commented Apr 13, 2018

@gsamokovarov Is web-console still monkey patching? At Discourse, we maintain logster which is monkey patching as well and being able to register callbacks in the middleware would be great for us.

@rafaelfranca @matthewd Is there anything I can do to help move this forward?

@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented Apr 13, 2018

@tgxworld Yes, it is still monkey patching... You can make logster and web-console play together by requiring it after web-console, e.g. put logster after web-console it in the Gemfile.

I still stand by the interceptors, though. They are super simple and provide what most of the plugins monkey patching ActionDispatch::DebugExceptions need.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Apr 20, 2018

The idea looks good to me. @gsamokovarov can you rebase it?

Introduce ActionDispatch::DebugExceptions interceptors
Plugins interacting with the exceptions caught and displayed by
ActionDispatch::DebugExceptions currently have to monkey patch it to get
the much needed exception for their calculation.

With DebugExceptions.register_interceptor, plugin authors can hook into
DebugExceptions and process the exception, before being rendered. They
can store it into the request and process it on the way back of the
middleware chain execution or act on it straight in the interceptor.

The interceptors can be play blocks, procs, lambdas or any object that
responds to `#call`.
@gsamokovarov

This comment has been minimized.

Contributor

gsamokovarov commented Apr 20, 2018

@rafaelfranca I have rebased it. 👌

@rafaelfranca rafaelfranca merged commit 357559f into rails:master Apr 20, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gsamokovarov gsamokovarov deleted the gsamokovarov:debug-exceptions-interceptors branch Apr 21, 2018

@tgxworld

This comment has been minimized.

Contributor

tgxworld commented Apr 22, 2018

😍 Thank you @gsamokovarov and @rafaelfranca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment