Fix preloading of st models on has_many through associations #14312

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
10 participants
Contributor

moktin commented Mar 7, 2014

This patch fixes #11078 by removing the type conditions from through_scope.
It is really inspired by #11082 and the comment from @neerajdotname.

activerecord/CHANGELOG.md
+ STI models.
+
+ Fixes #11078.
+
@pftg

pftg Mar 7, 2014

Contributor

Please add your name here 😉

activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+
@pftg

pftg Mar 7, 2014

Contributor

✂️

mikegee commented May 8, 2014

@moktin I think @pftg is asking for you to remove the blank lines with the ✂️

Also, you will probably be asked to swash these two commits and force-push before this can be merged.

Please either address these two things, or close this if you are no longer concerned with it.

Thanks!

Contributor

moktin commented May 9, 2014

HI @mikegee,
Thank you for the feedback.

I squashed these two commits and pushed them.
The blank line was already removed.

@moktin I know this is an old PR, but it could be rebased?

@moktin moktin closed this Nov 20, 2016

@moktin moktin reopened this Nov 20, 2016

Contributor

moktin commented Nov 20, 2016

@AlfonsoUceda done, but I can't promise it will be merge.

moktin added some commits Jan 14, 2017

activerecord/lib/active_record/core.rb
@@ -308,12 +308,22 @@ def relation
relation = Relation.create(self, arel_table, predicate_builder)
@rafaelfranca

rafaelfranca Jan 18, 2017

Owner

This line is not needed anymore.

activerecord/lib/active_record/core.rb
end
end
+ def relation_with_type_condition #:nodoc:
@rafaelfranca

rafaelfranca Jan 18, 2017

Owner

Could you make this method public and keep the # :nodoc: so we don't have to use send?

@rafaelfranca

rafaelfranca Jan 18, 2017

Owner

Also we should move those methods to inheritance.rb file with finder_needs_type_condition

activerecord/lib/active_record/core.rb
+ end
+
+ def relation_without_type_condition #:nodoc:
+ relation = Relation.create(self, arel_table, predicate_builder)
@rafaelfranca

rafaelfranca Jan 18, 2017

Owner

this variable is not needed.

moktin added some commits Jan 18, 2017

+ ::ActiveRecord::Relation::WhereClause.empty
+ end
+
+ scope.where_clause = reflection_scope.where_clause - base_wheres
@kamipo

kamipo Jan 18, 2017

Member

How about it?

--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -87,7 +87,11 @@ def through_scope
             else
               unless reflection_scope.where_clause.empty?
                 scope.includes_values = Array(reflection_scope.values[:includes] || options[:source])
-                scope.where_clause = reflection_scope.where_clause
+                where_clause = reflection_scope.where_clause
+                if reflection.klass.finder_needs_type_condition?
+                  where_clause = where_clause.except(reflection.klass.inheritance_column)
+                end
+                scope.where_clause = where_clause
               end

WhereClause#- not always works properly unlike WhereClause#except.

activerecord/lib/active_record/core.rb
if finder_needs_type_condition? && !ignore_default_scope?
- relation.where(type_condition).create_with(inheritance_column.to_s => sti_name)
+ relation_with_type_condition
@kamipo

kamipo Jan 18, 2017

Member

It is unneccesary changes anymore.

+ def relation_with_type_condition #:nodoc:
+ relation = relation_without_type_condition
+
+ relation.where(type_condition).create_with(inheritance_column.to_s => sti_name)
@kamipo

kamipo Jan 18, 2017

Member

ditto.

+ def -(other)
+ WhereClause.new(
+ predicates - other.predicates,
+ binds - other.binds
@kamipo

kamipo Jan 18, 2017

Member

ditto.

@maclover7 maclover7 added needs work and removed needs feedback labels Jan 18, 2017

else
- relation
+ relation_without_type_condition
@kamipo

kamipo Jan 24, 2017

Member

The changes to this file is unnecessary.

+ def relation_without_type_condition #:nodoc:
+ Relation.create(self, arel_table, predicate_builder)
+ end
+
@kamipo

kamipo Jan 24, 2017

Member

ditto.

Any updates on this issue?

@kamipo kamipo closed this in 82bfe3b Sep 1, 2017

sideshowdave7 added a commit to sideshowdave7/rails that referenced this pull request Oct 11, 2017

Add a test case for preloading through association with implicit source
If `reflection_scope.where_clause` is not empty, `through_scope` should
be joined the source association. But if the through association doesn't
have explicit `:source`, `options[:source]` will be nil and
`scope.includes_values` will also be empty. It should use
`source_reflection.name` rather than `options[:source]`.

Fixed by a26cff3.

Fixes #11078.
Fixes #26129.
Closes #14312.
Closes #29155.
Closes #29841.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment