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

Deprecate exception#original_exception in favor of exception#cause #18774

Merged
merged 1 commit into from Nov 3, 2015

Conversation

Projects
None yet
7 participants
@yuki24
Contributor

yuki24 commented Feb 1, 2015

This replaces #original_exception with #cause that has been available since Ruby 2.1.

Although this change simplifies many things, this would slightly change some of the existing behaviour as #cause is not the exact same thing as #original_exception (all exceptions automatically capture the original exception). AFAIK, there isn't a big internal issue within Rails, but for example #rescue_from would behave slightly differently:

class SomeController < ApplicationController
  # This won't rescue `OtherError` after this change.
  rescue_from OtherError do |error|
    do_something
  end

  def index
    begin
      raise SomeError # This exception would be rescued after this change, not the last one.
    rescue
      raise OtherError # This exception is rescued right now.
    end
  end
end

What we can do is to have a list of exceptions that should be replaced with #cause so errors like ActionView::TemplateError will be unwrapped properly. WDYT?

attr_reader :original_exception
def initialize(template, message, backtrace)
super(message)
set_backtrace(backtrace)

This comment has been minimized.

@jeremy

jeremy Feb 2, 2015

Member

Possible to delegate these to #cause, or is that assigned after initialization?

This comment has been minimized.

@yuki24

yuki24 Feb 3, 2015

Contributor

Yes, we can delegate :backtrace, :to_s, to: :cause. We don't have to delegate :message, to: :cause since what #message does is to just call #to_s internally.

@@ -483,7 +483,7 @@ def log(sql, name = "SQL", binds = [], statement_name = nil)
def translate_exception(exception, message)
# override in derived class
ActiveRecord::StatementInvalid.new(message, exception)
ActiveRecord::StatementInvalid.new(message)

This comment has been minimized.

@jeremy

jeremy Feb 2, 2015

Member

Will this lose #cause now?

This comment has been minimized.

@yuki24

yuki24 Feb 3, 2015

Contributor

No, it won't. As long as #translate_exception method is called from within rescue ... end, it'll capture $! automatically.

def raise_again
  raise SecondError
end

begin
  raise FirstError
rescue
  raise SecondError # This'll capture the first error.
  raise_again       # This'll also capture the first error.
end

raise SecondError # This won't.

I've added several assertions just in case #translate_exception is moved outside of rescue ... end.

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

👍

@original_exception = e
set_backtrace e.backtrace
def original_exception
ActiveSupport::Deprecation.warn("#original_exception is deprecated. Use #cause instead.")

This comment has been minimized.

@jeremy

jeremy Feb 2, 2015

Member

Need to pass caller to #warn so it will report the caller's file:line rather than this file's. (Ditto for other #warns)

template.encode!
end
raise Template::Error.new(template, e.message, e.backtrace)
end

This comment has been minimized.

@jeremy

jeremy Feb 2, 2015

Member

I think this is the only downside to this PR. We lose the flexibility to assign #cause=, but we rarely need it. Worthy trade.

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 2, 2015

Nice work @yuki24 👍

If Ruby supported #cause= our life would be a little simpler, but IMO this is a fair trade since it simplifies our code significantly (except tests, which can bear the extra complexity to trigger test case situations).

@jeremy jeremy added this to the 5.0.0 milestone Feb 2, 2015

@yuki24 yuki24 force-pushed the yuki24:deprecate-original-exception-infavor-of-cause branch from edc7ec9 to 3132a0a Feb 3, 2015

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Feb 3, 2015

@jeremy Updated the commit. Thanks for your time!

@carlosantoniodasilva

This comment has been minimized.

Member

carlosantoniodasilva commented Feb 3, 2015

It looks good, there are a couple places that @jeremy has commented we might lose the #cause because they don't seem to be wrapped in a rescue block, did you take a look at those? Thanks @yuki24 !

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Feb 3, 2015

@carlosantoniodasilva as I commented here, they won't lose #cause since #translate_exception is called from within rescue ... end block.

@yuki24 yuki24 force-pushed the yuki24:deprecate-original-exception-infavor-of-cause branch from 3132a0a to 8b447f6 Feb 3, 2015

@carlosantoniodasilva

This comment has been minimized.

Member

carlosantoniodasilva commented Feb 3, 2015

@yuki24 right, but there were a couple more that didn't seem to be tackled, that's why I asked :).

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Feb 3, 2015

These adapters are all overriding #translate_exception which is called from within rescue ... end. I've added some more assertions just in case #translate_exception is moved out of rescue ... end.

@@ -311,7 +311,7 @@ def session_options=(options)
def GET
@env["action_dispatch.request.query_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {}))
rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
raise ActionController::BadRequest.new(:query, e)
raise ActionController::BadRequest, "Invalid query parameters: #{e.message}", e.backtrace

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Coming back to this… apps can raise BadRequest.new themselves, so this changes public API. Tricky.

We could raise an ActionController::BadRequestParametersWithCause instead that derives its message and backtrace from its cause exception:

module ActionController
  class BadRequestParametersWithCause < BadRequest
    def initialize(type = nil)
      super type, cause
    end
  end
end

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

Yeah, I was wondering if the initializer is a public API too. Maybe we want to introduce a new exception class that always has #cause?

@@ -3,14 +3,9 @@ class ActionControllerError < StandardError #:nodoc:
end
class BadRequest < ActionControllerError #:nodoc:
attr_reader :original_exception
def initialize(type = nil, e = nil)

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

We'll need to deprecate apps instantiating BadRequest.new(:foo, exception) as well, so we may need to keep the old method signature until 5.1.

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

No problem, I'll change this to warn if e is passed into it.

handler_for_rescue(orig_exception))
exception = orig_exception
if exception.cause && handler_for_rescue(exception.cause)
exception = exception.cause

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

It'd be interesting to allow rescue handlers for wrapper exceptions as well as their causes. Then a handler could rescue all TemplateError and also know to include details about its #cause. Separate change to explore.

@@ -319,7 +319,7 @@ def GET
def POST
@env["action_dispatch.request.request_parameters"] ||= Utils.deep_munge(normalize_encode_params(super || {}))
rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
raise ActionController::BadRequest.new(:request, e)
raise ActionController::BadRequest, "Invalid request parameters: #{e.message}", e.backtrace

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Another thought on cause handling. Setting an exception's backtrace to its cause's backtrace may no longer be necessary, if exception handlers know to check causes. It's clearer to see the raised exception's backtrace, unobscured, and separately report the backtrace of its cause. That'd require updating our exception handlers and middleware to be cause-aware.

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

agreed. I really want to see both the original exception and the last one on the debug exceptions page.

@@ -103,17 +103,13 @@ def backtrace
end

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

e.g. add a cause_backtrace as well, and update the backtrace cleaner to take a backtrace arg instead of only cleaning the original exception's trace.

def initialize(message, original_exception)
super(message)
@original_exception = original_exception

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

We'll need to deprecate this method signature, too, since apps may provide custom params parsers.

Using super cause.message makes sense as well. Then we don't have to delegate to_s (or message or …)

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

#cause will return nil unless the exception is actually raised. Technically we can use $! instead which holds the same exception as Ruby internally captures $! and set it to #cause. But still extra interface to do one thing.

super("Session contains objects whose class definition isn't available.\n" +
"Remember to require the classes for all objects kept in the session.\n" +
"(Original exception: #{const_error.message} [#{const_error.class}])\n")
"(Original exception: #{message})\n")

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Could do

"(Original exception: #{cause.message} [#{cause.class}])\n")

and omit the message arg?

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

Unfortunately, #cause returns nil in #initialize. What do you think of using $!?

This comment has been minimized.

@jeremy

jeremy Feb 4, 2015

Member

Looks like $! would work! Same characteristics as #cause here

raise Template::Error.new(template, e)
end
end

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Let's move this back to the handle_render_error method. Then template subclasses can continue to override it.

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

👍 I'll do that.

attr_reader :original_exception
def initialize(template)
@template, @sub_templates = template, nil

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Can do

super cause.message, cause.backtrace

here rather than delegating.

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

Same thing, #cause returns nil here.

class DeserializationError < StandardError
attr_reader :original_exception
delegate :backtrace, to: :cause

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Ditto re. direct delegation vs super

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

again, #cause returns nil here.

@@ -20,7 +20,7 @@ def mysql2_connection(config)
ConnectionAdapters::Mysql2Adapter.new(client, logger, options, config)
rescue Mysql2::Error => error
if error.message.include?("Unknown database")
raise ActiveRecord::NoDatabaseError.new(error.message, error)
raise ActiveRecord::NoDatabaseError.new(error.message)

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Exceptions that take error.message as an argument could default to using cause.message

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

one more, #cause returns nil here. We can also do the below, but it doesn't guarantee that ActiveRecord::NoDatabaseError will always have the original error message.

raise ActiveRecord::NoDatabaseError, error.message
class StatementInvalid < ActiveRecordError
attr_reader :original_exception
def initialize(message, original_exception = nil)

This comment has been minimized.

@jeremy

jeremy Feb 3, 2015

Member

Will need to deprecate this method signature

This comment has been minimized.

@yuki24

yuki24 Feb 4, 2015

Contributor

👍 will work on it.

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 3, 2015

Thank you @yuki24. I missed that many of these exceptions are public classes, so we'll need to deprecate the old-style original_exception in initialize as well.

Also, we have a very common pattern of wrapping one exception in another and taking on the original's backtrace. We may be able to stop setting backtraces and, instead, rely on exception handlers to check for and understand the cause. That'd give us clearer, richer feedback when errors occur. It's confusing to get an exception whose backtrace points someplace else, but helpful to get an exception that says where it was raised… and why.

@yuki24 yuki24 force-pushed the yuki24:deprecate-original-exception-infavor-of-cause branch from 8b447f6 to 01f1a76 Feb 5, 2015

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Feb 5, 2015

updated the commit.

  • Deprecates initializers properly
  • No more delegate in favor of super($!.message) and set_backtrace($!.backtrace)
  • I know this might change later on, but the exceptions still do set_backtrace($!.backtrace) for now

Also, I was wondering if we can override exception#cause like this:

class Exception
  def cause
    super || $!
  end
end

With this, we can always use #cause to get the original one.

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Feb 5, 2015

travis build failed. looking into the failures now.

@yuki24 yuki24 force-pushed the yuki24:deprecate-original-exception-infavor-of-cause branch from 01f1a76 to e7aca41 Feb 5, 2015

@jeremy jeremy merged commit e670611 into rails:master Nov 3, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yuki24 yuki24 deleted the yuki24:deprecate-original-exception-infavor-of-cause branch Nov 4, 2015

@yuki24

This comment has been minimized.

Contributor

yuki24 commented Nov 4, 2015

jonatack added a commit to jonatack/will_paginate that referenced this pull request Nov 6, 2015

jonatack added a commit to jonatack/better_errors that referenced this pull request Dec 13, 2015

#original_exception is deprecated in favor of #cause in Rails 5
See rails/rails#18774.

exception#cause has been available in MRI Ruby since 2.1.

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

@skippy

This comment has been minimized.

skippy commented Feb 25, 2016

Hey folks,
(cc @jeremy @yuki24 )

this makes it very hard to use rescue_from to capture all errors. Let me explain:

  • ApplicationController has a default rescue_from that captures everything. This way the vast majority of errors can be handled in a consistent manner (think consistent json responses and logging msgs). E.g. {"error":{"type":"service_error","status_code":500,"request_id":"789fe88b-fd97-4199-81ac-bd10c45e74b3","message":"An unexpected error occurred"}}
  • some controllers can have more specific rescue_from as needed
  • by default, this means a lot of custom app logic, if an error occurs, is flagged as a 500.

And this is where things get tricky. A common pattern is to, where appropriate, wrap an exception with one that is handled by exception_wrapper so it can be handled differently, such as throwing a 400 or 403. Such as:

  private def fill_form_contents
    form_contents = fill_form_params[:form_contents]
    form_contents.blank? ? {} : JSON.parse(form_contents)
  rescue ::JSON::ParserError => e
    err_msg = "Failed parsing param 'form_contents': #{e.message}"
    raise ActionDispatch::ParamsParser::ParseError.new err_msg
  end

in Rails < 5.0, this was handled by rescue_from and outputted a more useful msg. But in rails 5.0 it now bubbles up the ::JSON::ParserError instead.

One solution is to just catch all ::JSON::ParserErrors, but that is not the correct behavior (I only want this one case to be treated differently; if a JSON::ParserError occurs somewhere else, then that should be treated as a 500).

The other, that I can see, is to do this:

  private def fill_form_contents
    form_contents = fill_form_params[:form_contents]
    form_contents.blank? ? {} : JSON.parse(form_contents)
  rescue ::JSON::ParserError => e
    orig_msg = "Failed parsing param 'form_contents': #{e.message}"
    begin
      raise ActionDispatch::ParamsParser::ParseError.new orig_msg
    rescue
      raise ActionController::BadRequest.new orig_msg
    end
  end

Exception wrapping is a common technique to handle errors, and now that is not easily available.

I'm a fan of Exception#cause, but the use of it within ActionController::Rescue is, I am starting to think, a mistake.

Thoughts? Comments?

sgrif added a commit that referenced this pull request Mar 11, 2016

Use the most highest priority exception handler when cause is set
There was some subtle breakage caused by #18774, when we removed
`#original_exception` in favor of `#cause`. However, `#cause` is
automatically set by Ruby when raising an exception from a rescue block.
With this change, we will use whichever handler has the highest priority
(whichever call to `rescue_from` came last). In cases where the outer
has lower precidence than the cause, but the outer is what should be
handled, cause will need to be explicitly unset.

Fixes #23925

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request May 3, 2016

Add `ActiveRecord::ValueTooLong` exception class
and supress `DEPRECATION WARNING: Passing #original_exception is deprecated and has no effect. Exceptions will automatically capture the original exception. `

Refer:
rails/rails#23522
rails/rails#18774

y-yagi added a commit to y-yagi/better_errors that referenced this pull request May 14, 2016

use `Exception#cause` insted of deprecated `Exception#original_except…
…ion`

`Exception#original_exception` is deprecated in Rails 5.
Ref: rails/rails#18774

Therefore, if `Exception#cause` can be used, should use`Exception#cause`.

Fixes BetterErrors#333
@jeremy

This comment has been minimized.

Member

jeremy commented May 14, 2016

@skippy Agreed. Changing this to fall back to the exception's cause only if the exception itself doesn't have a handler. #25018

taiki45 added a commit to taiki45/rails that referenced this pull request May 17, 2016

Remove passing original exception
Passing original exception was deprecated by rails#18774.
@skippy

This comment has been minimized.

skippy commented May 17, 2016

@jeremy thank you kindly!

greysteil added a commit to greysteil/rails that referenced this pull request Jul 15, 2016

Store original exception in action_dispatch.exception, not exception …
…cause

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 responsibilty for inspecting the error's cause, if required.

Reverses the (undesired) change in behaviour from
rails#18774
def initialize(message = nil, original_exception = nil)
@original_exception = original_exception
super(message)
if original_exception

This comment has been minimized.

@bf4

bf4 Feb 22, 2017

Contributor

removed in bc6c5df ( noting because I was just starting to use original_exception in my Rails 4.2. app's logging when I noticed it swallowing the original exception.... ) ¯\_(ツ)_/¯

ellimist added a commit to ellimist/better_errors that referenced this pull request Mar 15, 2017

use `Exception#cause` insted of deprecated `Exception#original_except…
…ion`

`Exception#original_exception` is deprecated in Rails 5.
Ref: rails/rails#18774

Therefore, if `Exception#cause` can be used, should use`Exception#cause`.

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