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 rails#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 rails#36761.
Fixes rails#34328.
Fixes rails#24281.
Fixes rails#12953.

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*

activerecord/lib/active_record/relation/query_methods.rb

@@ -1095,26 +1095,44 @@ def valid_association_list(associations)
       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 @@ def build_joins(manager, joins, aliases)
       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)

activerecord/test/cases/associations/inner_join_association_test.rb

@@ -13,7 +13,7 @@
 
 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 @@ def test_construct_finder_sql_does_not_table_name_collide_on_duplicate_associati
   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