Skip to content

Fix eager_loading? when ordering with Hash syntax #42782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

intrip
Copy link
Contributor

@intrip intrip commented Jul 14, 2021

Summary

eager_loading? is triggered correctly when using order with hash syntax on an outer table.

before:

Post.includes(:comments).order({ "comments.label": :ASC }).eager_loading?
=> raises ActiveRecord::StatementInvalid

after:

Post.includes(:comments).order({ "comments.label": :ASC }).eager_loading?
=> true

@intrip intrip force-pushed the fix-eager-loading-with-order-hash branch from 5a51121 to 4756c51 Compare July 14, 2021 20:13
@intrip intrip marked this pull request as ready for review July 14, 2021 20:14
@intrip intrip force-pushed the fix-eager-loading-with-order-hash branch 2 times, most recently from e9b3b59 to 49fb99a Compare July 14, 2021 20:23
@intrip
Copy link
Contributor Author

intrip commented Jul 14, 2021

@ghiculescu can you please take a look at this fix? 🙇

@ghiculescu
Copy link
Member

👍 it looks good to me.

Comment on lines 1551 to 1558
references = order_args.map do |arg|
case arg
when String
arg
when Hash
arg.keys
end
end.flatten
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
references = order_args.map do |arg|
case arg
when String
arg
when Hash
arg.keys
end
end.flatten
references = order_args.flat_map do |arg|
case arg
when String
arg
when Hash
arg.keys
end
end

If any of the hash's keys are arrays, we don't want to flatten them.

Copy link
Contributor Author

@intrip intrip Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eugeneius Thanks!

I've applied the suggestion but the above use case it's unclear to me: could you show me an example of using Array as Hash keys for order? I've tried but I couldn't manage to reproduce.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't concerned about breaking a valid use case, more just that flat_map corresponds better to what we're trying to do here. Using flap_map also saves an intermediate array allocation so it should be (marginally) faster, which is probably worthwhile since this code runs every time a relation with an order is built.

@intrip intrip force-pushed the fix-eager-loading-with-order-hash branch from a060295 to 59f0609 Compare July 15, 2021 07:12
`eager_loading?` is triggered correctly when using `order` with
hash syntax on an outer table.

before:
```ruby
Post.includes(:comments).order({ "comments.label": :ASC }).eager_loading?
=> raises ActiveRecord::StatementInvalid
```

after:
```ruby
Post.includes(:comments).order({ "comments.label": :ASC }).eager_loading?
=> true
```

Co-authored-by: Eugene Kenny <elkenny@gmail.com>
@intrip intrip force-pushed the fix-eager-loading-with-order-hash branch from 59f0609 to 8ab892e Compare July 15, 2021 12:26
@eugeneius eugeneius merged commit 110d365 into rails:main Jul 15, 2021
@eugeneius
Copy link
Member

Thanks @intrip!

@intrip intrip deleted the fix-eager-loading-with-order-hash branch July 15, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants