From 8f05035b7e595e2086759ee10ec9df9431e5e351 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 5 Apr 2019 06:09:44 +0900 Subject: 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. --- .../lib/active_record/relation/finder_methods.rb | 6 ----- activerecord/lib/active_record/relation/merger.rb | 23 +++++----------- .../lib/active_record/relation/query_methods.rb | 31 ++++++++++++++++------ .../associations/inner_join_association_test.rb | 5 +++- .../left_outer_join_association_test.rb | 10 +++++++ .../test/cases/relation/delegation_test.rb | 2 +- 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, -- cgit v1.2.3