Merge pull request #36304 from kamipo/fix_through_association_with_joins

Implicit through table joins should be appeared before user supplied joins

2 files changed, 19 insertions, 17 deletions

diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index b8fd2fce14..50ff733dc7 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1128,27 +1128,21 @@ module ActiveRecord
 
         association_joins = buckets[:association_join]
         stashed_joins     = buckets[:stashed_join]
-        join_nodes        = buckets[:join_node].uniq
-        string_joins      = buckets[:string_join].map(&:strip).uniq
+        join_nodes        = buckets[:join_node].tap(&:uniq!)
+        string_joins      = buckets[:string_join].delete_if(&:blank?).map!(&:strip).tap(&:uniq!)
 
-        join_list = join_nodes + convert_join_strings_to_ast(string_joins)
-        alias_tracker = alias_tracker(join_list, aliases)
+        string_joins.map! { |join| table.create_string_join(Arel.sql(join)) }
 
-        join_dependency = construct_join_dependency(association_joins, join_type)
+        join_sources = manager.join_sources
+        join_sources.concat(join_nodes) unless join_nodes.empty?
 
-        joins = join_dependency.join_constraints(stashed_joins, alias_tracker)
-        joins.each { |join| manager.from(join) }
-
-        manager.join_sources.concat(join_list)
-
-        alias_tracker.aliases
-      end
+        unless association_joins.empty? && stashed_joins.empty?
+          alias_tracker = alias_tracker(join_nodes + string_joins, aliases)
+          join_dependency = construct_join_dependency(association_joins, join_type)
+          join_sources.concat(join_dependency.join_constraints(stashed_joins, alias_tracker))
+        end
 
-      def convert_join_strings_to_ast(joins)
-        joins
-          .flatten
-          .reject(&:blank?)
-          .map { |join| table.create_string_join(Arel.sql(join)) }
+        join_sources.concat(string_joins) unless string_joins.empty?
       end
 
       def build_select(arel)
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 0ab99aa6cd..affa024d77 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -58,6 +58,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     assert_equal preloaded, Marshal.load(Marshal.dump(preloaded))
   end
 
+  def test_through_association_with_joins
+    assert_equal [comments(:eager_other_comment1)], authors(:mary).comments.merge(Post.joins(:comments))
+  end
+
+  def test_through_association_with_left_joins
+    assert_equal [comments(:eager_other_comment1)], authors(:mary).comments.merge(Post.left_joins(:comments))
+  end
+
   def test_preload_with_nested_association
     posts = Post.preload(:author, :author_favorites_with_scope).to_a