Property definitions for properties of same name shared between Models with same base class

[jhecking]

I've found a conflict between property definitions with the same property name in separate models if these models share a common base class.

Here is a test case to replicate this issue:

    describe "model inheritance" do
      context "multiple models with same base class" do
        it "should not overwrite property definitions of the same name in different models" do
          class MyBase < CouchRest::Model::Base; end
          class MyModelA < MyBase
            property :prop, Time
          end
          class MyModelB < MyBase
            property :prop, Integer
          end
          MyModelA.properties_by_name['prop'].type.should eq Time
          MyModelB.properties_by_name['prop'].type.should eq Integer
        end
      end
    end

And this is the error I get when running rspec:

  1) Model Base model inheritance multiple models with same base class should not overwrite property definitions of the same name in different models
     Failure/Error: MyModelA.properties_by_name['prop'].type.should eq Time

       expected Time
            got Integer

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -Time
       +Integer
     # ./spec/unit/base_spec.rb:577:in `block (4 levels) in <top (required)>'

The problem is that the class attribute properties_by_name is shared by all the subclasses that inherit from the same base class.

[jhecking]

Here is a slightly improved spec that doesn't need to access properties_by_name to demonstrate the problem:

  describe "model inheritance" do
    context "multiple models with same base class" do
      it "should not overwrite property definitions of the same name in different models" do
        class MyBase < CouchRest::Model::Base; end
        class MyModelA < MyBase
          property :prop, Time
        end
        class MyModelB < MyBase
          property :prop, Integer
        end
        MyModelA.new(prop: Time.now).prop.class.should eq Time
      end
    end
  end

Result:

  1) Model Base model inheritance multiple models with same base class should not overwrite property definitions of the same name in different models
     Failure/Error: MyModelA.new(prop: Time.now).prop.class.should eq Time

       expected Time
            got Fixnum

       (compared using ==)

       Diff:
       @@ -1,2 +1,2 @@
       -Time
       +Fixnum
     # ./spec/unit/base_spec.rb:577:in `block (4 levels) in <top (required)>'

[jhecking]

I found a potential fix for the issue. When new subclasses of the common base class get instantiated the properties_by_name data structure should be duplicated for the new subclass. The same already happens for the properties class attribute in Base.inherited:

      def self.inherited(subklass)
        super
        subklass.send(:include, CouchRest::Model::Properties)

        subklass.class_eval <<-EOS, __FILE__, __LINE__ + 1
          def self.inherited(subklass)
            super
            subklass.properties = self.properties.dup
            # This is nasty:
            subklass._validators = self._validators.dup
          end
        EOS
        subclasses << subklass
      end

I think another line should be added to the definition of the inherited method added to the subclass:

subklass.properties_by_name = self.properties_by_name.dup

I tried doing that and it fixes my new test case as well as the issues that I've been observing in our application. However this change breaks a couple of other unit tests:

  1) nested models (not casted) should correctly save single relation
     Failure/Error: @cat.save
     ArgumentError:
       Missing property definition for trainer
     # ./lib/couchrest/model/properties.rb:101:in `find_property!'
     # ./lib/couchrest/model/properties.rb:36:in `read_attribute'
     # ./lib/couchrest/model/properties.rb:236:in `block in create_property_getter'
     # ./lib/couchrest/model/validations.rb:31:in `valid?'
     # ./lib/couchrest/model/persistence.rb:108:in `perform_validations'
     # ./lib/couchrest/model/persistence.rb:10:in `create'
     # ./lib/couchrest/model/persistence.rb:47:in `save'
     # ./spec/unit/property_spec.rb:371:in `block (2 levels) in <top (required)>'

  2) nested models (not casted) should correctly save collection
     Failure/Error: @cat.save
     ArgumentError:
       Missing property definition for trainer
     # ./lib/couchrest/model/properties.rb:101:in `find_property!'
     # ./lib/couchrest/model/properties.rb:36:in `read_attribute'
     # ./lib/couchrest/model/properties.rb:236:in `block in create_property_getter'
     # ./lib/couchrest/model/validations.rb:31:in `valid?'
     # ./lib/couchrest/model/persistence.rb:108:in `perform_validations'
     # ./lib/couchrest/model/persistence.rb:10:in `create'
     # ./lib/couchrest/model/persistence.rb:47:in `save'
     # ./spec/unit/property_spec.rb:371:in `block (2 levels) in <top (required)>'

These two test cases in property_spec only fail if I run property_spec and base_spec together. When running property_spec on it's own it passes. The two tests fail because one of the test cases in base_spec defines a new property trainer on the Cat class which is the base class of the ChildCat instance which fails to save in the property_spec. This dependency between test cases is bad.

However the test case failure still indicates that my fix could impact functionality that might be require for some use cases. Namely the ability to add properties to a base class after subclasses to this base class have already been defined and still have the new property propagate to the subclasses.

So maybe there are better ways to fix this issue?

[ninovanhooff]

My team also encountered this issue in couchrest_model 2.0.4, 2 years after the original posting of this issue.

This is a very serious bug that will affect a large group of users. It means that properties with the same name defined in different models cannot have different types. Think about properties like 'status' which might be a String in one model and an Integer in another. The actual result will be that both properties will be cast to either a String or an Int, possibly depending on the order in which the classes are loaded.

Is this issue being worked on?