Fix undefined attribute method with attr_readonly

The problem is where an abstract class defines a readonly attribute.
When another class subclasses that parent with
`raise_on_assign_to_attr_readonly`, it will define the readonly method
before the abstract parent has defined its attributes. As a result, when
those methods are defined, they will not define the readonly attribute
writer, so that `super` called from the readonly accessor will fail with
a `NoMethodError`.

Instead of trying to make the overriden attribute method lazier by
defining it after the real attribute method has been defined, this
commit changes the method override to be one layer down on
_write_attribute. This avoids the issue of the attribute method being
undefined because the real _write_attribute will always be defined.

Co-authored-by: Adrianna Chang <[email protected]>
Co-authored-by: Chris Salzberg <[email protected]>

Author: 3 people

activerecord/lib/active_record/associations/collection_association.rb

@@ -324,7 +324,7 @@ def merge_target_lists(persisted, memory)
           persisted.map! do |record|
             if mem_record = memory.delete(record)
 
-              ((record.attribute_names & mem_record.attribute_names) - mem_record.changed_attribute_names_to_save).each do |name|
+              ((record.attribute_names & mem_record.attribute_names) - mem_record.changed_attribute_names_to_save - mem_record.class._attr_readonly).each do |name|
                 mem_record._write_attribute(name, record[name])
               end
 

activerecord/lib/active_record/readonly_attributes.rb

@@ -28,21 +28,11 @@ module ClassMethods
       #   post.title = "a different title" # raises ActiveRecord::ReadonlyAttributeError
       #   post.update(title: "a different title") # raises ActiveRecord::ReadonlyAttributeError
       def attr_readonly(*attributes)
-        new_attributes = attributes.map(&:to_s).reject { |a| _attr_readonly.include?(a) }
+        self._attr_readonly |= attributes.map(&:to_s)
 
         if ActiveRecord.raise_on_assign_to_attr_readonly
-          new_attributes.each do |attribute|
-            define_method("#{attribute}=") do |value|
-              raise ReadonlyAttributeError.new(attribute) unless new_record?
-
-              super(value)
-            end
-          end
-
           include(HasReadonlyAttributes)
         end
-
-        self._attr_readonly = Set.new(new_attributes) + _attr_readonly
       end
 
       # Returns an array of all the attributes that have been specified as readonly.
@@ -56,12 +46,14 @@ def readonly_attribute?(name) # :nodoc:
     end
 
     module HasReadonlyAttributes # :nodoc:
-      def write_attribute(attr_name, value)
-        if !new_record? && self.class.readonly_attribute?(attr_name.to_s)
-          raise ReadonlyAttributeError.new(attr_name)
-        end
+      [:write_attribute, :_write_attribute].each do |name|
+        define_method(name) do |attr_name, value|
+          if !new_record? && self.class.readonly_attribute?(attr_name.to_s)
+            raise ReadonlyAttributeError.new(attr_name)
+          end
 
-        super
+          super(attr_name, value)
+        end
       end
     end
   end

activerecord/test/cases/base_test.rb

@@ -66,6 +66,16 @@ class ReadonlyTitlePost < Post
   attr_readonly :title
 end
 
+class ReadonlyTitleAbstractPost < ActiveRecord::Base
+  self.abstract_class = true
+
+  attr_readonly :title
+end
+
+class ReadonlyTitlePostWithAbstractParent < ReadonlyTitleAbstractPost
+  self.table_name = "posts"
+end
+
 previous_value, ActiveRecord.raise_on_assign_to_attr_readonly = ActiveRecord.raise_on_assign_to_attr_readonly, false
 
 class NonRaisingPost < Post
@@ -699,7 +709,7 @@ def test_comparison_with_different_objects_in_array
   end
 
   def test_readonly_attributes
-    assert_equal Set.new([ "title" ]), ReadonlyTitlePost.readonly_attributes
+    assert_equal [ "title" ], ReadonlyTitlePost.readonly_attributes
 
     post = ReadonlyTitlePost.create(title: "cannot change this", body: "changeable")
     assert_equal "cannot change this", post.title
@@ -750,7 +760,7 @@ def test_readonly_attributes
   end
 
   def test_readonly_attributes_on_a_new_record
-    assert_equal Set.new([ "title" ]), ReadonlyTitlePost.readonly_attributes
+    assert_equal [ "title" ], ReadonlyTitlePost.readonly_attributes
 
     post = ReadonlyTitlePost.new(title: "can change this until you save", body: "changeable")
     assert_equal "can change this until you save", post.title
@@ -766,7 +776,6 @@ def test_readonly_attributes_on_a_new_record
     assert_equal "changed via write_attribute", post.title
     assert_equal "changed via write_attribute", post.body
 
-
     post.assign_attributes(body: "changed via assign_attributes", title: "changed via assign_attributes")
     assert_equal "changed via assign_attributes", post.title
     assert_equal "changed via assign_attributes", post.body
@@ -783,8 +792,16 @@ def test_readonly_attributes_on_a_new_record
     assert_equal "changed via []=", post.body
   end
 
+  def test_readonly_attributes_in_abstract_class_descendant
+    assert_equal [ "title" ], ReadonlyTitlePostWithAbstractParent.readonly_attributes
+
+    assert_nothing_raised do
+      ReadonlyTitlePostWithAbstractParent.new(title: "can change this until you save")
+    end
+  end
+
   def test_readonly_attributes_when_configured_to_not_raise
-    assert_equal Set.new([ "title" ]), NonRaisingPost.readonly_attributes
+    assert_equal [ "title" ], NonRaisingPost.readonly_attributes
 
     post = NonRaisingPost.create(title: "cannot change this", body: "changeable")
     assert_equal "cannot change this", post.title

activerecord/test/cases/locking_test.rb

@@ -455,7 +455,7 @@ def test_lock_with_custom_column_without_default_queries_count
   end
 
   def test_readonly_attributes
-    assert_equal Set.new([ "name" ]), ReadonlyNameShip.readonly_attributes
+    assert_equal [ "name" ], ReadonlyNameShip.readonly_attributes
 
     s = ReadonlyNameShip.create(name: "unchangeable name")
     s.reload