Funny bug fixing with ActiveRecord comparisons and Comparable

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