diff options
5 files changed, 58 insertions, 46 deletions
diff --git a/activerecord/lib/active_record/attribute_methods.rb b/activerecord/lib/active_record/attribute_methods.rb index 131ef09f57..237343c252 100644 --- a/activerecord/lib/active_record/attribute_methods.rb +++ b/activerecord/lib/active_record/attribute_methods.rb @@ -34,48 +34,26 @@ module ActiveRecord # accessors, mutators and query methods. def define_attribute_methods return if attribute_methods_generated? - - if base_class == self - super(column_names) - @attribute_methods_generated = true - else - base_class.define_attribute_methods - end + superclass.define_attribute_methods unless self == base_class + super(column_names) + @attribute_methods_generated = true end def attribute_methods_generated? - if base_class == self - @attribute_methods_generated ||= false - else - base_class.attribute_methods_generated? - end - end - - def generated_attribute_methods - @generated_attribute_methods ||= (base_class == self ? super : base_class.generated_attribute_methods) + @attribute_methods_generated ||= false end + # We will define the methods as instance methods, but will call them as singleton + # methods. This allows us to use method_defined? to check if the method exists, + # which is fast and won't give any false positives from the ancestors (because + # there are no ancestors). def generated_external_attribute_methods - @generated_external_attribute_methods ||= begin - if base_class == self - # We will define the methods as instance methods, but will call them as singleton - # methods. This allows us to use method_defined? to check if the method exists, - # which is fast and won't give any false positives from the ancestors (because - # there are no ancestors). - Module.new { extend self } - else - base_class.generated_external_attribute_methods - end - end + @generated_external_attribute_methods ||= Module.new { extend self } end def undefine_attribute_methods - if base_class == self - super - @attribute_methods_generated = false - else - base_class.undefine_attribute_methods - end + super + @attribute_methods_generated = false end def instance_method_already_implemented?(method_name) @@ -83,19 +61,32 @@ module ActiveRecord raise DangerousAttributeError, "#{method_name} is defined by ActiveRecord" end - super + if superclass == Base + super + else + method_defined_within?(method_name, superclass, superclass.generated_attribute_methods) || super + end end # A method name is 'dangerous' if it is already defined by Active Record, but # not by any ancestors. (So 'puts' is not dangerous but 'save' is.) - def dangerous_attribute_method?(method_name) - active_record = ActiveRecord::Base - superclass = ActiveRecord::Base.superclass - - (active_record.method_defined?(method_name) || - active_record.private_method_defined?(method_name)) && - !superclass.method_defined?(method_name) && - !superclass.private_method_defined?(method_name) + def dangerous_attribute_method?(name) + method_defined_within?(name, Base) + end + + # Note that we could do this via klass.instance_methods(false), but this would require us + # to maintain a cached Set (for speed) and invalidate it at the correct time, which would + # be a pain. This implementation is also O(1) while avoiding maintaining a cached Set. + def method_defined_within?(name, klass, sup = klass.superclass) + if klass.method_defined?(name) || klass.private_method_defined?(name) + if sup.method_defined?(name) || sup.private_method_defined?(name) + klass.instance_method(name).owner != sup.instance_method(name).owner + else + true + end + else + false + end end def attribute_method?(attribute) diff --git a/activerecord/lib/active_record/attribute_methods/read.rb b/activerecord/lib/active_record/attribute_methods/read.rb index a83d944d1b..995e2b2e58 100644 --- a/activerecord/lib/active_record/attribute_methods/read.rb +++ b/activerecord/lib/active_record/attribute_methods/read.rb @@ -30,10 +30,8 @@ module ActiveRecord end def undefine_attribute_methods - if base_class == self - generated_external_attribute_methods.module_eval do - instance_methods.each { |m| undef_method(m) } - end + generated_external_attribute_methods.module_eval do + instance_methods.each { |m| undef_method(m) } end super @@ -80,6 +78,7 @@ module ActiveRecord STR generated_external_attribute_methods.module_eval <<-STR, __FILE__, __LINE__ + 1 + # raise if method_defined?('#{attr_name}') def __temp__(v, attributes, attributes_cache, attr_name) #{external_attribute_access_code(attr_name, cast_code)} end diff --git a/activerecord/test/cases/attribute_methods/read_test.rb b/activerecord/test/cases/attribute_methods/read_test.rb index 86a240d93c..7665f1c12e 100644 --- a/activerecord/test/cases/attribute_methods/read_test.rb +++ b/activerecord/test/cases/attribute_methods/read_test.rb @@ -14,6 +14,7 @@ module ActiveRecord def setup @klass = Class.new do + def self.superclass; Base; end def self.base_class; self; end include ActiveRecord::AttributeMethods diff --git a/activerecord/test/cases/attribute_methods_test.rb b/activerecord/test/cases/attribute_methods_test.rb index 8e509a9792..b6cf26f978 100644 --- a/activerecord/test/cases/attribute_methods_test.rb +++ b/activerecord/test/cases/attribute_methods_test.rb @@ -711,6 +711,26 @@ class AttributeMethodsTest < ActiveRecord::TestCase assert_equal nil, Topic.new.read_attribute(nil) end + # If B < A, and A defines an accessor for 'foo', we don't want to override + # that by defining a 'foo' method in the generated methods module for B. + # (That module will be inserted between the two, e.g. [B, <GeneratedAttributes>, A].) + def test_inherited_custom_accessors + klass = Class.new(ActiveRecord::Base) do + self.table_name = "topics" + self.abstract_class = true + def title; "omg"; end + def title=(val); self.author_name = val; end + end + subklass = Class.new(klass) + [klass, subklass].each(&:define_attribute_methods) + + topic = subklass.find(1) + assert_equal "omg", topic.title + + topic.title = "lol" + assert_equal "lol", topic.author_name + end + private def cached_columns @cached_columns ||= time_related_columns_on_topic.map(&:name) diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 433b586971..6ff0c1355c 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -1251,6 +1251,7 @@ class BasicsTest < ActiveRecord::TestCase important_topic.reload assert_equal(hash, important_topic.important) + assert_equal(hash, important_topic.read_attribute(:important)) end def test_serialized_time_attribute |