Preserve user supplied JOIN order.

JOIN clauses order is important, previous implementation always
put string or arel joins at then end (after auto-generated
association joins).

Fixes rails#12953, rails#15488.

activerecord/lib/active_record/associations/join_dependency.rb

@@ -45,32 +45,85 @@ def column_aliases
         Column = Struct.new(:name, :alias)
       end
 
-      attr_reader :alias_tracker, :base_klass, :join_root
+      class Tree
+        def initialize(associations = nil)
+          @tree = {}
+          add_associations(associations) if associations
+        end
 
-      def self.make_tree(associations)
-        hash = {}
-        walk_tree associations, hash
-        hash
-      end
+        def add_associations(associations)
+          walk(associations, @tree)
+        end
+
+        def add_if_associations(associations)
+          walk(associations, @tree, false)
+        end
+
+        def map(&block)
+          @tree.map(&block)
+        end
+
+        def drain_associations_as_join_dependency_param(associations_param)
+          join_dependency_param = nil
+          drain(associations_param) do |associations_name, subtree, multiple_values_incoming|
+            if multiple_values_incoming
+              (join_dependency_param ||= {})[associations_name] = subtree
+            elsif subtree.empty?
+              join_dependency_param = associations_name # no need for Hash, can avoid allocation
+            else
+              join_dependency_param = {associations_name => subtree}
+            end
+          end
+          join_dependency_param
+        end
 
-      def self.walk_tree(associations, hash)
-        case associations
-        when Symbol, String
-          hash[associations.to_sym] ||= {}
-        when Array
-          associations.each do |assoc|
-            walk_tree assoc, hash
+        def drain_associations_as_join_infos(join_dependency, associations_param)
+          drain(associations_param) do |association_name, subtree, multiple_values_incoming|
+            yield join_dependency.association_make_inner_join(association_name)
           end
-        when Hash
-          associations.each do |k,v|
-            cache = hash[k] ||= {}
-            walk_tree v, cache
+        end
+
+        private
+        def drain(associations_param)
+          case associations_param
+          when Symbol
+            if subtree = @tree.delete(associations_param)
+              yield associations_param, subtree, false
+            end
+          when Hash, Array
+            associations_param.public_send(associations_param.kind_of?(Hash) ? :each_key : :each) do |association_name|
+              if subtree = @tree.delete(association_name)
+                yield association_name, subtree, true
+              end
+            end
           end
-        else
-          raise ConfigurationError, associations.inspect
+        end
+
+        def walk(associations, hash, strict = true) # recursion is always strict
+          case associations
+          when Symbol, String
+            hash[associations.to_sym] ||= {}
+          when Array
+            associations.each do |assoc|
+              walk assoc, hash
+            end
+          when Hash
+            associations.each do |k,v|
+              cache = hash[k] ||= {}
+              walk v, cache
+            end
+          else
+            raise ConfigurationError, associations.inspect if strict
+          end
+        end
+
+        def self.to_tree(associations = nil)
+          associations.kind_of?(self) ? associations : new(associations)
         end
       end
 
+      attr_reader :alias_tracker, :base_klass, :join_root
+
       # base is the base class on which operation is taking place.
       # associations is the list of associations which are joined using hash, symbol or array.
       # joins is the list of all string join commands and arel nodes.
@@ -92,10 +145,11 @@ def self.walk_tree(associations, hash)
       #    associations # => [:appointments]
       #    joins # =>  []
       #
-      def initialize(base, associations, joins)
+      def initialize(base, associations, joins = [])
         @alias_tracker = AliasTracker.create(base.connection, joins)
         @alias_tracker.aliased_name_for(base.table_name, base.table_name) # Updates the count for base.table_name to 1
-        tree = self.class.make_tree associations
+        # associations Hash can be used directly, no need to explicitly convert it into Tree
+        tree = associations.kind_of?(Hash) ? associations : Tree.to_tree(associations)
         @join_root = JoinBase.new base, build(tree, base)
         @join_root.children.each { |child| construct_tables! @join_root, child }
       end
@@ -104,20 +158,14 @@ def reflections
         join_root.drop(1).map!(&:reflection)
       end
 
-      def join_constraints(outer_joins)
-        joins = join_root.children.flat_map { |child|
-          make_inner_joins join_root, child
-        }
-
-        joins.concat outer_joins.flat_map { |oj|
-          if join_root.match? oj.join_root
-            walk join_root, oj.join_root
-          else
-            oj.join_root.children.flat_map { |child|
-              make_outer_joins oj.join_root, child
-            }
-          end
-        }
+      def join_constraints_for_join_dependency(join)
+        if join_root.match? join.join_root
+          walk join_root, join.join_root
+        else
+          join.join_root.children.flat_map { |child|
+            make_outer_joins join.join_root, child
+          }
+        end
       end
 
       def aliases
@@ -152,6 +200,13 @@ def instantiate(result_set, aliases)
         parents.values
       end
 
+      def association_make_inner_join(association_name)
+        join_root.children.each do |child|
+          return make_inner_joins(join_root, child) if child.reflection.name == association_name
+        end
+        nil
+      end
+
       private
 
       def make_constraints(parent, child, tables, join_type)

activerecord/lib/active_record/relation/merger.rb

@@ -83,26 +83,43 @@ def merge
       private
 
       def merge_joins
-        return if values[:joins].blank?
+        return if (joins = values[:joins]).blank?
 
         if other.klass == relation.klass
-          relation.joins!(*values[:joins])
+          relation.joins!(*joins)
         else
-          joins_dependency, rest = values[:joins].partition do |join|
-            case join
-            when Hash, Symbol, Array
-              true
+          # first need to build an association join tree (AR guarantees not to double join
+          # associations even if they've accidentally been specified twice,
+          # ie: Author.joins(:books).joins(:books))
+          association_tree = ActiveRecord::Associations::JoinDependency::Tree.new
+          joins.each {|join| association_tree.add_if_associations(join)}
+
+          # coalesce/pool JoinDependency allocation, since association joins usually come in batches,
+          # ie: joins # => [:posts, :comments, :categorizations], while non association joins are usually
+          # really rare
+          join_dependency_params = nil
+          join_values            = []
+
+          # build join_values iteratively to preserve user supplied JOIN clauses order
+          joins.each do |join|
+            if join_dependency_param = association_tree.drain_associations_as_join_dependency_param(join)
+              join_dependency_params ||= []
+              if join_dependency_param.kind_of?(Array)
+                join_dependency_params.concat(join_dependency_param)
+              else
+                join_dependency_params << join_dependency_param
+              end
             else
-              false
+              if join_dependency_params # can't delay JoinDependency.new anymore
+                join_values << ActiveRecord::Associations::JoinDependency.new(other.klass, join_dependency_params)
+                join_dependency_params = nil
+              end
+              join_values << join
             end
           end
+          join_values << ActiveRecord::Associations::JoinDependency.new(other.klass, join_dependency_params) if join_dependency_params
 
-          join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass,
-                                                                           joins_dependency,
-                                                                           [])
-          relation.joins! rest
-
-          @relation = relation.joins join_dependency
+          @relation = relation.joins(*join_values)
         end
       end
 

activerecord/lib/active_record/relation/query_methods.rb

@@ -914,22 +914,6 @@ def where_unscoping(target_value)
       bind_values.reject! { |col,_| col.name == target_value }
     end
 
-    def custom_join_ast(table, joins)
-      joins = joins.reject(&:blank?)
-
-      return [] if joins.empty?
-
-      joins.map! do |join|
-        case join
-        when Array
-          join = Arel.sql(join.join(' ')) if array_of_strings?(join)
-        when String
-          join = Arel.sql(join)
-        end
-        table.create_string_join(join)
-      end
-    end
-
     def collapse_wheres(arel, wheres)
       predicates = wheres.map do |where|
         next where if ::Arel::Nodes::Equality === where
@@ -998,44 +982,59 @@ def build_from
     end
 
     def build_joins(manager, joins)
-      buckets = joins.group_by do |join|
+      associations_tree = ActiveRecord::Associations::JoinDependency::Tree.new
+      other_joins_hash  = {}
+      joins_to_process  = []
+
+      # a first loop over joins does some pre-processing, uniquification and preparation
+      # of params for JoinDependency (it needs to aware of all joins, so it can perform
+      # non-conflicting table aliasing)
+      joins.each do |join|
         case join
-        when String
-          :string_join
         when Hash, Symbol, Array
-          :association_join
+          associations_tree.add_associations(join)
         when ActiveRecord::Associations::JoinDependency
-          :stashed_join
+        when String
+          next if (join = join.strip).blank? || other_joins_hash[join]
+          join = other_joins_hash[join] = manager.create_string_join(Arel.sql(join))
         when Arel::Nodes::Join
-          :join_node
+          # note: Arel::Nodes::Join subclasses can reliably used as hash keys
+          next if other_joins_hash[join]
+          other_joins_hash[join] = join
         else
           raise 'unknown class: %s' % join.class.name
         end
+        joins_to_process << join
       end
 
-      association_joins         = buckets[:association_join] || []
-      stashed_association_joins = buckets[:stashed_join] || []
-      join_nodes                = (buckets[:join_node] || []).uniq
-      string_joins              = (buckets[:string_join] || []).map(&:strip).uniq
-
-      join_list = join_nodes + custom_join_ast(manager, string_joins)
-
       join_dependency = ActiveRecord::Associations::JoinDependency.new(
         @klass,
-        association_joins,
-        join_list
+        associations_tree,
+        other_joins_hash.values
       )
 
-      join_infos = join_dependency.join_constraints stashed_association_joins
+      # second time looping over joins so that user supplied order of joins is maintained
+      joins_to_process.each do |join|
+        case join
+        when Hash, Symbol, Array
+          associations_tree.drain_associations_as_join_infos(join_dependency, join) do |join_infos|
+            append_join_infos(manager, join_infos)
+          end
+        when ActiveRecord::Associations::JoinDependency
+          append_join_infos(manager, join_dependency.join_constraints_for_join_dependency(join))
+        when Arel::Nodes::Join
+          manager.join_sources << join
+        end
+      end
 
+      manager
+    end
+
+    def append_join_infos(manager, join_infos)
       join_infos.each do |info|
         info.joins.each { |join| manager.from(join) }
         manager.bind_values.concat info.binds
       end
-
-      manager.join_sources.concat(join_list)
-
-      manager
     end
 
     def build_select(arel, selects)
@@ -1067,10 +1066,6 @@ def reverse_sql_order(order_query)
       end
     end
 
-    def array_of_strings?(o)
-      o.is_a?(Array) && o.all? { |obj| obj.is_a?(String) }
-    end
-
     def build_order(arel)
       orders = order_values.uniq
       orders.reject!(&:blank?)

activerecord/test/cases/associations/inner_join_association_test.rb

@@ -2,6 +2,7 @@
 require 'models/post'
 require 'models/comment'
 require 'models/author'
+require 'models/book'
 require 'models/essay'
 require 'models/category'
 require 'models/categorization'
@@ -136,4 +137,27 @@ def test_find_with_conditions_on_through_reflection
     assert_equal 0, categories.first.special_categorizations.size
     assert_equal 1, categories.second.special_categorizations.size
   end
+
+  test "join clauses are emitted in user specified order" do
+    [:posts, :categorizations].map do |association_sym|
+      author_table = Author.arel_table
+      reflection   = Author.reflect_on_association(association_sym)
+      other_table  = reflection.klass.arel_table
+      arel_join    = author_table.create_join(other_table, author_table.create_on(other_table[reflection.foreign_key].eq(author_table[reflection.association_primary_key])))
+      [association_sym, arel_join, arel_join.to_sql]
+    end.permutation do |join_a, join_b|
+      join_a.each do |join_a_version|
+        join_b.each do |join_b_version|
+          join_a_token = join_a_version.respond_to?(:to_sql) ? join_a_version.to_sql : join_a_version.to_s
+          join_b_token = join_b_version.respond_to?(:to_sql) ? join_b_version.to_sql : join_b_version.to_s
+          # this tests sql generation in AR::Relation::QueryMethods#build_joins
+          sql = Author.joins(join_a_version, join_b_version).to_sql
+          assert(sql.index(join_a_token) < sql.index(join_b_token), "#{join_a_token.inspect} must precede #{join_b_token.inspect} in #{sql.inspect}")
+          # this tests relation join merging in AR::Relation::Merger#merge_joins
+          sql = Book.all.merge(Author.joins(join_a_version, join_b_version)).to_sql
+          assert(sql.index(join_a_token) < sql.index(join_b_token), "#{join_a_token.inspect} must precede #{join_b_token.inspect} in #{sql.inspect}")
+        end
+      end
+    end
+  end
 end