Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Compare unserialized value when assigning serialized attributes to changed_attributes #12717

Closed
wants to merge 2 commits into from

5 participants

@batter

This is my attempt at a fix for #12505. Not certain that I put the test in the proper location, but it seemed like a logical spot.

activerecord/lib/active_record/core.rb
@@ -429,7 +429,10 @@ def init_changed_attributes
# optimistic locking) won't get written unless they get marked as changed
self.class.columns.each do |c|
attr, orig_value = c.name, c.default
- changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
+ # if the attribute is marked with `serialize`, need to compare the unserialized value to the original value
+ new_value = @attributes[attr].is_a?(ActiveRecord::AttributeMethods::Serialization::Attribute) ?
+ @attributes[attr].unserialized_value : @attributes[attr]
@dmathieu Collaborator

Could you change this to avoid having a one-liner condition?

@batter
batter added a note

Technically I did split it up into two lines :wink: but yea sure, that will make for improved readability.

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

I've been looking into some of the failures on this branch this morning. You definitely need to rebase this against upstream/master. There are definitely some problems still, but I'm not quite sure where yet.

@batter

@jherdman - I rebased before making the PR in the first place. Happy to do it again, or someone else can when they go to merge it. Regardless I'm not 100% sure this fixes everything but it appeared to fix the issue that I was seeing. Yes there were some errors in the test suite, but they were already there before I started work on this improvement.

@jherdman

Yeah, I'm fairly convinced your changes have nothing to do with the errors.

@rafaelfranca rafaelfranca modified the milestone: 4.0.5, 4.0.4, 4.0.6
@rafaelfranca

Closed by #15674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 31, 2013
  1. @batter

    Compare unserialized value to original value when assigning changed_a…

    batter authored
    …ttributes for a serialized attribute. fix #12505
  2. @batter
This page is out of date. Refresh to see the latest.
View
21 activerecord/CHANGELOG.md
@@ -1,3 +1,24 @@
+* Use unserialized value of an serialized attribute when comparing to the original
+ value during the initialization of the `changed_attributes` hash.
+
+ Example:
+ class User < ActiveRecord::Base
+ serialize :name
+ end
+
+ user = User.new
+
+ # Before
+ user.changes # => {"name"=>[nil, nil]}
+
+ # After
+ user.changes # => {}
+
+
+ Fixes #12505
+
+ *Ben Atkins*
+
* ActiveRecord::Base#attribute_for_inspect now truncates long arrays (more than 10 elements)
*Jan Bernacki*
View
6 activerecord/lib/active_record/core.rb
@@ -429,7 +429,11 @@ def init_changed_attributes
# optimistic locking) won't get written unless they get marked as changed
self.class.columns.each do |c|
attr, orig_value = c.name, c.default
- changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, @attributes[attr])
+ new_value = @attributes[attr]
+ # if the attribute is marked with `serialize`, need to compare the unserialized value to the original value
+ new_value = new_value.unserialized_value if new_value.is_a?(ActiveRecord::AttributeMethods::Serialization::Attribute)
+
+ changed_attributes[attr] = orig_value if _field_changed?(attr, orig_value, new_value)
end
end
View
5 activerecord/test/cases/serialized_attribute_test.rb
@@ -19,6 +19,11 @@ def test_list_of_serialized_attributes
assert_equal %w(content), Topic.serialized_attributes.keys
end
+ def test_active_model_dirty_serialized_attribute
+ topic = Topic.new
+ assert !topic.content_changed?
+ end
+
def test_serialized_attribute
Topic.serialize("content", MyObject)
Something went wrong with that request. Please try again.