Don't crash when mutating attributes in a getter

If a getter has side effects on the DB, `changes_applied` will be called
twice. The second time will try and remove the changed attributes cache,
and will crash because it's already been unset. This also demonstrates
that we shouldn't assume that calling getters won't change the value of
`changed_attributes`, and we need to clear the cache if an attribute is
modified.

Fixes #20531.

3 files changed, 29 insertions, 1 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 14906e441b..d389d7f6fe 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Fixed an error which would occur in dirty checking when calling
+    `update_attributes` from a getter.
+
+    Fixes #20531.
+
+    *Sean Griffin*
+
 *   Make `remove_foreign_key` reversible. Any foreign key options must be
     specified, similar to `remove_column`.
 
diff --git a/activerecord/lib/active_record/attribute_methods/dirty.rb b/activerecord/lib/active_record/attribute_methods/dirty.rb
index 7ba907f786..0171ef3bdf 100644
--- a/activerecord/lib/active_record/attribute_methods/dirty.rb
+++ b/activerecord/lib/active_record/attribute_methods/dirty.rb
@@ -108,6 +108,7 @@ module ActiveRecord
       end
 
       def save_changed_attribute(attr, old_value)
+        clear_changed_attributes_cache
         if attribute_changed_by_setter?(attr)
           clear_attribute_changes(attr) unless _field_changed?(attr, old_value)
         else
@@ -176,7 +177,11 @@ module ActiveRecord
         @cached_changed_attributes = changed_attributes
         yield
       ensure
-        remove_instance_variable(:@cached_changed_attributes)
+        clear_changed_attributes_cache
+      end
+
+      def clear_changed_attributes_cache
+        remove_instance_variable(:@cached_changed_attributes) if defined?(@cached_changed_attributes)
       end
     end
   end
diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb
index 216f228142..f5aaf22e13 100644
--- a/activerecord/test/cases/dirty_test.rb
+++ b/activerecord/test/cases/dirty_test.rb
@@ -703,6 +703,22 @@ class DirtyTest < ActiveRecord::TestCase
     assert pirate.catchphrase_changed?(from: "arrrr", to: "arrrr matey!")
   end
 
+  test "getters with side effects are allowed" do
+    klass = Class.new(Pirate) do
+      def catchphrase
+        if super.blank?
+          update_attribute(:catchphrase, "arr") # what could possibly go wrong?
+        end
+        super
+      end
+    end
+
+    pirate = klass.create!(catchphrase: "lol")
+    pirate.update_attribute(:catchphrase, nil)
+
+    assert_equal "arr", pirate.catchphrase
+  end
+
   private
     def with_partial_writes(klass, on = true)
       old = klass.partial_writes?