Merge pull request #27389 from kamipo/fix_mysql_pk_dumping_correctly

Restore the behaviour of the compatibility layer for integer-like PKs

* kamipo/fix_mysql_pk_dumping_correctly:
  Restore custom primary key tests lost at #26266
  Restore the behaviour of the compatibility layer for integer-like PKs
  Correctly dump integer-like primary key with default nil

11 files changed, 191 insertions, 236 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb
index f0f4b5e565..34036d8a1d 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb
@@ -14,6 +14,8 @@ module ActiveRecord
         return {} if default_primary_key?(column)
         spec = { id: schema_type(column).inspect }
         spec.merge!(prepare_column_options(column).except!(:null))
+        spec[:default] ||= "nil" if explicit_primary_key_default?(column)
+        spec
       end
 
       # This can be overridden on an Adapter level basis to support other
@@ -60,6 +62,10 @@ module ActiveRecord
           schema_type(column) == :bigint
         end
 
+        def explicit_primary_key_default?(column)
+          false
+        end
+
         def schema_type_with_virtual(column)
           if supports_virtual_columns? && column.virtual?
             :virtual
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 3a3b65c2d0..a06dd0f6b8 100644
--- a/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb
+++ b/activerecord/lib/active_record/connection_adapters/mysql/schema_dumper.rb
@@ -2,15 +2,6 @@ module ActiveRecord
   module ConnectionAdapters
     module MySQL
       module ColumnDumper # :nodoc:
-        def column_spec_for_primary_key(column)
-          spec = super
-          if [:integer, :bigint].include?(schema_type(column)) && !column.auto_increment?
-            spec[:default] ||= schema_default(column) || "nil"
-          end
-          spec[:unsigned] = "true" if column.unsigned?
-          spec
-        end
-
         def prepare_column_options(column)
           spec = super
           spec[:unsigned] = "true" if column.unsigned?
@@ -34,6 +25,10 @@ module ActiveRecord
             super && column.auto_increment?
           end
 
+          def explicit_primary_key_default?(column)
+            column.type == :integer && !column.auto_increment?
+          end
+
           def schema_type(column)
             if column.sql_type == "tinyblob"
               :blob
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb
index c3e182b43d..0443bd8bdd 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb
@@ -42,7 +42,7 @@ module ActiveRecord
         # a record (as primary keys cannot be +nil+). This might be done via the
         # +SecureRandom.uuid+ method and a +before_save+ callback, for instance.
         def primary_key(name, type = :primary_key, **options)
-          options[:auto_increment] = true if [:primary_key, :integer, :bigint].include?(type) && !options.key?(:default)
+          options[:auto_increment] = true if [:integer, :bigint].include?(type) && !options.key?(:default)
           if type == :uuid
             options[:default] = options.fetch(:default, "gen_random_uuid()")
           elsif options.delete(:auto_increment) == true && %i(integer bigint).include?(type)
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_dumper.rb
index 4aa0cbef50..5555a46b6b 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_dumper.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_dumper.rb
@@ -2,14 +2,6 @@ module ActiveRecord
   module ConnectionAdapters
     module PostgreSQL
       module ColumnDumper # :nodoc:
-        def column_spec_for_primary_key(column)
-          spec = super
-          if schema_type(column) == :uuid
-            spec[:default] ||= "nil"
-          end
-          spec
-        end
-
         # Adds +:array+ option to the default set
         def prepare_column_options(column)
           spec = super
@@ -28,6 +20,10 @@ module ActiveRecord
             schema_type(column) == :bigserial
           end
 
+          def explicit_primary_key_default?(column)
+            column.type == :uuid || (column.type == :integer && !column.serial?)
+          end
+
           def schema_type(column)
             return super unless column.serial?
 
diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_dumper.rb
index 2dca0b0f9c..eec018eda3 100644
--- a/activerecord/lib/active_record/connection_adapters/sqlite3/schema_dumper.rb
+++ b/activerecord/lib/active_record/connection_adapters/sqlite3/schema_dumper.rb
@@ -7,6 +7,10 @@ module ActiveRecord
           def default_primary_key?(column)
             schema_type(column) == :integer
           end
+
+          def explicit_primary_key_default?(column)
+            column.bigint?
+          end
       end
     end
   end
diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb
index ffeca2c91e..f6c9127d34 100644
--- a/activerecord/lib/active_record/migration/compatibility.rb
+++ b/activerecord/lib/active_record/migration/compatibility.rb
@@ -21,6 +21,12 @@ module ActiveRecord
             end
           end
 
+          unless adapter_name == "Mysql2" && options[:id] == :bigint
+            if [:integer, :bigint].include?(options[:id]) && !options.key?(:default)
+              options[:default] = nil
+            end
+          end
+
           # Since 5.1 Postgres adapter uses bigserial type for primary
           # keys by default and MySQL uses bigint. This compat layer makes old migrations utilize
           # serial/int type instead -- the way it used to work before 5.1.
diff --git a/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb b/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb
deleted file mode 100644
index 5d3125c2be..0000000000
--- a/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb
+++ /dev/null
@@ -1,60 +0,0 @@
-require "cases/helper"
-
-class MysqlLegacyMigrationTest < ActiveRecord::Mysql2TestCase
-  self.use_transactional_tests = false
-
-  class GenerateTableWithoutBigint < ActiveRecord::Migration[5.0]
-    def change
-      create_table :legacy_integer_pk do |table|
-        table.string :foo
-      end
-
-      create_table :override_pk, id: :bigint do |table|
-        table.string :bar
-      end
-    end
-  end
-
-  def setup
-    super
-    @connection = ActiveRecord::Base.connection
-
-    @migration_verbose_old = ActiveRecord::Migration.verbose
-    ActiveRecord::Migration.verbose = false
-
-    migrations = [GenerateTableWithoutBigint.new(nil, 1)]
-
-    ActiveRecord::Migrator.new(:up, migrations).migrate
-  end
-
-  def teardown
-    ActiveRecord::Migration.verbose = @migration_verbose_old
-    @connection.drop_table("legacy_integer_pk")
-    @connection.drop_table("override_pk")
-    ActiveRecord::SchemaMigration.delete_all rescue nil
-    super
-  end
-
-  def test_create_table_uses_integer_as_pkey_by_default
-    col = column(:legacy_integer_pk, :id)
-    assert_equal "int(11)", sql_type_for(col)
-    assert col.auto_increment?
-  end
-
-  def test_create_tables_respects_pk_column_type_override
-    col = column(:override_pk, :id)
-    assert_equal "bigint(20)", sql_type_for(col)
-  end
-
-  private
-
-    def column(table_name, column_name)
-      ActiveRecord::Base.connection
-        .columns(table_name.to_s)
-        .detect { |c| c.name == column_name.to_s }
-    end
-
-    def sql_type_for(col)
-      col && col.sql_type
-    end
-end
diff --git a/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb b/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb
deleted file mode 100644
index 082fe95053..0000000000
--- a/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb
+++ /dev/null
@@ -1,54 +0,0 @@
-require "cases/helper"
-
-class PostgresqlLegacyMigrationTest < ActiveRecord::PostgreSQLTestCase
-  class GenerateTableWithoutBigserial < ActiveRecord::Migration[5.0]
-    def change
-      create_table :legacy_integer_pk do |table|
-        table.string :foo
-      end
-
-      create_table :override_pk, id: :bigint do |table|
-        table.string :bar
-      end
-    end
-  end
-
-  def setup
-    super
-
-    @migration_verbose_old = ActiveRecord::Migration.verbose
-    ActiveRecord::Migration.verbose = false
-
-    migrations = [GenerateTableWithoutBigserial.new(nil, 1)]
-    ActiveRecord::Migrator.new(:up, migrations).migrate
-  end
-
-  def teardown
-    ActiveRecord::Migration.verbose = @migration_verbose_old
-
-    super
-  end
-
-  def test_create_table_uses_serial_as_pkey_by_default
-    col = column(:legacy_integer_pk, :id)
-    assert_equal "integer", sql_type_for(col)
-    assert col.serial?
-  end
-
-  def test_create_tables_respects_pk_column_type_override
-    col = column(:override_pk, :id)
-    assert_equal "bigint", sql_type_for(col)
-  end
-
-  private
-
-    def column(table_name, column_name)
-      ActiveRecord::Base.connection.
-        columns(table_name.to_s).
-        detect { |c| c.name == column_name.to_s }
-    end
-
-    def sql_type_for(col)
-      col && col.sql_type
-    end
-end
diff --git a/activerecord/test/cases/adapters/sqlite3/legacy_migration_test.rb b/activerecord/test/cases/adapters/sqlite3/legacy_migration_test.rb
deleted file mode 100644
index fcca8d66b5..0000000000
--- a/activerecord/test/cases/adapters/sqlite3/legacy_migration_test.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-require "cases/helper"
-
-class SqliteLegacyMigrationTest < ActiveRecord::SQLite3TestCase
-  self.use_transactional_tests = false
-
-  class GenerateTableWithoutBigint < ActiveRecord::Migration[5.0]
-    def change
-      create_table :legacy_integer_pk do |table|
-        table.string :foo
-      end
-
-      create_table :override_pk, id: :bigint do |table|
-        table.string :bar
-      end
-    end
-  end
-
-  def setup
-    super
-    @connection = ActiveRecord::Base.connection
-
-    @migration_verbose_old = ActiveRecord::Migration.verbose
-    ActiveRecord::Migration.verbose = false
-
-    migrations = [GenerateTableWithoutBigint.new(nil, 1)]
-
-    ActiveRecord::Migrator.new(:up, migrations).migrate
-  end
-
-  def teardown
-    ActiveRecord::Migration.verbose = @migration_verbose_old
-    @connection.drop_table("legacy_integer_pk")
-    @connection.drop_table("override_pk")
-    ActiveRecord::SchemaMigration.delete_all rescue nil
-    super
-  end
-
-  def test_create_table_uses_integer_as_pkey_by_default
-    col = column(:legacy_integer_pk, :id)
-    assert_equal "INTEGER", sql_type_for(col)
-    assert primary_key?(:legacy_integer_pk, "id"), "id is not primary key"
-  end
-
-  private
-
-    def column(table_name, column_name)
-      ActiveRecord::Base.connection
-        .columns(table_name.to_s)
-        .detect { |c| c.name == column_name.to_s }
-    end
-
-    def sql_type_for(col)
-      col && col.sql_type
-    end
-
-    def primary_key?(table_name, column)
-      ActiveRecord::Base.connection.execute("PRAGMA table_info(#{table_name})").find { |col| col["name"] == column }["pk"] == 1
-    end
-end
diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb
index 9296f3da90..fc3ec47825 100644
--- a/activerecord/test/cases/migration/compatibility_test.rb
+++ b/activerecord/test/cases/migration/compatibility_test.rb
@@ -1,4 +1,5 @@
 require "cases/helper"
+require "support/schema_dumping_helper"
 
 module ActiveRecord
   class Migration
@@ -111,3 +112,106 @@ module ActiveRecord
     end
   end
 end
+
+class LegacyPrimaryKeyTest < ActiveRecord::TestCase
+  include SchemaDumpingHelper
+
+  self.use_transactional_tests = false
+
+  class LegacyPrimaryKey < ActiveRecord::Base
+  end
+
+  def setup
+    @migration = nil
+    @verbose_was = ActiveRecord::Migration.verbose
+    ActiveRecord::Migration.verbose = false
+  end
+
+  def teardown
+    @migration.migrate(:down) if @migration
+    ActiveRecord::Migration.verbose = @verbose_was
+    ActiveRecord::SchemaMigration.delete_all rescue nil
+    LegacyPrimaryKey.reset_column_information
+  end
+
+  def test_legacy_primary_key_should_be_auto_incremented
+    @migration = Class.new(ActiveRecord::Migration[5.0]) {
+      def change
+        create_table :legacy_primary_keys do |t|
+        end
+      end
+    }.new
+
+    @migration.migrate(:up)
+
+    legacy_pk = LegacyPrimaryKey.columns_hash["id"]
+    assert_not legacy_pk.bigint?
+    assert_not legacy_pk.null
+
+    record1 = LegacyPrimaryKey.create!
+    assert_not_nil record1.id
+
+    record1.destroy
+
+    record2 = LegacyPrimaryKey.create!
+    assert_not_nil record2.id
+    assert_operator record2.id, :>, record1.id
+  end
+
+  def test_legacy_integer_primary_key_should_not_be_auto_incremented
+    skip if current_adapter?(:SQLite3Adapter)
+
+    @migration = Class.new(ActiveRecord::Migration[5.0]) {
+      def change
+        create_table :legacy_primary_keys, id: :integer do |t|
+        end
+      end
+    }.new
+
+    @migration.migrate(:up)
+
+    assert_raises(ActiveRecord::NotNullViolation) do
+      LegacyPrimaryKey.create!
+    end
+
+    schema = dump_table_schema "legacy_primary_keys"
+    assert_match %r{create_table "legacy_primary_keys", id: :integer, default: nil}, schema
+  end
+
+  if current_adapter?(:Mysql2Adapter)
+    def test_legacy_bigint_primary_key_should_be_auto_incremented
+      @migration = Class.new(ActiveRecord::Migration[5.0]) {
+        def change
+          create_table :legacy_primary_keys, id: :bigint
+        end
+      }.new
+
+      @migration.migrate(:up)
+
+      legacy_pk = LegacyPrimaryKey.columns_hash["id"]
+      assert legacy_pk.bigint?
+      assert legacy_pk.auto_increment?
+
+      schema = dump_table_schema "legacy_primary_keys"
+      assert_match %r{create_table "legacy_primary_keys", (?!id: :bigint, default: nil)}, schema
+    end
+  else
+    def test_legacy_bigint_primary_key_should_not_be_auto_incremented
+      @migration = Class.new(ActiveRecord::Migration[5.0]) {
+        def change
+          create_table :legacy_primary_keys, id: :bigint do |t|
+          end
+        end
+      }.new
+
+      @migration.migrate(:up)
+
+      assert_raises(ActiveRecord::NotNullViolation) do
+        LegacyPrimaryKey.create!
+      end
+
+      schema = dump_table_schema "legacy_primary_keys"
+      assert_match %r{create_table "legacy_primary_keys", id: :bigint, default: nil}, schema
+    end
+  end
+end
diff --git a/activerecord/test/cases/primary_keys_test.rb b/activerecord/test/cases/primary_keys_test.rb
index 1d72899102..f92ee15cfa 100644
--- a/activerecord/test/cases/primary_keys_test.rb
+++ b/activerecord/test/cases/primary_keys_test.rb
@@ -319,70 +319,87 @@ class CompositePrimaryKeyTest < ActiveRecord::TestCase
   end
 end
 
-if current_adapter?(:Mysql2Adapter)
-  class PrimaryKeyIntegerNilDefaultTest < ActiveRecord::TestCase
-    include SchemaDumpingHelper
+class PrimaryKeyIntegerNilDefaultTest < ActiveRecord::TestCase
+  include SchemaDumpingHelper
 
-    self.use_transactional_tests = false
+  self.use_transactional_tests = false
 
-    def setup
-      @connection = ActiveRecord::Base.connection
-      @connection.create_table(:int_defaults, id: :integer, default: nil, force: true)
-    end
+  def setup
+    @connection = ActiveRecord::Base.connection
+  end
 
-    def teardown
-      @connection.drop_table :int_defaults, if_exists: true
-    end
+  def teardown
+    @connection.drop_table :int_defaults, if_exists: true
+  end
 
-    test "primary key with integer allows default override via nil" do
-      column = @connection.columns(:int_defaults).find { |c| c.name == "id" }
-      assert_equal :integer, column.type
-      assert_not column.auto_increment?
-    end
+  def test_schema_dump_primary_key_integer_with_default_nil
+    skip if current_adapter?(:SQLite3Adapter)
+    @connection.create_table(:int_defaults, id: :integer, default: nil, force: true)
+    schema = dump_table_schema "int_defaults"
+    assert_match %r{create_table "int_defaults", id: :integer, default: nil}, schema
+  end
 
-    test "schema dump primary key with int default nil" do
-      schema = dump_table_schema "int_defaults"
-      assert_match %r{create_table "int_defaults", id: :integer, default: nil}, schema
-    end
+  def test_schema_dump_primary_key_bigint_with_default_nil
+    @connection.create_table(:int_defaults, id: :bigint, default: nil, force: true)
+    schema = dump_table_schema "int_defaults"
+    assert_match %r{create_table "int_defaults", id: :bigint, default: nil}, schema
   end
 end
 
-class PrimaryKeyIntegerTest < ActiveRecord::TestCase
-  include SchemaDumpingHelper
+if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
+  class PrimaryKeyIntegerTest < ActiveRecord::TestCase
+    include SchemaDumpingHelper
 
-  self.use_transactional_tests = false
+    self.use_transactional_tests = false
 
-  class Widget < ActiveRecord::Base
-  end
+    class Widget < ActiveRecord::Base
+    end
 
-  setup do
-    @connection = ActiveRecord::Base.connection
-    @connection.create_table(:widgets, force: true)
-  end
+    setup do
+      @connection = ActiveRecord::Base.connection
+      if current_adapter?(:PostgreSQLAdapter)
+        @connection.create_table(:widgets, id: :serial, force: true)
+      else
+        @connection.create_table(:widgets, id: :integer, force: true)
+      end
+    end
 
-  teardown do
-    @connection.drop_table :widgets, if_exists: true
-    Widget.reset_column_information
-  end
+    teardown do
+      @connection.drop_table :widgets, if_exists: true
+    end
 
-  if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
-    test "schema dump primary key with bigserial" do
-      schema = dump_table_schema "widgets"
-      assert_match %r{create_table "widgets", force: :cascade}, schema
+    test "primary key column type with serial/integer" do
+      column = @connection.columns(:widgets).find { |c| c.name == "id" }
+      assert_equal :integer, column.type
+      assert_not column.bigint?
     end
-  end
 
-  test "primary key column type" do
-    column_type = Widget.type_for_attribute(Widget.primary_key)
-    assert_equal :integer, column_type.type
+    test "primary key with serial/integer are automatically numbered" do
+      widget = Widget.create!
+      assert_not_nil widget.id
+    end
 
-    if current_adapter?(:PostgreSQLAdapter, :Mysql2Adapter)
-      assert_equal 8, column_type.limit
+    test "schema dump primary key with serial/integer" do
+      schema = dump_table_schema "widgets"
+      if current_adapter?(:PostgreSQLAdapter)
+        assert_match %r{create_table "widgets", id: :serial, force: :cascade}, schema
+      else
+        assert_match %r{create_table "widgets", id: :integer, force: :cascade}, schema
+      end
     end
 
     if current_adapter?(:Mysql2Adapter)
-      column = @connection.columns(:widgets).find { |c| c.name == "id" }
-      assert column.auto_increment?
+      test "primary key column type with options" do
+        @connection.create_table(:widgets, id: :primary_key, limit: 4, unsigned: true, force: true)
+        column = @connection.columns(:widgets).find { |c| c.name == "id" }
+        assert column.auto_increment?
+        assert_equal :integer, column.type
+        assert_equal 4, column.limit
+        assert column.unsigned?
+
+        schema = dump_table_schema "widgets"
+        assert_match %r{create_table "widgets", id: :integer, unsigned: true, force: :cascade}, schema
+      end
     end
   end
 end