Merge pull request #35864 from kamipo/improve_left_joins

Stash `left_joins` into `joins` to deduplicate redundant LEFT JOIN

activerecord/lib/active_record/relation/finder_methods.rb

@@ -370,12 +370,6 @@ def construct_relation_for_exists(conditions)
         relation
       end
 
-      def construct_join_dependency(associations)
-        ActiveRecord::Associations::JoinDependency.new(
-          klass, table, associations
-        )
-      end
-
       def apply_join_dependency(eager_loading: group_values.empty?)
         join_dependency = construct_join_dependency(eager_load_values + includes_values)
         relation = except(:includes, :eager_load, :preload).joins!(join_dependency)

activerecord/lib/active_record/relation/merger.rb

@@ -117,16 +117,14 @@ def merge_joins
           if other.klass == relation.klass
             relation.joins!(*other.joins_values)
           else
-            joins_dependency = other.joins_values.map do |join|
+            associations, others = other.joins_values.partition do |join|
               case join
-              when Hash, Symbol, Array
-                other.send(:construct_join_dependency, join)
-              else
-                join
+              when Hash, Symbol, Array; true
               end
             end
 
-            relation.joins!(*joins_dependency)
+            join_dependency = other.construct_join_dependency(associations)
+            relation.joins!(join_dependency, *others)
           end
         end
 
@@ -136,16 +134,9 @@ def merge_outer_joins
           if other.klass == relation.klass
             relation.left_outer_joins!(*other.left_outer_joins_values)
           else
-            joins_dependency = other.left_outer_joins_values.map do |join|
-              case join
-              when Hash, Symbol, Array
-                other.send(:construct_join_dependency, join)
-              else
-                join
-              end
-            end
-
-            relation.left_outer_joins!(*joins_dependency)
+            associations = other.left_outer_joins_values
+            join_dependency = other.construct_join_dependency(associations)
+            relation.joins!(join_dependency)
           end
         end
 

activerecord/lib/active_record/relation/query_methods.rb

@@ -992,6 +992,12 @@ def arel(aliases = nil) # :nodoc:
       @arel ||= build_arel(aliases)
     end
 
+    def construct_join_dependency(associations) # :nodoc:
+      ActiveRecord::Associations::JoinDependency.new(
+        klass, table, associations
+      )
+    end
+
     protected
       def build_subquery(subquery_alias, select_value) # :nodoc:
         subquery = except(:optimizer_hints).arel.as(subquery_alias)
@@ -1021,8 +1027,11 @@ def assert_mutability!
       def build_arel(aliases)
         arel = Arel::SelectManager.new(table)
 
-        aliases = build_joins(arel, joins_values.flatten, aliases) unless joins_values.empty?
-        build_left_outer_joins(arel, left_outer_joins_values.flatten, aliases) unless left_outer_joins_values.empty?
+        if !joins_values.empty?
+          build_joins(arel, joins_values.flatten, aliases)
+        elsif !left_outer_joins_values.empty?
+          build_left_outer_joins(arel, left_outer_joins_values.flatten, aliases)
+        end
 
         arel.where(where_clause.ast) unless where_clause.empty?
         arel.having(having_clause.ast) unless having_clause.empty?
@@ -1072,22 +1081,28 @@ def build_from
         end
       end
 
-      def build_left_outer_joins(manager, outer_joins, aliases)
-        buckets = outer_joins.group_by do |join|
-          case join
+      def valid_association_list(associations)
+        associations.each do |association|
+          case association
           when Hash, Symbol, Array
-            :association_join
-          when ActiveRecord::Associations::JoinDependency
-            :stashed_join
+            # valid
           else
             raise ArgumentError, "only Hash, Symbol and Array are allowed"
           end
         end
+      end
 
+      def build_left_outer_joins(manager, outer_joins, aliases)
+        buckets = { association_join: valid_association_list(outer_joins) }
         build_join_query(manager, buckets, Arel::Nodes::OuterJoin, aliases)
       end
 
       def build_joins(manager, joins, aliases)
+        unless left_outer_joins_values.empty?
+          left_joins = valid_association_list(left_outer_joins_values.flatten)
+          joins << construct_join_dependency(left_joins)
+        end
+
         buckets = joins.group_by do |join|
           case join
           when String

activerecord/test/cases/associations/inner_join_association_test.rb

@@ -29,7 +29,10 @@ def test_construct_finder_sql_does_not_table_name_collide_on_duplicate_associati
 
   def test_construct_finder_sql_does_not_table_name_collide_on_duplicate_associations_with_left_outer_joins
     sql = Person.joins(agents: :agents).left_outer_joins(agents: :agents).to_sql
-    assert_match(/agents_people_4/i, sql)
+    assert_match(/agents_people_2/i, sql)
+    assert_match(/INNER JOIN/i, sql)
+    assert_no_match(/agents_people_4/i, sql)
+    assert_no_match(/LEFT OUTER JOIN/i, sql)
   end
 
   def test_construct_finder_sql_does_not_table_name_collide_with_string_joins

activerecord/test/cases/associations/left_outer_join_association_test.rb

@@ -46,6 +46,12 @@ def test_left_outer_joins_actually_does_a_left_outer_join
     assert queries.any? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
   end
 
+  def test_left_outer_joins_is_deduped_when_same_association_is_joined
+    queries = capture_sql { Author.joins(:posts).left_outer_joins(:posts).to_a }
+    assert queries.any? { |sql| /INNER JOIN/i.match?(sql) }
+    assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
+  end
+
   def test_construct_finder_sql_ignores_empty_left_outer_joins_hash
     queries = capture_sql { Author.left_outer_joins({}).to_a }
     assert queries.none? { |sql| /LEFT OUTER JOIN/i.match?(sql) }
@@ -60,6 +66,10 @@ def test_left_outer_joins_forbids_to_use_string_as_argument
     assert_raise(ArgumentError) { Author.left_outer_joins('LEFT OUTER JOIN "posts" ON "posts"."user_id" = "users"."id"').to_a }
   end
 
+  def test_left_outer_joins_with_string_join
+    assert_equal 16, Author.left_outer_joins(:posts).joins("LEFT OUTER JOIN comments ON comments.post_id = posts.id").count
+  end
+
   def test_join_conditions_added_to_join_clause
     queries = capture_sql { Author.left_outer_joins(:essays).to_a }
     assert queries.any? { |sql| /writer_type.*?=.*?(Author|\?|\$1|\:a1)/i.match?(sql) }

activerecord/test/cases/relation/delegation_test.rb

@@ -51,7 +51,7 @@ class QueryingMethodsDelegationTest < ActiveRecord::TestCase
       ActiveRecord::SpawnMethods.public_instance_methods(false) - [:spawn, :merge!] +
       ActiveRecord::QueryMethods.public_instance_methods(false).reject { |method|
         method.to_s.end_with?("=", "!", "value", "values", "clause")
-      } - [:reverse_order, :arel, :extensions] + [
+      } - [:reverse_order, :arel, :extensions, :construct_join_dependency] + [
         :any?, :many?, :none?, :one?,
         :first_or_create, :first_or_create!, :first_or_initialize,
         :find_or_create_by, :find_or_create_by!, :find_or_initialize_by,