From 6361d4234ca7f7c2dcb98e6ed34187d2933b56d7 Mon Sep 17 00:00:00 2001 From: Paul Gillard Date: Tue, 6 Oct 2009 16:25:51 -0500 Subject: Call initialize_copy when cloning [#3164 state:resolved] Cloned AR objects are now instantiated through initialize_copy rather than new/initialize. This allows AR classes to override initialize_copy in order to implement deep cloning. Signed-off-by: Joshua Peek --- activerecord/lib/active_record/base.rb | 65 ++++++++++++++++++++-------------- activerecord/test/cases/base_test.rb | 2 +- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb index 502fe0442e..150e3fc263 100755 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -2427,6 +2427,29 @@ module ActiveRecord #:nodoc: result end + # Cloned objects have no id assigned and are treated as new records. Note that this is a "shallow" clone + # as it copies the object's attributes only, not its associations. The extent of a "deep" clone is + # application specific and is therefore left to the application to implement according to its need. + def initialize_copy(other) + # Think the assertion which fails if the after_initialize callback goes at the end of the method is wrong. The + # deleted clone method called new which therefore called the after_initialize callback. It then went on to copy + # over the attributes. But if it's copying the attributes afterwards then it hasn't finished initializing right? + # For example in the test suite the topic model's after_initialize method sets the author_email_address to + # test@test.com. I would have thought this would mean that all cloned models would have an author email address + # of test@test.com. However the test_clone test method seems to test that this is not the case. As a result the + # after_initialize callback has to be run *before* the copying of the atrributes rather than afterwards in order + # for all tests to pass. This makes no sense to me. + callback(:after_initialize) if respond_to_without_attributes?(:after_initialize) + cloned_attributes = other.clone_attributes(:read_attribute_before_type_cast) + cloned_attributes.delete(self.class.primary_key) + @attributes = cloned_attributes + clear_aggregation_cache + @attributes_cache = {} + @new_record = true + ensure_proper_type + self.class.send(:scope, :create).each { |att, value| self.send("#{att}=", value) } if self.class.send(:scoped?, :create) + end + # Returns a String, which Action Pack uses for constructing an URL to this # object. The default implementation returns this record's id as a String, # or nil if this record's unsaved. @@ -2555,19 +2578,6 @@ module ActiveRecord #:nodoc: freeze end - # Returns a clone of the record that hasn't been assigned an id yet and - # is treated as a new record. Note that this is a "shallow" clone: - # it copies the object's attributes only, not its associations. - # The extent of a "deep" clone is application-specific and is therefore - # left to the application to implement according to its need. - def clone - attrs = clone_attributes(:read_attribute_before_type_cast) - attrs.delete(self.class.primary_key) - record = self.class.new - record.send :instance_variable_set, '@attributes', attrs - record - end - # Returns an instance of the specified +klass+ with the attributes of the current record. This is mostly useful in relation to # single-table inheritance structures where you want a subclass to appear as the superclass. This can be used along with record # identification in Action Pack to allow, say, Client < Company to do something like render :partial => @client.becomes(Company) @@ -2831,6 +2841,21 @@ module ActiveRecord #:nodoc: "#<#{self.class} #{attributes_as_nice_string}>" end + protected + def clone_attributes(reader_method = :read_attribute, attributes = {}) + self.attribute_names.inject(attributes) do |attrs, name| + attrs[name] = clone_attribute_value(reader_method, name) + attrs + end + end + + def clone_attribute_value(reader_method, attribute_name) + value = send(reader_method, attribute_name) + value.duplicable? ? value.clone : value + rescue TypeError, NoMethodError + value + end + private def create_or_update raise ReadOnlyRecord if readonly? @@ -3093,20 +3118,6 @@ module ActiveRecord #:nodoc: return string unless string.is_a?(String) && string =~ /^---/ YAML::load(string) rescue string end - - def clone_attributes(reader_method = :read_attribute, attributes = {}) - self.attribute_names.inject(attributes) do |attrs, name| - attrs[name] = clone_attribute_value(reader_method, name) - attrs - end - end - - def clone_attribute_value(reader_method, attribute_name) - value = send(reader_method, attribute_name) - value.duplicable? ? value.clone : value - rescue TypeError, NoMethodError - value - end end Base.class_eval do diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 8421a8fb07..1e2d903492 100755 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1352,7 +1352,7 @@ class BasicsTest < ActiveRecord::TestCase cloned_topic.title["a"] = "c" assert_equal "b", topic.title["a"] - #test if attributes set as part of after_initialize are cloned correctly + # test if attributes set as part of after_initialize are cloned correctly assert_equal topic.author_email_address, cloned_topic.author_email_address # test if saved clone object differs from original -- cgit v1.2.3 From e57197a9679d5d7a682fc73f12c9f04e067e85cc Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Tue, 6 Oct 2009 22:36:14 -1000 Subject: Fix warning spew --- actionpack/lib/action_view/base.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index ec1b07797b..01f33203ed 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -236,7 +236,9 @@ module ActionView #:nodoc: # they are in AC. if controller.class.respond_to?(:_helper_serial) klass = @views[controller.class._helper_serial] ||= Class.new(self) do - Subclasses.const_set(controller.class.name.gsub(/::/, '__'), self) + name = controller.class.name.gsub(/::/, '__') + Subclasses.remove_const(name) if Subclasses.const_defined?(name) + Subclasses.const_set(name, self) if controller.respond_to?(:_helpers) include controller._helpers -- cgit v1.2.3 From 3916f0340e8714d36a64162be793192849a9e51f Mon Sep 17 00:00:00 2001 From: Yehuda Katz Date: Wed, 7 Oct 2009 00:33:13 -1000 Subject: Not calling a private method anymore --- actionpack/lib/action_view/base.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb index 01f33203ed..664cc3b562 100644 --- a/actionpack/lib/action_view/base.rb +++ b/actionpack/lib/action_view/base.rb @@ -237,8 +237,11 @@ module ActionView #:nodoc: if controller.class.respond_to?(:_helper_serial) klass = @views[controller.class._helper_serial] ||= Class.new(self) do name = controller.class.name.gsub(/::/, '__') - Subclasses.remove_const(name) if Subclasses.const_defined?(name) - Subclasses.const_set(name, self) + + Subclasses.class_eval do + remove_const(name) if const_defined?(name) + const_set(name, self) + end if controller.respond_to?(:_helpers) include controller._helpers -- cgit v1.2.3 From f8e91bda9c171dd0c31dbea95ba4e9ced4ee2547 Mon Sep 17 00:00:00 2001 From: Sam Pohlenz Date: Wed, 7 Oct 2009 09:05:54 -0500 Subject: Don't share attribute matchers between classes [#3216 state:resolved] Allows separate models that include ActiveModel::AttributeMethods to use different sets of attribute matchers. Signed-off-by: Joshua Peek --- activemodel/lib/active_model/attribute_methods.rb | 5 ++++- activemodel/test/cases/attribute_methods_test.rb | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 activemodel/test/cases/attribute_methods_test.rb diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index aa35a2726e..26ee9c4315 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -1,3 +1,6 @@ +require 'active_support/core_ext/hash/keys' +require 'active_support/core_ext/class/inheritable_attributes' + module ActiveModel class MissingAttributeError < NoMethodError end @@ -219,7 +222,7 @@ module ActiveModel end def attribute_method_matchers #:nodoc: - @@attribute_method_matchers ||= [] + read_inheritable_attribute(:attribute_method_matchers) || write_inheritable_attribute(:attribute_method_matchers, []) end end diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb new file mode 100644 index 0000000000..614c4a3645 --- /dev/null +++ b/activemodel/test/cases/attribute_methods_test.rb @@ -0,0 +1,20 @@ +require 'cases/helper' + +class ModelWithAttributes + include ActiveModel::AttributeMethods + + attribute_method_suffix '' +end + +class ModelWithAttributes2 + include ActiveModel::AttributeMethods + + attribute_method_suffix '_test' +end + +class AttributeMethodsTest < ActiveModel::TestCase + test 'unrelated classes should not share attribute method matchers' do + assert_not_equal ModelWithAttributes.send(:attribute_method_matchers), + ModelWithAttributes2.send(:attribute_method_matchers) + end +end -- cgit v1.2.3 From 4df96338ede62da8ae8c188de5454b4b6b186ff8 Mon Sep 17 00:00:00 2001 From: Sam Pohlenz Date: Wed, 7 Oct 2009 09:06:54 -0500 Subject: Fixed behavior of attribute_methods_generated? [#3220 state:resolved] Signed-off-by: Joshua Peek --- activemodel/lib/active_model/attribute_methods.rb | 2 +- activemodel/test/cases/attribute_methods_test.rb | 26 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/activemodel/lib/active_model/attribute_methods.rb b/activemodel/lib/active_model/attribute_methods.rb index 26ee9c4315..977a101277 100644 --- a/activemodel/lib/active_model/attribute_methods.rb +++ b/activemodel/lib/active_model/attribute_methods.rb @@ -165,6 +165,7 @@ module ActiveModel end end end + @attribute_methods_generated = true end def undefine_attribute_methods @@ -176,7 +177,6 @@ module ActiveModel def generated_attribute_methods #:nodoc: @generated_attribute_methods ||= begin - @attribute_methods_generated = true mod = Module.new include mod mod diff --git a/activemodel/test/cases/attribute_methods_test.rb b/activemodel/test/cases/attribute_methods_test.rb index 614c4a3645..5659dcbc48 100644 --- a/activemodel/test/cases/attribute_methods_test.rb +++ b/activemodel/test/cases/attribute_methods_test.rb @@ -4,6 +4,15 @@ class ModelWithAttributes include ActiveModel::AttributeMethods attribute_method_suffix '' + + def attributes + { :foo => 'value of foo' } + end + +private + def attribute(name) + attributes[name.to_sym] + end end class ModelWithAttributes2 @@ -17,4 +26,21 @@ class AttributeMethodsTest < ActiveModel::TestCase assert_not_equal ModelWithAttributes.send(:attribute_method_matchers), ModelWithAttributes2.send(:attribute_method_matchers) end + + test '#define_attribute_methods generates attribute methods' do + ModelWithAttributes.define_attribute_methods([:foo]) + + assert ModelWithAttributes.attribute_methods_generated? + assert ModelWithAttributes.new.respond_to?(:foo) + assert_equal "value of foo", ModelWithAttributes.new.foo + end + + test '#undefine_attribute_methods removes attribute methods' do + ModelWithAttributes.define_attribute_methods([:foo]) + ModelWithAttributes.undefine_attribute_methods + + assert !ModelWithAttributes.attribute_methods_generated? + assert !ModelWithAttributes.new.respond_to?(:foo) + assert_raises(NoMethodError) { ModelWithAttributes.new.foo } + end end -- cgit v1.2.3 From ff56f3d5e1c69b923625f20a80f25c2eee3bbb35 Mon Sep 17 00:00:00 2001 From: Joshua Peek Date: Wed, 7 Oct 2009 09:24:51 -0500 Subject: Rewrite ActiveModel::Lint as a simple TU mixin --- activemodel/lib/active_model/lint.rb | 78 ++++++++++++++---------------------- activemodel/test/cases/lint_test.rb | 40 ++++-------------- 2 files changed, 38 insertions(+), 80 deletions(-) diff --git a/activemodel/lib/active_model/lint.rb b/activemodel/lib/active_model/lint.rb index ceaa29dc8c..1c2347adbf 100644 --- a/activemodel/lib/active_model/lint.rb +++ b/activemodel/lib/active_model/lint.rb @@ -1,6 +1,6 @@ # You can test whether an object is compliant with the ActiveModel API by -# calling ActiveModel::Lint.test(object). It will emit a Test::Unit -# output that tells you whether your object is fully compliant, or if not, +# including ActiveModel::Lint::Tests in your TestCase. It will included +# tests that tell you whether your object is fully compliant, or if not, # which aspects of the API are not implemented. # # These tests do not attempt to determine the semantic correctness of the @@ -12,36 +12,15 @@ # call to to_model. It is perfectly fine for to_model to return self. module ActiveModel module Lint - def self.test(object, verbosity = 2, output = STDOUT) - require "test/unit" - require "test/unit/ui/console/testrunner" - - test_class = Class.new(::Test::Unit::TestCase) do - include Test - - define_method(:setup) do - assert object.respond_to?(:to_model), "The object should respond_to :to_model" - @object = object.to_model - super - end - end - - ::Test::Unit::UI::Console::TestRunner.new(test_class, verbosity, output).start - end - - module Test - def assert_boolean(name, result) - assert result == true || result == false, "#{name} should be a boolean" - end - + module Tests # valid? # ------ # # Returns a boolean that specifies whether the object is in a valid or invalid # state. def test_valid? - assert @object.respond_to?(:valid?), "The model should respond to valid?" - assert_boolean "valid?", @object.valid? + assert model.respond_to?(:valid?), "The model should respond to valid?" + assert_boolean model.valid?, "valid?" end # new_record? @@ -53,13 +32,13 @@ module ActiveModel # collection. If it is persisted, a form for the object will put PUTed to the # URL for the object. def test_new_record? - assert @object.respond_to?(:new_record?), "The model should respond to new_record?" - assert_boolean "new_record?", @object.new_record? + assert model.respond_to?(:new_record?), "The model should respond to new_record?" + assert_boolean model.new_record?, "new_record?" end def test_destroyed? - assert @object.respond_to?(:destroyed?), "The model should respond to destroyed?" - assert_boolean "destroyed?", @object.destroyed? + assert model.respond_to?(:destroyed?), "The model should respond to destroyed?" + assert_boolean model.destroyed?, "destroyed?" end # errors @@ -67,29 +46,32 @@ module ActiveModel # # Returns an object that has :[] and :full_messages defined on it. See below # for more details. - def setup - assert @object.respond_to?(:errors), "The model should respond to errors" - @errors = @object.errors + + # Returns an Array of Strings that are the errors for the attribute in + # question. If localization is used, the Strings should be localized + # for the current locale. If no error is present, this method should + # return an empty Array. + def test_errors_aref + assert model.respond_to?(:errors), "The model should respond to errors" + assert model.errors[:hello].is_a?(Array), "errors#[] should return an Array" end - # This module tests the #errors object - module Errors - # Returns an Array of Strings that are the errors for the attribute in - # question. If localization is used, the Strings should be localized - # for the current locale. If no error is present, this method should - # return an empty Array. - def test_errors_aref - assert @errors[:hello].is_a?(Array), "errors#[] should return an Array" - end + # Returns an Array of all error messages for the object. Each message + # should contain information about the field, if applicable. + def test_errors_full_messages + assert model.respond_to?(:errors), "The model should respond to errors" + assert model.errors.full_messages.is_a?(Array), "errors#full_messages should return an Array" + end - # Returns an Array of all error messages for the object. Each message - # should contain information about the field, if applicable. - def test_errors_full_messages - assert @errors.full_messages.is_a?(Array), "errors#full_messages should return an Array" + private + def model + assert @model.respond_to?(:to_model), "The object should respond_to to_model" + @model.to_model end - end - include Errors + def assert_boolean(result, name) + assert result == true || result == false, "#{name} should be a boolean" + end end end end diff --git a/activemodel/test/cases/lint_test.rb b/activemodel/test/cases/lint_test.rb index ed576a91e2..da7d2112dc 100644 --- a/activemodel/test/cases/lint_test.rb +++ b/activemodel/test/cases/lint_test.rb @@ -1,15 +1,17 @@ require "cases/helper" -class TestLint < ActiveModel::TestCase - class CompliantObject +class LintTest < ActiveModel::TestCase + include ActiveModel::Lint::Tests + + class CompliantModel def to_model self end - + def valid?() true end def new_record?() true end def destroyed?() true end - + def errors obj = Object.new def obj.[](key) [] end @@ -17,34 +19,8 @@ class TestLint < ActiveModel::TestCase obj end end - - def assert_output(object, failures, errors, *test_names) - ActiveModel::Lint.test(object, 3, output = StringIO.new) - regex = %r{#{failures} failures, #{errors} errors} - assert_match regex, output.string - - test_names.each do |test_name| - assert_match test_name, output.string - end - end - - def test_valid - assert_output(CompliantObject.new, 0, 0, /test_valid/) - end - - def test_new_record - assert_output(CompliantObject.new, 0, 0, /test_new_record?/) - end - - def test_destroyed - assert_output(CompliantObject.new, 0, 0, /test_destroyed/) - end - - def test_errors_aref - assert_output(CompliantObject.new, 0, 0, /test_errors_aref/) - end - def test_errors_full_messages - assert_output(CompliantObject.new, 0, 0, /test_errors_aref/) + def setup + @model = CompliantModel.new end end -- cgit v1.2.3