Watch Out When Overriding Memoized Methods

This post was inspired by a 🐛 that was caused by not thinking enough when overriding a memoized method. Once we noticed the buggy behaviour, it took me some binding.pry time to figure out what was going on.

Lets check out what happened.

Code snippets

module Actions
  class FormAction
    attr_reader ...

    def initialize(...); ...; end

    def execute_action
      interpolate_properties

      form.process(form_attributes)
    end

    private

    def interpolate_properties
      interpolatable_attributes.each do |attribute|
        form_attributes[attribute] = interpolate(attribute)
      end
    end

    def interpolatable_attributes
      raise NotImplementedError
    end

    ...

    def form_attributes
      @form_attributes ||= action[:attributes].stringify_keys!
    end
  end  
end

A simple Ruby class which is the base class for the specific actions we need to implement for our Rails app.

The action for which we figured out the bug was the action of creating comments on various objects. Its code looks like:

module Actions
  class CreateComment < FormAction
    private

    def interpolatable_attributes
      ['body']
    end

    ...

    def form_attributes
      super.merge("#{commentable_type}_id": action_object.id)
    end
  end
end

Not much code is actually shown but just enough to spot the bug 🐛. Found it?

What was going on

Basically what was happening when running CreateComment#execute_action was the following:

In the execute_action we firstly need to interpolate some attributes - defined by the specific action class. In this case, for the CreateComment action that was only the 'body' attribute.

def execute_action
  # form_attributes = { 'body' => 'non interpolated' } 
  interpolate_properties

  form.process(form_attributes)
end

That's fine - we do some logic, the 'body' attribute gets interpolated and that value is set as the new value for the 'body' key in the form_attributes hash.

def execute_action
  interpolate_properties
  # form_attributes = { 'body' => 'interpolated' }
  form.process(form_attributes)
end

But, as the implementation of the CreateComment class uses super.merge(...) what we get is actually a new hash each time the method is called.

def execute_action
  interpolate_properties

  form.process(form_attributes) 
  # process method called with { 'body' => 'non interpolated' }
end

The next time, after the interpolation is over, the method form_attributes is called when passing that hash to our form object and in that moment, the form object gets a newly generated hash that doesn't have the interpolated 'body' value. So the interpolation was actually pointless as would any other modification of the form_attributes hash prior to the form.process call be.

The solution 🐛🔨

module Actions
  class CreateComment < FormAction
    private

    ...

    def form_attributes
      @form_attributes ||= super.merge("#{commentable_type}_id": action_object.id)
    end
  end
end

Added memoization to the CreateComment#form_attributes method.

Simple as that and when you think about it, also pretty obvious that it needed to look like this from the start.

p.s. These snippets maybe look a bit too simple and memoization probably looks like not needed but it's a stripped version of the real classes. The form_attributes method is called and modified quite a few times 🙃