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

Associated has_one doesn't save when its serialized hashes are changed in-place #14407

Closed
jcoyne opened this Issue Mar 17, 2014 · 9 comments

Comments

Projects
None yet
3 participants
Contributor

jcoyne commented Mar 17, 2014

Starting in rails 4.0.4 (rails/rails#8313), has_one associations don't save if the child object hasn't changed. However, if you use serialized Hashes and change them in place (same object_id), then the object is not marked as changed.

Let's assume a Pirate has_one Ship. if our Ship class looks like this:

class Ship < ActiveRecord::Base
  serialize :sailplan, Hash
end

pirate = Pirate.create(ship: Ship.new(sailplan: {mainsail: true, staysail: true} } )
pirate.reload
pirate.ship.sailplan[:topgallant] = true
pirate.ship.changed? 
=> false
pirate.save
pirate.reload.ship.sailplan
=> {mainsail: true, staysail: true}

The work around here is to dup the Hash before modifying it, but that requires a fair amount of attention to implementation details.

Contributor

jcoyne commented Mar 17, 2014

@rafaelfranca rafaelfranca added this to the 4.0.5 milestone Mar 17, 2014

Owner

rafaelfranca commented Mar 17, 2014

I'll revert this on 4.0.5 and 4-1-stable but I think there is no way to fix your issue and the original issue.

I'll try to fix in-place edit of serialized columns on master but just to you be aware, the current behavior you are getting is expected since you did in-place edit and there is no way active record track these changes right now.

Contributor

jcoyne commented Mar 17, 2014

@rafaelfranca that's why I just added a "be aware" note to the old ticket and didn't open a new ticket originally. The only potential solution I can see would be to add some kind of proxy that tracks in place changes to serialized attributes.

Owner

rafaelfranca commented Mar 17, 2014

It is fine to open this issues since we changed the behavior in a patch release, and we should not. There are some possible solution for this problem but we can only implement in master branch.

Contributor

jcoyne commented Mar 17, 2014

@rafaelfranca BTW I'm not sure this needs to be reverted. I just had the wrong assumption that rails had change tracking on serialized hashes at a deeper level.

Owner

rafaelfranca commented Apr 14, 2014

@jcoyne I just tried to write a test case to reproduce your issue and I could not. Even reverting this change I'm getting this test failing. Could you write a test that fail on Rails 4.0.4 and pass on Rails 4.0.3?

diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index c49fa43..00741fa 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -11,6 +11,7 @@ require 'models/person'
 require 'models/pirate'
 require 'models/post'
 require 'models/reader'
+require 'models/sailplan'
 require 'models/ship'
 require 'models/ship_part'
 require 'models/tag'
@@ -632,17 +633,15 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase
     assert_not_nil @pirate.reload.ship
   end

-  def test_should_save_changed_has_one_changed_object_if_child_is_saved
-    @pirate.ship.name = "NewName"
-    @pirate.ship.expects(:save).once.returns(true)
+  def test_should_save_changed_has_one_object_with_serialized_field_changed_in_place
+    pirate = Pirate.create(catchphrase: 'arr', sailplan: Sailplan.new(details: { mainsail: true, staysail: true }))
+    pirate.reload
+    pirate.sailplan.details[:topgallant] = true
+    assert_not pirate.sailplan.changed?

-    assert @pirate.save
-  end
+    pirate.save

-  def test_should_not_save_changed_has_one_unchanged_object_if_child_is_saved
-    @pirate.ship.expects(:save).never
-
-    assert @pirate.save
+    assert_equal({ mainsail: true, staysail: true, topgallant: true }, pirate.reload.sailplan.details)
   end

   # belongs_to
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index 170fc2f..8e60caa 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -19,6 +19,7 @@ class Pirate < ActiveRecord::Base

   # These both have :autosave enabled because accepts_nested_attributes_for is used on them.
   has_one :ship
+  has_one :sailplan
   has_one :update_only_ship, :class_name => 'Ship'
   has_one :non_validated_ship, :class_name => 'Ship'
   has_many :birds, -> { order('birds.id ASC') }
diff --git a/activerecord/test/models/sailplan.rb b/activerecord/test/models/sailplan.rb
new file mode 100644
index 0000000..397db67
--- /dev/null
+++ b/activerecord/test/models/sailplan.rb
@@ -0,0 +1,5 @@
+class Sailplan < ActiveRecord::Base
+  serialize :details, Hash
+
+  belongs_to :pirate
+end
diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb
index e951893..c7c0aa5 100644
--- a/activerecord/test/schema/schema.rb
+++ b/activerecord/test/schema/schema.rb
@@ -597,6 +597,11 @@ ActiveRecord::Schema.define do
     t.integer :lock_version, :default => 0
   end

+  create_table :sailplans, force: true do |t|
+    t.string :details
+    t.integer :pirate_id
+  end
+
   create_table :shape_expressions, :force => true do |t|
     t.string  :paint_type
     t.integer :paint_id
Contributor

alan commented Apr 30, 2014

@rafaelfranca can you get my change re-reverted then? It seems like it was not causing problems.

Owner

rafaelfranca commented Apr 30, 2014

@alan I'm almost sure it was. There are a lot of places where Rails always save the object because of serialized fields. has_one is one of these places.

I prefer to be safer and keep this comment out of a stable branch.

I'd introduce this change again on master but for this I need to make sure we have test covering the serialized case.

@jcoyne could you take a look on the failing test?

Owner

rafaelfranca commented May 20, 2014

Ok. I'll keep the change at 4.0.6 and 4.1.2 until someone could create a failing test case. Reverting now without any proof we added a regression will only give us more problem.

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