Rails 6.1: change schema dumper condition to only dump explicit default for non identity integers (#902)

* coerse test to change binding syntax to @n

* add default option to pk dump

* Revert "coerse test to change binding syntax to @n"

This reverts commit 63eddd4.

* remove test coercion

* set primary_key_nonclustered type to bigint

seems this was missing when primary_key changed from integer to bigint
(620baf2#diff-86264ef1995b0800604b26d64ede427e8a3ed59347993bfb409c596e83f4b86f)

* remove redundat set of primary_key option. AbstractAdapter already does this

Absctract SchemaDefinition sets primary_key option
(https://github.com/rails/rails/blob/v6.1.0/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb#L234).

There is no point in doing this

* remove unnecesary primary check. default_primary_key is called on columns that return true to is_primary?

* rewrite 'explicit_primary_key_default?'. String primary keys should not have explicit default

* specs about primary keys

* fix typo

Author: mgrunberg

lib/active_record/connection_adapters/sqlserver/schema_dumper.rb

@@ -15,7 +15,7 @@ class SchemaDumper < ConnectionAdapters::SchemaDumper
         private
 
         def explicit_primary_key_default?(column)
-          column.is_primary? && !column.is_identity?
+          column.type == :integer && !column.is_identity?
         end
 
         def schema_limit(column)
@@ -37,7 +37,7 @@ def schema_collation(column)
         end
 
         def default_primary_key?(column)
-          super && column.is_primary? && column.is_identity?
+          super && column.is_identity?
         end
       end
     end

lib/active_record/connection_adapters/sqlserver/schema_statements.rb

@@ -332,7 +332,7 @@ def quoted_scope(name = nil, type: nil)
         def initialize_native_database_types
           {
             primary_key: "bigint NOT NULL IDENTITY(1,1) PRIMARY KEY",
-            primary_key_nonclustered: "int NOT NULL IDENTITY(1,1) PRIMARY KEY NONCLUSTERED",
+            primary_key_nonclustered: "bigint NOT NULL IDENTITY(1,1) PRIMARY KEY NONCLUSTERED",
             integer: { name: "int", limit: 4 },
             bigint: { name: "bigint" },
             boolean: { name: "bit" },

lib/active_record/connection_adapters/sqlserver/table_definition.rb

@@ -9,7 +9,6 @@ def primary_key(name, type = :primary_key, **options)
             options[:is_identity] = true unless options.key?(:default)
           elsif type == :uuid
             options[:default] = options.fetch(:default, "NEWID()")
-            options[:primary_key] = true
           end
           super
         end

test/cases/primary_keys_test_sqlserver.rb

@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require "cases/helper_sqlserver"
+require "support/schema_dumping_helper"
+
+class PrimaryKeyUuidTypeTest < ActiveRecord::TestCase
+  include SchemaDumpingHelper
+
+  self.use_transactional_tests = false
+
+  class Barcode < ActiveRecord::Base
+  end
+
+  setup do
+    @connection = ActiveRecord::Base.connection
+    @connection.create_table(:barcodes, primary_key: "code", id: :uuid, force: true)
+  end
+
+  teardown do
+    @connection.drop_table(:barcodes, if_exists: true)
+  end
+
+  def test_any_type_primary_key
+    assert_equal "code", Barcode.primary_key
+
+    column = Barcode.column_for_attribute(Barcode.primary_key)
+    assert_not column.null
+    assert_equal :uuid, column.type
+    assert_not_predicate column, :is_identity?
+    assert_predicate column, :is_primary?
+  ensure
+    Barcode.reset_column_information
+  end
+
+  test "schema dump primary key includes default" do
+    schema = dump_table_schema "barcodes"
+    assert_match %r/create_table "barcodes", primary_key: "code", id: :uuid, default: -> { "newid\(\)" }/, schema
+  end
+end
+
+class PrimaryKeyIntegerTest < ActiveRecord::TestCase
+  include SchemaDumpingHelper
+
+  self.use_transactional_tests = false
+
+  class Barcode < ActiveRecord::Base
+  end
+
+  class Widget < ActiveRecord::Base
+  end
+
+  setup do
+    @connection = ActiveRecord::Base.connection
+  end
+
+  teardown do
+    @connection.drop_table :barcodes, if_exists: true
+    @connection.drop_table :widgets, if_exists: true
+  end
+
+  test "integer primary key without default" do
+    @connection.create_table(:widgets, id: :integer, force: true)
+    column = @connection.columns(:widgets).find { |c| c.name == "id" }
+    assert_predicate column, :is_primary?
+    assert_predicate column, :is_identity?
+    assert_equal :integer, column.type
+    assert_not_predicate column, :bigint?
+
+    schema = dump_table_schema "widgets"
+    assert_match %r/create_table "widgets", id: :integer, force: :cascade do/, schema
+  end
+
+  test "bigint primary key without default" do
+    @connection.create_table(:widgets, id: :bigint, force: true)
+    column = @connection.columns(:widgets).find { |c| c.name == "id" }
+    assert_predicate column, :is_primary?
+    assert_predicate column, :is_identity?
+    assert_equal :integer, column.type
+    assert_predicate column, :bigint?
+
+    schema = dump_table_schema "widgets"
+    assert_match %r/create_table "widgets", force: :cascade do/, schema
+  end
+
+  test "don't set identity to integer and bigint when there is a default" do
+    @connection.create_table(:barcodes, id: :integer, default: nil, force: true)
+    @connection.create_table(:widgets, id: :bigint, default: nil, force: true)
+
+    column = @connection.columns(:widgets).find { |c| c.name == "id" }
+    assert_predicate column, :is_primary?
+    assert_not_predicate column, :is_identity?
+
+    schema = dump_table_schema "widgets"
+    assert_match %r/create_table "widgets", id: :bigint, default: nil, force: :cascade do/, schema
+
+    column = @connection.columns(:barcodes).find { |c| c.name == "id" }
+    assert_predicate column, :is_primary?
+    assert_not_predicate column, :is_identity?
+
+    schema = dump_table_schema "barcodes"
+    assert_match %r/create_table "barcodes", id: :integer, default: nil, force: :cascade do/, schema
+  end
+end