Merge pull request #23677 from kamipo/passing_table_name_to_column_initialize

Passing `table_name` to `Column#initialize` to avoid `instance_variable_set`

9 files changed, 58 insertions, 29 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
index fcc1ef9d5f..6704843c07 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -420,8 +420,8 @@ module ActiveRecord
         end
       end
 
-      def new_column(name, default, sql_type_metadata = nil, null = true, default_function = nil, collation = nil)
-        Column.new(name, default, sql_type_metadata, null, default_function, collation)
+      def new_column(name, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil) # :nodoc:
+        Column.new(name, default, sql_type_metadata, null, table_name, default_function, collation)
       end
 
       def lookup_cast_type(sql_type) # :nodoc:
diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
index 50f461b746..f453e717ed 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb
@@ -153,8 +153,8 @@ module ActiveRecord
         raise NotImplementedError
       end
 
-      def new_column(field, default, sql_type_metadata = nil, null = true, default_function = nil, collation = nil) # :nodoc:
-        MySQL::Column.new(field, default, sql_type_metadata, null, default_function, collation)
+      def new_column(field, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil) # :nodoc:
+        MySQL::Column.new(field, default, sql_type_metadata, null, table_name, default_function, collation)
       end
 
       # Must return the MySQL error number from the exception, if the exception has an
@@ -421,16 +421,15 @@ module ActiveRecord
 
       # Returns an array of +Column+ objects for the table specified by +table_name+.
       def columns(table_name) # :nodoc:
-        sql = "SHOW FULL FIELDS FROM #{quote_table_name(table_name)}"
-        execute_and_free(sql, 'SCHEMA') do |result|
-          each_hash(result).map do |field|
-            type_metadata = fetch_type_metadata(field[:Type], field[:Extra])
-            if type_metadata.type == :datetime && field[:Default] == "CURRENT_TIMESTAMP"
-              new_column(field[:Field], nil, type_metadata, field[:Null] == "YES", field[:Default], field[:Collation])
-            else
-              new_column(field[:Field], field[:Default], type_metadata, field[:Null] == "YES", nil, field[:Collation])
-            end
+        table_name = table_name.to_s
+        column_definitions(table_name).map do |field|
+          type_metadata = fetch_type_metadata(field[:Type], field[:Extra])
+          if type_metadata.type == :datetime && field[:Default] == "CURRENT_TIMESTAMP"
+            default, default_function = nil, field[:Default]
+          else
+            default, default_function = field[:Default], nil
           end
+          new_column(field[:Field], default, type_metadata, field[:Null] == "YES", table_name, default_function, field[:Collation])
         end
       end
 
@@ -874,6 +873,12 @@ module ActiveRecord
         @connection.query  "SET #{encoding} #{variable_assignments}"
       end
 
+      def column_definitions(table_name) # :nodoc:
+        execute_and_free("SHOW FULL FIELDS FROM #{quote_table_name(table_name)}", 'SCHEMA') do |result|
+          each_hash(result)
+        end
+      end
+
       def extract_foreign_key_action(structure, name, action) # :nodoc:
         if structure =~ /CONSTRAINT #{quote_column_name(name)} FOREIGN KEY .* REFERENCES .* ON #{action} (CASCADE|SET NULL|RESTRICT)/
           case $1
diff --git a/activerecord/lib/active_record/connection_adapters/column.rb b/activerecord/lib/active_record/connection_adapters/column.rb
index 10f908538f..2e718b29fa 100644
--- a/activerecord/lib/active_record/connection_adapters/column.rb
+++ b/activerecord/lib/active_record/connection_adapters/column.rb
@@ -5,7 +5,7 @@ module ActiveRecord
   module ConnectionAdapters
     # An abstract definition of a column in a table.
     class Column
-      attr_reader :name, :null, :sql_type_metadata, :default, :default_function, :collation
+      attr_reader :name, :default, :sql_type_metadata, :null, :table_name, :default_function, :collation
 
       delegate :precision, :scale, :limit, :type, :sql_type, to: :sql_type_metadata, allow_nil: true
 
@@ -15,14 +15,14 @@ module ActiveRecord
       # +default+ is the type-casted default value, such as +new+ in <tt>sales_stage varchar(20) default 'new'</tt>.
       # +sql_type_metadata+ is various information about the type of the column
       # +null+ determines if this column allows +NULL+ values.
-      def initialize(name, default, sql_type_metadata = nil, null = true, default_function = nil, collation = nil)
+      def initialize(name, default, sql_type_metadata = nil, null = true, table_name = nil, default_function = nil, collation = nil)
         @name = name.freeze
+        @table_name = table_name
         @sql_type_metadata = sql_type_metadata
         @null = null
         @default = default
         @default_function = default_function
         @collation = collation
-        @table_name = nil
       end
 
       def has_default?
@@ -54,7 +54,7 @@ module ActiveRecord
       protected
 
       def attributes_for_hash
-        [self.class, name, default, sql_type_metadata, null, default_function, collation]
+        [self.class, name, default, sql_type_metadata, null, table_name, default_function, collation]
       end
     end
 
diff --git a/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb
index 914ea98f79..90e5420e71 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb
@@ -43,7 +43,7 @@ module ActiveRecord
         end
 
         def schema_collation(column)
-          if column.collation && table_name = column.instance_variable_get(:@table_name)
+          if column.collation && table_name = column.table_name
             @table_collation_cache ||= {}
             @table_collation_cache[table_name] ||= select_one("SHOW TABLE STATUS LIKE '#{table_name}'")["Collation"]
             column.collation.inspect if column.collation != @table_collation_cache[table_name]
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb
index bfa03fa136..3ad1911a28 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb
@@ -8,7 +8,6 @@ module ActiveRecord
       def serial?
         return unless default_function
 
-        table_name = @table_name || '(?<table_name>.+)'
         %r{\Anextval\('"?#{table_name}_#{name}_seq"?'::regclass\)\z} === default_function
       end
     end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
index 0579c1a92e..ca2a41b136 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb
@@ -213,19 +213,20 @@ module ActiveRecord
         end
 
         # Returns the list of all column definitions for a table.
-        def columns(table_name)
+        def columns(table_name) # :nodoc:
+          table_name = table_name.to_s
           column_definitions(table_name).map do |column_name, type, default, notnull, oid, fmod, collation|
             oid = oid.to_i
             fmod = fmod.to_i
             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, type_metadata, !notnull, default_function, collation)
+            new_column(column_name, default_value, type_metadata, !notnull, table_name, default_function, collation)
           end
         end
 
-        def new_column(name, default, sql_type_metadata = nil, null = true, default_function = nil, collation = nil) # :nodoc:
-          PostgreSQLColumn.new(name, default, sql_type_metadata, null, default_function, collation)
+        def new_column(name, default, sql_type_metadata, null, table_name, default_function = nil, collation = nil) # :nodoc:
+          PostgreSQLColumn.new(name, default, sql_type_metadata, null, table_name, default_function, collation)
         end
 
         # Returns the current database name.
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
index c65d33ccb3..7ac81bdf23 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb
@@ -337,7 +337,8 @@ module ActiveRecord
       end
 
       # Returns an array of +Column+ objects for the table specified by +table_name+.
-      def columns(table_name) #:nodoc:
+      def columns(table_name) # :nodoc:
+        table_name = table_name.to_s
         table_structure(table_name).map do |field|
           case field["dflt_value"]
           when /^null$/i
@@ -351,7 +352,7 @@ module ActiveRecord
           collation = field['collation']
           sql_type = field['type']
           type_metadata = fetch_type_metadata(sql_type)
-          new_column(field['name'], field['dflt_value'], type_metadata, field['notnull'].to_i == 0, nil, collation)
+          new_column(field['name'], field['dflt_value'], type_metadata, field['notnull'].to_i == 0, table_name, nil, collation)
         end
       end
 
diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb
index f115c7542b..8c76c5f62d 100644
--- a/activerecord/lib/active_record/schema_dumper.rb
+++ b/activerecord/lib/active_record/schema_dumper.rb
@@ -104,10 +104,7 @@ HEADER
       end
 
       def table(table, stream)
-        columns = @connection.columns(table).map do |column|
-          column.instance_variable_set(:@table_name, table)
-          column
-        end
+        columns = @connection.columns(table)
         begin
           tbl = StringIO.new
 
diff --git a/activerecord/test/cases/adapters/postgresql/serial_test.rb b/activerecord/test/cases/adapters/postgresql/serial_test.rb
index 2d6f1e0910..76bcec9672 100644
--- a/activerecord/test/cases/adapters/postgresql/serial_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/serial_test.rb
@@ -10,6 +10,7 @@ class PostgresqlSerialTest < ActiveRecord::PostgreSQLTestCase
     @connection = ActiveRecord::Base.connection
     @connection.create_table "postgresql_serials", force: true do |t|
       t.serial :seq
+      t.integer :serials_id, default: -> { "nextval('postgresql_serials_id_seq')" }
     end
   end
 
@@ -24,10 +25,22 @@ class PostgresqlSerialTest < ActiveRecord::PostgreSQLTestCase
     assert column.serial?
   end
 
+  def test_not_serial_column
+    column = PostgresqlSerial.columns_hash["serials_id"]
+    assert_equal :integer, column.type
+    assert_equal "integer", column.sql_type
+    assert_not column.serial?
+  end
+
   def test_schema_dump_with_shorthand
     output = dump_table_schema "postgresql_serials"
     assert_match %r{t\.serial\s+"seq",\s+null: false$}, output
   end
+
+  def test_schema_dump_with_not_serial
+    output = dump_table_schema "postgresql_serials"
+    assert_match %r{t\.integer\s+"serials_id",\s+default: -> \{ "nextval\('postgresql_serials_id_seq'::regclass\)" \}$}, output
+  end
 end
 
 class PostgresqlBigSerialTest < ActiveRecord::PostgreSQLTestCase
@@ -39,6 +52,7 @@ class PostgresqlBigSerialTest < ActiveRecord::PostgreSQLTestCase
     @connection = ActiveRecord::Base.connection
     @connection.create_table "postgresql_big_serials", force: true do |t|
       t.bigserial :seq
+      t.bigint :serials_id, default: -> { "nextval('postgresql_big_serials_id_seq')" }
     end
   end
 
@@ -53,8 +67,20 @@ class PostgresqlBigSerialTest < ActiveRecord::PostgreSQLTestCase
     assert column.serial?
   end
 
+  def test_not_bigserial_column
+    column = PostgresqlBigSerial.columns_hash["serials_id"]
+    assert_equal :integer, column.type
+    assert_equal "bigint", column.sql_type
+    assert_not column.serial?
+  end
+
   def test_schema_dump_with_shorthand
     output = dump_table_schema "postgresql_big_serials"
     assert_match %r{t\.bigserial\s+"seq",\s+null: false$}, output
   end
+
+  def test_schema_dump_with_not_bigserial
+    output = dump_table_schema "postgresql_big_serials"
+    assert_match %r{t\.integer\s+"serials_id",\s+limit: 8,\s+default: -> \{ "nextval\('postgresql_big_serials_id_seq'::regclass\)" \}$}, output
+  end
 end