rm `Column#cast_type`

The type from the column is never used, except when being passed to the
attributes API. While leaving the type on the column wasn't necessarily
a bad thing, I worry that it's existence there implies that it is
something which should be used.

During the design and implementation process of the attributes API,
there have been plenty of cases where getting the "right" type object
was hard, but I had easy access to the column objects. For any
contributor who isn't intimately familiar with the intents behind the
type casting system, grabbing the type from the column might easily seem
like the "correct" thing to do.

As such, the goal of this change is to express that the column is not
something that should be used for type casting. The only places that are
"valid" (at the time of this commit) uses of acquiring a type object
from the column are fixtures (as the YAML file is going to mirror the
database more closely than the AR object), and looking up the type
during schema detection to pass to the attributes API

Many of the failing tests were removed, as they've been made obsolete
over the last year. All of the PG column tests were testing nothing
beyond polymorphism. The Mysql2 tests were duplicating the mysql tests,
since they now share a column class.

The implementation is a little hairy, and slightly verbose, but it felt
preferable to going back to 20 constructor options for the columns. If
you are git blaming to figure out wtf I was thinking with them, and have
a better idea, go for it. Just don't use a type object for this.

activerecord/lib/active_record/connection_adapters/abstract/quoting.rb

@@ -63,6 +63,17 @@ def lookup_cast_type_from_column(column) # :nodoc:
         lookup_cast_type(column.sql_type)
       end
 
+      def fetch_type_metadata(sql_type)
+        cast_type = lookup_cast_type(sql_type)
+        SqlTypeMetadata.new(
+          sql_type: sql_type,
+          type: cast_type.type,
+          limit: cast_type.limit,
+          precision: cast_type.precision,
+          scale: cast_type.scale,
+        )
+      end
+
       # Quotes a string, escaping any ' (single quote) and \ (backslash)
       # characters.
       def quote_string(s)

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -4,6 +4,7 @@
 require 'active_record/type'
 require 'active_support/core_ext/benchmark'
 require 'active_record/connection_adapters/schema_cache'
+require 'active_record/connection_adapters/sql_type_metadata'
 require 'active_record/connection_adapters/abstract/schema_dumper'
 require 'active_record/connection_adapters/abstract/schema_creation'
 require 'monitor'
@@ -384,8 +385,8 @@ def type_map # :nodoc:
         end
       end
 
-      def new_column(name, default, cast_type, sql_type = nil, null = true)
-        Column.new(name, default, cast_type, sql_type, null)
+      def new_column(name, default, sql_type_metadata = nil, null = true)
+        Column.new(name, default, sql_type_metadata, null)
       end
 
       def lookup_cast_type(sql_type) # :nodoc:

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -74,13 +74,10 @@ def column_spec_for_primary_key(column)
       end
 
       class Column < ConnectionAdapters::Column # :nodoc:
-        attr_reader :collation, :strict, :extra
+        delegate :strict, :collation, :extra, to: :sql_type_metadata, allow_nil: true
 
-        def initialize(name, default, cast_type, sql_type = nil, null = true, collation = nil, strict = false, extra = "")
-          @strict    = strict
-          @collation = collation
-          @extra     = extra
-          super(name, default, cast_type, sql_type, null)
+        def initialize(*)
+          super
           assert_valid_default(default)
           extract_default
         end
@@ -88,7 +85,7 @@ def initialize(name, default, cast_type, sql_type = nil, null = true, collation
         def extract_default
           if blob_or_text_column?
             @default = null || strict ? nil : ''
-          elsif missing_default_forged_as_empty_string?(@default)
+          elsif missing_default_forged_as_empty_string?(default)
             @default = nil
           end
         end
@@ -106,13 +103,6 @@ def case_sensitive?
           collation && !collation.match(/_ci$/)
         end
 
-        def ==(other)
-          super &&
-            collation == other.collation &&
-            strict == other.strict &&
-            extra == other.extra
-        end
-
         private
 
         # MySQL misreports NOT NULL column default when none is given.
@@ -131,9 +121,33 @@ def assert_valid_default(default)
             raise ArgumentError, "#{type} columns cannot have a default value: #{default.inspect}"
           end
         end
+      end
+
+      class MysqlTypeMetadata < DelegateClass(SqlTypeMetadata) # :nodoc:
+        attr_reader :collation, :extra, :strict
+
+        def initialize(type_metadata, collation: "", extra: "", strict: false)
+          super(type_metadata)
+          @type_metadata = type_metadata
+          @collation = collation
+          @extra = extra
+          @strict = strict
+        end
+
+        def ==(other)
+          other.is_a?(MysqlTypeMetadata) &&
+            attributes_for_hash == other.attributes_for_hash
+        end
+        alias eql? ==
+
+        def hash
+          attributes_for_hash.hash
+        end
+
+        protected
 
         def attributes_for_hash
-          super + [collation, strict, extra]
+          [self.class, @type_metadata, collation, extra, strict]
         end
       end
 
@@ -243,8 +257,8 @@ def each_hash(result) # :nodoc:
         raise NotImplementedError
       end
 
-      def new_column(field, default, cast_type, sql_type = nil, null = true, collation = "", extra = "") # :nodoc:
-        Column.new(field, default, cast_type, sql_type, null, collation, strict_mode?, extra)
+      def new_column(field, default, sql_type_metadata = nil, null = true) # :nodoc:
+        Column.new(field, default, sql_type_metadata, null)
       end
 
       # Must return the MySQL error number from the exception, if the exception has an
@@ -467,8 +481,8 @@ def columns(table_name)#:nodoc:
           each_hash(result).map do |field|
             field_name = set_field_encoding(field[:Field])
             sql_type = field[:Type]
-            cast_type = lookup_cast_type(sql_type)
-            new_column(field_name, field[:Default], cast_type, sql_type, field[:Null] == "YES", field[:Collation], field[:Extra])
+            type_metadata = fetch_type_metadata(sql_type, field[:Collation], field[:Extra])
+            new_column(field_name, field[:Default], type_metadata, field[:Null] == "YES")
           end
         end
       end
@@ -716,6 +730,10 @@ def register_integer_type(mapping, key, options) # :nodoc:
         end
       end
 
+      def fetch_type_metadata(sql_type, collation = "", extra = "")
+        MysqlTypeMetadata.new(super(sql_type), collation: collation, extra: extra, strict: strict_mode?)
+      end
+
       # MySQL is too stupid to create a temporary table for use subquery, so we have
       # to give it some prompting in the form of a subsubquery. Ugh!
       def subquery_for(key, select)

activerecord/lib/active_record/connection_adapters/column.rb

@@ -12,25 +12,21 @@ module Format
         ISO_DATETIME = /\A(\d{4})-(\d\d)-(\d\d) (\d\d):(\d\d):(\d\d)(\.\d+)?\z/
       end
 
-      attr_reader :name, :cast_type, :null, :sql_type, :default, :default_function
+      attr_reader :name, :null, :sql_type_metadata, :default, :default_function
 
-      delegate :precision, :scale, :limit, :type, to: :cast_type
+      delegate :precision, :scale, :limit, :type, :sql_type, to: :sql_type_metadata, allow_nil: true
 
       # Instantiates a new column in the table.
       #
       # +name+ is the column's name, such as <tt>supplier_id</tt> in <tt>supplier_id int(11)</tt>.
       # +default+ is the type-casted default value, such as +new+ in <tt>sales_stage varchar(20) default 'new'</tt>.
-      # +cast_type+ is the object used for type casting and type information.
-      # +sql_type+ is used to extract the column's length, if necessary. For example +60+ in
-      # <tt>company_name varchar(60)</tt>.
-      # It will be mapped to one of the standard Rails SQL types in the <tt>type</tt> attribute.
+      # +sql_type_metadata+ is various information about the type of the column
       # +null+ determines if this column allows +NULL+ values.
-      def initialize(name, default, cast_type, sql_type = nil, null = true, default_function = nil)
-        @name             = name
-        @cast_type        = cast_type
-        @sql_type         = sql_type
-        @null             = null
-        @default          = default
+      def initialize(name, default, sql_type_metadata = nil, null = true, default_function = nil)
+        @name = name
+        @sql_type_metadata = sql_type_metadata
+        @null = null
+        @default = default
         @default_function = default_function
       end
 
@@ -47,12 +43,8 @@ def human_name
       end
 
       def ==(other)
-        other.name == name &&
-          other.default == default &&
-          other.cast_type == cast_type &&
-          other.sql_type == sql_type &&
-          other.null == null &&
-          other.default_function == default_function
+        other.is_a?(Column) &&
+          attributes_for_hash == other.attributes_for_hash
       end
       alias :eql? :==
 
@@ -60,16 +52,16 @@ def hash
         attributes_for_hash.hash
       end
 
-      private
+      protected
 
       def attributes_for_hash
-        [self.class, name, default, cast_type, sql_type, null, default_function]
+        [self.class, name, default, sql_type_metadata, null, default_function]
       end
     end
 
     class NullColumn < Column
       def initialize(name)
-        super name, nil, Type::Value.new
+        super(name, nil)
       end
     end
   end

activerecord/lib/active_record/connection_adapters/postgresql/column.rb

@@ -2,21 +2,9 @@ module ActiveRecord
   module ConnectionAdapters
     # PostgreSQL-specific extensions to column definitions in a table.
     class PostgreSQLColumn < Column #:nodoc:
-      attr_reader :array, :oid, :fmod
+      delegate :array, :oid, :fmod, to: :sql_type_metadata
       alias :array? :array
 
-      def initialize(name, default, cast_type, sql_type = nil, null = true, default_function = nil, oid = nil, fmod = nil)
-        if sql_type =~ /\[\]$/
-          @array = true
-          sql_type = sql_type[0..sql_type.length - 3]
-        else
-          @array = false
-        end
-        @oid = oid
-        @fmod = fmod
-        super(name, default, cast_type, sql_type, null, default_function)
-      end
-
       def serial?
         default_function && default_function =~ /\Anextval\(.*\)\z/
       end

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -185,15 +185,15 @@ def columns(table_name)
           column_definitions(table_name).map do |column_name, type, default, notnull, oid, fmod|
             oid = oid.to_i
             fmod = fmod.to_i
-            cast_type = get_oid_type(oid, fmod, column_name, type)
-            default_value = extract_value_from_default(cast_type, default)
+            type_metadata = fetch_type_metadata(column_name, type, oid, fmod)
+            default_value = extract_value_from_default(default)
             default_function = extract_default_function(default_value, default)
-            new_column(column_name, default_value, cast_type, type, notnull == 'f', default_function, oid, fmod)
+            new_column(column_name, default_value, type_metadata, notnull == 'f', default_function)
           end
         end
 
-        def new_column(name, default, cast_type, sql_type = nil, null = true, default_function = nil, oid = nil, fmod = nil) # :nodoc:
-          PostgreSQLColumn.new(name, default, cast_type, sql_type, null, default_function, oid, fmod)
+        def new_column(name, default, sql_type_metadata = nil, null = true, default_function = nil) # :nodoc:
+          PostgreSQLColumn.new(name, default, sql_type_metadata, null, default_function)
         end
 
         # Returns the current database name.
@@ -582,6 +582,18 @@ def columns_for_distinct(columns, orders) #:nodoc:
 
           [super, *order_columns].join(', ')
         end
+
+        def fetch_type_metadata(column_name, sql_type, oid, fmod)
+          cast_type = get_oid_type(oid, fmod, column_name, sql_type)
+          simple_type = SqlTypeMetadata.new(
+            sql_type: sql_type,
+            type: cast_type.type,
+            limit: cast_type.limit,
+            precision: cast_type.precision,
+            scale: cast_type.scale,
+          )
+          PostgreSQLTypeMetadata.new(simple_type, oid: oid, fmod: fmod)
+        end
       end
     end
   end

activerecord/lib/active_record/connection_adapters/postgresql/type_metadata.rb

+module ActiveRecord
+  module ConnectionAdapters
+    class PostgreSQLTypeMetadata < DelegateClass(SqlTypeMetadata)
+      attr_reader :oid, :fmod, :array
+
+      def initialize(type_metadata, oid: nil, fmod: nil)
+        super(type_metadata)
+        @type_metadata = type_metadata
+        @oid = oid
+        @fmod = fmod
+        @array = /\[\]$/ === type_metadata.sql_type
+      end
+
+      def sql_type
+        super.gsub(/\[\]$/, "")
+      end
+
+      def ==(other)
+        other.is_a?(PostgreSQLTypeMetadata) &&
+          attributes_for_hash == other.attributes_for_hash
+      end
+      alias eql? ==
+
+      def hash
+        attributes_for_hash.hash
+      end
+
+      protected
+
+      def attributes_for_hash
+        [self.class, @type_metadata, oid, fmod]
+      end
+    end
+  end
+end

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

-require 'active_record/connection_adapters/abstract_adapter'
-require 'active_record/connection_adapters/statement_pool'
-
-require 'active_record/connection_adapters/postgresql/utils'
-require 'active_record/connection_adapters/postgresql/column'
-require 'active_record/connection_adapters/postgresql/oid'
-require 'active_record/connection_adapters/postgresql/quoting'
-require 'active_record/connection_adapters/postgresql/referential_integrity'
-require 'active_record/connection_adapters/postgresql/schema_definitions'
-require 'active_record/connection_adapters/postgresql/schema_statements'
-require 'active_record/connection_adapters/postgresql/database_statements'
+require "active_record/connection_adapters/abstract_adapter"
+require "active_record/connection_adapters/postgresql/column"
+require "active_record/connection_adapters/postgresql/database_statements"
+require "active_record/connection_adapters/postgresql/oid"
+require "active_record/connection_adapters/postgresql/quoting"
+require "active_record/connection_adapters/postgresql/referential_integrity"
+require "active_record/connection_adapters/postgresql/schema_definitions"
+require "active_record/connection_adapters/postgresql/schema_statements"
+require "active_record/connection_adapters/postgresql/type_metadata"
+require "active_record/connection_adapters/postgresql/utils"
+require "active_record/connection_adapters/statement_pool"
 
 require 'arel/visitors/bind_visitor'
 
@@ -541,7 +541,7 @@ def extract_limit(sql_type) # :nodoc:
         end
 
         # Extracts the value from a PostgreSQL column default definition.
-        def extract_value_from_default(oid, default) # :nodoc:
+        def extract_value_from_default(default) # :nodoc:
           case default
             # Quoted types
             when /\A[\(B]?'(.*)'::/m

activerecord/lib/active_record/connection_adapters/sql_type_metadata.rb

+module ActiveRecord
+  # :stopdoc:
+  module ConnectionAdapters
+    class SqlTypeMetadata
+      attr_reader :sql_type, :type, :limit, :precision, :scale
+
+      def initialize(sql_type: nil, type: nil, limit: nil, precision: nil, scale: nil)
+        @sql_type = sql_type
+        @type = type
+        @limit = limit
+        @precision = precision
+        @scale = scale
+      end
+
+      def ==(other)
+        other.is_a?(SqlTypeMetadata) &&
+          attributes_for_hash == other.attributes_for_hash
+      end
+      alias eql? ==
+
+      def hash
+        attributes_for_hash.hash
+      end
+
+      protected
+
+      def attributes_for_hash
+        [self.class, sql_type, type, limit, precision, scale]
+      end
+    end
+  end
+end

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -381,8 +381,8 @@ def columns(table_name) #:nodoc:
           end
 
           sql_type = field['type']
-          cast_type = lookup_cast_type(sql_type)
-          new_column(field['name'], field['dflt_value'], cast_type, sql_type, field['notnull'].to_i == 0)
+          type_metadata = fetch_type_metadata(sql_type)
+          new_column(field['name'], field['dflt_value'], type_metadata, field['notnull'].to_i == 0)
         end
       end
 

activerecord/lib/active_record/model_schema.rb

@@ -312,7 +312,7 @@ def load_schema!
         @columns_hash.each do |name, column|
           define_attribute(
             name,
-            column.cast_type,
+            connection.lookup_cast_type_from_column(column),
             default: column.default,
             user_provided_default: false
           )

activerecord/test/active_record/connection_adapters/fake_adapter.rb

@@ -29,8 +29,7 @@ def merge_column(table_name, name, sql_type = nil, options = {})
         @columns[table_name] << ActiveRecord::ConnectionAdapters::Column.new(
           name.to_s,
           options[:default],
-          lookup_cast_type(sql_type.to_s),
-          sql_type.to_s,
+          fetch_type_metadata(sql_type),
           options[:null])
       end
 

activerecord/test/cases/bind_parameter_test.rb

@@ -23,7 +23,7 @@ def setup
       super
       @connection = ActiveRecord::Base.connection
       @subscriber = LogListener.new
-      @pk = ConnectionAdapters::Column.new(Topic.primary_key, nil, Topic.type_for_attribute(Topic.primary_key))
+      @pk = Topic.columns_hash[Topic.primary_key]
       @subscription = ActiveSupport::Notifications.subscribe('sql.active_record', @subscriber)
     end
 

activerecord/test/cases/column_definition_test.rb

@@ -14,7 +14,7 @@ def @adapter.native_database_types
       # Avoid column definitions in create table statements like:
       # `title` varchar(255) DEFAULT NULL
       def test_should_not_include_default_clause_when_default_is_null
-        column = Column.new("title", nil, Type::String.new(limit: 20))
+        column = Column.new("title", nil, SqlTypeMetadata.new(limit: 20))
         column_def = ColumnDefinition.new(
           column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
@@ -22,7 +22,7 @@ def test_should_not_include_default_clause_when_default_is_null
       end
 
       def test_should_include_default_clause_when_default_is_present
-        column = Column.new("title", "Hello", Type::String.new(limit: 20))
+        column = Column.new("title", "Hello", SqlTypeMetadata.new(limit: 20))
         column_def = ColumnDefinition.new(
           column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
@@ -30,94 +30,53 @@ def test_should_include_default_clause_when_default_is_present
       end
 
       def test_should_specify_not_null_if_null_option_is_false
-        column = Column.new("title", "Hello", Type::String.new(limit: 20), "varchar(20)", false)
+        type_metadata = SqlTypeMetadata.new(limit: 20)
+        column = Column.new("title", "Hello", type_metadata, false)
         column_def = ColumnDefinition.new(
           column.name, "string",
           column.limit, column.precision, column.scale, column.default, column.null)
           assert_equal %Q{title varchar(20) DEFAULT 'Hello' NOT NULL}, @viz.accept(column_def)
       end
 
-      if current_adapter?(:MysqlAdapter)
+      if current_adapter?(:MysqlAdapter, :Mysql2Adapter)
         def test_should_set_default_for_mysql_binary_data_types
-          binary_column = MysqlAdapter::Column.new("title", "a", Type::Binary.new, "binary(1)")
+          type = SqlTypeMetadata.new(type: :binary, sql_type: "binary(1)")
+          binary_column = AbstractMysqlAdapter::Column.new("title", "a", type)
           assert_equal "a", binary_column.default
 
-          varbinary_column = MysqlAdapter::Column.new("title", "a", Type::Binary.new, "varbinary(1)")
+          type = SqlTypeMetadata.new(type: :binary, sql_type: "varbinary")
+          varbinary_column = AbstractMysqlAdapter::Column.new("title", "a", type)
           assert_equal "a", varbinary_column.default
         end
 
         def test_should_not_set_default_for_blob_and_text_data_types
           assert_raise ArgumentError do
-            MysqlAdapter::Column.new("title", "a", Type::Binary.new, "blob")
+            AbstractMysqlAdapter::Column.new("title", "a", SqlTypeMetadata.new(sql_type: "blob"))
           end
 
+          text_type = AbstractMysqlAdapter::MysqlTypeMetadata.new(
+            SqlTypeMetadata.new(type: :text))
           assert_raise ArgumentError do
-            MysqlAdapter::Column.new("title", "Hello", Type::Text.new)
+            AbstractMysqlAdapter::Column.new("title", "Hello", text_type)
           end
 
-          text_column = MysqlAdapter::Column.new("title", nil, Type::Text.new)
+          text_column = AbstractMysqlAdapter::Column.new("title", nil, text_type)
           assert_equal nil, text_column.default
 
-          not_null_text_column = MysqlAdapter::Column.new("title", nil, Type::Text.new, "text", false)
+          not_null_text_column = AbstractMysqlAdapter::Column.new("title", nil, text_type, false)
           assert_equal "", not_null_text_column.default
         end
 
         def test_has_default_should_return_false_for_blob_and_text_data_types
-          blob_column = MysqlAdapter::Column.new("title", nil, Type::Binary.new, "blob")
+          binary_type = SqlTypeMetadata.new(sql_type: "blob")
+          blob_column = AbstractMysqlAdapter::Column.new("title", nil, binary_type)
           assert !blob_column.has_default?
 
-          text_column = MysqlAdapter::Column.new("title", nil, Type::Text.new)
+          text_type = SqlTypeMetadata.new(type: :text)
+          text_column = AbstractMysqlAdapter::Column.new("title", nil, text_type)
           assert !text_column.has_default?
         end
       end
-
-      if current_adapter?(:Mysql2Adapter)
-        def test_should_set_default_for_mysql_binary_data_types
-          binary_column = Mysql2Adapter::Column.new("title", "a", Type::Binary.new, "binary(1)")
-          assert_equal "a", binary_column.default
-
-          varbinary_column = Mysql2Adapter::Column.new("title", "a", Type::Binary.new, "varbinary(1)")
-          assert_equal "a", varbinary_column.default
-        end
-
-        def test_should_not_set_default_for_blob_and_text_data_types
-          assert_raise ArgumentError do
-            Mysql2Adapter::Column.new("title", "a", Type::Binary.new, "blob")
-          end
-
-          assert_raise ArgumentError do
-            Mysql2Adapter::Column.new("title", "Hello", Type::Text.new)
-          end
-
-          text_column = Mysql2Adapter::Column.new("title", nil, Type::Text.new)
-          assert_equal nil, text_column.default
-
-          not_null_text_column = Mysql2Adapter::Column.new("title", nil, Type::Text.new, "text", false)
-          assert_equal "", not_null_text_column.default
-        end
-
-        def test_has_default_should_return_false_for_blob_and_text_data_types
-          blob_column = Mysql2Adapter::Column.new("title", nil, Type::Binary.new, "blob")
-          assert !blob_column.has_default?
-
-          text_column = Mysql2Adapter::Column.new("title", nil, Type::Text.new)
-          assert !text_column.has_default?
-        end
-      end
-
-      if current_adapter?(:PostgreSQLAdapter)
-        def test_bigint_column_should_map_to_integer
-          oid = PostgreSQLAdapter::OID::Integer.new
-          bigint_column = PostgreSQLColumn.new('number', nil, oid, "bigint")
-          assert_equal :integer, bigint_column.type
-        end
-
-        def test_smallint_column_should_map_to_integer
-          oid = PostgreSQLAdapter::OID::Integer.new
-          smallint_column = PostgreSQLColumn.new('number', nil, oid, "smallint")
-          assert_equal :integer, smallint_column.type
-        end
-      end
     end
   end
 end