From 11751b2ea20b6d9df7a4116e81c6bbc4dee018fb Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 13 May 2020 02:26:55 +0900 Subject: [PATCH] Fix `minimum` and `maximum` on time zone aware attributes This is the opposite direction of #39039. #39111 fixes `minimum` and `maximum` on date columns with type casting by column type on the database. But column type has no information for time zone aware attributes, it means that attribute type should always be precedence over column type. I've realized that fact in the related issue report #39248. I've reverted the expectation of #39039, to make time zone aware attributes works. --- activerecord/CHANGELOG.md | 4 -- .../active_record/relation/calculations.rb | 8 +-- activerecord/test/cases/calculations_test.rb | 50 +++++++++++++++++-- 3 files changed, 48 insertions(+), 14 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 5a9f0ff5e3..4b251d9684 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -62,10 +62,6 @@ *Ryuta Kamizono* -* 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* diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index decf847144..d4c0907d9b 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -307,9 +307,7 @@ module ActiveRecord result = skip_query_cache_if_necessary { @klass.connection.select_all(query_builder) } type_cast_calculated_value(result.cast_values.first, operation) do |value| - if value.is_a?(String) && - column = klass.columns_hash[column_name.to_s] - type = connection.lookup_cast_type_from_column(column) + if type = klass.attribute_types[column_name.to_s] type.deserialize(value) else value @@ -379,9 +377,7 @@ module ActiveRecord key = key_records[key] if associated result[key] = type_cast_calculated_value(row[column_alias], operation) do |value| - if value.is_a?(String) && - (type || column = klass.columns_hash[column_name.to_s]) - type ||= connection.lookup_cast_type_from_column(column) + if type ||= klass.attribute_types[column_name.to_s] type.deserialize(value) else value diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index 1a7aa3e849..c77436920f 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -1044,12 +1044,12 @@ class CalculationsTest < ActiveRecord::TestCase 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 "easy", Book.minimum(:difficulty) + assert_equal "medium", 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)) + assert_equal({ "proposed" => "easy", "published" => "easy" }, Book.group(:status).minimum(:difficulty)) + assert_equal({ "proposed" => "easy", "published" => "medium" }, Book.group(:status).maximum(:difficulty)) end def test_minimum_and_maximum_on_non_numeric_type @@ -1059,6 +1059,48 @@ class CalculationsTest < ActiveRecord::TestCase assert_equal({ false => Date.new(2004, 4, 15), true => nil }, Topic.group(:approved).maximum(:last_read)) end + def test_minimum_and_maximum_on_time_attributes + assert_minimum_and_maximum_on_time_attributes(Time) + end + + def test_minimum_and_maximum_on_tz_aware_attributes + with_timezone_config aware_attributes: true, zone: "Pacific Time (US & Canada)" do + Topic.reset_column_information + assert_minimum_and_maximum_on_time_attributes(ActiveSupport::TimeWithZone) + end + ensure + Topic.reset_column_information + end + + def assert_minimum_and_maximum_on_time_attributes(time_class) + actual = Topic.minimum(:written_on) + assert_equal Time.utc(2003, 7, 16, 14, 28, 11, 223300), actual + assert_instance_of time_class, actual + + actual = Topic.maximum(:written_on) + assert_equal Time.utc(2013, 7, 13, 11, 11, 0, 9900), actual + assert_instance_of time_class, actual + + expected = { + false => Time.utc(2003, 7, 16, 14, 28, 11, 223300), + true => Time.utc(2004, 7, 15, 14, 28, 0, 9900), + } + actual = Topic.group(:approved).minimum(:written_on) + assert_equal expected, actual + assert_instance_of time_class, actual[true] + assert_instance_of time_class, actual[true] + + expected = { + false => Time.utc(2003, 7, 16, 14, 28, 11, 223300), + true => Time.utc(2013, 7, 13, 11, 11, 0, 9900), + } + actual = Topic.group(:approved).maximum(:written_on) + assert_equal expected, actual + assert_instance_of time_class, actual[true] + assert_instance_of time_class, actual[true] + end + private :assert_minimum_and_maximum_on_time_attributes + def test_select_avg_with_group_by_as_virtual_attribute_with_sql rails_core = companies(:rails_core)