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
    if Maybe(

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


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

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.


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