Merge pull request #27334 from kamipo/pk_and_ref_columns_should_be_identical_type

`primary_key` and `references` columns should be identical type

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

@@ -354,8 +354,8 @@ def timestamps(**options)
       #
       # See {connection.add_reference}[rdoc-ref:SchemaStatements#add_reference] for details of the options you can use.
       def references(*args, **options)
-        args.each do |col|
-          ReferenceDefinition.new(col, **options).add_to(self)
+        args.each do |ref_name|
+          ReferenceDefinition.new(ref_name, options).add_to(self)
         end
       end
       alias :belongs_to :references
@@ -589,8 +589,7 @@ def rename(column_name, new_column_name)
       #  t.belongs_to(:supplier, foreign_key: true)
       #
       # See {connection.add_reference}[rdoc-ref:SchemaStatements#add_reference] for details of the options you can use.
-      def references(*args)
-        options = args.extract_options!
+      def references(*args, **options)
         args.each do |ref_name|
           @base.add_reference(name, ref_name, options)
         end
@@ -603,8 +602,7 @@ def references(*args)
       #  t.remove_belongs_to(:supplier, polymorphic: true)
       #
       # See {connection.remove_reference}[rdoc-ref:SchemaStatements#remove_reference]
-      def remove_references(*args)
-        options = args.extract_options!
+      def remove_references(*args, **options)
         args.each do |ref_name|
           @base.remove_reference(name, ref_name, options)
         end

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

@@ -828,8 +828,8 @@ def index_name_exists?(table_name, index_name, default)
       #
       #   add_reference(:products, :supplier, foreign_key: {to_table: :firms})
       #
-      def add_reference(table_name, *args)
-        ReferenceDefinition.new(*args).add_to(update_table_definition(table_name, self))
+      def add_reference(table_name, ref_name, **options)
+        ReferenceDefinition.new(ref_name, options).add_to(update_table_definition(table_name, self))
       end
       alias :add_belongs_to :add_reference
 

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

@@ -13,6 +13,11 @@ def primary_key(name, type = :primary_key, **options)
 
       class TableDefinition < ActiveRecord::ConnectionAdapters::TableDefinition
         include ColumnMethods
+
+        def references(*args, **options)
+          super(*args, type: :integer, **options)
+        end
+        alias :belongs_to :references
       end
 
       class Table < ActiveRecord::ConnectionAdapters::Table

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -443,6 +443,11 @@ def rename_column(table_name, column_name, new_column_name) #:nodoc:
         rename_column_indexes(table_name, column.name, new_column_name)
       end
 
+      def add_reference(table_name, ref_name, **options) # :nodoc:
+        super(table_name, ref_name, type: :integer, **options)
+      end
+      alias :add_belongs_to :add_reference
+
       def foreign_keys(table_name)
         fk_info = select_all("PRAGMA foreign_key_list(#{quote(table_name)})", "SCHEMA")
         fk_info.map do |row|

activerecord/lib/active_record/migration/compatibility.rb

@@ -14,6 +14,13 @@ def self.find(version)
       V5_1 = Current
 
       class V5_0 < V5_1
+        module TableDefinition
+          def references(*args, **options)
+            super(*args, type: :integer, **options)
+          end
+          alias :belongs_to :references
+        end
+
         def create_table(table_name, options = {})
           if adapter_name == "PostgreSQL"
             if options[:id] == :uuid && !options.key?(:default)
@@ -34,8 +41,35 @@ def create_table(table_name, options = {})
             options[:id] = :integer
           end
 
-          super
+          if block_given?
+            super(table_name, options) do |t|
+              class << t
+                prepend TableDefinition
+              end
+              yield t
+            end
+          else
+            super
+          end
         end
+
+        def change_table(table_name, options = {})
+          if block_given?
+            super(table_name, options) do |t|
+              class << t
+                prepend TableDefinition
+              end
+              yield t
+            end
+          else
+            super
+          end
+        end
+
+        def add_reference(table_name, ref_name, **options)
+          super(table_name, ref_name, type: :integer, **options)
+        end
+        alias :add_belongs_to :add_reference
       end
 
       class V4_2 < V5_0

activerecord/test/cases/adapter_test.rb

@@ -194,7 +194,7 @@ def test_value_limit_violations_are_translated_to_specific_exception
 
       def test_numeric_value_out_of_ranges_are_translated_to_specific_exception
         error = assert_raises(ActiveRecord::RangeError) do
-          Book.connection.create("INSERT INTO books(author_id) VALUES (2147483648)")
+          Book.connection.create("INSERT INTO books(author_id) VALUES (9223372036854775808)")
         end
 
         assert_not_nil error.cause

activerecord/test/cases/base_test.rb

@@ -98,6 +98,13 @@ def test_primary_key_with_no_id
     assert_nil Edge.primary_key
   end
 
+  def test_primary_key_and_references_columns_should_be_identical_type
+    pk = Author.columns_hash["id"]
+    ref = Post.columns_hash["author_id"]
+
+    assert_equal pk.bigint?, ref.bigint?
+  end
+
   def test_many_mutations
     car = Car.new name: "<3<3<3"
     car.engines_count = 0

activerecord/test/cases/migration/compatibility_test.rb

@@ -138,6 +138,7 @@ 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|
+          t.references :legacy_ref
         end
       end
     }.new
@@ -148,6 +149,9 @@ def change
     assert_not legacy_pk.bigint?
     assert_not legacy_pk.null
 
+    legacy_ref = LegacyPrimaryKey.columns_hash["legacy_ref_id"]
+    assert_not legacy_ref.bigint?
+
     record1 = LegacyPrimaryKey.create!
     assert_not_nil record1.id
 

activerecord/test/cases/migration/foreign_key_test.rb

@@ -247,7 +247,7 @@ def change
             create_table("cities") { |t| }
 
             create_table("houses") do |t|
-              t.column :city_id, :bigint
+              t.references :city
             end
             add_foreign_key :houses, :cities, column: "city_id"
 
@@ -279,7 +279,7 @@ def change
             create_table(:schools)
 
             create_table(:classes) do |t|
-              t.column :school_id, :bigint
+              t.references :school
             end
             add_foreign_key :classes, :schools
           end

activerecord/test/cases/migration/references_foreign_key_test.rb

@@ -42,8 +42,7 @@ class ReferencesForeignKeyInCreateTest < ActiveRecord::TestCase
 
         test "options hash can be passed" do
           @connection.change_table :testing_parents do |t|
-            t.bigint :other_id
-            t.index :other_id, unique: true
+            t.references :other, index: { unique: true }
           end
           @connection.create_table :testings do |t|
             t.references :testing_parent, foreign_key: { primary_key: :other_id }
@@ -110,8 +109,7 @@ class ReferencesForeignKeyTest < ActiveRecord::TestCase
 
         test "foreign keys accept options when changing the table" do
           @connection.change_table :testing_parents do |t|
-            t.bigint :other_id
-            t.index :other_id, unique: true
+            t.references :other, index: { unique: true }
           end
           @connection.create_table :testings
           @connection.change_table :testings do |t|
@@ -195,18 +193,15 @@ def test_references_foreign_key_with_suffix
 
         test "multiple foreign keys can be added to the same table" do
           @connection.create_table :testings do |t|
-            t.bigint :col_1
-            t.bigint :col_2
-
-            t.foreign_key :testing_parents, column: :col_1
-            t.foreign_key :testing_parents, column: :col_2
+            t.references :parent1, foreign_key: { to_table: :testing_parents }
+            t.references :parent2, foreign_key: { to_table: :testing_parents }
           end
 
-          fks = @connection.foreign_keys("testings")
+          fks = @connection.foreign_keys("testings").sort_by(&:column)
 
           fk_definitions = fks.map { |fk| [fk.from_table, fk.to_table, fk.column] }
-          assert_equal([["testings", "testing_parents", "col_1"],
-                        ["testings", "testing_parents", "col_2"]], fk_definitions)
+          assert_equal([["testings", "testing_parents", "parent1_id"],
+                        ["testings", "testing_parents", "parent2_id"]], fk_definitions)
         end
       end
     end

activerecord/test/cases/schema_dumper_test.rb

@@ -341,7 +341,7 @@ def up
 
       create_table("dogs") do |t|
         t.column :name, :string
-        t.column :owner_id, :bigint
+        t.references :owner
         t.index [:name]
         t.foreign_key :dog_owners, column: "owner_id"
       end

activerecord/test/schema/schema.rb

@@ -54,8 +54,8 @@
 
   create_table :authors, force: true do |t|
     t.string :name, null: false
-    t.bigint :author_address_id
-    t.integer :author_address_extra_id
+    t.references :author_address
+    t.references :author_address_extra
     t.string :organization_id
     t.string :owned_essay_id
   end
@@ -88,7 +88,7 @@
   end
 
   create_table :books, force: true do |t|
-    t.integer :author_id
+    t.references :author
     t.string :format
     t.column :name, :string
     t.column :status, :integer, default: 0
@@ -306,7 +306,7 @@
   end
 
   create_table :engines, force: true do |t|
-    t.bigint :car_id
+    t.references :car, index: false
   end
 
   create_table :entrants, force: true do |t|
@@ -664,8 +664,8 @@
   end
 
   create_table :posts, force: true do |t|
-    t.integer :author_id
-    t.string  :title, null: false
+    t.references :author
+    t.string :title, null: false
     # use VARCHAR2(4000) instead of CLOB datatype as CLOB data type has many limitations in
     # Oracle SELECT WHERE clause which causes many unit test failures
     if current_adapter?(:OracleAdapter)