Fixing issues with dirty tracking on nested models and related improvements

VERSION

@@ -1 +1 @@
-1.1.0.beta3
+1.1.0.beta4

couchrest_model.gemspec

@@ -25,7 +25,7 @@ Gem::Specification.new do |s|
   
   s.add_dependency(%q<couchrest>, "1.1.0.pre2")
   s.add_dependency(%q<mime-types>, "~> 1.15")
-  s.add_dependency(%q<activemodel>, "~> 3.0.0")
+  s.add_dependency(%q<activemodel>, "~> 3.0.5")
   s.add_dependency(%q<tzinfo>, "~> 0.3.22")
   s.add_dependency(%q<railties>, "~> 3.0.0")
   s.add_development_dependency(%q<rspec>, ">= 2.0.0")

history.txt

@@ -1,4 +1,4 @@
-== 1.1.0.beta3
+== 1.1.0.beta4
 
 * Major changes:
   * Fast Dirty Tracking! Many thanks to @sobakasu (Andrew Williams)
@@ -10,6 +10,10 @@
   * Added "auto_update_design_doc" configuration option.
   * Using #descending on View object will automatically swap startkey with endkey.
 
+== 1.1.0.beta3
+
+* Removed
+
 == 1.1.0.beta2
 
 * Minor enhancements:

lib/couchrest/model/base.rb

@@ -18,8 +18,8 @@ module CouchRest
       include CouchRest::Model::Associations
       include CouchRest::Model::Validations
       include CouchRest::Model::Designs
-      include CouchRest::Model::Dirty
       include CouchRest::Model::CastedBy
+      include CouchRest::Model::Dirty
 
       def self.subclasses
         @subclasses ||= []
@@ -74,14 +74,6 @@ module CouchRest
         super
       end
 
-      ### instance methods
-
-      # Checks if we're the top document
-      # (overrides base_doc? in casted_by.rb)
-      def base_doc?
-        !@casted_by
-      end
-
       ## Compatibility with ActiveSupport and older frameworks
 
       # Hack so that CouchRest::Document, which descends from Hash,

lib/couchrest/model/casted_array.rb

@@ -5,25 +5,36 @@
 
 module CouchRest::Model
   class CastedArray < Array
+    include CouchRest::Model::CastedBy
     include CouchRest::Model::Dirty
-    attr_accessor :casted_by
+    attr_accessor :casted_by_property
-    attr_accessor :property
 
-    def initialize(array, property)
+    def initialize(array, property, parent = nil)
-      self.property = property
+      self.casted_by_property = property
+      self.casted_by = parent unless parent.nil?
       super(array)
     end
-    
+
+    # Adding new entries
+
     def << obj
-      couchrest_parent_will_change! if use_dirty?
       super(instantiate_and_cast(obj))
     end
-    
+
     def push(obj)
-      couchrest_parent_will_change! if use_dirty?
       super(instantiate_and_cast(obj))
     end
 
+    def unshift(obj)
+      super(instantiate_and_cast(obj))
+    end
+
+    def []= index, obj
+      value = instantiate_and_cast(obj, false)
+      couchrest_parent_will_change! if use_dirty? && value != self[index]
+      super(index, value)
+    end
+
     def pop
       couchrest_parent_will_change! if use_dirty? && self.length > 0
       super
@@ -34,17 +45,6 @@ module CouchRest::Model
       super
     end
 
-    def unshift(obj)
-      couchrest_parent_will_change! if use_dirty?
-      super(instantiate_and_cast(obj))
-    end
-    
-    def []= index, obj
-      value = instantiate_and_cast(obj)
-      couchrest_parent_will_change! if use_dirty? && value != self[index]
-      super(index, value)
-    end
-
     def clear
       couchrest_parent_will_change! if use_dirty? && self.length > 0
       super
@@ -52,11 +52,14 @@ module CouchRest::Model
 
     protected
 
-    def instantiate_and_cast(obj)
+    def instantiate_and_cast(obj, change = true)
-      if self.casted_by && self.property && obj.class != self.property.type_class
+      property = casted_by_property
-        self.property.cast_value(self.casted_by, obj)
+      couchrest_parent_will_change! if change && use_dirty?
+      if casted_by && property && obj.class != property.type_class
+        property.cast_value(casted_by, obj)
       else
-        obj.casted_by = self.casted_by if obj.respond_to?(:casted_by)
+        obj.casted_by = casted_by if obj.respond_to?(:casted_by)
+        obj.casted_by_property = casted_by_property if obj.respond_to?(:casted_by_property)
         obj
       end
     end

lib/couchrest/model/casted_by.rb

@@ -4,6 +4,7 @@ module CouchRest::Model
     extend ActiveSupport::Concern
     included do
       self.send(:attr_accessor, :casted_by)
+      self.send(:attr_accessor, :casted_by_property)
     end
 
     # Gets a reference to the actual document in the DB
@@ -11,13 +12,22 @@ module CouchRest::Model
     # Otherwise we're at the top and we return self
     def base_doc
       return self if base_doc?
-      @casted_by ? @casted_by.base_doc : nil
+      casted_by ? casted_by.base_doc : nil
     end
 
     # Checks if we're the top document
     def base_doc?
-      false
+      !casted_by
     end
 
+    # Provide the property this casted model instance has been
+    # used by. If it has not been set, search through the 
+    # casted_by objects properties to try and find it.
+    #def casted_by_property
+    #  return nil unless casted_by
+    #  attrs = casted_by.attributes
+    #  @casted_by_property ||= casted_by.properties.detect{ |k| attrs[k.to_s] === self }
+    #end
+
   end
 end

lib/couchrest/model/casted_hash.rb

@@ -3,8 +3,16 @@
 
 module CouchRest::Model
   class CastedHash < Hash
+    include CouchRest::Model::CastedBy
     include CouchRest::Model::Dirty
-    attr_accessor :casted_by
+    attr_accessor :casted_by_property
+
+    def self.[](hash, property, parent = nil)
+      obj = super(hash)
+      obj.casted_by_property = property
+      obj.casted_by = parent unless parent.nil?
+      obj
+    end
 
     # needed for dirty
     def attributes

lib/couchrest/model/casted_model.rb

@@ -1,6 +1,6 @@
 module CouchRest::Model
   module CastedModel
-   
+
     extend ActiveSupport::Concern
 
     included do
@@ -10,8 +10,14 @@ module CouchRest::Model
       include CouchRest::Model::PropertyProtection
       include CouchRest::Model::Associations
       include CouchRest::Model::Validations
+      include CouchRest::Model::CastedBy
       include CouchRest::Model::Dirty
-      # attr_accessor :casted_by
+      class_eval do
+        # Override CastedBy's base_doc?
+        def base_doc?
+          false # Can never be base doc!
+        end
+      end
     end
 
     def initialize(keys = {})
@@ -21,7 +27,6 @@ module CouchRest::Model
     end
 
     def []= key, value
-      couchrest_attribute_will_change!(key) if self[key] != value
       super(key.to_s, value)
     end
 
@@ -29,17 +34,10 @@ module CouchRest::Model
       super(key.to_s)
     end
 
-    # Gets a reference to the top level extended
-    # document that a model is saved inside of
-    def base_doc
-      return nil unless @casted_by
-      @casted_by.base_doc
-    end
-
     # False if the casted model has already
     # been saved in the containing document
     def new?
-      @casted_by.nil? ? true : @casted_by.new?
+      casted_by.nil? ? true : casted_by.new?
     end
     alias :new_record? :new?
 
@@ -68,4 +66,5 @@ module CouchRest::Model
     alias :attributes= :update_attributes_without_saving
 
   end
+
 end

lib/couchrest/model/dirty.rb

@@ -10,7 +10,6 @@ module CouchRest
     # This applies to both Model::Base and Model::CastedModel
     module Dirty
       extend ActiveSupport::Concern
-      include CouchRest::Model::CastedBy  # needed for base_doc
       include ActiveModel::Dirty
 
       included do
@@ -21,8 +20,8 @@ module CouchRest
       end
 
       def use_dirty?
-        bdoc = base_doc
+        doc = base_doc
-        bdoc && !bdoc.disable_dirty
+        doc && !doc.disable_dirty
       end
 
       def couchrest_attribute_will_change!(attr)
@@ -32,16 +31,7 @@ module CouchRest
       end
 
       def couchrest_parent_will_change!
-        @casted_by.couchrest_attribute_will_change!(casted_by_attribute) if @casted_by
+        casted_by.couchrest_attribute_will_change!(casted_by_property.name) if casted_by_property
-      end
-
-      private
-
-      # return the attribute name this object is referenced by in the parent
-      def casted_by_attribute
-        return @casted_by_attribute if @casted_by_attribute
-        attr = @casted_by.attributes
-        @casted_by_attribute = attr.keys.detect { |k| attr[k] == self }
       end
 
     end

lib/couchrest/model/persistence.rb

@@ -30,7 +30,7 @@ module CouchRest
       def update(options = {})
         raise "Calling #{self.class.name}#update on document that has not been created!" if self.new?
         return false unless perform_validations(options)
-        return true if !self.changed?
+        return true if !self.disable_dirty && !self.changed?
         _run_update_callbacks do
           _run_save_callbacks do
             result = database.save_doc(self)
@@ -143,20 +143,14 @@ module CouchRest
         # must be globally unique across all document types which share a
         # database, so if you'd like to scope uniqueness to this class, you
         # should use the class name as part of the unique id.
-        def unique_id method = nil, &block
+        def unique_id(method = nil, &block)
           if method
-            define_method :get_unique_id do
-              self.send(method)
-            end
             define_method :set_unique_id do
-              self['_id'] ||= get_unique_id
+              self['_id'] ||= self.send(method)
             end
           elsif block
-            define_method :get_unique_id do
-              block.call(self)
-            end
             define_method :set_unique_id do
-              uniqid = get_unique_id
+              uniqid = block.call(self)
               raise ArgumentError, "unique_id block must not return nil" if uniqid.nil?
               self['_id'] ||= uniqid
             end

lib/couchrest/model/properties.rb

@@ -6,9 +6,9 @@ module CouchRest
 
       included do
         extlib_inheritable_accessor(:properties) unless self.respond_to?(:properties)
-        extlib_inheritable_accessor(:property_by_name) unless self.respond_to?(:property_by_name)
+        extlib_inheritable_accessor(:properties_by_name) unless self.respond_to?(:properties_by_name)
         self.properties ||= []
-        self.property_by_name ||= {}
+        self.properties_by_name ||= {}
         raise "You can only mixin Properties in a class responding to [] and []=, if you tried to mixin CastedModel, make sure your class inherits from Hash or responds to the proper methods" unless (method_defined?(:[]) && method_defined?(:[]=))
       end
 
@@ -20,6 +20,12 @@ module CouchRest
         self.class.properties
       end
 
+      # Returns all the class's properties as a Hash where the key is the name
+      # of the property.
+      def properties_by_name
+        self.class.properties_by_name
+      end
+
       # Returns the Class properties with their values
       #
       # ==== Returns
@@ -43,31 +49,10 @@ module CouchRest
       def write_attribute(property, value)
         prop = find_property!(property)
         value = prop.is_a?(String) ? value : prop.cast(self, value)
-        attribute_will_change!(prop.name) if use_dirty? && self[prop.name] != value
+        couchrest_attribute_will_change!(prop.name) if use_dirty? && self[prop.name] != value
         self[prop.name] = value
       end
 
-      def []=(key,value)
-        return super(key,value) unless use_dirty?
-
-        has_changes = self.changed?
-        if !has_changes && self.respond_to?(:get_unique_id)
-          check_id_change = true
-          old_id = get_unique_id
-        end
-
-        ret = super(key, value)
-
-        if check_id_change
-          # if we have set an attribute that results in the _id changing (unique_id),
-          # force changed? to return true so that the record can be saved
-          new_id = get_unique_id
-          changed_attributes["_id"] = new_id if old_id != new_id
-        end
-
-        ret
-      end
-
       # Takes a hash as argument, and applies the values by using writer methods
       # for each key. It doesn't save the document at the end. Raises a NoMethodError if the corresponding methods are
       # missing. In case of error, no attributes are changed.
@@ -90,7 +75,7 @@ module CouchRest
       protected
 
       def find_property(property)
-        property.is_a?(Property) ? property : self.class.property_by_name[property.to_s]
+        property.is_a?(Property) ? property : self.class.properties_by_name[property.to_s]
       end
 
       # The following methods should be accessable by the Model::Base Class, but not by anything else!
@@ -212,7 +197,7 @@ module CouchRest
               validates_casted_model property.name
             end
             properties << property
-            property_by_name[property.to_s] = property
+            properties_by_name[property.to_s] = property
             property
           end
 

lib/couchrest/model/property.rb

@@ -38,16 +38,13 @@ module CouchRest::Model
         end
         arr = value.collect { |data| cast_value(parent, data) }
         # allow casted_by calls to be passed up chain by wrapping in CastedArray
-        value = CastedArray.new(arr, self)
+        CastedArray.new(arr, self, parent)
-        value.casted_by = parent
       elsif (type == Object || type == Hash) && (value.class == Hash)
         # allow casted_by calls to be passed up chain by wrapping in CastedHash
-        value = CouchRest::Model::CastedHash[value]
+        CastedHash[value, self, parent]
-        value.casted_by = parent
       elsif !value.nil?
-        value = cast_value(parent, value)
+        cast_value(parent, value)
       end
-      value
     end
 
     # Cast an individual value, not an array
@@ -71,6 +68,7 @@ module CouchRest::Model
 
       def associate_casted_value_to_parent(parent, value)
         value.casted_by = parent if value.respond_to?(:casted_by)
+        value.casted_by_property = self if value.respond_to?(:casted_by_property)
         value
       end
 

spec/couchrest/casted_model_spec.rb

@@ -79,6 +79,10 @@ describe CouchRest::Model::CastedModel do
       @obj.name.should == 'Eric'
       @obj.details['color'].should == 'orange'
     end
+    it "should always return base_doc? as false" do
+      @obj.base_doc?.should be_false
+    end
+
   end
 
   describe "casted as an attribute, but without a value" do
@@ -132,6 +136,10 @@ describe CouchRest::Model::CastedModel do
       @casted_obj.casted_by.should == @obj
     end
 
+    it "should know which property casted it" do
+      @casted_obj.casted_by_property.should == @obj.properties.detect{|p| p.to_s == 'casted_attribute'}
+    end
+
     it "should return nil for the 'no_value' attribute" do
       @casted_obj.no_value.should be_nil
     end

spec/couchrest/dirty_spec.rb

@@ -13,7 +13,7 @@ class WithCastedModelMixin < Hash
   property :casted_attribute, WithCastedModelMixin
 end 
 
-class DummyModel < CouchRest::Model::Base
+class DirtyModel < CouchRest::Model::Base
   use_database DB
 
   property :casted_attribute, WithCastedModelMixin
@@ -25,6 +25,16 @@ class DummyModel < CouchRest::Model::Base
   end
 end
 
+class DirtyUniqueIdModel < CouchRest::Model::Base
+  use_database DB
+  attr_accessor :code
+  unique_id :code
+  property :title, String, :default => "Sample Title"
+  timestamps!
+
+  def code; self['_id'] || @code; end
+end
+
 describe "Dirty" do
 
   describe "changes" do
@@ -66,18 +76,21 @@ describe "Dirty" do
     end
 
     it "should report no changes on a hash property with a default value" do
-      @obj = DummyModel.new
+      @obj = DirtyModel.new
       @obj.details.changed?.should be_false
     end
 
-=begin
     # match activerecord behaviour
-    # not currently working - not too important
     it "should report changes on a new object with attributes set" do
       @card = Card.new(:first_name => "matt")
       @card.changed?.should be_true
     end
-=end
+
+    it "should report no changes on new object with 'unique_id' set" do
+      @obj = DirtyUniqueIdModel.new
+      @obj.changed?.should be_false
+      @obj.changes.should be_empty
+    end
 
     it "should report no changes on objects fetched from the database" do
       card_id = Card.create!(:first_name => "matt").id
@@ -156,15 +169,37 @@ describe "Dirty" do
     it "should report changes to casted models" do
       @cat = Cat.create!(:name => "Felix", :favorite_toy => { :name => "Mouse" })
       @cat = Cat.find(@cat.id)
-      @cat.favorite_toy['name'] = 'Feather'
+      @cat.favorite_toy.name = 'Feather'
       @cat.changed?.should be_true
     end
 
+    it "should report changes to casted model in array" do
+      @obj = Cat.create!(:name => 'felix', :toys => [{:name => "Catnip"}])
+      @obj = Cat.get(@obj.id)
+      @obj.toys.first.name.should eql('Catnip')
+      @obj.toys.first.changed?.should be_false
+      @obj.changed?.should be_false
+      @obj.toys.first.name = "Super Catnip"
+      @obj.toys.first.changed?.should be_true
+      @obj.changed?.should be_true
+    end
+
+    it "should report changes to anonymous casted models in array" do
+      @obj = DirtyModel.create!(:sub_models => [{:title => "Sample"}])
+      @obj = DirtyModel.get(@obj.id)
+      @obj.sub_models.first.title.should eql("Sample")
+      @obj.sub_models.first.changed?.should be_false
+      @obj.changed?.should be_false
+      @obj.sub_models.first.title = "Another Sample"
+      @obj.sub_models.first.changed?.should be_true
+      @obj.changed?.should be_true
+    end
+
     # casted arrays
 
     def test_casted_array(change_expected)
-      obj = DummyModel.create!
+      obj = DirtyModel.create!
-      obj = DummyModel.get(obj.id)
+      obj = DirtyModel.get(obj.id)
       array = obj.keywords
       yield array, obj
       if change_expected
@@ -249,8 +284,8 @@ describe "Dirty" do
     # Object, {}  (casted hash)
 
     def test_casted_hash(change_expected)
-      obj = DummyModel.create!
+      obj = DirtyModel.create!
-      obj = DummyModel.get(obj.id)
+      obj = DirtyModel.get(obj.id)
       hash = obj.details
       yield hash, obj
       if change_expected

spec/couchrest/persistence_spec.rb

@@ -227,7 +227,7 @@ describe "Model Persistence" do
       @templated['important-field'] = 'not-important'
       @templated.save.should be_true
       t = WithTemplateAndUniqueID.get('very-important')
-      t.should == @templated
+      t.id.should == @templated.id
     end
     
     it "should raise an error when the id is taken" do