Skip to content

Commit

Permalink
Don't crash when mutating attributes in a getter
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
sgrif committed Jun 12, 2015
1 parent 8beb328 commit 07b4078
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 1 deletion.
7 changes: 7 additions & 0 deletions 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`.

Expand Down
7 changes: 6 additions & 1 deletion activerecord/lib/active_record/attribute_methods/dirty.rb
Expand Up @@ -108,6 +108,7 @@ def raw_write_attribute(attr, value)
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
Expand Down Expand Up @@ -176,7 +177,11 @@ def cache_changed_attributes
@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
Expand Down
16 changes: 16 additions & 0 deletions activerecord/test/cases/dirty_test.rb
Expand Up @@ -703,6 +703,22 @@ def test_datetime_attribute_doesnt_change_if_zone_is_modified_in_string
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?
Expand Down

0 comments on commit 07b4078

Please sign in to comment.