Fix STI-related bug in validates_timeliness exposed in Rails 3.2

[jimherz]

This pull request addresses a previously reported issue: https://github.com/adzap/validates_timeliness/issues/88

The cause is quite complicated, but I'll try to keep my explanation brief.

The issue is exposed by changes in ActiveRecord::AttributeMethods#define_attribute_methods between rails 3.1 and 3.2. This method now calls itself on an AR subclass's superclass. The effect of this is that ValidatesTimeliness::AttributeMethods#define_attribute_methods is called on all levels of an STI hierarchy. Hence, define_timeliness_methods is called on all levels of an STI hierarchy. And herein lies the problem:

module ValidatesTimeliness
  module AttributeMethods
    extend ActiveSupport::Concern

    included do
      class_attribute :timeliness_validated_attributes
      self.timeliness_validated_attributes = []
    end
    ...

end

As you can see above, timeliness_validated_attributes is defined as a class_attribute, which means that any subclasses will "inherit" a copy of it, including the values added to it by the superclass. For instance,

class Author < ActiveRecord::Base
  validates_date :published_on
  validates_date :famous_on
end

class BadAuthor < Author
  validates_date :died_on
end

Loading development environment (Rails 3.2.14)
1.9.3-p448 :001 > Author.timeliness_validated_attributes
 => [:published_on, :famous_on] 
1.9.3-p448 :002 > BadAuthor.timeliness_validated_attributes
 => [:published_on, :famous_on, :died_on] 

Validates_timeliness generates "#{attr_name}=" methods for each of the attributes in a class's timeliness_validated_attributes. The methods are defined in an anonymous module which is inserted into the corresponding class's ancestor chain.

In the context of our STI example, this effectively means that the same "published_on=" method is defined twice in two separate modules in BadAuthor's heirarchy. Via super, these calls are chained together with the result of the first being the argument to the next. When you combine this with :use_plugin_parser set to true, it messes up the @timeliness_cache and, hence, the published_on_before_type_cast, and, hence, the validation of an invalid date...

The solution is to use a class level instance variable instead of a class_attribute. This way, each variable with a validates_date declaration (or validates_time, etc.) is only tracked and acted upon once via the class it is declared in.

Here is the change:

module ValidatesTimeliness
  module AttributeMethods
    extend ActiveSupport::Concern

    included do
      class << self
        attr_accessor :timeliness_validated_attributes
      end
      self.timeliness_validated_attributes = []
    end
    ...

end

And now things look right:

Loading development environment (Rails 3.2.14)
1.9.3-p448 :001 > Author.timeliness_validated_attributes
 => [:published_on, :famous_on] 
1.9.3-p448 :002 > BadAuthor.timeliness_validated_attributes
 => [:died_on] 

Phew! Long explanation for such a small change :)

[jimherz]

Hi,

I'm still wondering if you would consider this pull request. We had to release our own version of the gem in the meantime, which is obviously not ideal.

Thanks, Jim

[adzap]

Getting a spec for this would be great. I have released v4 of the gem for Rails 4.x. So things are slightly different, though the definition of the timeliness_validated_attributes is the same.

[bboe]

I was able to reproduce the previously described behavior on Rails 5.2 with validates_timeliness 5.0.0.alpha2. What I found is that BadAuthor still outputs all fields as part of its timeliness_validates_attributes, however, the bug making it such that it doesn't property validate the parent class's attributes (#88) no longer appears to be an issue.

2.3.3 :004 > BadAuthor.timeliness_validated_attributes
 => [:published_on, :famous_on, :died_on] 

I wonder if this PR is no longer needed.

[adzap]

@bboe great thanks for this. BadAuthor should inherit all of the parent's validated attributes and its validations. So I don't think that is part of the bug. The bug from reading is not that the child has the parent's validation, that is desired, but that it gets messed up when validating.

So if you have tested it, I have happy to close this.