Skip to content
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.

Sign up for GitHub

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

Jump to bottom

activerecord: Use a simpler query condition for aggregates with one mapping #34963

Merged
merged 3 commits into from
Jan 17, 2019
Merged

activerecord: Use a simpler query condition for aggregates with one mapping #34963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
242fc54
activerecord: Use a simpler query condition for aggregates with one m…
dylanahsmith Jan 17, 2019
b278db1
Avoid using yield_self to make it easier to backport
dylanahsmith Jan 17, 2019
9905cdc
Use public_send instead since respond_to? doesn't include private/pro…
kamipo Jan 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 12 additions & 8 deletions activerecord/lib/active_record/relation/predicate_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,20 @@ def expand_from_hash(attributes)
queries.reduce(&:or)
elsif table.aggregated_with?(key)
mapping = table.reflect_on_aggregation(key).mapping
queries = Array.wrap(value).map do |object|
mapping.map do |field_attr, aggregate_attr|
if mapping.size == 1 && !object.respond_to?(aggregate_attr)
build(table.arel_attribute(field_attr), object)
else
if mapping.length == 1
column_name, aggr_attr = mapping.first
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer using the same variable names in both path.

Suggested change
column_name, aggr_attr = mapping.first
field_attr, aggregate_attr = mapping.first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That causes "shadowing outer local variable" ruby warnings

Copy link
Member

Choose a reason for hiding this comment

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

oh... I see...

values = Array.wrap(value).map do |object|
object.respond_to?(aggr_attr) ? object.public_send(aggr_attr) : object
end
build(table.arel_attribute(column_name), values)
else
queries = Array.wrap(value).map do |object|
mapping.map do |field_attr, aggregate_attr|
build(table.arel_attribute(field_attr), object.send(aggregate_attr))
end
end.reduce(&:and)
end.reduce(&:and)
end
queries.reduce(&:or)
end
queries.reduce(&:or)
else
build(table.arel_attribute(key), value)
end
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/cases/finder_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,7 @@ def test_hash_condition_find_with_aggregate_having_one_mapping_array
assert_kind_of Money, zaphod_balance
found_customers = Customer.where(balance: [david_balance, zaphod_balance])
assert_equal [customers(:david), customers(:zaphod)], found_customers.sort_by(&:id)
assert_equal Customer.where(balance: [david_balance.amount, zaphod_balance.amount]).to_sql, found_customers.to_sql
end

def test_hash_condition_find_with_aggregate_attribute_having_same_name_as_field_and_key_value_being_aggregate
Expand Down