Today I was facing a very annoying bug in some old code that demonstrated to be quite hard to track down.
There’s a form where the user can set some dates, which have to be valid, meaning that each date must be greater than the previous one. Here’s a screenshot of the relevant part of the form:
Each date in the form is actually part of an Event ActiveRecord object. Each event belongs to a calendar, which is the actual record that gets saved when the form is submittted.
In order to validate the correct order of the dates the Comparable module was included into the Event model. Here it is some pseudocode, a simplified version of the original one:
class Event < ActiveRecord::Base
include Comparable
belongs_to :calendar
def <=> other
if calendar == other.calendar and calendar.present?
date <=> other.date
end
end
# ...
end
class Calendar
has_many :events
validate :validate_event_dates_sequence
#....
end
The starship method (<=>) in the Event class is quite straightforward: it compares the records dates in order to decide which one is greater, given that they belong to the same calendar. Calendars have many events, and its the calendar responsibility to make sure all events have dates in the right order via the validate_event_dates_sequence method.
So, where’s the problem? When you edit an existing calendar and try to change the first date value, validations somehow fail and an extra date coming out of the blue is added to the form. We used to have 5 events, now we have 6:
Why is that happening? I used pry in order to inspect the issue and I noticed that calendar.events
contained an extra record, which had the same id of the first one:
calendar.events.map &:id
# => [16, 17, 18, 19, 20, 16]
For some reason ActiveRecord instantiated a new record instead of using the existing one. To confirm this oddity I did some more inspection. All records are saved in the database and that’s expected since I’m editing existing data:
events.map &:persisted?
=> [true, true, true, true, true, true]
The first record and the last one, while having the same id and referring to the same record in the database, are totally different objects for ruby:
events.first.object_id
=> 2162200580
events.last.object_id
=> 2214847000
Event dates are not ordered from smaller to greater (the last date, which should have replaced the first one, is in the wrong position as well):
[Thu, 06 Nov 2014 15:49:00 CET +01:00,
Fri, 07 Nov 2014 15:49:00 CET +01:00,
Sat, 08 Nov 2014 15:49:00 CET +01:00,
Sun, 09 Nov 2014 15:49:00 CET +01:00,
Mon, 10 Nov 2014 15:49:00 CET +01:00,
Wed, 05 Nov 2014 17:49:00 CET +01:00]
Finally, let’s compare the first event to the last one, just to confirm that something is wrong:
events.first <=> events.last
# => 1
It was supposed to return 0, because they refer to the same record on the database.
So, the first problem is that the old implementation was too naive: it doesn’t check the records ID equality before comparing their dates. The first record has the same ID of the last one but its date is different, so the starship method <=> doesn’t return 0 as it should. Lack of complete rspec tests allowed for this bug to happen.
But there’s more: even though ActiveRecord::Base doesn’t include the Comparable module, it definitely has the <=>
method:
ActiveRecord::Base.methods.include? :<=>
# => true
ActiveRecord::Base.ancestors.include? Comparable
# => false
Here’s the default ActiveRecord implementation:
module ActiveRecord
module Core
def <=>(other_object)
if other_object.is_a?(self.class)
self.to_key <=> other_object.to_key
else
super
end
end
end
end
The main issues were:
- The new implementation of the <=> method was not completely tested, leaving out one of the most important cases (record equality)
- Comparable module was added but was useless
- There’s always a good explaination behind odd behaviors, it’s just a matter of taking the time to find it out.
By the way my final implementation was:
def <=> (other)
unless super.zero? # they are not the same record
if calendar == other.calendar and calendar.present?
date <=> other.date
end
end
end
Leave a Reply