Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix serialized attributes always being saved #14333

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
7 participants

quainjn commented Mar 9, 2014

Fixes #8328. Models with serialized attributes will only be saved if the record has changed.

Uses #hash to compare original serialize attribute value to current value. Similar to #13428

Chose to use #hash for comparison for performance reasons. The downside of using #hash rather than #deep_dup is that serialized_attribute_was will not be stored, and will return nil.

Member

senny commented Mar 10, 2014

@kbrock kbrock commented on the diff Apr 11, 2014

activerecord/test/cases/dirty_test.rb
topic.content[:hello] = 'world'
topic.save!
assert_not_equal updated_at, topic.updated_at
assert_equal 'world', topic.content[:hello]
+
+ topic.update_column(:updated_at, updated_at)
@kbrock

kbrock Apr 11, 2014

Contributor

I think explicitly setting updated_at will force topic.changed? == true, and changes the test significantly.

Won't the existing behavior pass this test, too.

@quainjn

quainjn Apr 11, 2014

Using #update_column to set the updated_at field prevents topic.changed? from being true automatically, which is why the updated_at column does not change when #save! is called.

@kbrock kbrock commented on the diff Apr 11, 2014

activerecord/test/cases/store_test.rb
@@ -48,7 +48,7 @@ class StoreTest < ActiveRecord::TestCase
test "updating the store populates the changed array correctly" do
@john.color = 'red'
- assert_equal 'black', @john.settings_change[0]['color']
+ assert_nil @john.settings_change[0], "original value of stored attributes should be nil"
@kbrock

kbrock Apr 11, 2014

Contributor

I think I missed something.
I expect assert_equal ['black', 'red'], @john.settings_changed

@quainjn

quainjn Apr 11, 2014

For performance reasons, the original value of the changed serialized attributes is not being saved, and will always return nil.

👍 would like to see this kind of behavior. Not only is it bad to perform unnecessary database transactions, but this can also cause race conditions. For example, consider 2 background jobs that work on the same resource. One of the jobs consists of altering the serialized field's data, while the other focusses on other fields. Now the initial worker finishes and commits the new serialized field data. Now the other worker finishes and commits its fields, but also commits the clean state of the serialized field, basically overwriting all the work the other worker did. Now data is lost.

I tried the PR and it seems to solve this particular issue.

Member

sgrif commented May 31, 2014

Thank you for working on this long standing problem. However, the problem is more general than this, and applies to JSON and HStore columns on PostgreSQL as well.

Owner

rafaelfranca commented Jun 13, 2014

Fixed by #15674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment