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

Originally the `JoinDependency` has the deduplication for eager loading
(LEFT JOIN). This re-uses that deduplication for `left_joins`.

And also, This makes left join order into part of joins, i.e.:

Before:

```
association joins -> stash joins (eager loading, etc) -> string joins -> left joins
```

After:

```
association joins -> stash joins (eager loading, left joins, etc) -> string joins
```

Now string joins are able to refer left joins.

Fixes #34325.
Fixes #34332.
Fixes #34536.

6 files changed, 45 insertions, 32 deletions

diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index e2efd4aa0d..fcb0da1a42 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -370,12 +370,6 @@ module ActiveRecord
         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)
diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb
index 4de7465128..6bb77b355c 100644
--- a/activerecord/lib/active_record/relation/merger.rb
+++ b/activerecord/lib/active_record/relation/merger.rb
@@ -117,16 +117,14 @@ module ActiveRecord
           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 @@ module ActiveRecord
           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
 
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 5f728f2263..7ea18c3069 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -992,6 +992,12 @@ module ActiveRecord
       @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 @@ module ActiveRecord
       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 @@ module ActiveRecord
         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
diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb
index c33dcdee61..e0dac01f4a 100644
--- a/activerecord/test/cases/associations/inner_join_association_test.rb
+++ b/activerecord/test/cases/associations/inner_join_association_test.rb
@@ -29,7 +29,10 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase
 
   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
diff --git a/activerecord/test/cases/associations/left_outer_join_association_test.rb b/activerecord/test/cases/associations/left_outer_join_association_test.rb
index 0e54e8c1b0..0a8863c35d 100644
--- a/activerecord/test/cases/associations/left_outer_join_association_test.rb
+++ b/activerecord/test/cases/associations/left_outer_join_association_test.rb
@@ -46,6 +46,12 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase
     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 @@ class LeftOuterJoinAssociationTest < ActiveRecord::TestCase
     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) }
diff --git a/activerecord/test/cases/relation/delegation_test.rb b/activerecord/test/cases/relation/delegation_test.rb
index 172fa20bc3..8208c45df1 100644
--- a/activerecord/test/cases/relation/delegation_test.rb
+++ b/activerecord/test/cases/relation/delegation_test.rb
@@ -51,7 +51,7 @@ module ActiveRecord
       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,