From 4dbf694e5112952ed6e4abd6016e3b50da620694 Mon Sep 17 00:00:00 2001 From: Andrew Williams Date: Tue, 1 Mar 2011 01:30:41 +1030 Subject: [PATCH] now using ActiveModel::Dirty. only writes to database if model.changed? --- .gitignore | 2 +- lib/couchrest/model/base.rb | 3 +- lib/couchrest/model/casted_array.rb | 22 ++- lib/couchrest/model/casted_hash.rb | 20 +++ lib/couchrest/model/casted_model.rb | 3 + lib/couchrest/model/dirty.rb | 41 +++++ lib/couchrest/model/extended_attachments.rb | 9 ++ lib/couchrest/model/persistence.rb | 19 ++- lib/couchrest/model/properties.rb | 61 +++++-- lib/couchrest/model/property.rb | 4 + lib/couchrest_model.rb | 6 +- spec/couchrest/base_spec.rb | 1 + spec/couchrest/dirty_spec.rb | 167 ++++++++++++++++++++ spec/fixtures/base.rb | 2 +- 14 files changed, 342 insertions(+), 18 deletions(-) create mode 100644 lib/couchrest/model/casted_hash.rb create mode 100644 lib/couchrest/model/dirty.rb create mode 100644 spec/couchrest/dirty_spec.rb diff --git a/.gitignore b/.gitignore index 5b1ad4c..f6c7e5a 100644 --- a/.gitignore +++ b/.gitignore @@ -6,4 +6,4 @@ pkg .bundle couchdb.std* *.*~ - +spec.out diff --git a/lib/couchrest/model/base.rb b/lib/couchrest/model/base.rb index b52ad59..9d47d1b 100644 --- a/lib/couchrest/model/base.rb +++ b/lib/couchrest/model/base.rb @@ -16,6 +16,7 @@ module CouchRest include CouchRest::Model::PropertyProtection include CouchRest::Model::Associations include CouchRest::Model::Validations + include CouchRest::Model::Dirty def self.subclasses @subclasses ||= [] @@ -55,7 +56,7 @@ module CouchRest after_initialize if respond_to?(:after_initialize) end - + # Temp solution to make the view_by methods available def self.method_missing(m, *args, &block) if has_view?(m) diff --git a/lib/couchrest/model/casted_array.rb b/lib/couchrest/model/casted_array.rb index a52f152..6d187b5 100644 --- a/lib/couchrest/model/casted_array.rb +++ b/lib/couchrest/model/casted_array.rb @@ -5,6 +5,7 @@ module CouchRest::Model class CastedArray < Array + include CouchRest::Model::Dirty attr_accessor :casted_by attr_accessor :property @@ -14,15 +15,34 @@ module CouchRest::Model end def << obj + couchrest_parent_will_change! super(instantiate_and_cast(obj)) end def push(obj) + couchrest_parent_will_change! super(instantiate_and_cast(obj)) end + + def pop + couchrest_parent_will_change! + super + end + + def shift + couchrest_parent_will_change! + super + end + + def unshift(obj) + couchrest_parent_will_change! + super(obj) + end def []= index, obj - super(index, instantiate_and_cast(obj)) + value = instantiate_and_cast(obj) + couchrest_parent_will_change! if value != self[index] + super(index, value) end protected diff --git a/lib/couchrest/model/casted_hash.rb b/lib/couchrest/model/casted_hash.rb new file mode 100644 index 0000000..737c91b --- /dev/null +++ b/lib/couchrest/model/casted_hash.rb @@ -0,0 +1,20 @@ +# +# Wrapper around Hash so that the casted_by attribute is set. + +module CouchRest::Model + class CastedHash < Hash + include CouchRest::Model::Dirty + attr_accessor :casted_by + + def []= index, obj + couchrest_parent_will_change! if obj != self[index] + super(index, obj) + end + + # needed for dirty + def attributes + self + end + + end +end diff --git a/lib/couchrest/model/casted_model.rb b/lib/couchrest/model/casted_model.rb index 4d931fc..19b6fc9 100644 --- a/lib/couchrest/model/casted_model.rb +++ b/lib/couchrest/model/casted_model.rb @@ -10,6 +10,7 @@ module CouchRest::Model include CouchRest::Model::PropertyProtection include CouchRest::Model::Associations include CouchRest::Model::Validations + include CouchRest::Model::Dirty attr_accessor :casted_by end @@ -20,6 +21,7 @@ module CouchRest::Model end def []= key, value + couchrest_attribute_will_change!(key) unless self[key] == value super(key.to_s, value) end @@ -64,5 +66,6 @@ module CouchRest::Model end end alias :attributes= :update_attributes_without_saving + end end diff --git a/lib/couchrest/model/dirty.rb b/lib/couchrest/model/dirty.rb new file mode 100644 index 0000000..31199b8 --- /dev/null +++ b/lib/couchrest/model/dirty.rb @@ -0,0 +1,41 @@ +# encoding: utf-8 + +I18n.load_path << File.join( + File.dirname(__FILE__), "validations", "locale", "en.yml" +) + +module CouchRest + module Model + + # This applies to both Model::Base and Model::CastedModel + module Dirty + extend ActiveSupport::Concern + + included do + include ActiveModel::Dirty + end + + def couchrest_attribute_will_change!(attr) + return if attr.nil? + self.send("#{attr}_will_change!") + if pkey = casted_by_attribute + @casted_by.couchrest_attribute_will_change!(pkey) + end + end + + def couchrest_parent_will_change! + @casted_by.couchrest_attribute_will_change!(casted_by_attribute) if @casted_by + end + + private + + # return the attribute name this object is referenced by in the parent + def casted_by_attribute + return nil unless @casted_by + attr = @casted_by.attributes + attr.keys.detect { |k| attr[k] == self } + end + + end + end +end diff --git a/lib/couchrest/model/extended_attachments.rb b/lib/couchrest/model/extended_attachments.rb index 42bccec..c2a5ffc 100644 --- a/lib/couchrest/model/extended_attachments.rb +++ b/lib/couchrest/model/extended_attachments.rb @@ -1,6 +1,12 @@ module CouchRest module Model module ExtendedAttachments + extend ActiveSupport::Concern + include ActiveModel::Dirty + included do + # for _attachments_will_change! + define_attribute_methods [:_attachments] + end # Add a file attachment to the current document. Expects # :file and :name to be included in the arguments. @@ -35,6 +41,7 @@ module CouchRest # deletes a file attachment from the current doc def delete_attachment(attachment_name) return unless attachments + _attachments_will_change! if attachments.include?(attachment_name) attachments.delete attachment_name end @@ -66,6 +73,8 @@ module CouchRest def set_attachment_attr(args) content_type = args[:content_type] ? args[:content_type] : get_mime_type(args[:file].path) content_type ||= (get_mime_type(args[:name]) || 'text/plain') + + _attachments_will_change! attachments[args[:name]] = { 'content_type' => content_type, 'data' => args[:file].read diff --git a/lib/couchrest/model/persistence.rb b/lib/couchrest/model/persistence.rb index 9f67175..f4c7970 100644 --- a/lib/couchrest/model/persistence.rb +++ b/lib/couchrest/model/persistence.rb @@ -12,7 +12,9 @@ module CouchRest _run_save_callbacks do set_unique_id if new? && self.respond_to?(:set_unique_id) result = database.save_doc(self) - (result["ok"] == true) ? self : false + ret = (result["ok"] == true) ? self : false + @changed_attributes.clear if ret && @changed_attributes + ret end end end @@ -28,10 +30,13 @@ 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 unless self.changed? _run_update_callbacks do _run_save_callbacks do result = database.save_doc(self) - result["ok"] == true + ret = result["ok"] == true + @changed_attributes.clear if ret && @changed_attributes + ret end end end @@ -140,12 +145,18 @@ module CouchRest # should use the class name as part of the unique id. 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'] ||= self.send(method) + self['_id'] ||= get_unique_id end elsif block + define_method :get_unique_id do + block.call(self) + end define_method :set_unique_id do - uniqid = block.call(self) + uniqid = get_unique_id raise ArgumentError, "unique_id block must not return nil" if uniqid.nil? self['_id'] ||= uniqid end diff --git a/lib/couchrest/model/properties.rb b/lib/couchrest/model/properties.rb index 481d135..8f54fee 100644 --- a/lib/couchrest/model/properties.rb +++ b/lib/couchrest/model/properties.rb @@ -3,6 +3,7 @@ module CouchRest module Model module Properties extend ActiveSupport::Concern + include ActiveModel::Dirty included do extlib_inheritable_accessor(:properties) unless self.respond_to?(:properties) @@ -38,11 +39,33 @@ module CouchRest # Store a casted value in the current instance of an attribute defined # with a property. + # TODO: mixin dirty functionality into value (?) def write_attribute(property, value) prop = find_property!(property) self[prop.to_s] = prop.is_a?(String) ? value : prop.cast(self, value) end + # write property, update dirty status + def write_attribute_dirty(property, value) + prop = find_property!(property) + value = prop.is_a?(String) ? value : prop.cast(self, value) + self.send("#{prop}_will_change!") unless self[prop.to_s] == value + write_attribute(property, value) + end + + def []=(key,value) + old_id = get_unique_id if self.respond_to?(:get_unique_id) + + super(key, value) + + if self.respond_to?(:get_unique_id) + # 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 + 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. @@ -50,12 +73,23 @@ module CouchRest # Remove any protected and update all the rest. Any attributes # which do not have a property will simply be ignored. attrs = remove_protected_attributes(hash) - directly_set_attributes(attrs) + directly_set_attributes(attrs, :dirty => true) end alias :attributes= :update_attributes_without_saving + # needed for Dirty + def attributes + ret = {} + self.class.properties.each do |property| + ret[property.name] = read_attribute(property) + end + ret + end + + def find_property(property) + property.is_a?(Property) ? property : self.class.properties.detect {|p| p.to_s == property.to_s} + end - private # The following methods should be accessable by the Model::Base Class, but not by anything else! def apply_all_property_defaults return if self.respond_to?(:new?) && (new? == false) @@ -76,17 +110,26 @@ module CouchRest end def find_property!(property) - prop = property.is_a?(Property) ? property : self.class.properties.detect {|p| p.to_s == property.to_s} + prop = find_property(property) raise ArgumentError, "Missing property definition for #{property.to_s}" if prop.nil? prop end # Set all the attributes and return a hash with the attributes # that have not been accepted. - def directly_set_attributes(hash) + def directly_set_attributes(hash, options = {}) hash.reject do |attribute_name, attribute_value| if self.respond_to?("#{attribute_name}=") - self.send("#{attribute_name}=", attribute_value) + if find_property(attribute_name) + if options[:dirty] + self.write_attribute_dirty(attribute_name, attribute_value) + else + # set attribute without updating dirty status + self.write_attribute(attribute_name, attribute_value) + end + else + self.send("#{attribute_name}=", attribute_value) + end true elsif mass_assign_any_attribute # config option self[attribute_name] = attribute_value @@ -126,7 +169,7 @@ module CouchRest end existing_property = self.properties.find{|p| p.name == name.to_s} if existing_property.nil? || (existing_property.default != opts[:default]) - define_property(name, opts, &block) + define_property(name, opts, &block) end end @@ -139,8 +182,8 @@ module CouchRest property(:created_at, Time, :read_only => true, :protected => true, :auto_validation => false) set_callback :save, :before do |object| - write_attribute('updated_at', Time.now) - write_attribute('created_at', Time.now) if object.new? + write_attribute_dirty('updated_at', Time.now) + write_attribute_dirty('created_at', Time.now) if object.new? end EOS end @@ -203,7 +246,7 @@ module CouchRest property_name = property.name class_eval <<-EOS def #{property_name}=(value) - write_attribute('#{property_name}', value) + write_attribute_dirty('#{property_name}', value) end EOS diff --git a/lib/couchrest/model/property.rb b/lib/couchrest/model/property.rb index f08405c..2e49a00 100644 --- a/lib/couchrest/model/property.rb +++ b/lib/couchrest/model/property.rb @@ -40,6 +40,10 @@ module CouchRest::Model # allow casted_by calls to be passed up chain by wrapping in CastedArray value = type_class != String ? CastedArray.new(arr, self) : arr value.casted_by = parent if value.respond_to?(:casted_by) + 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] + value.casted_by = parent elsif !value.nil? value = cast_value(parent, value) end diff --git a/lib/couchrest_model.rb b/lib/couchrest_model.rb index 2e0070d..4d3e7c6 100644 --- a/lib/couchrest_model.rb +++ b/lib/couchrest_model.rb @@ -8,6 +8,7 @@ require "active_model/serialization" require "active_model/translation" require "active_model/validator" require "active_model/validations" +require "active_model/dirty" require 'active_support/core_ext' require 'active_support/json' @@ -28,8 +29,11 @@ require "couchrest/model/persistence" require "couchrest/model/typecast" require "couchrest/model/property" require "couchrest/model/property_protection" -require "couchrest/model/casted_array" require "couchrest/model/properties" +require "couchrest/model/dirty" +require "couchrest/model/casted_array" +require "couchrest/model/casted_hash" +require "couchrest/model/casted_model" require "couchrest/model/validations" require "couchrest/model/callbacks" require "couchrest/model/document_queries" diff --git a/spec/couchrest/base_spec.rb b/spec/couchrest/base_spec.rb index d64641c..ed1ba30 100644 --- a/spec/couchrest/base_spec.rb +++ b/spec/couchrest/base_spec.rb @@ -372,6 +372,7 @@ describe "Model Base" do foundart.created_at.should == foundart.updated_at end it "should set the time on update" do + @art.title = "new title" # only saved if @art.changed? == true @art.save @art.created_at.should < @art.updated_at end diff --git a/spec/couchrest/dirty_spec.rb b/spec/couchrest/dirty_spec.rb new file mode 100644 index 0000000..9184edc --- /dev/null +++ b/spec/couchrest/dirty_spec.rb @@ -0,0 +1,167 @@ +require File.expand_path("../../spec_helper", __FILE__) + +require File.join(FIXTURE_PATH, 'more', 'cat') +require File.join(FIXTURE_PATH, 'more', 'article') +require File.join(FIXTURE_PATH, 'more', 'course') +require File.join(FIXTURE_PATH, 'more', 'card') +require File.join(FIXTURE_PATH, 'base') + +class WithCastedModelMixin < Hash + include CouchRest::Model::CastedModel + property :name + property :details, Object, :default => {} + property :casted_attribute, WithCastedModelMixin +end + +class DummyModel < CouchRest::Model::Base + use_database TEST_SERVER.default_database + raise "Default DB not set" if TEST_SERVER.default_database.nil? + property :casted_attribute, WithCastedModelMixin + property :details, Object, :default => {} + property :keywords, [String] + property :sub_models do |child| + child.property :title + end +end + + +describe "Dirty" do + + describe "changes" do + + it "should return changes on an attribute" do + @card = Card.new(:first_name => "matt") + @card.first_name = "andrew" + @card.changes.should == { "first_name" => ["matt", "andrew"] } + end + + end + + describe "save" do + + it "should not save unchanged records" do + card_id = Card.create!(:first_name => "matt").id + @card = Card.find(card_id) + @card.database.should_not_receive(:save_doc) + @card.save + end + + it "should save changed records" do + card_id = Card.create!(:first_name => "matt").id + @card = Card.find(card_id) + @card.first_name = "andrew" + @card.database.should_receive(:save_doc).and_return({"ok" => true}) + @card.save + end + + end + + describe "changed?" do + + # match activerecord behaviour + it "should report no changes on a new object with no attributes set" do + @card = Card.new + @card.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 objects fetched from the database" do + card_id = Card.create!(:first_name => "matt").id + @card = Card.find(card_id) + @card.changed?.should be_false + end + + it "should report changes if the record is modified" do + @card = Card.new + @card.first_name = "andrew" + @card.changed?.should be_true + @card.first_name_changed?.should be_true + end + + it "should report no changes for unmodified records" do + card_id = Card.create!(:first_name => "matt").id + @card = Card.find(card_id) + @card.first_name = "matt" + @card.changed?.should be_false + @card.first_name_changed?.should be_false + end + + it "should report no changes after a new record has been saved" do + @card = Card.new(:first_name => "matt") + @card.save! + @card.changed?.should be_false + end + + it "should report no changes after a record has been saved" do + card_id = Card.create!(:first_name => "matt").id + @card = Card.find(card_id) + @card.first_name = "andrew" + @card.save! + @card.changed?.should be_false + end + + # test changing list properties + + it "should report changes if a list property is modified" do + cat_id = Cat.create!(:name => "Felix", :toys => [{:name => "Mouse"}]).id + @cat = Cat.find(cat_id) + @cat.toys = [{:name => "Feather"}] + @cat.changed?.should be_true + end + + it "should report no changes if a list property is unmodified" do + cat_id = Cat.create!(:name => "Felix", :toys => [{:name => "Mouse"}]).id + @cat = Cat.find(cat_id) + @cat.toys = [{:name => "Mouse"}] # same as original list + @cat.changed?.should be_false + end + + # attachments + + it "should report changes if an attachment is added" do + cat_id = Cat.create!(:name => "Felix", :toys => [{:name => "Mouse"}]).id + @file = File.open(FIXTURE_PATH + '/attachments/test.html') + @cat = Cat.find(cat_id) + @cat.create_attachment(:file => @file, :name => "my_attachment") + @cat.changed?.should be_true + end + + it "should report changes if an attachment is deleted" do + @cat = Cat.create!(:name => "Felix", :toys => [{:name => "Mouse"}]) + @file = File.open(FIXTURE_PATH + '/attachments/test.html') + @attachment_name = "my_attachment" + @cat.create_attachment(:file => @file, :name => @attachment_name) + @cat.save + @cat = Cat.find(@cat.id) + @cat.delete_attachment(@attachment_name) + @cat.changed?.should be_true + end + + # casted models + + 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.changed?.should be_true + end + + it "should report changes to hashes" do + @obj = DummyModel.create! + @obj = DummyModel.get(@obj.id) + deets = @obj.details + deets['color'] = 'orange' + @obj.changed?.should be_true + end + + end + +end diff --git a/spec/fixtures/base.rb b/spec/fixtures/base.rb index 34f6778..2d7e31c 100644 --- a/spec/fixtures/base.rb +++ b/spec/fixtures/base.rb @@ -131,7 +131,7 @@ class WithUniqueValidationView < CouchRest::Model::Base attr_accessor :code unique_id :code def code - self["_id"] ||= @code + @code end property :title