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)