Preserve user supplied joins order as much as possible

Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since #36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes #36761.
Fixes #34328.
Fixes #24281.
Fixes #12953.

3 files changed, 65 insertions, 24 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index e9a7d9f597..56a8796318 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Preserve user supplied joins order as much as possible.
+
+    Fixes #36761, #34328, #24281, #12953.
+
+    *Ryuta Kamizono*
+
 *   Allow `matches_regex` and `does_not_match_regexp` on the MySQL Arel visitor.
 
     *James Pearson*
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 6957ba052b..29e2e2cedc 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -1095,26 +1095,44 @@ module ActiveRecord
       end
 
       def build_left_outer_joins(manager, outer_joins, aliases)
-        buckets = { association_join: valid_association_list(outer_joins) }
+        buckets = Hash.new { |h, k| h[k] = [] }
+        buckets[:association_join] = valid_association_list(outer_joins)
         build_join_query(manager, buckets, Arel::Nodes::OuterJoin, aliases)
       end
 
       def build_joins(manager, joins, aliases)
+        buckets = Hash.new { |h, k| h[k] = [] }
+
         unless left_outer_joins_values.empty?
           left_joins = valid_association_list(left_outer_joins_values.flatten)
-          joins.unshift construct_join_dependency(left_joins, Arel::Nodes::OuterJoin)
+          buckets[:stashed_join] << construct_join_dependency(left_joins, Arel::Nodes::OuterJoin)
         end
 
-        buckets = joins.group_by do |join|
+        joins.map! do |join|
+          if join.is_a?(String)
+            table.create_string_join(Arel.sql(join.strip)) unless join.blank?
+          else
+            join
+          end
+        end.compact_blank!
+
+        while joins.first.is_a?(Arel::Nodes::Join)
+          join_node = joins.shift
+          if join_node.is_a?(Arel::Nodes::StringJoin) && !buckets[:stashed_join].empty?
+            buckets[:join_node] << join_node
+          else
+            buckets[:leading_join] << join_node
+          end
+        end
+
+        joins.each do |join|
           case join
-          when String
-            :string_join
           when Hash, Symbol, Array
-            :association_join
+            buckets[:association_join] << join
           when ActiveRecord::Associations::JoinDependency
-            :stashed_join
+            buckets[:stashed_join] << join
           when Arel::Nodes::Join
-            :join_node
+            buckets[:join_node] << join
           else
             raise "unknown class: %s" % join.class.name
           end
@@ -1124,25 +1142,21 @@ module ActiveRecord
       end
 
       def build_join_query(manager, buckets, join_type, aliases)
-        buckets.default = []
-
         association_joins = buckets[:association_join]
         stashed_joins     = buckets[:stashed_join]
+        leading_joins     = buckets[:leading_join].tap(&:uniq!)
         join_nodes        = buckets[:join_node].tap(&:uniq!)
-        string_joins      = buckets[:string_join].compact_blank!.map!(&:strip).tap(&:uniq!)
-
-        string_joins.map! { |join| table.create_string_join(Arel.sql(join)) }
 
         join_sources = manager.join_sources
-        join_sources.concat(join_nodes) unless join_nodes.empty?
+        join_sources.concat(leading_joins) unless leading_joins.empty?
 
         unless association_joins.empty? && stashed_joins.empty?
-          alias_tracker = alias_tracker(join_nodes + string_joins, aliases)
+          alias_tracker = alias_tracker(leading_joins + join_nodes, aliases)
           join_dependency = construct_join_dependency(association_joins, join_type)
           join_sources.concat(join_dependency.join_constraints(stashed_joins, alias_tracker))
         end
 
-        join_sources.concat(string_joins) unless string_joins.empty?
+        join_sources.concat(join_nodes) unless join_nodes.empty?
       end
 
       def build_select(arel)
diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb
index e0dac01f4a..b65af4b819 100644
--- a/activerecord/test/cases/associations/inner_join_association_test.rb
+++ b/activerecord/test/cases/associations/inner_join_association_test.rb
@@ -13,7 +13,7 @@ require "models/tag"
 
 class InnerJoinAssociationTest < ActiveRecord::TestCase
   fixtures :authors, :author_addresses, :essays, :posts, :comments, :categories, :categories_posts, :categorizations,
-           :taggings, :tags
+           :taggings, :tags, :people
 
   def test_construct_finder_sql_applies_aliases_tables_on_association_conditions
     result = Author.joins(:thinking_posts, :welcome_posts).to_a
@@ -36,16 +36,37 @@ class InnerJoinAssociationTest < ActiveRecord::TestCase
   end
 
   def test_construct_finder_sql_does_not_table_name_collide_with_string_joins
-    sql = Person.joins(:agents).joins("JOIN people agents_people ON agents_people.primary_contact_id = people.id").to_sql
-    assert_match(/agents_people_2/i, sql)
+    string_join = <<~SQL
+      JOIN people agents_people ON agents_people.primary_contact_id = agents_people_2.id AND agents_people.id > agents_people_2.id
+    SQL
+
+    expected = people(:susan)
+    assert_sql(/agents_people_2/i) do
+      assert_equal [expected], Person.joins(:agents).joins(string_join)
+    end
   end
 
   def test_construct_finder_sql_does_not_table_name_collide_with_aliased_joins
-    people = Person.arel_table
-    agents = people.alias("agents_people")
-    constraint = agents[:primary_contact_id].eq(people[:id])
-    sql = Person.joins(:agents).joins(agents.create_join(agents, agents.create_on(constraint))).to_sql
-    assert_match(/agents_people_2/i, sql)
+    agents = Person.arel_table.alias("agents_people")
+    agents_2 = Person.arel_table.alias("agents_people_2")
+    constraint = agents[:primary_contact_id].eq(agents_2[:id]).and(agents[:id].gt(agents_2[:id]))
+
+    expected = people(:susan)
+    assert_sql(/agents_people_2/i) do
+      assert_equal [expected], Person.joins(:agents).joins(agents.create_join(agents, agents.create_on(constraint)))
+    end
+  end
+
+  def test_user_supplied_joins_order_should_be_preserved
+    string_join = <<~SQL
+      JOIN people agents_people_2 ON agents_people_2.primary_contact_id = people.id
+    SQL
+    agents = Person.arel_table.alias("agents_people")
+    agents_2 = Person.arel_table.alias("agents_people_2")
+    constraint = agents[:primary_contact_id].eq(agents_2[:id]).and(agents[:id].gt(agents_2[:id]))
+
+    expected = people(:susan)
+    assert_equal [expected], Person.joins(string_join).joins(agents.create_join(agents, agents.create_on(constraint)))
   end
 
   def test_construct_finder_sql_ignores_empty_joins_hash