Raise on assignment to readonly attributes

Previously, assignment would succeed but silently not write to the
database.

The changes to counter_cache are necessary because incrementing the
counter cache for a column calls []=. I investigated an approach to use
_write_attribute instead, however counter caches are expected to resolve
attribute aliases so write_attribute/[]= seems more correct.

Similarly, []= was replaced with _write_attribute in merge_target_lists
to skip the overriden []= and the primary key check. attribute_names
will already return custom primary keys so the primary_key check in
write_attribute is not needed.

Co-authored-by: Alex Ghiculescu <[email protected]>

Author: hmcguire-shopify

activerecord/CHANGELOG.md

@@ -1,3 +1,26 @@
+*   Raise on assignment to readonly attributes
+
+    ```ruby
+    class Post < ActiveRecord::Base
+      attr_readonly :content
+    end
+    Post.create!(content: "cannot be updated")
+    post.content # "cannot be updated"
+    post.content = "something else" # => ActiveRecord::ReadonlyAttributeError
+    ```
+
+    Previously, assignment would succeed but silently not write to the database.
+
+    This behavior can be controlled by configuration:
+
+    ```ruby
+    config.active_record.raise_on_assign_to_attr_readonly = true
+    ```
+
+    and will be enabled by default with `load_defaults 7.1`
+
+    *Alex Ghiculescu*, *Hartley McGuire*
+
 *   Allow unscoping of preload and eager_load associations
 
     Added the ability to unscope preload and eager_load associations just like

activerecord/lib/active_record.rb

@@ -269,6 +269,9 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :maintain_test_schema
   self.maintain_test_schema = nil
 
+  singleton_class.attr_accessor :raise_on_assign_to_attr_readonly
+  self.raise_on_assign_to_attr_readonly = false
+
   ##
   # :singleton-method:
   # Specify a threshold for the size of query result sets. If the number of

activerecord/lib/active_record/associations/builder/belongs_to.rb

@@ -37,7 +37,7 @@ def self.add_counter_cache_callbacks(model, reflection)
       }
 
       klass = reflection.class_name.safe_constantize
-      klass.attr_readonly cache_column if klass && klass.respond_to?(:attr_readonly)
+      klass._counter_cache_columns << cache_column if klass && klass.respond_to?(:_counter_cache_columns)
     end
 
     def self.touch_record(o, changes, foreign_key, name, touch, touch_method) # :nodoc:

activerecord/lib/active_record/associations/collection_association.rb

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

activerecord/lib/active_record/attribute_methods.rb

@@ -396,6 +396,7 @@ def attributes_for_update(attribute_names)
         attribute_names &= self.class.column_names
         attribute_names.delete_if do |name|
           self.class.readonly_attribute?(name) ||
+            self.class.counter_cache_column?(name) ||
             column_for_attribute(name).virtual?
         end
       end

activerecord/lib/active_record/counter_cache.rb

@@ -5,6 +5,10 @@ module ActiveRecord
   module CounterCache
     extend ActiveSupport::Concern
 
+    included do
+      class_attribute :_counter_cache_columns, instance_accessor: false, default: []
+    end
+
     module ClassMethods
       # Resets one or more counter caches to their correct value using an SQL
       # count query. This is useful when adding new counter caches, or if the
@@ -162,6 +166,10 @@ def increment_counter(counter_name, id, touch: nil)
       def decrement_counter(counter_name, id, touch: nil)
         update_counters(id, counter_name => -1, touch: touch)
       end
+
+      def counter_cache_column?(name) # :nodoc:
+        _counter_cache_columns.include?(name)
+      end
     end
 
     private

activerecord/lib/active_record/railtie.rb

@@ -36,6 +36,7 @@ class Railtie < Rails::Railtie # :nodoc:
     config.active_record.query_log_tags = [ :application ]
     config.active_record.query_log_tags_format = :legacy
     config.active_record.cache_query_log_tags = false
+    config.active_record.raise_on_assign_to_attr_readonly = false
 
     config.active_record.queues = ActiveSupport::InheritableOptions.new
 

activerecord/lib/active_record/readonly_attributes.rb

@@ -1,6 +1,9 @@
 # frozen_string_literal: true
 
 module ActiveRecord
+  class ReadonlyAttributeError < ActiveRecordError
+  end
+
   module ReadonlyAttributes
     extend ActiveSupport::Concern
 
@@ -23,7 +26,21 @@ module ClassMethods
       #   post = Post.create!(title: "Introducing Ruby on Rails!")
       #   post.update(title: "a different title") # change to title will be ignored
       def attr_readonly(*attributes)
-        self._attr_readonly = Set.new(attributes.map(&:to_s)) + (_attr_readonly || [])
+        new_attributes = attributes.map(&:to_s).reject { |a| _attr_readonly.include?(a) }
+
+        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.
@@ -35,5 +52,15 @@ def readonly_attribute?(name) # :nodoc:
         _attr_readonly.include?(name)
       end
     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
+
+        super
+      end
+    end
   end
 end

activerecord/test/cases/base_test.rb

@@ -66,6 +66,14 @@ class ReadonlyTitlePost < Post
   attr_readonly :title
 end
 
+previous_value, ActiveRecord.raise_on_assign_to_attr_readonly = ActiveRecord.raise_on_assign_to_attr_readonly, false
+
+class NonRaisingPost < Post
+  attr_readonly :title
+end
+
+ActiveRecord.raise_on_assign_to_attr_readonly = previous_value
+
 class Weird < ActiveRecord::Base; end
 
 class LintTest < ActiveRecord::TestCase
@@ -691,16 +699,132 @@ def test_comparison_with_different_objects_in_array
   end
 
   def test_readonly_attributes
-    assert_equal Set.new([ "title", "comments_count" ]), ReadonlyTitlePost.readonly_attributes
+    assert_equal Set.new([ "title" ]), ReadonlyTitlePost.readonly_attributes
 
     post = ReadonlyTitlePost.create(title: "cannot change this", body: "changeable")
+    assert_equal "cannot change this", post.title
+    assert_equal "changeable", post.body
+
+    post = Post.find(post.id)
+    assert_equal "cannot change this", post.title
+    assert_equal "changeable", post.body
+
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      post.title = "changed via assignment"
+    end
+    post.body = "changed via assignment"
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via assignment", post.body
+
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      post.write_attribute(:title, "changed via write_attribute")
+    end
+    post.write_attribute(:body, "changed via write_attribute")
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via write_attribute", post.body
+
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      post.assign_attributes(body: "changed via assign_attributes", title: "changed via assign_attributes")
+    end
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via assign_attributes", post.body
+
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      post.update(title: "changed via update", body: "changed via update")
+    end
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via assign_attributes", post.body
+
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      post[:title] = "changed via []="
+    end
+    post[:body] = "changed via []="
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via []=", post.body
+
+    post.save!
+
+    post = Post.find(post.id)
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via []=", post.body
+  end
+
+  def test_readonly_attributes_on_a_new_record
+    assert_equal Set.new([ "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
+    assert_equal "changeable", post.body
+
+    post.title = "changed via assignment"
+    post.body = "changed via assignment"
+    assert_equal "changed via assignment", post.title
+    assert_equal "changed via assignment", post.body
+
+    post.write_attribute(:title, "changed via write_attribute")
+    post.write_attribute(:body, "changed via write_attribute")
+    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
+
+    post[:title] = "changed via []="
+    post[:body] = "changed via []="
+    assert_equal "changed via []=", post.title
+    assert_equal "changed via []=", post.body
+
+    post.save!
+
+    post = Post.find(post.id)
+    assert_equal "changed via []=", post.title
+    assert_equal "changed via []=", post.body
+  end
+
+  def test_readonly_attributes_when_configured_to_not_raise
+    assert_equal Set.new([ "title" ]), NonRaisingPost.readonly_attributes
+
+    post = NonRaisingPost.create(title: "cannot change this", body: "changeable")
+    assert_equal "cannot change this", post.title
+    assert_equal "changeable", post.body
+
+    post = Post.find(post.id)
+    assert_equal "cannot change this", post.title
+    assert_equal "changeable", post.body
+
+    post.title = "changed via assignment"
+    post.body = "changed via assignment"
+    post.save!
+    post.reload
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via assignment", post.body
+
+    post.write_attribute(:title, "changed via write_attribute")
+    post.write_attribute(:body, "changed via write_attribute")
+    post.save!
+    post.reload
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via write_attribute", post.body
+
+    post.assign_attributes(body: "changed via assign_attributes", title: "changed via assign_attributes")
+    post.save!
+    post.reload
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via assign_attributes", post.body
+
+    post.update(title: "changed via update", body: "changed via update")
     post.reload
     assert_equal "cannot change this", post.title
+    assert_equal "changed via update", post.body
 
-    post.update(title: "try to change", body: "changed")
+    post[:title] = "changed via []="
+    post[:body] = "changed via []="
+    post.save!
     post.reload
     assert_equal "cannot change this", post.title
-    assert_equal "changed", post.body
+    assert_equal "changed via []=", post.body
   end
 
   def test_unicode_column_name

activerecord/test/cases/helper.rb

@@ -30,6 +30,8 @@
 # Quote "type" if it's a reserved word for the current connection.
 QUOTED_TYPE = ActiveRecord::Base.connection.quote_column_name("type")
 
+ActiveRecord.raise_on_assign_to_attr_readonly = true
+
 def current_adapter?(*types)
   types.any? do |type|
     ActiveRecord::ConnectionAdapters.const_defined?(type) &&