Fix #4046.

activerecord/lib/active_record/attribute_methods.rb

@@ -34,48 +34,26 @@ module ClassMethods
       # accessors, mutators and query methods.
       def define_attribute_methods
         return if attribute_methods_generated?
-
+        superclass.define_attribute_methods unless self == base_class
-        if base_class == self
+        super(column_names)
-          super(column_names)
+        @attribute_methods_generated = true
-          @attribute_methods_generated = true
-        else
-          base_class.define_attribute_methods
-        end
       end
 
       def attribute_methods_generated?
-        if base_class == self
+        @attribute_methods_generated ||= false
-          @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)
       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
+        @generated_external_attribute_methods ||= Module.new { extend self }
-          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
       end
 
       def undefine_attribute_methods
-        if base_class == self
+        super
-          super
+        @attribute_methods_generated = false
-          @attribute_methods_generated = false
-        else
-          base_class.undefine_attribute_methods
-        end
       end
 
       def instance_method_already_implemented?(method_name)
@@ -83,19 +61,32 @@ def instance_method_already_implemented?(method_name)
           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)
+      def dangerous_attribute_method?(name)
-        active_record = ActiveRecord::Base
+        method_defined_within?(name, Base)
-        superclass    = ActiveRecord::Base.superclass
+      end
 
-        (active_record.method_defined?(method_name) ||
+      # Note that we could do this via klass.instance_methods(false), but this would require us
-         active_record.private_method_defined?(method_name)) &&
+      # to maintain a cached Set (for speed) and invalidate it at the correct time, which would
-        !superclass.method_defined?(method_name) &&
+      # be a pain. This implementation is also O(1) while avoiding maintaining a cached Set.
-        !superclass.private_method_defined?(method_name)
+      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)

activerecord/lib/active_record/attribute_methods/read.rb

@@ -30,10 +30,8 @@ def cache_attribute?(attr_name)
         end
 
         def undefine_attribute_methods
-          if base_class == self
+          generated_external_attribute_methods.module_eval do
-            generated_external_attribute_methods.module_eval do
+            instance_methods.each { |m| undef_method(m) }
-              instance_methods.each { |m| undef_method(m) }
-            end
           end
 
           super
@@ -80,6 +78,7 @@ def __temp__
             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

activerecord/test/cases/attribute_methods/read_test.rb

@@ -14,6 +14,7 @@ def type; :integer; end
 
       def setup
         @klass = Class.new do
+          def self.superclass; Base; end
           def self.base_class; self; end
 
           include ActiveRecord::AttributeMethods

activerecord/test/cases/attribute_methods_test.rb

@@ -711,6 +711,26 @@ def test_read_attribute_with_nil_should_not_asplode
     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)

activerecord/test/cases/base_test.rb

@@ -1251,6 +1251,7 @@ def test_serialized_attribute_declared_in_subclass
 
     important_topic.reload
     assert_equal(hash, important_topic.important)
+    assert_equal(hash, important_topic.read_attribute(:important))
   end
 
   def test_serialized_time_attribute