Introduce an Attribute object to handle the type casting dance

There's a lot more that can be moved to these, but this felt like a good
place to introduce the object. Plans are:

- Remove all knowledge of type casting from the columns, beyond a
  reference to the cast_type
- Move type_cast_for_database to these objects
- Potentially make them mutable, introduce a state machine, and have
  dirty checking handled here as well
- Move `attribute`, `decorate_attribute`, and anything else that
  modifies types to mess with this object, not the columns hash
- Introduce a collection object to manage these, reduce allocations, and
  not require serializing the types

activerecord/lib/active_record.rb

@@ -31,6 +31,7 @@ require 'active_record/version'
 module ActiveRecord
   extend ActiveSupport::Autoload
 
+  autoload :Attribute
   autoload :Base
   autoload :Callbacks
   autoload :Core

activerecord/lib/active_record/attribute.rb

@@ -0,0 +1,56 @@
+module ActiveRecord
+  class Attribute # :nodoc:
+    class << self
+      def from_database(value, type)
+        FromDatabase.new(value, type)
+      end
+
+      def from_user(value, type)
+        FromUser.new(value, type)
+      end
+    end
+
+    attr_reader :value_before_type_cast, :type
+
+    # This method should not be called directly.
+    # Use #from_database or #from_user
+    def initialize(value_before_type_cast, type)
+      @value_before_type_cast = value_before_type_cast
+      @type = type
+    end
+
+    def value
+      # `defined?` is cheaper than `||=` when we get back falsy values
+      @value = type_cast(value_before_type_cast) unless defined?(@value)
+      @value
+    end
+
+    def value_for_database
+      type.type_cast_for_database(value)
+    end
+
+    def type_cast
+      raise NotImplementedError
+    end
+
+    protected
+
+    def initialize_dup(other)
+      if defined?(@value) && @value.duplicable?
+        @value = @value.dup
+      end
+    end
+
+    class FromDatabase < Attribute
+      def type_cast(value)
+        type.type_cast_from_database(value)
+      end
+    end
+
+    class FromUser < Attribute
+      def type_cast(value)
+        type.type_cast_from_user(value)
+      end
+    end
+  end
+end

activerecord/lib/active_record/attribute_methods.rb

@@ -261,9 +261,9 @@ module ActiveRecord
 
       # If the result is true then check for the select case.
       # For queries selecting a subset of columns, return false for unselected columns.
-      # We check defined?(@raw_attributes) not to issue warnings if called on objects that
+      # We check defined?(@attributes) not to issue warnings if called on objects that
       # have been allocated but not yet initialized.
-      if defined?(@raw_attributes) && @raw_attributes.any? && self.class.column_names.include?(name)
+      if defined?(@attributes) && @attributes.any? && self.class.column_names.include?(name)
         return has_attribute?(name)
       end
 
@@ -280,7 +280,7 @@ module ActiveRecord
     #   person.has_attribute?('age')    # => true
     #   person.has_attribute?(:nothing) # => false
     def has_attribute?(attr_name)
-      @raw_attributes.has_key?(attr_name.to_s)
+      @attributes.has_key?(attr_name.to_s)
     end
 
     # Returns an array of names for the attributes available on this object.
@@ -292,7 +292,7 @@ module ActiveRecord
     #   person.attribute_names
     #   # => ["id", "created_at", "updated_at", "name", "age"]
     def attribute_names
-      @raw_attributes.keys
+      @attributes.keys
     end
 
     # Returns a hash of all the attributes with their names as keys and the values of the attributes as values.
@@ -400,11 +400,10 @@ module ActiveRecord
 
     protected
 
-    def clone_attributes(reader_method = :read_attribute, attributes = {}) # :nodoc:
-      attribute_names.each do |name|
-        attributes[name] = clone_attribute_value(reader_method, name)
+    def clone_attributes # :nodoc:
+      @attributes.each_with_object({}) do |(name, attr), h|
+        h[name] = attr.dup
       end
-      attributes
     end
 
     def clone_attribute_value(reader_method, attribute_name) # :nodoc:
@@ -424,7 +423,7 @@ module ActiveRecord
 
     def attribute_method?(attr_name) # :nodoc:
       # We check defined? because Syck calls respond_to? before actually calling initialize.
-      defined?(@raw_attributes) && @raw_attributes.include?(attr_name)
+      defined?(@attributes) && @attributes.include?(attr_name)
     end
 
     private
@@ -465,9 +464,6 @@ module ActiveRecord
     end
 
     def typecasted_attribute_value(name)
-      # FIXME: we need @attributes to be used consistently.
-      # If the values stored in @attributes were already typecasted, this code
-      # could be simplified
       read_attribute(name)
     end
   end

activerecord/lib/active_record/attribute_methods/before_type_cast.rb

@@ -43,7 +43,9 @@ module ActiveRecord
       #   task.read_attribute_before_type_cast('completed_on') # => "2012-10-21"
       #   task.read_attribute_before_type_cast(:completed_on)  # => "2012-10-21"
       def read_attribute_before_type_cast(attr_name)
-        @raw_attributes[attr_name.to_s]
+        if attr = @attributes[attr_name.to_s]
+          attr.value_before_type_cast
+        end
       end
 
       # Returns a hash of attributes before typecasting and deserialization.
@@ -57,7 +59,7 @@ module ActiveRecord
       #   task.attributes_before_type_cast
       #   # => {"id"=>nil, "title"=>nil, "is_done"=>true, "completed_on"=>"2012-10-21", "created_at"=>nil, "updated_at"=>nil}
       def attributes_before_type_cast
-        @raw_attributes
+        @attributes.each_with_object({}) { |(k, v), h| h[k] = v.value_before_type_cast }
       end
 
       private

activerecord/lib/active_record/attribute_methods/read.rb

@@ -82,25 +82,16 @@ module ActiveRecord
       # it has been typecast (for example, "2004-12-12" in a date column is cast
       # to a date object, like Date.new(2004, 12, 12)).
       def read_attribute(attr_name)
-        # If it's cached, just return it
-        # We use #[] first as a perf optimization for non-nil values. See https://gist.github.com/jonleighton/3552829.
         name = attr_name.to_s
-        @attributes[name] || @attributes.fetch(name) {
-          column = @column_types_override[name] if @column_types_override
-          column ||= @column_types[name]
-
-          return @raw_attributes.fetch(name) {
-            if name == 'id' && self.class.primary_key != name
-              read_attribute(self.class.primary_key)
-            end
-          } unless column
-
-          value = @raw_attributes.fetch(name) {
-            return block_given? ? yield(name) : nil
-          }
-
-          @attributes[name] = column.type_cast_from_database(value)
-        }
+        @attributes.fetch(name) {
+          if name == 'id'
+            return read_attribute(self.class.primary_key)
+          elsif block_given? && self.class.columns_hash.key?(name)
+            return yield(name)
+          else
+            return nil
+          end
+        }.value
       end
 
       private

activerecord/lib/active_record/attribute_methods/write.rb

@@ -69,22 +69,19 @@ module ActiveRecord
       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 = type_for_attribute(attr_name)
+        type = 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}'"
         end
 
-        # If we're dealing with a binary column, write the data to the cache
-        # 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)
+        if should_type_cast
+          @attributes[attr_name] = Attribute.from_user(value, type)
+        else
+          @attributes[attr_name] = Attribute.from_database(value, type)
         end
 
-        @raw_attributes[attr_name] = value
+        value
       end
     end
   end

activerecord/lib/active_record/connection_adapters/postgresql/oid/bytea.rb

@@ -3,8 +3,9 @@ module ActiveRecord
     module PostgreSQL
       module OID # :nodoc:
         class Bytea < Type::Binary
-          def cast_value(value)
-            PGconn.unescape_bytea value
+          def type_cast_from_database(value)
+            return if value.nil?
+            PGconn.unescape_bytea(super)
           end
         end
       end

activerecord/lib/active_record/core.rb

@@ -249,10 +249,13 @@ module ActiveRecord
     #   # Instantiates a single new object
     #   User.new(first_name: 'Jamie')
     def initialize(attributes = nil, options = {})
-      defaults = self.class.raw_column_defaults.dup
-      defaults.each { |k, v| defaults[k] = v.dup if v.duplicable? }
+      defaults = {}
+      self.class.raw_column_defaults.each do |k, v|
+        default = v.duplicable? ? v.dup : v
+        defaults[k] = Attribute.from_database(default, type_for_attribute(k))
+      end
 
-      @raw_attributes = defaults
+      @attributes = defaults
       @column_types_override = nil
       @column_types = self.class.column_types
 
@@ -278,13 +281,12 @@ module ActiveRecord
     #   post.init_with('attributes' => { 'title' => 'hello world' })
     #   post.title # => 'hello world'
     def init_with(coder)
-      @raw_attributes = coder['raw_attributes']
+      @attributes = coder['attributes']
       @column_types_override = coder['column_types']
       @column_types = self.class.column_types
 
       init_internals
 
-      @attributes = coder['attributes'] if coder['attributes']
       @new_record = coder['new_record']
 
       self.class.define_attribute_methods
@@ -323,12 +325,9 @@ module ActiveRecord
 
     ##
     def initialize_dup(other) # :nodoc:
-      cloned_attributes = other.clone_attributes(:read_attribute_before_type_cast)
-
-      @raw_attributes = cloned_attributes
-      @raw_attributes[self.class.primary_key] = nil
-      @attributes = other.clone_attributes(:read_attribute)
-      @attributes[self.class.primary_key] = nil
+      pk = self.class.primary_key
+      @attributes = other.clone_attributes
+      @attributes[pk] = Attribute.from_database(nil, type_for_attribute(pk))
 
       run_callbacks(:initialize) unless _initialize_callbacks.empty?
 
@@ -354,7 +353,8 @@ module ActiveRecord
     #   Post.new.encode_with(coder)
     #   coder # => {"attributes" => {"id" => nil, ... }}
     def encode_with(coder)
-      coder['raw_attributes'] = @raw_attributes
+      # FIXME: Remove this when we better serialize attributes
+      coder['raw_attributes'] = attributes_before_type_cast
       coder['attributes'] = @attributes
       coder['column_types'] = @column_types_override
       coder['new_record'] = new_record?
@@ -387,13 +387,13 @@ module ActiveRecord
     # accessible, even on destroyed records, but cloned models will not be
     # frozen.
     def freeze
-      @raw_attributes = @raw_attributes.clone.freeze
+      @attributes = @attributes.clone.freeze
       self
     end
 
     # Returns +true+ if the attributes hash has been frozen.
     def frozen?
-      @raw_attributes.frozen?
+      @attributes.frozen?
     end
 
     # Allows sort on objects
@@ -422,9 +422,9 @@ module ActiveRecord
 
     # Returns the contents of the record as a nicely formatted string.
     def inspect
-      # We check defined?(@raw_attributes) not to issue warnings if the object is
+      # We check defined?(@attributes) not to issue warnings if the object is
       # allocated but not initialized.
-      inspection = if defined?(@raw_attributes) && @raw_attributes
+      inspection = if defined?(@attributes) && @attributes
                      self.class.column_names.collect { |name|
                        if has_attribute?(name)
                          "#{name}: #{attribute_for_inspect(name)}"
@@ -523,11 +523,10 @@ module ActiveRecord
 
     def init_internals
       pk = self.class.primary_key
-      @raw_attributes[pk] = nil unless @raw_attributes.key?(pk)
+      @attributes[pk] ||= Attribute.from_database(nil, type_for_attribute(pk))
 
       @aggregation_cache        = {}
       @association_cache        = {}
-      @attributes               = {}
       @readonly                 = false
       @destroyed                = false
       @marked_for_destruction   = false
@@ -550,7 +549,7 @@ module ActiveRecord
 
     def thaw
       if frozen?
-        @raw_attributes = @raw_attributes.dup
+        @attributes = @attributes.dup
       end
     end
   end

activerecord/lib/active_record/persistence.rb

@@ -48,8 +48,14 @@ module ActiveRecord
       # how this "single-table" inheritance mapping is implemented.
       def instantiate(attributes, column_types = {})
         klass = discriminate_class_for_record(attributes)
+
+        attributes = attributes.each_with_object({}) do |(name, value), h|
+          type = column_types.fetch(name) { klass.type_for_attribute(name) }
+          h[name] = Attribute.from_database(value, type)
+        end
+
         klass.allocate.init_with(
-          'raw_attributes' => attributes,
+          'attributes' => attributes,
           'column_types' => column_types,
           'new_record' => false,
         )
@@ -182,7 +188,6 @@ module ActiveRecord
     # So any change to the attributes in either instance will affect the other.
     def becomes(klass)
       became = klass.new
-      became.instance_variable_set("@raw_attributes", @raw_attributes)
       became.instance_variable_set("@attributes", @attributes)
       became.instance_variable_set("@changed_attributes", @changed_attributes) if defined?(@changed_attributes)
       became.instance_variable_set("@new_record", new_record?)
@@ -399,11 +404,10 @@ module ActiveRecord
           self.class.unscoped { self.class.find(id) }
         end
 
-      @raw_attributes.update(fresh_object.instance_variable_get('@raw_attributes'))
+      @attributes.update(fresh_object.instance_variable_get('@attributes'))
 
       @column_types           = self.class.column_types
       @column_types_override  = fresh_object.instance_variable_get('@column_types_override')
-      @attributes             = {}
       self
     end
 

activerecord/lib/active_record/result.rb

@@ -95,7 +95,7 @@ module ActiveRecord
       @hash_rows ||=
         begin
           # We freeze the strings to prevent them getting duped when
-          # used as keys in ActiveRecord::Base's @raw_attributes hash
+          # used as keys in ActiveRecord::Base's @attributes hash
           columns = @columns.map { |c| c.dup.freeze }
           @rows.map { |row|
             # In the past we used Hash[columns.zip(row)]

activerecord/test/cases/adapters/postgresql/composite_test.rb

@@ -84,7 +84,7 @@ class PostgresqlCompositeWithCustomOIDTest < ActiveRecord::TestCase
   class FullAddressType < ActiveRecord::Type::Value
     def type; :full_address end
 
-    def type_cast(value)
+    def type_cast_from_database(value)
       if value =~ /\("?([^",]*)"?,"?([^",]*)"?\)/
         FullAddress.new($1, $2)
       end

activerecord/test/cases/attribute_test.rb

@@ -0,0 +1,103 @@
+require 'cases/helper'
+require 'minitest/mock'
+
+module ActiveRecord
+  class AttributeTest < ActiveRecord::TestCase
+    setup do
+      @type = MiniTest::Mock.new
+    end
+
+    teardown do
+      assert @type.verify
+    end
+
+    test "from_database + read type casts from database" do
+      @type.expect(:type_cast_from_database, 'type cast from database', ['a value'])
+      attribute = Attribute.from_database('a value', @type)
+
+      type_cast_value = attribute.value
+
+      assert_equal 'type cast from database', type_cast_value
+    end
+
+    test "from_user + read type casts from user" do
+      @type.expect(:type_cast_from_user, 'type cast from user', ['a value'])
+      attribute = Attribute.from_user('a value', @type)
+
+      type_cast_value = attribute.value
+
+      assert_equal 'type cast from user', type_cast_value
+    end
+
+    test "reading memoizes the value" do
+      @type.expect(:type_cast_from_database, 'from the database', ['whatever'])
+      attribute = Attribute.from_database('whatever', @type)
+
+      type_cast_value = attribute.value
+      second_read = attribute.value
+
+      assert_equal 'from the database', type_cast_value
+      assert_same type_cast_value, second_read
+    end
+
+    test "reading memoizes falsy values" do
+      @type.expect(:type_cast_from_database, false, ['whatever'])
+      attribute = Attribute.from_database('whatever', @type)
+
+      attribute.value
+      attribute.value
+    end
+
+    test "read_before_typecast returns the given value" do
+      attribute = Attribute.from_database('raw value', @type)
+
+      raw_value = attribute.value_before_type_cast
+
+      assert_equal 'raw value', raw_value
+    end
+
+    test "from_database + read_for_database type casts to and from database" do
+      @type.expect(:type_cast_from_database, 'read from database', ['whatever'])
+      @type.expect(:type_cast_for_database, 'ready for database', ['read from database'])
+      attribute = Attribute.from_database('whatever', @type)
+
+      type_cast_for_database = attribute.value_for_database
+
+      assert_equal 'ready for database', type_cast_for_database
+    end
+
+    test "from_user + read_for_database type casts from the user to the database" do
+      @type.expect(:type_cast_from_user, 'read from user', ['whatever'])
+      @type.expect(:type_cast_for_database, 'ready for database', ['read from user'])
+      attribute = Attribute.from_user('whatever', @type)
+
+      type_cast_for_database = attribute.value_for_database
+
+      assert_equal 'ready for database', type_cast_for_database
+    end
+
+    test "duping dups the value" do
+      @type.expect(:type_cast_from_database, 'type cast', ['a value'])
+      attribute = Attribute.from_database('a value', @type)
+
+      value_from_orig = attribute.value
+      value_from_clone = attribute.dup.value
+      value_from_orig << ' foo'
+
+      assert_equal 'type cast foo', value_from_orig
+      assert_equal 'type cast', value_from_clone
+    end
+
+    test "duping does not dup the value if it is not dupable" do
+      @type.expect(:type_cast_from_database, false, ['a value'])
+      attribute = Attribute.from_database('a value', @type)
+
+      assert_same attribute.value, attribute.dup.value
+    end
+
+    test "duping does not eagerly type cast if we have not yet type cast" do
+      attribute = Attribute.from_database('a value', @type)
+      attribute.dup
+    end
+  end
+end

activerecord/test/cases/base_test.rb

@@ -1488,15 +1488,14 @@ class BasicsTest < ActiveRecord::TestCase
     attrs = topic.attributes.dup
     attrs.delete 'id'
 
-    typecast = Class.new {
-      def type_cast_from_database value
+    typecast = Class.new(ActiveRecord::Type::Value) {
+      def type_cast value
         "t.lo"
       end
     }
 
     types = { 'author_name' => typecast.new }
-    topic = Topic.allocate.init_with 'raw_attributes' => attrs,
-                                     'column_types' => types
+    topic = Topic.instantiate(attrs, types)
 
     assert_equal 't.lo', topic.author_name
   end

activerecord/test/cases/persistence_test.rb

@@ -250,11 +250,9 @@ class PersistenceTest < ActiveRecord::TestCase
   end
 
   def test_create_columns_not_equal_attributes
-    topic = Topic.allocate.init_with(
-      'raw_attributes' => {
-        'title'          => 'Another New Topic',
-        'does_not_exist' => 'test'
-      }
+    topic = Topic.instantiate(
+      'title'          => 'Another New Topic',
+      'does_not_exist' => 'test'
     )
     assert_nothing_raised { topic.save }
   end
@@ -300,10 +298,7 @@ class PersistenceTest < ActiveRecord::TestCase
     topic.title = "Still another topic"
     topic.save
 
-    topic_reloaded = Topic.allocate
-    topic_reloaded.init_with(
-      'raw_attributes' => topic.attributes.merge('does_not_exist' => 'test')
-    )
+    topic_reloaded = Topic.instantiate(topic.attributes.merge('does_not_exist' => 'test'))
     topic_reloaded.title = 'A New Topic'
     assert_nothing_raised { topic_reloaded.save }
   end

activerecord/test/cases/serialized_attribute_test.rb

@@ -36,12 +36,6 @@ class SerializedAttributeTest < ActiveRecord::TestCase
     assert_equal(myobj, topic.content)
   end
 
-  def test_serialized_attribute_init_with
-    topic = Topic.allocate
-    topic.init_with('raw_attributes' => { 'content' => '--- foo' })
-    assert_equal 'foo', topic.content
-  end
-
   def test_serialized_attribute_in_base_class
     Topic.serialize("content", Hash)
 

activerecord/test/cases/store_test.rb

@@ -189,7 +189,6 @@ class StoreTest < ActiveRecord::TestCase
     assert_equal @john, loaded
 
     second_dump = YAML.dump(loaded)
-    assert_equal dumped, second_dump
     assert_equal @john, YAML.load(second_dump)
   end
 end