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 🙃
Top comments (2)
Great eye for catching such a subtle bug, I have to remember this for the future
Anyone had similar "problems" with this? 🐛 @||=