merge parent joins for through associations #24682

sevaorlov · 2016-04-21T20:41:10Z

Summary

Fixes #22538
Merges parent association joins for through associations into a scope. These joins are not taken into account at all right now.

For example direct_appointments join joins(:role) will not be merged for direct_users association and a call for direct_users will fail because direct column will not be found in users table.

class User < ActiveRecord::Base
end

class Role < ActiveRecord::Base
  scope :direct, ->{ where(direct: true) }
end

class Institution < ActiveRecord::Base
  has_many :direct_appointments, ->{ joins(:role).merge(Role.direct) }, 
class_name: 'Appointment'
  has_many :direct_users, through: :direct_appointments, source: :user
end

class Appointment < ActiveRecord::Base
  belongs_to :role
  belongs_to :institution
  belongs_to :user
end

My request adds these joins merging in association scope.

Other Information

I open request to master, but that issue is also present in Rails 4+, do I need to open requests to branches for 4+ versions?

Update

Also fixes #12933

rails-bot · 2016-04-21T20:41:13Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @chancancode (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.

Please see the contribution instructions for more information.

sevaorlov · 2016-04-21T20:41:24Z

activerecord/lib/active_record/associations/association_scope.rb

@@ -134,13 +134,13 @@ def add_constraints(scope, owner, association_klass, refl, chain_head, chain_tai
            scope = next_chain_scope(scope, table, reflection, association_klass, foreign_table, next_reflection)
          end

-          # Exclude the scope of the association itself, because that
-          # was already merged in the #scope method.


outdated comment

sevaorlov · 2016-04-23T21:11:24Z

activerecord/lib/active_record/associations/association_scope.rb

@@ -158,6 +158,20 @@ def add_constraints(scope, owner, association_klass, refl, chain_head, chain_tai
        scope
      end

+      def merge_joins(scope, other)
+        joins_dependency, rest = other.joins_values.partition do |join|


the same code we have here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/merger.rb#L113
I think it will be good to extract it somewhere

sevaorlov · 2016-04-23T21:22:09Z

Will be good to add tests for pure sql joins and maybe also extract the logic to some sort of a merger class, but I would like to here some feedback first. Am I moving in right direction?

sgrif · 2016-04-24T00:20:06Z

Can you please give more context in your commit message about what the issue is, why it occurs, and why this fixes it? "Fix joins in association scope" isn't much information for future readers.

sevaorlov · 2016-04-25T19:00:23Z

@sgrif, sorry for that, I've updated commit and pull request descriptions

sevaorlov · 2016-04-25T19:14:13Z

activerecord/lib/active_record/associations/association_scope.rb

+        end
+
+        join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass, joins_dependency, rest)
+        scope.joins! join_dependency.join_constraints([], Arel::Nodes::InnerJoin).map(&:joins)


I also need to merge outer joins

Merges parent associations joins for through associations into a scope. Previously joins in association scope were taken into account only for root associations with that scope.

sevaorlov · 2016-05-07T18:18:49Z

Rails converts inner joins to outer joins when merging #24281
Instead of using Relation::Merger I've created my own custom method that doesn't convert inner joins to outer joins so that expected behaviour is met. Behaviour that is described in this PR description.

That maybe strange and counterintuitive that joins with merge in through associations scope doesn't convert joins to outer, but in scope it does, like here #22021.

If #24281 is bug that needs to be fixed, than I'll be able to use Relation::Merger here.

oyeanuj · 2016-07-10T21:53:57Z

@sgrif Any chance this can be merged soon?

mattgibson · 2016-09-14T11:08:27Z

Just ran into this. It breaks quite a few includes calls for us. Any chance of a merge?

sgrif · 2016-12-08T19:02:01Z

activerecord/lib/active_record/associations/association_scope.rb


            if scope_chain_item == refl.scope
              scope.merge! item.except(:where, :includes)
+            elsif item.joins_values.any?
+              merge_joins(scope, item)


How is this different than the merge that will occur in the call to scope.merge!

scope.merge! merges joins with Relation::Merger, which converts inner joins to outer when merge_joins doesn't do that

That doesn't appear to be the case. The code is almost exactly identical to what you have here. https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/merger.rb#L107-L129

Yes, it looks pretty much the same, but there are differences at the end. Instead of passing ActiveRecord::Associations::JoinDependency instance to relation.joins!, I pass an array of Arel::Nodes::InnerJoin objects, which are built as inner joins instead of outer for JoinDependency here
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1015
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1030
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1042
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/join_dependency.rb#L122

at-hatran · 2017-03-04T17:41:13Z

Is anyone working on this pr?

sevaorlov · 2017-03-04T17:53:44Z

@at-hatran, at the time there is nothing to work on in this pr. I am waiting for any comments.

at-hatran · 2017-03-05T14:52:42Z

@sevaorlov I've just discovered this issue and my project use a lot of associations like this. Do you have any workaround for this?

sevaorlov · 2017-03-06T06:45:12Z

@at-hatran, just don't user merges in conditions. For example, you could create a separate Query class for that particular query.

rails-bot assigned chancancode Apr 21, 2016

sevaorlov reviewed Apr 21, 2016
View reviewed changes

maclover7 added the activerecord label Apr 21, 2016

sevaorlov force-pushed the fix/joins-in-association-conditions branch from b420e1d to c7c1453 Compare April 23, 2016 21:02

sevaorlov reviewed Apr 23, 2016
View reviewed changes

sevaorlov force-pushed the fix/joins-in-association-conditions branch from c7c1453 to 291bcf1 Compare April 25, 2016 18:45

sevaorlov changed the title ~~fix Joins in association scope~~ merge parent joins for through associations Apr 25, 2016

sevaorlov reviewed Apr 25, 2016
View reviewed changes

sevaorlov force-pushed the fix/joins-in-association-conditions branch from 291bcf1 to 9e9e484 Compare May 7, 2016 18:05

sevaorlov added 2 commits May 7, 2016 21:06

merge parent joins for through associations

8ef7073

Merges parent associations joins for through associations into a scope. Previously joins in association scope were taken into account only for root associations with that scope.

rename variables, add changelog

2767ea5

sevaorlov force-pushed the fix/joins-in-association-conditions branch from 9e9e484 to 2767ea5 Compare May 7, 2016 18:07

sgrif reviewed Dec 8, 2016

View reviewed changes

sevaorlov closed this Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge parent joins for through associations #24682

merge parent joins for through associations #24682

sevaorlov commented Apr 21, 2016 •

edited

rails-bot commented Apr 21, 2016

sevaorlov Apr 21, 2016

sevaorlov Apr 23, 2016 •

edited

sevaorlov commented Apr 23, 2016 •

edited

sgrif commented Apr 24, 2016

sevaorlov commented Apr 25, 2016

sevaorlov Apr 25, 2016

sevaorlov commented May 7, 2016 •

edited

oyeanuj commented Jul 10, 2016

mattgibson commented Sep 14, 2016

sgrif Dec 8, 2016

sevaorlov Dec 9, 2016

sgrif Dec 9, 2016

sevaorlov Dec 9, 2016

at-hatran commented Mar 4, 2017

sevaorlov commented Mar 4, 2017

at-hatran commented Mar 5, 2017

sevaorlov commented Mar 6, 2017

merge parent joins for through associations #24682

merge parent joins for through associations #24682

Conversation

sevaorlov commented Apr 21, 2016 • edited

Summary

Other Information

Update

rails-bot commented Apr 21, 2016

sevaorlov Apr 21, 2016

Choose a reason for hiding this comment

sevaorlov Apr 23, 2016 • edited

Choose a reason for hiding this comment

sevaorlov commented Apr 23, 2016 • edited

sgrif commented Apr 24, 2016

sevaorlov commented Apr 25, 2016

sevaorlov Apr 25, 2016

Choose a reason for hiding this comment

sevaorlov commented May 7, 2016 • edited

oyeanuj commented Jul 10, 2016

mattgibson commented Sep 14, 2016

sgrif Dec 8, 2016

Choose a reason for hiding this comment

sevaorlov Dec 9, 2016

Choose a reason for hiding this comment

sgrif Dec 9, 2016

Choose a reason for hiding this comment

sevaorlov Dec 9, 2016

Choose a reason for hiding this comment

at-hatran commented Mar 4, 2017

sevaorlov commented Mar 4, 2017

at-hatran commented Mar 5, 2017

sevaorlov commented Mar 6, 2017

sevaorlov commented Apr 21, 2016 •

edited

sevaorlov Apr 23, 2016 •

edited

sevaorlov commented Apr 23, 2016 •

edited

sevaorlov commented May 7, 2016 •

edited