Why it is Important to dig in Deep

Skylight is a great service for monitoring and profiling Rails applications (we’re using it in several of our projects). Recently they published a blog post about upgrading their application to Rails 5. Reading this post I stumbled about one particular detail that puzzled me. Here’s the story:

One of the errors they faced during the upgrade was caused by this code (slightly simplified):

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
class CustomersController < BlingBling::ApplicationController
  rescue_from ::Stripe::CardError, with: :invalid_card, only: [:update, :update_card]

  def show
    # stuff
  end

  def update
    # other stuff
  end

  def update_card
    # yet moar stuff
  end

  private

  def invalid_card
    render_errors card: "is invalid"
  end
end

Line 2 raises ArgumentError: unknown keyword: only. It looks like Rails 5 no longer supports this particular option for #rescue_from. A quick glance at the documention seems to confirm this: No mention of only: (and none of except: either, eventhough they are always used together).

So the team at Skylight decided to work around this now missing option. It just takes a few additional lines of code:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
class CustomersController < BlingBling::ApplicationController
  def show
    # stuff
  end

  def update
    handle_invalid_card {
      # other stuff
    }
  end

  def update_card
    handle_invalid_card {
      # yet more stuff
    }
  end

  private

  def handle_invalid_card
    begin
      yield
    rescue ::Stripe::CardError => error
      render_errors card: "is invalid"
    end
  end
end

Problem solved! Except for … that it’s not. In fact, this change introduces a bug. The updated coded behaves differently than the original code. To understand why, we need to dig in deeper.

Let’s Dig

First we take a look at the documentation entry for #rescue_from in the previous Rails version (4.2):

Rescue exceptions raised in controller actions.

rescue_from receives a series of exception classes or class names, and a trailing :with option with the name of a method or a Proc object to be called to handle them. Alternatively a block can be given. […]

Hmm, no mention of :only or :except here either, that’s strange. Maybe they had been deprecated? Let’s take a look at the source:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
def rescue_from(*klasses, &block)
  options = klasses.extract_options!

  unless options.has_key?(:with)
    if block_given?
      options[:with] = block
    else
      raise ArgumentError, "Need a handler. Supply an options hash that has a :with key as the last argument."
    end
  end

  klasses.each do |klass|
    key = if klass.is_a?(Class) && klass <= Exception
      klass.name
    elsif klass.is_a?(String)
      klass
    else
      raise ArgumentError, "#{klass} is neither an Exception nor a String"
    end

    # put the new handler at the end because the list is read in reverse
    self.rescue_handlers += [[key, options[:with]]]
  end
end

No, nothing: Even in Rails 4.2, #rescue_from does not support the :only or :except options. But it doesn’t complain about them either. They are just silently ignored.

The Root of the Bug

To repeat: In Rails 4.2, using :only or :except together with #rescue_from has no effect whatsoever.

And that’s why the upgrade introduced a bug: In the original version of the code, eventhough it suggested otherwise, the #rescue_from clause affected all controller actions. After the upgrade, the clause now affects some actions only.

Of course this bug is not severe, and it may not be a problem at all. But it’s an unintended change of behaviour nonetheless. If at all possible, do not introduce behavioural changes when upgrading your code.

(Fun fact: One could argue that the original code already had a bug – the #rescue_from should not have affected all actions –, and that by introducing another bug while incorrectly replicating the original behaviour this has now been fixed. Unintentional bugfixing ftw.)

Summary

Here’s what happened in a nutshell:

• While upgrading the framework, a piece of code stopped working.
• The failing code was analysed to understand its behaviour.
• A workaround was introduced, implementing the very same behaviour in a way compatible with the framework’s new version.

So far, so good. However:

• The analysis of the original code was incomplete.
• Thus the workaround was implemented based on a wrong understanding of the original behaviour.
• Consequently, the workaround now behaves differently than the original code.

How to not Introduce Behavioural Changes Inadvertently

Not introducing unintended behavioural changes requires you to to completely understand the current behaviour. In some cases (like in this example), code may behave differently from what it looks like at first glance. To develop a thorough understanding you need to dig in deep.

And: Don’t be afraid to look at the source code of frameworks and libraries. It helps. Documentation may be incomplete our outdated, but the source code never is.

Post Scriptum: Option Hashes vs. Keyword Arguments

Are you wondering why the original code started to fail in Rails 5? Let’s dig into this, too. Here is the definition and first line of #rescue_from in Rails 4.2:

1
2
3
4
5
# Rails 4.2
def rescue_from(*klasses, &block)
  options = klasses.extract_options!
  # …
end

The method uses an array/hash argument: It accepts any number of arguments and stores it as an array in klasses (let’s ignore the &block argument for now):

1
2
3
rescue_from(Foo)       # => klasses is [Foo]
rescue_from(Foo, :bar) # => klasses is [Foo, :bar]
rescue_from()          # => klasses is []

But what happens when we call the method like this: rescue_from(Foo, with: bar)? Well, there’s this particular nice syntactic feature in Ruby:

The array argument will capture a Hash as the last entry if a hash was sent by the caller after all positional arguments.

Some examples to clarify:

1
2
3
rescue_from(Foo, with: :bar)                 # => klasses is [Foo, {with: :bar}]
rescue_from(Foo, :bar, with: :baz, and: 123) # => klasses is [Foo, :bar, {with: baz, and: 123}]
rescue_from(with: :bar)                      # => klasses is [{with: :bar}]

If the last argument looks like a hash, is is treated as such and added to the end of the array. The method then uses #extract_options! (documentation) to extract this hash from the array. This is a commonly used idiom in Ruby.

However, in Ruby 2.0 keyword arguments were introduced. And #rescue_from has been updated accordingly in Rails 5:

1
2
3
4
# Rails 5 (note the explicit keyword argument `with:`)
def rescue_from(*klasses, with: nil, &block)
  # …
end

With the introduction of keyword arguments, the behaviour of array/hash arguments changed in one important way (emphasis mine):

The array argument will capture a Hash as the last entry if a hash was sent by the caller after all positional arguments. However, this only occurs if the method does not declare any keyword arguments.

So because in Rails 5 #rescue_from does declare with: as keyword argument, it is no longer possible to call the method with any other hash-like/keyword arguments:

1
2
rescue_from(Foo, with: :bar)           # => klasses is [Foo], with is :bar
rescue_from(Foo, with: :baz, and: 123) # => ArgumentError: unknown keyword: and

And that’s why the original line of code started to fail in Rails 5:

1
2
rescue_from ::Stripe::CardError, with: :invalid_card, only: [:update, :update_card]
  # => ArgumentError: unknown keyword: only