Skip to content

Commit

Permalink
Fix order of LEFT JOINS when using eager_load:
Browse files Browse the repository at this point in the history
- Fix rails#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.
  • Loading branch information
Edouard-chin committed Jan 16, 2025
1 parent fa6886b commit 6822554
Show file tree
Hide file tree
Showing 5 changed files with 43 additions and 1 deletion.
24 changes: 24 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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.

Expand Down
2 changes: 1 addition & 1 deletion activerecord/lib/active_record/relation/finder_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Expand Down
2 changes: 2 additions & 0 deletions activerecord/lib/active_record/relation/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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|
Expand Down
6 changes: 6 additions & 0 deletions activerecord/test/cases/relation/merging_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
10 changes: 10 additions & 0 deletions activerecord/test/cases/scoping/default_scoping_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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").order(:id)

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)
Expand Down

0 comments on commit 6822554

Please sign in to comment.