Store original exception in action_dispatch.exception, not exception cause

[greysteil]

Previously, when an error was raised that had a cause, we would store the cause
of the error in request.env["action_dispatch.exception"], rather than the
error itself. That causes a loss of important information - it's not possible
to get back to the top-level error from the stored exception (since the cause
relationship on errors in one-way).

After this change, it is the top-level error, rather than its cause, that will
be stored in request.env["action_dispatch.exception"]. Any exception handler
app can then take responsibility for inspecting the error's cause, if required.

Reverses the (undesired?) change in behaviour from
#18774

In our app, there are a bunch of places where we want to avoid serving 422s for validation errors, and instead want to blow up. The recent change to store the exception's cause means we can no longer do the following:

begin
  # code
rescue ActionRecord::ValidationError => e
  raise "Validation error raised when it should not be possible. Someone should look into this! #{e.message}"
end

[rails-bot]

r? @sgrif

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

[greysteil]

@sgrif - any thoughts?

[rafaelfranca]

r? @jeremy

[matthewd]

#18774 didn't change this assertion; did it really previously contain the outermost exception? 😕

[greysteil]

Not in this case - ActionView::Template::Error was a special, and used to store the original_exception (basically its cause), and ExceptionWrapper would use this, rather than the outermost exception. We should be fine for it to change now, though. Just put a comment on the PR explaining.

[matthewd]

👍 to the change (or something like it), but I'm unsure about compatibility

[greysteil]

Awesome, thanks @matthewd.

On compatibility, this change would:

• have no effect for integrators using the standard Rails exception handling apps, since DebugExceptions re-wraps the error anyway, so still render details of its cause, and PublicExceptions doesn't use the exception itself at all
• change the exception provided to integrators who have defined a custom exception app. The exception would always be the top-level error, rather than its cause. To get back the previous behaviour, they could just wrap the exception in the way it's currently always done in ShowExceptions - something like exception = ActionDispatch::ExceptionWrapper.new(nil, exception).exception.

The undesired change in #18774 was that, previously, when anything other than a few Rails-specific errors were raised (grep for attr_reader :original_exception in that PR to see which ones) as the top-level error, it was the top-level error, not its cause, that was exposed to the exceptions app. After that PR, the top-level error is discarded in favour of its cause whenever that cause has a handler. Hence the loss of the ability to explicitly override the error, as in the code in the description on this PR.

The upside of this change is that with it, integrators can get the current behaviour, but without it it's impossible to get back the old behaviour.

[greysteil]

@matthewd any thoughts on the above?

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.