mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
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.
This commit is contained in:
parent
8beb328bef
commit
07b4078eb6
3 changed files with 29 additions and 1 deletions
|
@ -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
|
* Make `remove_foreign_key` reversible. Any foreign key options must be
|
||||||
specified, similar to `remove_column`.
|
specified, similar to `remove_column`.
|
||||||
|
|
||||||
|
|
|
@ -108,6 +108,7 @@ module ActiveRecord
|
||||||
end
|
end
|
||||||
|
|
||||||
def save_changed_attribute(attr, old_value)
|
def save_changed_attribute(attr, old_value)
|
||||||
|
clear_changed_attributes_cache
|
||||||
if attribute_changed_by_setter?(attr)
|
if attribute_changed_by_setter?(attr)
|
||||||
clear_attribute_changes(attr) unless _field_changed?(attr, old_value)
|
clear_attribute_changes(attr) unless _field_changed?(attr, old_value)
|
||||||
else
|
else
|
||||||
|
@ -176,7 +177,11 @@ module ActiveRecord
|
||||||
@cached_changed_attributes = changed_attributes
|
@cached_changed_attributes = changed_attributes
|
||||||
yield
|
yield
|
||||||
ensure
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -703,6 +703,22 @@ class DirtyTest < ActiveRecord::TestCase
|
||||||
assert pirate.catchphrase_changed?(from: "arrrr", to: "arrrr matey!")
|
assert pirate.catchphrase_changed?(from: "arrrr", to: "arrrr matey!")
|
||||||
end
|
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
|
private
|
||||||
def with_partial_writes(klass, on = true)
|
def with_partial_writes(klass, on = true)
|
||||||
old = klass.partial_writes?
|
old = klass.partial_writes?
|
||||||
|
|
Loading…
Reference in a new issue