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.

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 aggregate functions to return numeric value consistently even on custom attribute type #39039

Merged
merged 1 commit into from Apr 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
* Fix aggregate functions to return numeric value consistently even on custom attribute type.

*Ryuta Kamizono*

* Support bulk insert/upsert on relation to preserve scope values.

*Josef Šimánek*, *Ryuta Kamizono*
Expand Down
29 changes: 13 additions & 16 deletions activerecord/lib/active_record/relation/calculations.rb
Expand Up @@ -286,13 +286,12 @@ def operation_over_aggregate_column(column, operation, distinct)
end

def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
column_alias = column_name

if operation == "count" && (column_name == :all && distinct || has_limit_or_offset?)
# Shortcut when limit is zero.
return 0 if limit_value == 0

query_builder = build_count_subquery(spawn, column_name, distinct)
skip_query_cache_if_necessary { @klass.connection.select_value(query_builder) }
else
# PostgreSQL doesn't like ORDER BY when there are no GROUP BY
relation = unscope(:order).distinct!(false)
Expand All @@ -309,16 +308,14 @@ def execute_simple_calculation(operation, column_name, distinct) #:nodoc:
relation.select_values = [select_value]

query_builder = relation.arel
end

result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder, nil) }
row = result.first
value = row && row.values.first
type = result.column_types.fetch(column_alias) do
type_for(column_name)
end
result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder, nil) }
row = result.rows.first
value = row && row.first
type = result.column_types.fetch(column_alias, Type.default_value)

type_cast_calculated_value(value, type, operation)
type_cast_calculated_value(value, type, operation)
end
end

def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
Expand Down Expand Up @@ -367,19 +364,19 @@ def execute_grouped_calculation(operation, column_name, distinct) #:nodoc:
key_records = key_records.index_by(&:id)
end

Hash[calculated_data.map do |row|
calculated_data.each_with_object({}) do |row, result|
key = group_columns.map { |aliaz, col_name|
type = type_for(col_name) do
calculated_data.column_types.fetch(aliaz, Type.default_value)
end
type_cast_calculated_value(row[aliaz], type)
type.deserialize(row[aliaz])
}
key = key.first if key.size == 1
key = key_records[key] if associated

type = calculated_data.column_types.fetch(aggregate_alias) { type_for(column_name) }
[key, type_cast_calculated_value(row[aggregate_alias], type, operation)]
end]
type = calculated_data.column_types.fetch(aggregate_alias, Type.default_value)
result[key] = type_cast_calculated_value(row[aggregate_alias], type, operation)
end
end

# Converts the given field to the value that the database adapter returns as
Expand All @@ -404,7 +401,7 @@ def type_for(field, &block)
@klass.type_for_attribute(field_name, &block)
end

def type_cast_calculated_value(value, type, operation = nil)
def type_cast_calculated_value(value, type, operation)
case operation
when "count" then value.to_i
when "sum" then type.deserialize(value || 0)
Expand Down
11 changes: 11 additions & 0 deletions activerecord/test/cases/calculations_test.rb
Expand Up @@ -990,6 +990,17 @@ def test_group_by_attribute_with_custom_type
assert_equal({ "proposed" => 2, "published" => 2 }, Book.group(:status).count)
end

def test_aggregate_attribute_on_custom_type
assert_equal 4, Book.sum(:status)
assert_equal 1, Book.sum(:difficulty)
assert_equal 0, Book.minimum(:status)
assert_equal 1, Book.maximum(:difficulty)
assert_equal({ "proposed" => 0, "published" => 4 }, Book.group(:status).sum(:status))
assert_equal({ "proposed" => 0, "published" => 1 }, Book.group(:status).sum(:difficulty))
assert_equal({ "proposed" => 0, "published" => 2 }, Book.group(:status).minimum(:status))
assert_equal({ "proposed" => 0, "published" => 1 }, Book.group(:status).maximum(:difficulty))
end

def test_select_avg_with_group_by_as_virtual_attribute_with_sql
rails_core = companies(:rails_core)

Expand Down