From 331e0ea55ce62b50f062ec9fc88240db7e2e7c9e Mon Sep 17 00:00:00 2001 From: Edouard CHIN Date: Wed, 15 Jan 2025 06:47:50 +0100 Subject: [PATCH] Fix order of LEFT JOINS when using `eager_load`: - Fix #54181 - When multiple stashed joins (eager load, left joins) were added, the join produced by eager loading always had the least precedence. This could result in producing invalid query in the following case: ```ruby Post.eager_load(:comments).merge(Comment.left_joins(:user)).count # SELECT COUNT(DISTINCT "posts"."id") FROM "posts" LEFT OUTER JOIN "users" ON "users"."id" = "comments"."user_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" WHERE "users"."id" IS NULL ``` Now the order of LEFT JOIN produced by eager load will be respected. --- activerecord/CHANGELOG.md | 24 +++++++++++++++++++ .../active_record/relation/finder_methods.rb | 2 +- .../active_record/relation/query_methods.rb | 2 ++ .../test/cases/relation/merging_test.rb | 6 +++++ .../cases/scoping/default_scoping_test.rb | 10 ++++++++ 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 7658cb5f6737e..460f0fbf077e5 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,27 @@ +* Fix orders of produced left joins when using `eager_load`. + + A `eager_load` followed by a `left_joins` was always resulting in the `LEFT JOIN` + produced by `left_joins` to have precedence over the `LEFT JOIN` produced by + the `eager_load` call. + + This was resulting in an invalid SQL query in some cases + + ```ruby + Post.select(:id).eager_load(:author).merge(Author.left_joins(:address)) + ``` + + ### Before (Invalid query) + ```sql + SELECT DISTINCT "posts"."id" FROM "posts" LEFT OUTER JOIN "addresses" ON "addresses"."id" = "authors"."address_id" LEFT OUTER JOIN "authors" ON "authors"."post_id" = "posts"."id" + ``` + + ### After + ```sql + SELECT DISTINCT "posts"."id" FROM "posts" LEFT OUTER JOIN "authors" ON "authors"."post_id" = "posts"."id" LEFT OUTER JOIN "addresses" ON "addresses"."id" = "authors"."address_id" + ``` + + *Edouard Chin* + * Enable automatically retrying idempotent `#exists?` queries on connection errors. diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 383505e7c0cd5..4e5d7f44ee43e 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -458,7 +458,7 @@ def apply_join_dependency(eager_loading: group_values.empty?) join_dependency = construct_join_dependency( eager_load_values | includes_values, Arel::Nodes::OuterJoin ) - relation = except(:includes, :eager_load, :preload).joins!(join_dependency) + relation = except(:includes, :eager_load, :preload).left_outer_joins!(join_dependency) if eager_loading && has_limit_or_offset? && !( using_limitable_reflections?(join_dependency.reflections) && diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 1ca1ef9f3b1b7..488db79fa390f 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -294,6 +294,7 @@ def eager_load(*args) def eager_load!(*args) # :nodoc: self.eager_load_values |= args + self.left_outer_joins_values |= args self end @@ -819,6 +820,7 @@ def unscope!(*args) # :nodoc: raise ArgumentError, "Called unscope() with invalid unscoping argument ':#{scope}'. Valid arguments are :#{VALID_UNSCOPING_VALUES.to_a.join(", :")}." end assert_modifiable! + self.left_outer_joins_values -= eager_load_values if scope == :eager_load @values.delete(scope) when Hash scope.each do |key, target_value| diff --git a/activerecord/test/cases/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb index 302ee1a87f3e8..2a3bbab053024 100644 --- a/activerecord/test/cases/relation/merging_test.rb +++ b/activerecord/test/cases/relation/merging_test.rb @@ -208,6 +208,12 @@ def test_relation_merging_with_eager_load end end + def test_relation_merging_with_eager_load_and_left_joins + result = Post.eager_load(:author).merge(Author.where.missing(:author_address_extra)).count + + assert_equal(6, result) + end + def test_relation_merging_with_locks devs = Developer.lock.where("salary >= 80000").order("id DESC").merge(Developer.limit(2)) assert_predicate devs, :locked? diff --git a/activerecord/test/cases/scoping/default_scoping_test.rb b/activerecord/test/cases/scoping/default_scoping_test.rb index c849743ae4e94..2cb9c74d39f34 100644 --- a/activerecord/test/cases/scoping/default_scoping_test.rb +++ b/activerecord/test/cases/scoping/default_scoping_test.rb @@ -418,6 +418,16 @@ def test_unscope_eager_load assert_equal false, received.first.projects.loaded? end + def test_unscope_eager_load_keeps_left_joins + Computer.destroy_all + + expected = Developer.all.collect(&:name) + received = Developer.eager_load(:projects).left_joins(:computers).unscope(:eager_load).group("developers.id") + .having("COUNT(computers.id) = 0") + + assert_equal expected, received.collect(&:name) + end + def test_unscope_preloads expected = Developer.all.collect(&:name) received = Developer.preload(:projects).select(:id).unscope(:preload, :select)