From c93dbfef36c9b095121650beec2362de42d6b715 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 9 Jun 2014 12:30:12 -0600 Subject: [PATCH] Make `_before_type_cast` actually be before type cast - The following is now true for all types, all the time - `model.attribute_before_type_cast == given_value` - `model.attribute == model.save_and_reload.attribute` - `model.attribute == model.dup.attribute` - `model.attribute == YAML.load(YAML.dump(model)).attribute` - Removes the remaining types implementing `type_cast_for_write` - Simplifies the implementation of time zone aware attributes - Brings tz aware attributes closer to being implemented as an attribute decorator - Adds additional point of control for custom types --- .../attribute_methods/time_zone_conversion.rb | 53 +++++-------------- .../active_record/attribute_methods/write.rb | 10 ++-- .../connection_adapters/column.rb | 2 +- .../postgresql/oid/hstore.rb | 6 ++- .../postgresql/oid/json.rb | 6 ++- .../lib/active_record/model_schema.rb | 8 ++- activerecord/lib/active_record/type/binary.rb | 9 ++++ .../lib/active_record/type/serialized.rb | 8 +-- activerecord/lib/active_record/type/value.rb | 10 ++-- .../adapters/postgresql/composite_test.rb | 6 ++- .../cases/adapters/postgresql/hstore_test.rb | 1 + .../cases/adapters/postgresql/json_test.rb | 1 + .../test/cases/attribute_decorators_test.rb | 2 + activerecord/test/cases/dirty_test.rb | 6 +-- activerecord/test/cases/reflection_test.rb | 2 +- 15 files changed, 65 insertions(+), 65 deletions(-) diff --git a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb index c1c3987cf5..684be2e845 100644 --- a/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb +++ b/activerecord/lib/active_record/attribute_methods/time_zone_conversion.rb @@ -1,21 +1,22 @@ module ActiveRecord module AttributeMethods module TimeZoneConversion - class Type # :nodoc: - delegate :type, :type_cast_for_database, to: :@column - - def initialize(column) - @column = column - end - + class Type < SimpleDelegator # :nodoc: def type_cast(value) - value = @column.type_cast(value) - convert_value_to_time_zone(value) + convert_time_to_time_zone(super) end - def convert_value_to_time_zone(value) + def type_cast_from_user(value) if value.is_a?(Array) - value.map { |v| convert_value_to_time_zone(v) } + value.map { |v| type_cast_from_user(v) } + elsif value.respond_to?(:in_time_zone) + value.in_time_zone + end + end + + def convert_time_to_time_zone(value) + if value.is_a?(Array) + value.map { |v| convert_time_to_time_zone(v) } elsif value.acts_like?(:time) value.in_time_zone else @@ -35,26 +36,6 @@ def convert_value_to_time_zone(value) end module ClassMethods - protected - # Defined for all +datetime+ attributes when +time_zone_aware_attributes+ are enabled. - # This enhanced write method will automatically convert the time passed to it to the zone stored in Time.zone. - def define_method_attribute=(attr_name) - if create_time_zone_conversion_attribute?(attr_name, columns_hash[attr_name]) - method_body, line = <<-EOV, __LINE__ + 1 - def #{attr_name}=(time) - time_with_zone = convert_value_to_time_zone(time) - previous_time = attribute_changed?("#{attr_name}") ? changed_attributes["#{attr_name}"] : read_attribute(:#{attr_name}) - write_attribute(:#{attr_name}, time) - #{attr_name}_will_change! if previous_time != time_with_zone - @attributes["#{attr_name}"] = time_with_zone - end - EOV - generated_attribute_methods.module_eval(method_body, __FILE__, line) - else - super - end - end - private def create_time_zone_conversion_attribute?(name, column) time_zone_aware_attributes && @@ -62,16 +43,6 @@ def create_time_zone_conversion_attribute?(name, column) (:datetime == column.type) end end - - private - - def convert_value_to_time_zone(value) - if value.is_a?(Array) - value.map { |v| convert_value_to_time_zone(v) } - elsif value.respond_to?(:in_time_zone) - value.in_time_zone - end - end end end end diff --git a/activerecord/lib/active_record/attribute_methods/write.rb b/activerecord/lib/active_record/attribute_methods/write.rb index 5203b30462..b72a6219b0 100644 --- a/activerecord/lib/active_record/attribute_methods/write.rb +++ b/activerecord/lib/active_record/attribute_methods/write.rb @@ -70,7 +70,7 @@ def write_attribute_with_type_cast(attr_name, value, should_type_cast) attr_name = attr_name.to_s attr_name = self.class.primary_key if attr_name == 'id' && self.class.primary_key @attributes.delete(attr_name) - column = column_for_attribute(attr_name) + column = type_for_attribute(attr_name) unless has_attribute?(attr_name) || self.class.columns_hash.key?(attr_name) raise ActiveModel::MissingAttributeError, "can't write unknown attribute `#{attr_name}'" @@ -80,13 +80,11 @@ def write_attribute_with_type_cast(attr_name, value, should_type_cast) # so we don't attempt to typecast multiple times. if column.binary? @attributes[attr_name] = value + elsif should_type_cast + @attributes[attr_name] = column.type_cast_from_user(value) end - if should_type_cast - @raw_attributes[attr_name] = column.type_cast_for_write(value) - else - @raw_attributes[attr_name] = value - end + @raw_attributes[attr_name] = value end end end diff --git a/activerecord/lib/active_record/connection_adapters/column.rb b/activerecord/lib/active_record/connection_adapters/column.rb index 23434df1fe..22decbc2da 100644 --- a/activerecord/lib/active_record/connection_adapters/column.rb +++ b/activerecord/lib/active_record/connection_adapters/column.rb @@ -17,7 +17,7 @@ module Format delegate :type, :precision, :scale, :limit, :klass, :accessor, :text?, :number?, :binary?, :serialized?, :changed?, - :type_cast, :type_cast_for_write, :type_cast_for_database, + :type_cast, :type_cast_from_user, :type_cast_for_database, :type_cast_for_schema, to: :cast_type diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb index a65ca83f77..0a48a14b06 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/hstore.rb @@ -7,7 +7,11 @@ def type :hstore end - def type_cast_for_write(value) + def type_cast_from_user(value) + type_cast(type_cast_for_database(value)) + end + + def type_cast_for_database(value) ConnectionAdapters::PostgreSQLColumn.hstore_to_string(value) end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb index c87422fe32..c64cf27797 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb @@ -7,7 +7,11 @@ def type :json end - def type_cast_for_write(value) + def type_cast_from_user(value) + type_cast(type_cast_for_database(value)) + end + + def type_cast_for_database(value) ConnectionAdapters::PostgreSQLColumn.json_to_string(value) end diff --git a/activerecord/lib/active_record/model_schema.rb b/activerecord/lib/active_record/model_schema.rb index baf2b5fbf8..f96f77f696 100644 --- a/activerecord/lib/active_record/model_schema.rb +++ b/activerecord/lib/active_record/model_schema.rb @@ -51,6 +51,8 @@ module ModelSchema self.pluralize_table_names = true self.inheritance_column = 'type' + + delegate :type_for_attribute, to: :class end module ClassMethods @@ -221,6 +223,10 @@ def column_types # :nodoc: @column_types ||= decorate_columns(columns_hash.dup) end + def type_for_attribute(attr_name) # :nodoc: + column_types.fetch(attr_name) { column_for_attribute(attr_name) } + end + def decorate_columns(columns_hash) # :nodoc: return if columns_hash.empty? @@ -245,7 +251,7 @@ def column_defaults # are the default values suitable for use in `@raw_attriubtes` def raw_column_defaults # :nodoc: @raw_column_defauts ||= Hash[column_defaults.map { |name, default| - [name, columns_hash[name].type_cast_for_write(default)] + [name, columns_hash[name].type_cast_for_database(default)] }] end diff --git a/activerecord/lib/active_record/type/binary.rb b/activerecord/lib/active_record/type/binary.rb index bc93f6e1bf..3bf29b5026 100644 --- a/activerecord/lib/active_record/type/binary.rb +++ b/activerecord/lib/active_record/type/binary.rb @@ -9,7 +9,16 @@ def binary? true end + def type_cast(value) + if value.is_a?(Data) + value.to_s + else + super + end + end + def type_cast_for_database(value) + return if value.nil? Data.new(super) end diff --git a/activerecord/lib/active_record/type/serialized.rb b/activerecord/lib/active_record/type/serialized.rb index 0866383de9..a1205fa819 100644 --- a/activerecord/lib/active_record/type/serialized.rb +++ b/activerecord/lib/active_record/type/serialized.rb @@ -17,15 +17,17 @@ def type_cast(value) end end - def type_cast_for_write(value) + def type_cast_from_user(value) + type_cast(type_cast_for_database(value)) + end + + def type_cast_for_database(value) return if value.nil? unless is_default_value?(value) super coder.dump(value) end end - alias type_cast_for_database type_cast_for_write - def serialized? true end diff --git a/activerecord/lib/active_record/type/value.rb b/activerecord/lib/active_record/type/value.rb index 1c41b28646..deacc398ef 100644 --- a/activerecord/lib/active_record/type/value.rb +++ b/activerecord/lib/active_record/type/value.rb @@ -23,8 +23,12 @@ def type_cast(value) cast_value(value) unless value.nil? end + def type_cast_from_user(value) + type_cast(value) + end + def type_cast_for_database(value) - type_cast_for_write(value) + value end def type_cast_for_schema(value) @@ -50,10 +54,6 @@ def serialized? def klass # :nodoc: end - def type_cast_for_write(value) # :nodoc: - value - end - # +old_value+ will always be type-cast. # +new_value+ will come straight from the database # or from assignment, so it could be anything. Types diff --git a/activerecord/test/cases/adapters/postgresql/composite_test.rb b/activerecord/test/cases/adapters/postgresql/composite_test.rb index a925263098..0b48fe9af8 100644 --- a/activerecord/test/cases/adapters/postgresql/composite_test.rb +++ b/activerecord/test/cases/adapters/postgresql/composite_test.rb @@ -90,7 +90,11 @@ def type_cast(value) end end - def type_cast_for_write(value) + def type_cast_from_user(value) + value + end + + def type_cast_for_database(value) return if value.nil? "(#{value.city},#{value.street})" end diff --git a/activerecord/test/cases/adapters/postgresql/hstore_test.rb b/activerecord/test/cases/adapters/postgresql/hstore_test.rb index a6482786c7..0b8c9fc052 100644 --- a/activerecord/test/cases/adapters/postgresql/hstore_test.rb +++ b/activerecord/test/cases/adapters/postgresql/hstore_test.rb @@ -106,6 +106,7 @@ def change def test_cast_value_on_write x = Hstore.new tags: {"bool" => true, "number" => 5} + assert_equal({"bool" => true, "number" => 5}, x.tags_before_type_cast) assert_equal({"bool" => "true", "number" => "5"}, x.tags) x.save assert_equal({"bool" => "true", "number" => "5"}, x.reload.tags) diff --git a/activerecord/test/cases/adapters/postgresql/json_test.rb b/activerecord/test/cases/adapters/postgresql/json_test.rb index 61d4e2b8ae..eab6049956 100644 --- a/activerecord/test/cases/adapters/postgresql/json_test.rb +++ b/activerecord/test/cases/adapters/postgresql/json_test.rb @@ -68,6 +68,7 @@ def test_change_table_supports_json def test_cast_value_on_write x = JsonDataType.new payload: {"string" => "foo", :symbol => :bar} + assert_equal({"string" => "foo", :symbol => :bar}, x.payload_before_type_cast) assert_equal({"string" => "foo", "symbol" => "bar"}, x.payload) x.save assert_equal({"string" => "foo", "symbol" => "bar"}, x.reload.payload) diff --git a/activerecord/test/cases/attribute_decorators_test.rb b/activerecord/test/cases/attribute_decorators_test.rb index f17cc02f53..416eb650f6 100644 --- a/activerecord/test/cases/attribute_decorators_test.rb +++ b/activerecord/test/cases/attribute_decorators_test.rb @@ -15,6 +15,8 @@ def initialize(delegate, decoration = "decorated!") def type_cast(value) "#{super} #{@decoration}" end + + alias type_cast_from_user type_cast end setup do diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index 2beac84fb1..87f24e32b2 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -616,17 +616,15 @@ def test_datetime_attribute_doesnt_change_if_zone_is_modified_in_string end end - test "defaults with type that implements `type_cast_for_write`" do + test "defaults with type that implements `type_cast_for_database`" do type = Class.new(ActiveRecord::Type::Value) do def type_cast(value) value.to_i end - def type_cast_for_write(value) + def type_cast_for_database(value) value.to_s end - - alias type_cast_for_database type_cast_for_write end model_class = Class.new(ActiveRecord::Base) do diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index 793e193329..70d52634f7 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -96,7 +96,7 @@ def test_non_existent_columns_are_identity_types object = Object.new assert_equal object, column.type_cast(object) - assert_equal object, column.type_cast_for_write(object) + assert_equal object, column.type_cast_from_user(object) assert_equal object, column.type_cast_for_database(object) end