From 3e452b12043ecfd983c6132d6eb97eb79db952b7 Mon Sep 17 00:00:00 2001 From: Pavel Pravosud Date: Tue, 10 May 2016 15:10:46 -0700 Subject: [PATCH] Make pg adapter use bigserial for pk by default --- .gitignore | 1 - activerecord/CHANGELOG.md | 4 ++ .../abstract_mysql_adapter.rb | 24 +++++++- activerecord/lib/active_record/errors.rb | 28 +++++++++ .../active_record/migration/compatibility.rb | 12 +++- .../adapters/mysql2/legacy_migration_test.rb | 60 +++++++++++++++++++ .../adapters/mysql2/mysql2_adapter_test.rb | 13 ++++ .../postgresql/legacy_migration_test.rb | 54 +++++++++++++++++ .../adapters/sqlite3/legacy_migration_test.rb | 59 ++++++++++++++++++ activerecord/test/cases/primary_keys_test.rb | 7 +++ activerecord/test/schema/schema.rb | 3 + 11 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb create mode 100644 activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb create mode 100644 activerecord/test/cases/adapters/sqlite3/legacy_migration_test.rb diff --git a/.gitignore b/.gitignore index d48828ea03..9268977c2f 100644 --- a/.gitignore +++ b/.gitignore @@ -19,4 +19,3 @@ pkg /railties/doc /railties/tmp /guides/output -/*/.byebug_history \ No newline at end of file diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 96d0ad3b30..2182ee78bb 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,7 @@ +* PostgreSQL & MySQL: Use big integer as primary key type for new tables + + *Jon McCartie*, *Pavel Pravosud* + * Change the type argument of `ActiveRecord::Base#attribute` to be optional. The default is now `ActiveRecord::Type::Value.new`, which provides no type casting behavior. diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 6eb7e0ae52..971b274265 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -39,7 +39,7 @@ def arel_visitor # :nodoc: self.emulate_booleans = true NATIVE_DATABASE_TYPES = { - primary_key: "BIGINT(8) UNSIGNED DEFAULT NULL auto_increment PRIMARY KEY", + primary_key: "BIGINT(8) UNSIGNED auto_increment PRIMARY KEY", string: { name: "varchar", limit: 255 }, text: { name: "text", limit: 65535 }, integer: { name: "int", limit: 4 }, @@ -736,6 +736,8 @@ def add_options_for_index_columns(quoted_columns, **options) ER_NO_REFERENCED_ROW_2 = 1452 ER_DATA_TOO_LONG = 1406 ER_LOCK_DEADLOCK = 1213 + ER_CANNOT_ADD_FOREIGN = 1215 + ER_CANNOT_CREATE_TABLE = 1005 def translate_exception(exception, message) case error_number(exception) @@ -743,6 +745,14 @@ def translate_exception(exception, message) RecordNotUnique.new(message) when ER_NO_REFERENCED_ROW_2 InvalidForeignKey.new(message) + when ER_CANNOT_ADD_FOREIGN + mismatched_foreign_key(message) + when ER_CANNOT_CREATE_TABLE + if message.include?("errno: 150") + mismatched_foreign_key(message) + else + super + end when ER_DATA_TOO_LONG ValueTooLong.new(message) when ER_LOCK_DEADLOCK @@ -914,6 +924,18 @@ def create_table_definition(*args) # :nodoc: MySQL::TableDefinition.new(*args) end + def mismatched_foreign_key(message) + parts = message.scan(/`(\w+)`[ $)]/).flatten + MismatchedForeignKey.new( + self, + message: message, + table: parts[0], + foreign_key: parts[1], + target_table: parts[2], + primary_key: parts[3], + ) + end + def extract_schema_qualified_name(string) # :nodoc: schema, name = string.to_s.scan(/[^`.\s]+|`[^`]*`/) schema, name = @config[:database], schema unless name diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index 6464d40c94..7c8be37326 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -123,6 +123,34 @@ class RecordNotUnique < WrappedDatabaseException class InvalidForeignKey < WrappedDatabaseException end + # Raised when a foreign key constraint cannot be added because the column type does not match the referenced column type. + class MismatchedForeignKey < WrappedDatabaseException + def initialize(adapter = nil, message: nil, table: nil, foreign_key: nil, target_table: nil, primary_key: nil) + @adapter = adapter + if table + msg = <<-EOM.strip_heredoc + Column `#{foreign_key}` on table `#{table}` has a type of `#{column_type(table, foreign_key)}`. + This does not match column `#{primary_key}` on `#{target_table}`, which has type `#{column_type(target_table, primary_key)}`. + To resolve this issue, change the type of the `#{foreign_key}` column on `#{table}` to be :integer. (For example `t.integer #{foreign_key}`). + EOM + else + msg = <<-EOM + There is a mismatch between the foreign key and primary key column types. + Verify that the foreign key column type and the primary key of the associated table match types. + EOM + end + if message + msg << "\nOriginal message: #{message}" + end + super(msg) + end + + private + def column_type(table, column) + @adapter.columns(table).detect { |c| c.name == column }.sql_type + end + end + # Raised when a record cannot be inserted or updated because a value too long for a column type. class ValueTooLong < StatementInvalid end diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index ae45ac7157..9d3652d63b 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -104,11 +104,21 @@ def index_name_for_remove(table_name, options = {}) class V5_0 < V5_1 def create_table(table_name, options = {}) - if ActiveRecord::Base.connection.adapter_name == "PostgreSQL" + connection_name = self.connection.adapter_name + if connection_name == "PostgreSQL" if options[:id] == :uuid && !options[:default] options[:default] = "uuid_generate_v4()" 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. + if options[:id].blank? + options[:id] = :integer + options[:auto_increment] = true + end + super end end diff --git a/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb b/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb new file mode 100644 index 0000000000..5d3125c2be --- /dev/null +++ b/activerecord/test/cases/adapters/mysql2/legacy_migration_test.rb @@ -0,0 +1,60 @@ +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/mysql2/mysql2_adapter_test.rb b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb index 69336eb906..aab3dcb724 100644 --- a/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb +++ b/activerecord/test/cases/adapters/mysql2/mysql2_adapter_test.rb @@ -65,6 +65,19 @@ def order.to_sql @conn.columns_for_distinct("posts.id", [order]) end + def test_errors_for_bigint_fks_on_integer_pk_table + # table old_cars has primary key of integer + + error = assert_raises(ActiveRecord::MismatchedForeignKey) do + @conn.add_reference :engines, :old_car + @conn.add_foreign_key :engines, :old_cars + end + + assert_match "Column `old_car_id` on table `engines` has a type of `bigint(20)`", error.message + assert_not_nil error.cause + @conn.exec_query("ALTER TABLE engines DROP COLUMN old_car_id") + end + private def with_example_table(definition = "id int auto_increment primary key, number int, data varchar(255)", &block) diff --git a/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb b/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb new file mode 100644 index 0000000000..082fe95053 --- /dev/null +++ b/activerecord/test/cases/adapters/postgresql/legacy_migration_test.rb @@ -0,0 +1,54 @@ +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 new file mode 100644 index 0000000000..fcca8d66b5 --- /dev/null +++ b/activerecord/test/cases/adapters/sqlite3/legacy_migration_test.rb @@ -0,0 +1,59 @@ +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/primary_keys_test.rb b/activerecord/test/cases/primary_keys_test.rb index 2af2a2df89..30c161cd99 100644 --- a/activerecord/test/cases/primary_keys_test.rb +++ b/activerecord/test/cases/primary_keys_test.rb @@ -361,6 +361,13 @@ class Widget < ActiveRecord::Base Widget.reset_column_information 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 + end + end + test "primary key column type" do column_type = Widget.type_for_attribute(Widget.primary_key) assert_equal :integer, column_type.type diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index dcf29d36f4..658591b6ec 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -126,6 +126,9 @@ t.timestamps null: false end + create_table :old_cars, id: :integer, force: true do |t| + end + create_table :carriers, force: true create_table :categories, force: true do |t| -- GitLab