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 <alex@tanda.co>

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 @@ module ActiveRecord
   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 @@ module ActiveRecord::Associations::Builder # :nodoc:
       }
 
       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 @@ module ActiveRecord
             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 @@ module ActiveRecord
         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 @@ module ActiveRecord
       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 @@ module ActiveRecord
     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 ActiveRecord
       #   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 @@ module ActiveRecord
         _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 @@ class BasicsTest < ActiveRecord::TestCase
   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")
-    post.reload
     assert_equal "cannot change this", post.title
+    assert_equal "changeable", post.body
 
-    post.update(title: "try to change", body: "changed")
+    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", post.body
+    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[:title] = "changed via []="
+    post[:body] = "changed via []="
+    post.save!
+    post.reload
+    assert_equal "cannot change this", post.title
+    assert_equal "changed via []=", post.body
   end
 
   def test_unicode_column_name

activerecord/test/cases/helper.rb

@@ -30,6 +30,8 @@ ARTest.connect
 # 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) &&

activerecord/test/cases/locking_test.rb

@@ -461,7 +461,9 @@ class OptimisticLockingTest < ActiveRecord::TestCase
     s.reload
     assert_equal "unchangeable name", s.name
 
-    s.update(name: "changed name")
+    assert_raises(ActiveRecord::ReadonlyAttributeError) do
+      s.update(name: "changed name")
+    end
     s.reload
     assert_equal "unchangeable name", s.name
   end

guides/source/configuring.md

@@ -65,6 +65,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_job.use_big_decimal_serializer`](#config-active-job-use-big-decimal-serializer): `true`
 - [`config.active_record.allow_deprecated_singular_associations_name`](#config-active-record-allow-deprecated-singular-associations-name): `false`
 - [`config.active_record.query_log_tags_format`](#config-active-record-query-log-tags-format): `:sqlcommenter`
+- [`config.active_record.raise_on_assign_to_attr_readonly`](#config-active-record-raise-on-assign-to-attr-readonly): `true`
 - [`config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction`](#config-active-record-run-commit-callbacks-on-first-saved-instances-in-transaction): `false`
 - [`config.active_record.sqlite3_adapter_strict_strings_by_default`](#config-active-record-sqlite3-adapter-strict-strings-by-default): `true`
 - [`config.active_support.default_message_encryptor_serializer`](#config-active-support-default-message-encryptor-serializer): `:json`
@@ -1095,6 +1096,17 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `false`              |
 | 7.0                   | `true`               |
 
+#### `config.active_record.raise_on_assign_to_attr_readonly`
+
+Enable raising on assignment to attr_readonly attributes. The previous
+behavior would allow assignment but silently not persist changes to the
+database.
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `false`              |
+| 7.1                   | `true`               |
+
 #### `config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction`
 
 When multiple Active Record instances change the same record within a transaction, Rails runs `after_commit` or `after_rollback` callbacks for only one of them. This option specifies how Rails chooses which instance receives the callbacks.

railties/lib/rails/application/configuration.rb

@@ -288,6 +288,7 @@ module Rails
             active_record.allow_deprecated_singular_associations_name = false
             active_record.sqlite3_adapter_strict_strings_by_default = true
             active_record.query_log_tags_format = :sqlcommenter
+            active_record.raise_on_assign_to_attr_readonly = true
           end
 
           if respond_to?(:action_dispatch)

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -107,3 +107,8 @@
 # if Rails.env.development? || Rails.env.test?
 #   Rails.application.config.log_file_size = 100 * 1024 * 1024
 # end
+
+# Enable raising on assignment to attr_readonly attributes. The previous
+# behavior would allow assignment but silently not persist changes to the
+# database.
+# Rails.application.config.active_record.raise_on_assign_to_attr_readonly