From c7adc610f0233b9afd51a6d68aa9061cdb250fad Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 1 Nov 2016 13:28:56 -0400 Subject: Allow `autosave: true` to be used with inverse of With the changes in #25337, double save bugs are pretty much impossible, so we can just lift this restriction with pretty much no change. There were a handful of cases where we were relying on specific quirks in tests that had to be updated. The change to has_one associations was due to a particularly interesting test where an autosaved has_one association was replaced with a new child, where the child failed to save but the test wanted to check that the parent id persisted to `nil`. I think this is almost certainly the wrong behavior, and I may change that behavior later. But ultimately the root cause was because we never remove the parent in memory when nullifying the child. This makes #23197 no longer needed, but it is what we'll do to fix some issues on 5.0 Close #23197 --- activerecord/lib/active_record/associations/association.rb | 9 +++++++++ .../lib/active_record/associations/has_one_association.rb | 3 ++- activerecord/lib/active_record/reflection.rb | 11 +---------- .../test/cases/associations/has_one_associations_test.rb | 2 +- activerecord/test/cases/autosave_association_test.rb | 1 + 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index d651864f3a..bbf3561916 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/activerecord/lib/active_record/associations/association.rb @@ -112,6 +112,15 @@ module ActiveRecord record end + # Remove the inverse association, if possible + def remove_inverse_instance(record) + if invertible_for?(record) + inverse = record.association(inverse_reflection_for(record).name) + inverse.target = nil + inverse.inversed = false + end + end + # Returns the class of the target. belongs_to polymorphic overrides this to look at the # polymorphic_type field on the owner. def klass diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 82ff0fa704..21bd668dff 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -86,8 +86,9 @@ module ActiveRecord target.delete when :destroy target.destroy - else + else nullify_owner_attributes(target) + remove_inverse_instance(target) if target.persisted? && owner.persisted? && !target.save set_owner_attributes(target) diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb index e912f97d63..147685e32c 100644 --- a/activerecord/lib/active_record/reflection.rb +++ b/activerecord/lib/active_record/reflection.rb @@ -282,11 +282,6 @@ module ActiveRecord end def autosave=(autosave) - # autosave and inverse_of do not get along together nowadays. They may - # for example cause double saves. Thus, we disable this flag. If in the - # future those two flags are known to work well together, this could be - # removed. - @automatic_inverse_of = false @options[:autosave] = autosave parent_reflection = self.parent_reflection if parent_reflection @@ -544,11 +539,7 @@ module ActiveRecord # nil. def inverse_name options.fetch(:inverse_of) do - if @automatic_inverse_of == false - nil - else - @automatic_inverse_of ||= automatic_inverse_of - end + @automatic_inverse_of ||= automatic_inverse_of end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 483d1162bc..862f33a1a0 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -601,7 +601,7 @@ class HasOneAssociationsTest < ActiveRecord::TestCase new_ship = Ship.create(name: "new name") assert_queries(2) do - # One query for updating name and second query for updating pirate_id + # One query to nullify the old ship, one query to update the new ship pirate.ship = new_ship end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 80e76caaaa..a3f82ed49d 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -792,6 +792,7 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase end @ship.pirate.catchphrase = "Changed Catchphrase" + @ship.name_will_change! assert_raise(RuntimeError) { assert !@pirate.save } assert_not_nil @pirate.reload.ship -- cgit v1.2.3