From 2580b83f424678d91d2d6cf38205521dd7eb4b0a Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Fri, 15 May 2020 15:13:59 +0900 Subject: [PATCH] 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. https://github.com/rails/rails/commit/bed9179aa1496f6d28891cf515af0d7e515ebbab 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. https://github.com/rails/rails/commit/a5cdf0b9eb860c4370ae5fde231e1b61f71b6b65 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 # } ``` --- activerecord/CHANGELOG.md | 42 +++++++++++++++++++ .../active_record/relation/calculations.rb | 10 +++++ .../active_record/relation/query_methods.rb | 23 +++++++++- activerecord/test/cases/calculations_test.rb | 19 ++++++++- .../test/cases/relation/merging_test.rb | 28 +++++++++++++ 5 files changed, 119 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index a28f835476..b216597417 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -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 diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index ef2fec6788..ebf9b19e32 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -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) diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 40fc35d960..c559a5d540 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -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 diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 37a87c3fb8..5fae3aaefa 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -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 diff --git a/activerecord/test/cases/relation/merging_test.rb b/activerecord/test/cases/relation/merging_test.rb index d0de9254bc..91ceaa1641 100644 --- a/activerecord/test/cases/relation/merging_test.rb +++ b/activerecord/test/cases/relation/merging_test.rb @@ -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