I have an ActiveRecord model that saves the going Canadian exchange rate when needed. I (in)conveniently called that method
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.
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'
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
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.
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.