Separate primary key column options from table options

kamipo · kamipo · commit 859681ec5bf9 · 2020-05-24T08:15:39.000+09:00
Otherwise we cannot handle the same name options for both column and
table (e.g. `:comment`, `:charset`, `:collation`).
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb b/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
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
@@ -300,6 +300,11 @@ def create_table(table_name, id: :primary_key, primary_key: nil, force: nil, **o
         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
diff --git a/activerecord/lib/active_record/schema_dumper.rb b/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)
diff --git a/activerecord/test/cases/adapters/mysql2/charset_collation_test.rb b/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
diff --git a/activerecord/test/cases/comment_test.rb b/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
diff --git a/activerecord/test/cases/primary_keys_test.rb b/activerecord/test/cases/primary_keys_test.rb
@@ -312,15 +312,15 @@ 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
 
   if current_adapter?(:Mysql2Adapter) && supports_datetime_with_precision?
     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
