From 31c15b4791a8581cbf354c22bbadb463a90bcd1a Mon Sep 17 00:00:00 2001 From: Dooor Date: Thu, 20 Oct 2022 14:40:48 +0900 Subject: [PATCH] Fix a bug where using groups and counts with long table names would return incorrect results. Fixed a bug that caused the alias name of "group by" to be too long and the first half of the name would be the same in both cases if it was cut by max identifier length. Fix #46285 Co-authored-by: Yusaku ONO --- activerecord/CHANGELOG.md | 4 ++ .../active_record/relation/calculations.rb | 64 +++++++++++++------ activerecord/test/cases/calculations_test.rb | 14 ++++ .../test/models/too_long_table_name.rb | 5 ++ activerecord/test/schema/schema.rb | 5 ++ 5 files changed, 73 insertions(+), 19 deletions(-) create mode 100644 activerecord/test/models/too_long_table_name.rb diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f282fb2a4d..68ce8300fd 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* Fix a bug where using groups and counts with long table names would return incorrect results. + + *Shota Toguchi, Yusaku Ono* + * Fix encryption of column default values. Previously, encrypted attributes that used column default values appeared to diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 5e72e7becd..44d07f3698 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -4,6 +4,47 @@ require "active_support/core_ext/enumerable" module ActiveRecord module Calculations + class ColumnAliasTracker # :nodoc: + def initialize(connection) + @connection = connection + @aliases = Hash.new(0) + end + + def alias_for(field) + aliased_name = column_alias_for(field) + + if @aliases[aliased_name] == 0 + @aliases[aliased_name] = 1 + aliased_name + else + # Update the count + count = @aliases[aliased_name] += 1 + "#{truncate(aliased_name)}_#{count}" + end + end + + private + # Converts the given field to the value that the database adapter returns as + # a usable column name: + # + # column_alias_for("users.id") # => "users_id" + # column_alias_for("sum(id)") # => "sum_id" + # column_alias_for("count(distinct users.id)") # => "count_distinct_users_id" + # column_alias_for("count(*)") # => "count_all" + def column_alias_for(field) + column_alias = +field + column_alias.gsub!(/\*/, "all") + column_alias.gsub!(/\W+/, " ") + column_alias.strip! + column_alias.gsub!(/ +/, "_") + @connection.table_alias_for(column_alias) + end + + def truncate(name) + name.slice(0, @connection.table_alias_length - 2) + end + end + # Count the records. # # Person.count @@ -386,14 +427,16 @@ module ActiveRecord end group_fields = arel_columns(group_fields) + column_alias_tracker = ColumnAliasTracker.new(connection) + group_aliases = group_fields.map { |field| field = connection.visitor.compile(field) if Arel.arel_node?(field) - column_alias_for(field.to_s.downcase) + column_alias_tracker.alias_for(field.to_s.downcase) } group_columns = group_aliases.zip(group_fields) column = aggregate_column(column_name) - column_alias = column_alias_for("#{operation} #{column_name.to_s.downcase}") + column_alias = column_alias_tracker.alias_for("#{operation} #{column_name.to_s.downcase}") select_value = operation_over_aggregate_column(column, operation, distinct) select_value.as(connection.quote_column_name(column_alias)) @@ -449,23 +492,6 @@ module ActiveRecord end end - # Converts the given field to the value that the database adapter returns as - # a usable column name: - # - # column_alias_for("users.id") # => "users_id" - # column_alias_for("sum(id)") # => "sum_id" - # column_alias_for("count(distinct users.id)") # => "count_distinct_users_id" - # column_alias_for("count(*)") # => "count_all" - def column_alias_for(field) - column_alias = +field - column_alias.gsub!(/\*/, "all") - column_alias.gsub!(/\W+/, " ") - column_alias.strip! - column_alias.gsub!(/ +/, "_") - - connection.table_alias_for(column_alias) - end - def type_for(field, &block) field_name = field.respond_to?(:name) ? field.name.to_s : field.to_s.split(".").last @klass.type_for_attribute(field_name, &block) diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index bb2e6bfe71..22522a24eb 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -21,6 +21,7 @@ require "models/developer" require "models/post" require "models/comment" require "models/rating" +require "models/too_long_table_name" require "support/stubs/strong_parameters" require "support/async_helper" @@ -145,6 +146,19 @@ class CalculationsTest < ActiveRecord::TestCase [ [nil, 50], [1, 50], [6, 50], [6, 55], [9, 53], [2, 60] ].each { |firm_and_limit| assert_includes c.keys, firm_and_limit } end + def test_should_group_by_multiple_fields_when_table_name_is_too_long + 2.times do + TooLongTableName.create!( + toooooooo_long_a_id: 1, + toooooooo_long_b_id: 2 + ) + end + + res = TooLongTableName.group(:toooooooo_long_a_id, :toooooooo_long_b_id).count + + assert_equal({ [1, 2] => 2 }, res) + end + def test_should_group_by_multiple_fields_having_functions c = Topic.group(:author_name, "COALESCE(type, title)").count(:all) assert_equal 1, c[["Carl", "The Third Topic of the day"]] diff --git a/activerecord/test/models/too_long_table_name.rb b/activerecord/test/models/too_long_table_name.rb new file mode 100644 index 0000000000..ce793e7d36 --- /dev/null +++ b/activerecord/test/models/too_long_table_name.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class TooLongTableName < ActiveRecord::Base + self.table_name = "toooooooooooooooooooooooooooooooooo_long_table_names" +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index daaf0dda19..3f402e9003 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -1317,6 +1317,11 @@ ActiveRecord::Schema.define do t.integer :id t.datetime :created_at end + + create_table :toooooooooooooooooooooooooooooooooo_long_table_names, force: true do |t| + t.bigint :toooooooo_long_a_id, null: false + t.bigint :toooooooo_long_b_id, null: false + end end Course.connection.create_table :courses, force: true do |t|