mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Deprecate aggregations with group by duplicated fields
We've learned that `merge` causes duplicated multiple values easily, so if we missed to deduplicate the values, it will cause weird behavior like #38052, #39171. I've investigated the deduplication for the values, at least that had existed since Rails 3.0.bed9179aa1
Aggregations with group by multiple fields was introduced at Rails 3.1, but we missed the deduplication for the aggregation result, unlike the generated SQL.a5cdf0b9eb
While the investigation, I've found that `annotate` is also missed the deduplication. I don't suppose this weird behavior is intended for both. So I'd like to deprecate the duplicated behavior in Rails 6.1, and will be deduplicated all multiple values in Rails 6.2. To migrate to Rails 6.2's behavior, use `uniq!(:group)` to deduplicate group fields. ```ruby accounts = Account.group(:firm_id) # duplicated group fields, deprecated. accounts.merge(accounts.where.not(credit_limit: nil)).sum(:credit_limit) # => { # [1, 1] => 50, # [2, 2] => 60 # } # use `uniq!(:group)` to deduplicate group fields. accounts.merge(accounts.where.not(credit_limit: nil)).uniq!(:group).sum(:credit_limit) # => { # 1 => 50, # 2 => 60 # } ```
This commit is contained in:
parent
4a1b57ce70
commit
2580b83f42
5 changed files with 119 additions and 3 deletions
|
@ -1,3 +1,45 @@
|
|||
* Deprecate aggregations with group by duplicated fields.
|
||||
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:group)` to deduplicate group fields.
|
||||
|
||||
```ruby
|
||||
accounts = Account.group(:firm_id)
|
||||
|
||||
# duplicated group fields, deprecated.
|
||||
accounts.merge(accounts.where.not(credit_limit: nil)).sum(:credit_limit)
|
||||
# => {
|
||||
# [1, 1] => 50,
|
||||
# [2, 2] => 60
|
||||
# }
|
||||
|
||||
# use `uniq!(:group)` to deduplicate group fields.
|
||||
accounts.merge(accounts.where.not(credit_limit: nil)).uniq!(:group).sum(:credit_limit)
|
||||
# => {
|
||||
# 1 => 50,
|
||||
# 2 => 60
|
||||
# }
|
||||
```
|
||||
|
||||
*Ryuta Kamizono*
|
||||
|
||||
* Deprecate duplicated query annotations.
|
||||
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:annotate)` to deduplicate query annotations.
|
||||
|
||||
```ruby
|
||||
accounts = Account.where(id: [1, 2]).annotate("david and mary")
|
||||
|
||||
# duplicated annotations, deprecated.
|
||||
accounts.merge(accounts.rewhere(id: 3))
|
||||
# SELECT accounts.* FROM accounts WHERE accounts.id = 3 /* david and mary */ /* david and mary */
|
||||
|
||||
# use `uniq!(:annotate)` to deduplicate annotations.
|
||||
accounts.merge(accounts.rewhere(id: 3)).uniq!(:annotate)
|
||||
# SELECT accounts.* FROM accounts WHERE accounts.id = 3 /* david and mary */
|
||||
```
|
||||
|
||||
*Ryuta Kamizono*
|
||||
|
||||
* Resolve conflict between counter cache and optimistic locking.
|
||||
|
||||
Bump an Active Record instance's lock version after updating its counter
|
||||
|
|
|
@ -316,6 +316,16 @@ module ActiveRecord
|
|||
|
||||
def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
|
||||
group_fields = group_values
|
||||
group_fields = group_fields.uniq if group_fields.size > 1
|
||||
|
||||
unless group_fields == group_values
|
||||
ActiveSupport::Deprecation.warn(<<-MSG.squish)
|
||||
`#{operation}` with group by duplicated fields does no longer affect to result in Rails 6.2.
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:group)` to deduplicate group fields
|
||||
(`#{klass.name&.tableize || klass.table_name}.uniq!(:group).#{operation}(#{column_name.inspect})`).
|
||||
MSG
|
||||
group_fields = group_values
|
||||
end
|
||||
|
||||
if group_fields.size == 1 && group_fields.first.respond_to?(:to_sym)
|
||||
association = klass._reflect_on_association(group_fields.first)
|
||||
|
|
|
@ -1048,6 +1048,14 @@ module ActiveRecord
|
|||
self
|
||||
end
|
||||
|
||||
# Deduplicate multiple values.
|
||||
def uniq!(name)
|
||||
if values = @values[name]
|
||||
values.uniq! if values.is_a?(Array) && !values.empty?
|
||||
end
|
||||
self
|
||||
end
|
||||
|
||||
# Returns the Arel object associated with the relation.
|
||||
def arel(aliases = nil) # :nodoc:
|
||||
@arel ||= build_arel(aliases)
|
||||
|
@ -1098,7 +1106,20 @@ module ActiveRecord
|
|||
arel.distinct(distinct_value)
|
||||
arel.from(build_from) unless from_clause.empty?
|
||||
arel.lock(lock_value) if lock_value
|
||||
arel.comment(*annotate_values) unless annotate_values.empty?
|
||||
|
||||
unless annotate_values.empty?
|
||||
annotates = annotate_values
|
||||
annotates = annotates.uniq if annotates.size > 1
|
||||
unless annotates == annotate_values
|
||||
ActiveSupport::Deprecation.warn(<<-MSG.squish)
|
||||
Duplicated query annotations are no longer shown in queries in Rails 6.2.
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:annotate)` to deduplicate query annotations
|
||||
(`#{klass.name&.tableize || klass.table_name}.uniq!(:annotate)`).
|
||||
MSG
|
||||
annotates = annotate_values
|
||||
end
|
||||
arel.comment(*annotates)
|
||||
end
|
||||
|
||||
arel
|
||||
end
|
||||
|
|
|
@ -130,6 +130,7 @@ class CalculationsTest < ActiveRecord::TestCase
|
|||
9 => 53
|
||||
}
|
||||
assert_equal expected, accounts.sum(:credit_limit)
|
||||
assert_equal expected, accounts.merge!(accounts).uniq!(:group).sum(:credit_limit)
|
||||
|
||||
expected = {
|
||||
[nil, nil] => 50,
|
||||
|
@ -138,7 +139,14 @@ class CalculationsTest < ActiveRecord::TestCase
|
|||
[6, 6] => 55,
|
||||
[9, 9] => 53
|
||||
}
|
||||
assert_equal expected, accounts.merge!(accounts).maximum(:credit_limit)
|
||||
message = <<-MSG.squish
|
||||
`maximum` with group by duplicated fields does no longer affect to result in Rails 6.2.
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:group)` to deduplicate group fields
|
||||
(`accounts.uniq!(:group).maximum(:credit_limit)`).
|
||||
MSG
|
||||
assert_deprecated(message) do
|
||||
assert_equal expected, accounts.merge!(accounts).maximum(:credit_limit)
|
||||
end
|
||||
|
||||
expected = {
|
||||
[nil, nil, nil, nil] => 50,
|
||||
|
@ -147,7 +155,14 @@ class CalculationsTest < ActiveRecord::TestCase
|
|||
[6, 6, 6, 6] => 50,
|
||||
[9, 9, 9, 9] => 53
|
||||
}
|
||||
assert_equal expected, accounts.merge!(accounts).minimum(:credit_limit)
|
||||
message = <<-MSG.squish
|
||||
`minimum` with group by duplicated fields does no longer affect to result in Rails 6.2.
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:group)` to deduplicate group fields
|
||||
(`accounts.uniq!(:group).minimum(:credit_limit)`).
|
||||
MSG
|
||||
assert_deprecated(message) do
|
||||
assert_equal expected, accounts.merge!(accounts).minimum(:credit_limit)
|
||||
end
|
||||
end
|
||||
|
||||
def test_should_generate_valid_sql_with_joins_and_group
|
||||
|
|
|
@ -233,6 +233,34 @@ class RelationMergingTest < ActiveRecord::TestCase
|
|||
Post.annotate("foo").annotate("bar").merge(Post.annotate("baz").annotate("qux")).first
|
||||
end
|
||||
end
|
||||
|
||||
def test_merging_duplicated_annotations
|
||||
posts = Post.annotate("foo")
|
||||
assert_sql(%r{FROM #{Regexp.escape(Post.quoted_table_name)} /\* foo \*/\z}) do
|
||||
posts.merge(posts).uniq!(:annotate).to_a
|
||||
end
|
||||
|
||||
message = <<-MSG.squish
|
||||
Duplicated query annotations are no longer shown in queries in Rails 6.2.
|
||||
To migrate to Rails 6.2's behavior, use `uniq!(:annotate)` to deduplicate query annotations
|
||||
(`posts.uniq!(:annotate)`).
|
||||
MSG
|
||||
assert_deprecated(message) do
|
||||
assert_sql(%r{FROM #{Regexp.escape(Post.quoted_table_name)} /\* foo \*/ /\* foo \*/\z}) do
|
||||
posts.merge(posts).to_a
|
||||
end
|
||||
end
|
||||
assert_deprecated(message) do
|
||||
assert_sql(%r{FROM #{Regexp.escape(Post.quoted_table_name)} /\* foo \*/ /\* bar \*/ /\* foo \*/\z}) do
|
||||
Post.annotate("foo").merge(Post.annotate("bar")).merge(posts).to_a
|
||||
end
|
||||
end
|
||||
assert_deprecated(message) do
|
||||
assert_sql(%r{FROM #{Regexp.escape(Post.quoted_table_name)} /\* bar \*/ /\* foo \*/ /\* foo \*/\z}) do
|
||||
Post.annotate("bar").merge(Post.annotate("foo")).merge(posts).to_a
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class MergingDifferentRelationsTest < ActiveRecord::TestCase
|
||||
|
|
Loading…
Reference in a new issue