Daniel D. Pence

Recently, at work, I’ve had the pleasure of working on upgrading a legacy Ruby on Rails application from 4.2.11.1 to 5.2, and eventually v6.0. The app has been around a long time and has gone through many upgrades since the initial commit 10+ years ago. Needless to say, most of the people who worked on the product in the past have moved on and aren’t available to provide context on decisions they made. Thankfully, the app is fairly well covered by unit and integration tests which have been tremendously helpful in surfacing problem areas.

Where the Errors Lead, I Will Follow

Most of the tests that failed after I bumped the Rails version were pretty well anticipated and not too difficult to make pass again. I spent a fair amount of time before I got started reading through the Rails upgrade guides, changelogs, and blog posts from other people who have upgraded from 4.2 to 5.0, so I had a pretty good idea of what things would break ahead of time. But occasionally, a test would break in a way that I wasn’t expecting.

For example, I noticed that almost all of the tests related to ActionMailer were failing, and all for what appeared to be the same reason:

Failure/Error:
    def initialize(method_name, *args)
      unless authorized?(*args)
        return @_message = NullMail.new
      end


      mail = super


      if @_message.to.count > 1
        raise "Don't send mail to more than one recipient."
      end


ArgumentError:
  wrong number of arguments (given 0, expected 1+)

Weird. What even is this code? Thankfully, RSpec (and most other test suite runners) will tell you exactly where the failure occurs, and usually provide some level of stack trace. In this case, RSpec pointed me to the same method it printed out above. Let’s take a look at that class:

class ExternalMailer < ActionMailer::Base
    ... 
   def initialize(method_name, *args)
    unless authorized?(*args)
      return @_message = NullMail.new
    end


    mail = super


    if @_message.to.count > 1
      raise "Don't send mail to more than one recipient."
    end


    if MailBlock.blocked?(@_message.to.first, method_name)
      return @_message = NullMail.new
    end


    mail
  end


  private
  def authorized?(*args)
    true
  end
end

Here we can see that we have a pretty standard ActionMailer class. What’s unusual is what appears to be an override, or monkey patch, of the ActionMailer::Base#initialize method.

Why was this code written this way?

After reading through some commit messages and digging through the other mailer classes that subclass ExternalMailer, it appeared that this #initialize method was overriding the ActionMailer::Base#initialize method in order to provide a hook, via the private #authorized?(*args) method. That hook could then be overridden in subclasses of ExternalMailer in order to check user authorization before sending an email.

OK. Not the way I would have written this today, but I’m sure they wrote it this way for a reason.

Digging Deeper

At this point, we’ve identified where the code is failing, but we still don’t know why. Let’s review the error message again:

ArgumentError:
  wrong number of arguments (given 0, expected 1+)

Side note: learning to read error messages, and not immediately responding with fear, is one of the most valuable lessons I’ve learned in my programming journey so far. Error messages can be extremely helpful, and they’re put there for a reason. Learn to read them!

What this error message is telling me is: some method is getting called without the arguments it expects.

Which method, though? Unfortunately RSpec doesn’t tell us a much about which method is generating this specific error, so we have to do a little digging. However, we can rule out this custom #initialize method itself as the culprit. The error message would be reversed if that were the case since its method signature defines two arguments it expects.

There must be some other method getting called as part of the ExternalMailer object initialization process.

Use the Source, Luke.

Since this error appears to stem from this #initialize method override, I thought there was a good chance something changed in the way ActionMailer instantiates itself from Rails 4.2 to Rails 5.0. What does the original ActionMailer::Base#initialize method even do? To answer that question, there’s no better place to go than straight to the source.

If you’ve never dug into the source code for Rails, or any codebase for that matter, it can be super enlightening. Here’s how:

First, let’s head over to the Rails repo on Github and navigate to the 4.2.11.1 release tag. This will allow us to drill down into the ActionMailer code as it existed in v4.2.11.1. In actionmailer/lib/action_mailer/base.rb, we can see the method definition of the initialize method our code is overriding:

def initialize(method_name=nil, *args) 
  super() 
  @_mail_was_called = false
  @_message = Mail.new
  process(method_name, *args) if method_name 
end

Look familiar? Indeed, it does. The method signature is identical to what is in our application. Also notice that the process method is called at the end.

Hmm… our code doesn’t touch this process method, does it?

Now, let’s compare what we see in v4.2.11.1 to the code at v5.0.7.2:

def initialize
  super() 
  @_mail_was_called = false
  @_message = Mail.new
end

def process(method_name, *args) #:nodoc:
  payload = { 
    mailer: self.class.name, 
    action: method_name 
  } 

  ActiveSupport::Notifications.instrument("process.action_mailer", payload) do
    super
    @_message = NullMail.new unless @_mail_was_called
  end
end

Interesting! Do you see what happened? The method definition of the initialize method changed, as well as the call to the process method has been moved out of the initialize method. It looks like the authors of our codebase relied on the implementation of the ActionMailer::Base#initialize method, that is to say, they relied on #initialize to call #process as a side effect. Since our custom #initialize method doesn’t call #process, the arguments it expects to receive are never being passed, causing the ArgumentError we see in our failure message.

Whew!

Are We Green Yet?

So what’s the solution here? There’s a couple ways we could approach making these tests pass now that we know the root cause.

The quickest fix is simply to change the name of the method we’re overriding in our ExternalMailer class from initialize to process. Doing so causes the test suite to go green, which is great. Yay!

But monkey patching an internal Rails class method like this is not a great approach - as evidenced by all the digging we had to do to figure out what made these tests fail in the first place. Ideally, we should put the logic to check whether the user is authorized to send an email in another place. Otherwise, we could find ourselves in the exact same position in the future if the source changes the implementation of ActionMailer::Base#process.

But we’ll leave that refactor for another day, and another blog post.