diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index e9a7d9f597b3d..56a8796318d5c 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 6957ba052b535..29e2e2cedc87a 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/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) diff --git a/activerecord/test/cases/associations/inner_join_association_test.rb b/activerecord/test/cases/associations/inner_join_association_test.rb index e0dac01f4a5f2..b65af4b819341 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 @@ 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