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
[Fix #50812] Don't duplicate selects, inner and outer joins when merging with STI #50886
base: main
Are you sure you want to change the base?
[Fix #50812] Don't duplicate selects, inner and outer joins when merging with STI #50886
Conversation
comments = Comment.select(:id, :post_id).merge(SpecialComment.select(:id, :post_id)) | ||
assert_equal SpecialComment.pluck(:id).sort, comments.to_a.map(&:id).sort | ||
assert_equal SpecialComment.pluck(:post_id).sort, comments.to_a.map(&:post_id).sort | ||
assert_equal [:id, :post_id], comments.select_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_relation_merging_with_joins_and_sti | ||
comments = Comment.joins(:post).merge(SpecialComment.joins(:post)) | ||
assert_equal SpecialComment.count, comments.count | ||
assert_equal [:post], comments.joins_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def test_relation_merging_with_left_outer_joins_and_sti | ||
comments = Comment.left_outer_joins(:post).merge(SpecialComment.left_outer_joins(:post)) | ||
assert_equal SpecialComment.count, comments.count | ||
assert_equal [:post], comments.left_outer_joins_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def base_class | ||
self | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was necessary else this test now fails with:
40566c5
to
b2e4fad
Compare
b2e4fad
to
7c46503
Compare
7c46503
to
d9946d2
Compare
c58d2a1
to
0b9cb38
Compare
0b9cb38
to
ebfbac6
Compare
… merging with STI
ebfbac6
to
dd7d2e1
Compare
Motivation / Background
Fixes #50812
Detail
This Pull Request changes
#merge_select_values
,#merge_joins
and#merge_outer_joins
inActiveRecord::Relation::Merger
to unionise selects, inner and outer joins when the two sources of amerge
are related via STI.Additional information
The only similar case left in
ActiveRecord::Relation::Merger
is when merging preloads.I don't think that is similar to these cases since the preload values are simply used to preload associations. I thought I would just mention it anyway.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]