ActiveRecord#freeze

Blissful Ignorance

I have an ActiveRecord model that saves the going Canadian exchange rate when needed. I (in)conveniently called that method freeze:

def freeze  
  if unit_order
    unit_order.set_freight
    if Maybe(unit_order.dealer).canadian?
      unit_order.set_exchange_rate(true)
    end
    unit_order.save
  end
end  

My tests showed no issues, so this method made it into production.

Failure!

Things went fine for a week, until someone tried to delete a Canadian order. The system threw the following error:

ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved  

Usually I can recognize my own errors. Not this time! So I started reading the stack trace (I've cleaned this up for readability):

app/models/unit_order.rb:71:in `block in freeze'  
app/models/unit_order.rb:58:in `freeze'  
app/models/unit_order.rb:151:in `freeze'  
...
lib/active_support/callbacks.rb:385:in `_run_destroy_callbacks'  
lib/active_support/callbacks.rb:81:in `run_callbacks'  
lib/active_record/callbacks.rb:254:in `destroy'  
...
app/controllers/unit_orders_controller.rb:79:in `destroy'  

Inexplicably, the freeze method was being called as a destroy callback. I searched all through my code, and freeze wasn't anywhere near a callback.

I tried using pry to walk my way into it, but it had trouble with the anonymous methods in the callback process.

And then it hit me: ActiveRecord must have its own freeze method! Sure enough, in ActiveRecord::Base we find freeze:

def freeze  
  @attributes.freeze; self
end  

In the case of destroy, freeze is called so the attributes are still available after the record is destroyed.

So I renamed the method to freeze_exchange_rates and all was well.

No Testy Testy

So why didn't my test find this? I'm not sure it could. The test would have to anticipate both a special condition (a Canadian order) and a method name conflict. If I tested the deletion of this class through every possible set of values, then it would have shown up. It is debatable if such an effort would have worked at all.

At this point I do not think the time spent to create such a test would be worth the return value. But I may change my mind.

ActiveRecord#dont_use_generic_callbacks

So the moral is, don't use generic names for your callbacks! It is better to have descriptive method names for self documenting code, sure. But sometimes a short name, that seems descriptive enough, can get you into trouble.

comments powered by Disqus