Merge pull request #39365 from kamipo/table_options

Default engine `ENGINE=InnoDB` is no longer dumped to make schema more agnostic

activerecord/CHANGELOG.md

+*   Default engine `ENGINE=InnoDB` is no longer dumped to make schema more agnostic.
+
+    Before:
+
+    ```ruby
+    create_table "accounts", options: "ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_0900_ai_ci", force: :cascade do |t|
+    end
+    ```
+
+    After:
+
+    ```ruby
+    create_table "accounts", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
+    end
+    ```
+
+    *Ryuta Kamizono*
+
 *   Added delegated type as an alternative to single-table inheritance for representing class hierarchies.
     See ActiveRecord::DelegatedType for the full description.
-    
+
     *DHH*
 
 *   Deprecate aggregations with group by duplicated fields.

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

@@ -53,7 +53,7 @@ def visit_TableDefinition(o)
           end
 
           create_sql << "(#{statements.join(', ')})" if statements.present?
-          add_table_options!(create_sql, table_options(o))
+          add_table_options!(create_sql, o)
           create_sql << " AS #{to_sql(o.as)}" if o.as
           create_sql
         end
@@ -106,17 +106,8 @@ def supports_index_using?
           true
         end
 
-        def table_options(o)
-          table_options = {}
-          table_options[:comment] = o.comment
-          table_options[:options] = o.options
-          table_options
-        end
-
-        def add_table_options!(create_sql, options)
-          if options_sql = options[:options]
-            create_sql << " #{options_sql}"
-          end
+        def add_table_options!(create_sql, o)
+          create_sql << " #{o.options}" if o.options
           create_sql
         end
 

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

@@ -277,7 +277,8 @@ def initialize(
         if_not_exists: false,
         options: nil,
         as: nil,
-        comment: nil
+        comment: nil,
+        **
       )
         @conn = conn
         @columns_hash = {}

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

@@ -13,9 +13,9 @@ def column_spec(column)
         end
 
         def column_spec_for_primary_key(column)
-          return {} if default_primary_key?(column)
-          spec = { id: schema_type(column).inspect }
-          spec.merge!(prepare_column_options(column).except!(:null, :comment))
+          spec = {}
+          spec[:id] = schema_type(column).inspect unless default_primary_key?(column)
+          spec.merge!(prepare_column_options(column).except!(:null))
           spec[:default] ||= "nil" if explicit_primary_key_default?(column)
           spec
         end

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

@@ -295,13 +295,16 @@ def primary_key(table_name)
       #
       # See also TableDefinition#column for details on how to create columns.
       def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **options)
-        td = create_table_definition(
-          table_name, **options.extract!(:temporary, :if_not_exists, :options, :as, :comment)
-        )
+        td = create_table_definition(table_name, **extract_table_options!(options))
 
         if id && !td.as
           pk = primary_key || Base.get_primary_key(table_name.to_s.singularize)
 
+          if id.is_a?(Hash)
+            options.merge!(id.except(:type))
+            id = id.fetch(:type, :primary_key)
+          end
+
           if pk.is_a?(Array)
             td.primary_keys pk
           else
@@ -1379,14 +1382,18 @@ def schema_creation
           SchemaCreation.new(self)
         end
 
-        def create_table_definition(*args, **options)
-          TableDefinition.new(self, *args, **options)
+        def create_table_definition(name, **options)
+          TableDefinition.new(self, name, **options)
         end
 
         def create_alter_table(name)
           AlterTable.new create_table_definition(name)
         end
 
+        def extract_table_options!(options)
+          options.extract!(:temporary, :if_not_exists, :options, :as, :comment, :charset, :collation)
+        end
+
         def fetch_type_metadata(sql_type)
           cast_type = lookup_cast_type(sql_type)
           SqlTypeMetadata.new(

activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb

@@ -414,24 +414,31 @@ def foreign_keys(table_name)
       end
 
       def table_options(table_name) # :nodoc:
-        table_options = {}
-
         create_table_info = create_table_info(table_name)
 
         # strip create_definitions and partition_options
         # Be aware that `create_table_info` might not include any table options due to `NO_TABLE_OPTIONS` sql mode.
         raw_table_options = create_table_info.sub(/\A.*\n\) ?/m, "").sub(/\n\/\*!.*\*\/\n\z/m, "").strip
 
+        return if raw_table_options.empty?
+
+        table_options = {}
+
+        if / DEFAULT CHARSET=(?<charset>\w+)(?: COLLATE=(?<collation>\w+))?/ =~ raw_table_options
+          raw_table_options = $` + $' # before part + after part
+          table_options[:charset] = charset
+          table_options[:collation] = collation if collation
+        end
+
         # strip AUTO_INCREMENT
         raw_table_options.sub!(/(ENGINE=\w+)(?: AUTO_INCREMENT=\d+)/, '\1')
 
-        table_options[:options] = raw_table_options unless raw_table_options.blank?
-
         # strip COMMENT
         if raw_table_options.sub!(/ COMMENT='.+'/, "")
           table_options[:comment] = table_comment(table_name)
         end
 
+        table_options[:options] = raw_table_options unless raw_table_options == "ENGINE=InnoDB"
         table_options
       end
 

activerecord/lib/active_record/connection_adapters/mysql/schema_creation.rb

@@ -40,8 +40,11 @@ def visit_IndexDefinition(o, create = false)
             add_sql_comment!(sql.join(" "), o.comment)
           end
 
-          def add_table_options!(create_sql, options)
-            add_sql_comment!(super, options[:comment])
+          def add_table_options!(create_sql, o)
+            create_sql = super
+            create_sql << " DEFAULT CHARSET=#{o.charset}" if o.charset
+            create_sql << " COLLATE=#{o.collation}" if o.collation
+            add_sql_comment!(create_sql, o.comment)
           end
 
           def add_column_options!(sql, options)

activerecord/lib/active_record/connection_adapters/mysql/schema_definitions.rb

@@ -60,6 +60,14 @@ module ColumnMethods
       class TableDefinition < ActiveRecord::ConnectionAdapters::TableDefinition
         include ColumnMethods
 
+        attr_reader :charset, :collation
+
+        def initialize(conn, name, charset: nil, collation: nil, **)
+          super
+          @charset = charset
+          @collation = collation
+        end
+
         def new_column_definition(name, type, **options) # :nodoc:
           case type
           when :virtual

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

@@ -154,8 +154,8 @@ def schema_creation
             MySQL::SchemaCreation.new(self)
           end
 
-          def create_table_definition(*args, **options)
-            MySQL::TableDefinition.new(self, *args, **options)
+          def create_table_definition(name, **options)
+            MySQL::TableDefinition.new(self, name, **options)
           end
 
           def new_column_from_field(table_name, field)

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

@@ -621,8 +621,8 @@ def schema_creation
             PostgreSQL::SchemaCreation.new(self)
           end
 
-          def create_table_definition(*args, **options)
-            PostgreSQL::TableDefinition.new(self, *args, **options)
+          def create_table_definition(name, **options)
+            PostgreSQL::TableDefinition.new(self, name, **options)
           end
 
           def create_alter_table(name)

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

@@ -87,8 +87,8 @@ def schema_creation
             SQLite3::SchemaCreation.new(self)
           end
 
-          def create_table_definition(*args, **options)
-            SQLite3::TableDefinition.new(self, *args, **options)
+          def create_table_definition(name, **options)
+            SQLite3::TableDefinition.new(self, name, **options)
           end
 
           def validate_index_length!(table_name, new_name, internal = false)

activerecord/lib/active_record/schema_dumper.rb

@@ -125,7 +125,10 @@ def table(table, stream)
             tbl.print ", primary_key: #{pk.inspect}" unless pk == "id"
             pkcol = columns.detect { |c| c.name == pk }
             pkcolspec = column_spec_for_primary_key(pkcol)
-            if pkcolspec.present?
+            unless pkcolspec.empty?
+              if pkcolspec != pkcolspec.slice(:id, :default)
+                pkcolspec = { id: { type: pkcolspec.delete(:id), **pkcolspec }.compact }
+              end
               tbl.print ", #{format_colspec(pkcolspec)}"
             end
           when Array
@@ -240,7 +243,9 @@ def foreign_keys(table, stream)
       end
 
       def format_colspec(colspec)
-        colspec.map { |key, value| "#{key}: #{value}" }.join(", ")
+        colspec.map do |key, value|
+          "#{key}: #{ value.is_a?(Hash) ? "{ #{format_colspec(value)} }" : value }"
+        end.join(", ")
       end
 
       def format_options(options)

activerecord/test/cases/adapters/mysql2/charset_collation_test.rb

@@ -9,7 +9,7 @@ class Mysql2CharsetCollationTest < ActiveRecord::Mysql2TestCase
 
   setup do
     @connection = ActiveRecord::Base.connection
-    @connection.create_table :charset_collations, force: true do |t|
+    @connection.create_table :charset_collations, id: { type: :string, collation: "utf8mb4_bin" }, force: true do |t|
       t.string :string_ascii_bin, charset: "ascii", collation: "ascii_bin"
       t.text :text_ucs2_unicode_ci, charset: "ucs2", collation: "ucs2_unicode_ci"
     end
@@ -50,6 +50,7 @@ class Mysql2CharsetCollationTest < ActiveRecord::Mysql2TestCase
 
   test "schema dump includes collation" do
     output = dump_table_schema("charset_collations")
+    assert_match %r/create_table "charset_collations", id: { type: :string, collation: "utf8mb4_bin" }/, output
     assert_match %r{t\.string\s+"string_ascii_bin",\s+collation: "ascii_bin"$}, output
     assert_match %r{t\.text\s+"text_ucs2_unicode_ci",\s+collation: "ucs2_unicode_ci"$}, output
   end

activerecord/test/cases/adapters/mysql2/table_options_test.rb

@@ -5,6 +5,7 @@
 
 class Mysql2TableOptionsTest < ActiveRecord::Mysql2TestCase
   include SchemaDumpingHelper
+  self.use_transactional_tests = false
 
   def setup
     @connection = ActiveRecord::Base.connection
@@ -17,29 +18,36 @@ def teardown
   test "table options with ENGINE" do
     @connection.create_table "mysql_table_options", force: true, options: "ENGINE=MyISAM"
     output = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{ENGINE=MyISAM}, options
+    expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, options: "ENGINE=MyISAM", force: :cascade/
+    assert_match expected, output
   end
 
   test "table options with ROW_FORMAT" do
     @connection.create_table "mysql_table_options", force: true, options: "ROW_FORMAT=REDUNDANT"
     output = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{ROW_FORMAT=REDUNDANT}, options
+    expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, options: "ENGINE=InnoDB ROW_FORMAT=REDUNDANT", force: :cascade/
+    assert_match expected, output
   end
 
   test "table options with CHARSET" do
-    @connection.create_table "mysql_table_options", force: true, options: "CHARSET=utf8mb4"
+    @connection.create_table "mysql_table_options", force: true, options: "CHARSET=latin1"
     output = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{CHARSET=utf8mb4}, options
+    expected = /create_table "mysql_table_options", charset: "latin1", force: :cascade/
+    assert_match expected, output
   end
 
   test "table options with COLLATE" do
     @connection.create_table "mysql_table_options", force: true, options: "COLLATE=utf8mb4_bin"
     output = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{COLLATE=utf8mb4_bin}, options
+    expected = /create_table "mysql_table_options", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade/
+    assert_match expected, output
+  end
+
+  test "charset and collation options" do
+    @connection.create_table "mysql_table_options", force: true, charset: "utf8mb4", collation: "utf8mb4_bin"
+    output = dump_table_schema("mysql_table_options")
+    expected = /create_table "mysql_table_options", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade/
+    assert_match expected, output
   end
 
   test "schema dump works with NO_TABLE_OPTIONS sql mode" do
@@ -60,47 +68,10 @@ def teardown
   end
 end
 
-class Mysql2DefaultEngineOptionSchemaDumpTest < ActiveRecord::Mysql2TestCase
+class Mysql2DefaultEngineOptionTest < ActiveRecord::Mysql2TestCase
   include SchemaDumpingHelper
   self.use_transactional_tests = false
 
-  def setup
-    @verbose_was = ActiveRecord::Migration.verbose
-    ActiveRecord::Migration.verbose = false
-  end
-
-  def teardown
-    ActiveRecord::Base.connection.drop_table "mysql_table_options", if_exists: true
-    ActiveRecord::Migration.verbose = @verbose_was
-    ActiveRecord::SchemaMigration.delete_all rescue nil
-  end
-
-  test "schema dump includes ENGINE=InnoDB if not provided" do
-    ActiveRecord::Base.connection.create_table "mysql_table_options", force: true
-
-    output  = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{ENGINE=InnoDB}, options
-  end
-
-  test "schema dump includes ENGINE=InnoDB in legacy migrations" do
-    migration = Class.new(ActiveRecord::Migration[5.1]) do
-      def migrate(x)
-        create_table "mysql_table_options", force: true
-      end
-    end.new
-
-    ActiveRecord::Migrator.new(:up, [migration], ActiveRecord::Base.connection.schema_migration).migrate
-
-    output  = dump_table_schema("mysql_table_options")
-    options = %r{create_table "mysql_table_options", options: "(?<options>.*)"}.match(output)[:options]
-    assert_match %r{ENGINE=InnoDB}, options
-  end
-end
-
-class Mysql2DefaultEngineOptionSqlOutputTest < ActiveRecord::Mysql2TestCase
-  self.use_transactional_tests = false
-
   def setup
     @logger_was  = ActiveRecord::Base.logger
     @log         = StringIO.new
@@ -120,6 +91,10 @@ def teardown
     ActiveRecord::Base.connection.create_table "mysql_table_options", force: true
 
     assert_no_match %r{ENGINE=InnoDB}, @log.string
+
+    output = dump_table_schema("mysql_table_options")
+    expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, force: :cascade/
+    assert_match expected, output
   end
 
   test "legacy migrations contain default ENGINE=InnoDB option" do
@@ -132,5 +107,9 @@ def migrate(x)
     ActiveRecord::Migrator.new(:up, [migration], ActiveRecord::Base.connection.schema_migration).migrate
 
     assert_match %r{ENGINE=InnoDB}, @log.string
+
+    output = dump_table_schema("mysql_table_options")
+    expected = /create_table "mysql_table_options", charset: "utf8mb4"(?:, collation: "\w+")?, force: :cascade/
+    assert_match expected, output
   end
 end

activerecord/test/cases/comment_test.rb

@@ -39,7 +39,7 @@ class PkCommented < ActiveRecord::Base
       end
 
       @connection.create_table("pk_commenteds", comment: "Table comment", id: false, force: true) do |t|
-        t.integer :id, comment: "Primary key comment", primary_key: true
+        t.primary_key :id, comment: "Primary key comment"
       end
 
       Commented.reset_column_information
@@ -197,8 +197,7 @@ def test_comment_on_primary_key
 
     def test_schema_dump_with_primary_key_comment
       output = dump_table_schema "pk_commenteds"
-      assert_match %r[create_table "pk_commenteds",.*\s+comment: "Table comment"], output
-      assert_no_match %r[create_table "pk_commenteds",.*\s+comment: "Primary key comment"], output
+      assert_match %r[create_table "pk_commenteds", id: { comment: "Primary key comment" }.*, comment: "Table comment"], output
     end
   end
 end

activerecord/test/cases/primary_keys_test.rb

@@ -312,7 +312,7 @@ def test_any_type_primary_key
 
   test "schema dump primary key includes type and options" do
     schema = dump_table_schema "barcodes"
-    assert_match %r{create_table "barcodes", primary_key: "code", id: :string, limit: 42}, schema
+    assert_match %r/create_table "barcodes", primary_key: "code", id: { type: :string, limit: 42 }/, schema
     assert_no_match %r{t\.index \["code"\]}, schema
   end
 
@@ -320,7 +320,7 @@ def test_any_type_primary_key
     test "schema typed primary key column" do
       @connection.create_table(:scheduled_logs, id: :timestamp, precision: 6, force: true)
       schema = dump_table_schema("scheduled_logs")
-      assert_match %r/create_table "scheduled_logs", id: :timestamp, precision: 6/, schema
+      assert_match %r/create_table "scheduled_logs", id: { type: :timestamp, precision: 6.* }/, schema
     end
   end
 end
@@ -462,7 +462,7 @@ class Widget < ActiveRecord::Base
         assert_predicate column, :unsigned?
 
         schema = dump_table_schema "widgets"
-        assert_match %r{create_table "widgets", id: :integer, unsigned: true, }, schema
+        assert_match %r/create_table "widgets", id: { type: :integer, unsigned: true }/, schema
       end
 
       test "bigint primary key with unsigned" do
@@ -474,7 +474,7 @@ class Widget < ActiveRecord::Base
         assert_predicate column, :unsigned?
 
         schema = dump_table_schema "widgets"
-        assert_match %r{create_table "widgets", id: :bigint, unsigned: true, }, schema
+        assert_match %r/create_table "widgets", id: { type: :bigint, unsigned: true }/, schema
       end
     end
   end