Only replace association keys if they've changed [5.2 Regression] #32375
Conversation
|
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kamipo (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review. Please see the contribution instructions for more information. |
|
|
|
The closest I can come to do this with a public API is this: class Post < ActiveRecord::Base
belongs_to :blog
after_destroy do
association(:blog).target = blog
end
endThe full gist is here. The All that said the example above is contrived. I hit the issue because goldiloader uses the internal |
| @@ -85,7 +85,7 @@ def update_counters_on_replace(record) | |||
|
|
|||
| # Checks whether record is different to the current target, without loading it | |||
| def different_target?(record) | |||
| record.id != owner._read_attribute(reflection.foreign_key) | |||
| record.try(:id) != owner._read_attribute(reflection.foreign_key) | |||
| post = posts(:welcome) | ||
| author = Author.find(post.author_id) | ||
| post.destroy! | ||
| ActiveRecord::Associations::Preloader.new.preload([post], [:author]) |
There was a problem hiding this comment.
Let's use your example that uses the public API here.
|
Here's a repro that uses the public preloader API to trigger the problem: # frozen_string_literal: true
begin
require "bundler/inline"
rescue LoadError => e
$stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
raise e
end
gemfile(true) do
source "https://rubygems.org"
git_source(:github) { |repo| "https://github.com/#{repo}.git" }
gem "rails", path: "."
gem "sqlite3"
end
require "active_record"
require "minitest/autorun"
require "logger"
# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)
ActiveRecord::Schema.define do
create_table :posts, force: true do |t|
end
create_table :comments, force: true do |t|
t.integer :post_id
end
end
class Post < ActiveRecord::Base
has_many :comments
end
class Comment < ActiveRecord::Base
belongs_to :post
after_find { destroy! }
end
class BugTest < Minitest::Test
def test_association_stuff
post = Post.create!
post.comments << Comment.create!
comment = Comment.preload(:post).first
assert_equal post, comment.post
end
end |
|
Thanks @eugeneius. That's a much cleaner (and devious) test case. |
jturkel
force-pushed
the
feature/frozen-preload
branch
from
5ed95df
to
af4b54f
Compare
| @@ -30,7 +30,7 @@ def replace(record) | |||
| end | |||
|
|
|||
| def target=(record) | |||
| replace_keys(record) | |||
| replace_keys(record) if different_target?(record) | |||
There was a problem hiding this comment.
Passing nil to different_target? may break any other functionality (e.g. face.association(:polymorphic_man).target = nil).
record.nil? || different_target?(record) is better to me.
There was a problem hiding this comment.
Wouldn't making the condition record.nil? || different_target?(record) incorrectly replace the keys if target is nil and record is nil?
It looks like BelongsToPolymorphicAssociation needs to be changed to something like this to work properly for the null and non-null record cases (assuming it's safer than having BelongsToPolymorphicAssociation#klass return NilClass for the nil case):
def different_target?(record)
super || record && record.class != klass || record.nil? && klass
end There was a problem hiding this comment.
There was a problem hiding this comment.
Great point. I've pushed a change with a nil check.
| @@ -85,7 +85,7 @@ def update_counters_on_replace(record) | |||
|
|
|||
| # Checks whether record is different to the current target, without loading it | |||
| def different_target?(record) | |||
| record.id != owner._read_attribute(reflection.foreign_key) | |||
| (record && record.id) != owner._read_attribute(reflection.foreign_key) | |||
There was a problem hiding this comment.
Unnecessary this since nil record won't be passed.
This ensures the Preloader workers on destroyed models.
jturkel
force-pushed
the
feature/frozen-preload
branch
from
6f8bf40
to
9fbf455
Compare
| client.destroy! | ||
| assert_raise(frozen_error_class) { client.firm = nil } | ||
| assert_raise(frozen_error_class) { client.firm = Firm.new(name: "Firm") } | ||
| assert_raise(frozen_error_class) { client.firm = Firm.new(name: "New Firm") } |
There was a problem hiding this comment.
If this change is required, it means that this PR will change existing public behavior in exchange for preloading on destroyed record.
There was a problem hiding this comment.
Yup. The change in public behavior is that frozen errors will only be raised if the foreign key is actually changing (except for the nil case that we punted on). This behavior is also inconsistent with changing "regular" attributes on destroyed models which will always throw an exception. Thinking through it a bit more I can see a few ways forward:
- Make the assignments to any attributes (i.e. foreign keys or primitives) a no-op if they don't actually change anything but a) that doesn't feel particular useful outside of the preloader use case; b) the required additional equality checks could have a negative performance impact.
- Rollback Bugfix foreign key replacement in inverse association #31575 which caused the change in preloader behavior introduced in 5.2.0. That issue seems more serious than the preloader not working on destroyed models so I'm not crazy about rolling it back.
- Define a separate API on
Associationfor setting thetargetthat would have the behavior needed by the preloader (i.e. it should never change the foreign key). - Punt on making the preloader work with destroyed models
- Move forward with the change as currently implemented which gets preloading working on destroyed models but is inconsistent with setting attributes on destroyed models
Option 3 seems like the best option but I'm not sure it's worth it given Rails 5.2 has already been released with the change in preloader behavior and I've released a workaround in goldiloader. What are your thoughts?
There was a problem hiding this comment.
@kamipo - I pushed a commit that implements option 3 above (define a separate private API on Association for the Preloader to set the target that bypasses any scenarios that change the attributes hash). Let me know what you think.
There was a problem hiding this comment.
The behaviour of option 3 sounds correct, but the design feels wrong; we're adding a method that duplicates the implementation of target= so that we can avoid the behaviour added in BelongsToAssociation, in order to fix a bug that only affects belongs to associations.
Another potential solution is to revert #31575 and #32338, and solve the original problem in a different way: set both the target and the foreign key when assigning an inverse association.
There was a problem hiding this comment.
Right, my preference is here:
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
index d883eda52d..9301ccc124 100644
--- a/activerecord/lib/active_record/associations/association.rb
+++ b/activerecord/lib/active_record/associations/association.rb
@@ -18,8 +18,7 @@ module Associations
# HasManyAssociation + ForeignAssociation
# HasManyThroughAssociation + ThroughAssociation
class Association #:nodoc:
- attr_reader :owner, :target, :reflection
- attr_accessor :inversed
+ attr_reader :owner, :target, :reflection, :inversed
delegate :options, to: :reflection
@@ -76,11 +75,6 @@ def target=(target)
loaded!
end
- def preloaded_target=(target) #:nodoc:
- @target = target
- loaded!
- end
-
def scope
target_scope.merge!(association_scope)
end
@@ -103,23 +97,24 @@ def reset_scope
# Set the inverse association, if possible
def set_inverse_instance(record)
- if invertible_for?(record)
- inverse = record.association(inverse_reflection_for(record).name)
- inverse.target = owner
- inverse.inversed = true
+ if inverse = inverse_association_for(record)
+ inverse.inversed_from(owner)
end
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
+ if inverse = inverse_association_for(record)
+ inverse.inversed_from(nil)
end
end
+ def inversed_from(record)
+ self.target = record
+ @inversed = !!record
+ end
+
# Returns the class of the target. belongs_to polymorphic overrides this to look at the
# polymorphic_type field on the owner.
def klass
@@ -245,6 +240,12 @@ def raise_on_type_mismatch!(record)
end
end
+ def inverse_association_for(record)
+ if invertible_for?(record)
+ record.association(inverse_reflection_for(record).name)
+ end
+ end
+
# Can be redefined by subclasses, notably polymorphic belongs_to
# The record parameter is necessary to support polymorphic inverses as we must check for
# the association in the specific class of the record.
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 1109fee462..e5fcdb06dc 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -26,10 +26,12 @@ def replace(record)
decrement_counters
end
+ replace_keys(record)
+
self.target = record
end
- def target=(record)
+ def inversed_from(record)
replace_keys(record)
super
end
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index f076f53af0..d6f7359055 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -46,7 +46,7 @@ def associate_records_to_owner(owner, records)
if reflection.collection?
association.target.concat(records)
else
- association.preloaded_target = records.first unless records.empty?
+ association.target = records.first unless records.empty?
end
end
Since #31575, `BelongsToAssociation#target=` replaces owner record's foreign key to fix an inverse association bug. But the method is not only used for inverse association but also used for eager loading/preloading, it caused some public behavior changes (#32338, #32375). To avoid any side-effect in loading associations, I reverted the overriding `#target=`, then introduced `#inversed_from` to replace foreign key in `set_inverse_instance`. Closes #32375.
|
Thanks @kamipo! Sorry I couldn't roll in the PR feedback more quickly. |
|
No worries, thanks for the PR which demonstrates the issue anyway! |
Since #31575, `BelongsToAssociation#target=` replaces owner record's foreign key to fix an inverse association bug. But the method is not only used for inverse association but also used for eager loading/preloading, it caused some public behavior changes (#32338, #32375). To avoid any side-effect in loading associations, I reverted the overriding `#target=`, then introduced `#inversed_from` to replace foreign key in `set_inverse_instance`. Closes #32375.
Summary
This is a bit obscure but as of Rails 5.2.0 RC1 (when commit a84c765 landed) the
ActiveRecord::Associations::Preloadercan no longer invoked on associations of a destroyed model. This worked fine in older versions of Rails.This results in a stack trace something like the following:
Other Information
This was discovered while testing the goldiloader gem with Rails 5.2. More details on the goldiloader side can be found in salsify/goldiloader#63 if you're interested.