New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Default Primary Keys to BIGINT #26266

Merged
merged 2 commits into from Dec 5, 2016
Jump to file or symbol
Failed to load files and symbols.
+332 −74
Diff settings

Always

Just for now

@@ -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.
@@ -71,7 +71,7 @@ def initialize(
polymorphic: false,
index: true,
foreign_key: false,
type: :integer,
type: :bigint,
**options
)
@name = name
@@ -56,7 +56,7 @@ def migration_keys
private
def default_primary_key?(column)
schema_type(column) == :integer
schema_type(column) == :bigint
end
def schema_type(column)
@@ -39,7 +39,7 @@ def arel_visitor # :nodoc:
self.emulate_booleans = true
NATIVE_DATABASE_TYPES = {
primary_key: "int 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,13 +736,23 @@ 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)
when ER_DUP_ENTRY
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
@@ -3,7 +3,10 @@ module ConnectionAdapters
module MySQL
module ColumnMethods
def primary_key(name, type = :primary_key, **options)
options[:auto_increment] = true if type == :bigint && !options.key?(:default)
if type == :primary_key && !options.key?(:default)
options[:auto_increment] = true
options[:limit] = 8
end
super
end
@@ -3,11 +3,9 @@ module ConnectionAdapters
module MySQL
module ColumnDumper
def column_spec_for_primary_key(column)
if column.bigint?
spec = { id: :bigint.inspect }
spec[:default] = schema_default(column) || "nil" unless column.auto_increment?
else
spec = super
spec = super
if column.type == :integer && !column.auto_increment?
spec[:default] = schema_default(column) || "nil"
end
spec[:unsigned] = "true" if column.unsigned?
spec
@@ -25,7 +25,7 @@ def migration_keys
private
def default_primary_key?(column)
schema_type(column) == :serial
schema_type(column) == :bigserial
end
def schema_type(column)
@@ -70,7 +70,7 @@ class PostgreSQLAdapter < AbstractAdapter
ADAPTER_NAME = "PostgreSQL".freeze
NATIVE_DATABASE_TYPES = {
primary_key: "serial primary key",
primary_key: "bigserial primary key",
string: { name: "character varying" },
text: { name: "text" },
integer: { name: "integer" },
@@ -0,0 +1,13 @@
module ActiveRecord
module ConnectionAdapters
module SQLite3
module ColumnDumper
private
def default_primary_key?(column)
schema_type(column) == :integer
end

This comment has been minimized.

@kamipo

kamipo Nov 4, 2016

Member

Looks like this module is unnecessary.

@kamipo

kamipo Nov 4, 2016

Member

Looks like this module is unnecessary.

This comment has been minimized.

@jmccartie

jmccartie Nov 7, 2016

Contributor

I'd prefer not to remove code that is not directly related to this PR (postgres and mysql)

@jmccartie

jmccartie Nov 7, 2016

Contributor

I'd prefer not to remove code that is not directly related to this PR (postgres and mysql)

This comment has been minimized.

@kamipo

kamipo Nov 7, 2016

Member

This PR adds sqlite3/schema_dumper.rb even though does not change sqlite3 behavior (only change postgres and mysql). Why?

@kamipo

kamipo Nov 7, 2016

Member

This PR adds sqlite3/schema_dumper.rb even though does not change sqlite3 behavior (only change postgres and mysql). Why?

This comment has been minimized.

@jmccartie

jmccartie Nov 7, 2016

Contributor

@kamipo Ah -- forgot that was added (this PR has been going on for awhile 😄 ). The parent class has it set as bigint, so we're locking SQLite at integer.

@jmccartie

jmccartie Nov 7, 2016

Contributor

@kamipo Ah -- forgot that was added (this PR has been going on for awhile 😄 ). The parent class has it set as bigint, so we're locking SQLite at integer.

This comment has been minimized.

@kamipo

kamipo Nov 7, 2016

Member

Oh, I see. Thanks. This (the parent class has it set as bigint) means that all third party adapters should choose whether implement default bigint pk (and implement Compatibility::V5_0) or override def default_primary_key?(column) schema_type(column) == :integer; end, right?

@kamipo

kamipo Nov 7, 2016

Member

Oh, I see. Thanks. This (the parent class has it set as bigint) means that all third party adapters should choose whether implement default bigint pk (and implement Compatibility::V5_0) or override def default_primary_key?(column) schema_type(column) == :integer; end, right?

This comment has been minimized.

@jmccartie

jmccartie Nov 7, 2016

Contributor

@kamipo That's my understanding. Since the default has changed, other adapters can either choose to join us in bigint land (by doing nothing), or can choose to stay in int land by overriding.

@jmccartie

jmccartie Nov 7, 2016

Contributor

@kamipo That's my understanding. Since the default has changed, other adapters can either choose to join us in bigint land (by doing nothing), or can choose to stay in int land by overriding.

This comment has been minimized.

@kamipo

kamipo Nov 9, 2016

Member

I see. Thanks.

@kamipo

kamipo Nov 9, 2016

Member

I see. Thanks.

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

hmmm. Can we invert this? I think it is a breaking change to require other adapters to change their code to opt-out our default behavior.

@rafaelfranca

rafaelfranca Nov 17, 2016

Member

hmmm. Can we invert this? I think it is a breaking change to require other adapters to change their code to opt-out our default behavior.

This comment has been minimized.

@sgrif

sgrif Nov 17, 2016

Member

I'm not sure other adapters would actually need to change here -- SQLite is special cased because it has no bigint type, but it's unique in that regard. However, as long as https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 is specific to those two adapters, I do agree that we should invert this.

@sgrif

sgrif Nov 17, 2016

Member

I'm not sure other adapters would actually need to change here -- SQLite is special cased because it has no bigint type, but it's unique in that regard. However, as long as https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 is specific to those two adapters, I do agree that we should invert this.

This comment has been minimized.

@matthewd

matthewd Nov 18, 2016

Member

Yeah, I think the most compatible choice would be to default to pushing everyone to bigint. SQLite just happens to spell that 'integer'.

We've decided everyone's database should use bigint PKs, and that's not something that individual adapters should be revisiting -- their only interest should be if they need to do something special to represent bigint (again, as SQLite does).

(As Sean noted, though, this means the migration compatibility thing needs to change.)

@matthewd

matthewd Nov 18, 2016

Member

Yeah, I think the most compatible choice would be to default to pushing everyone to bigint. SQLite just happens to spell that 'integer'.

We've decided everyone's database should use bigint PKs, and that's not something that individual adapters should be revisiting -- their only interest should be if they need to do something special to represent bigint (again, as SQLite does).

(As Sean noted, though, this means the migration compatibility thing needs to change.)

This comment has been minimized.

@jmccartie

jmccartie Nov 18, 2016

Contributor

@matthewd @sgrif Trying to follow... what needs to change exactly? I'm reading conflicting opinions and want to get it right.

@jmccartie

jmccartie Nov 18, 2016

Contributor

@matthewd @sgrif Trying to follow... what needs to change exactly? I'm reading conflicting opinions and want to get it right.

This comment has been minimized.

@sgrif

sgrif Nov 30, 2016

Member

Either https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 needs to be changed to apply to everything except SQLite, or the default implementation of default_primary_key? needs to be reverted to the original implementation on the abstract adapter, and overridden on PostgreSQL and MySQL2. As this is written, any out of tree adapters are going to get incorrect behavior.

@sgrif

sgrif Nov 30, 2016

Member

Either https://github.com/rails/rails/pull/26266/files#diff-2a8be25f82da6b3935cc6a41300a1b01R112 needs to be changed to apply to everything except SQLite, or the default implementation of default_primary_key? needs to be reverted to the original implementation on the abstract adapter, and overridden on PostgreSQL and MySQL2. As this is written, any out of tree adapters are going to get incorrect behavior.

This comment has been minimized.

@jmccartie

jmccartie Nov 30, 2016

Contributor

Thanks @sgrif -- will do.

@jmccartie

jmccartie Nov 30, 2016

Contributor

Thanks @sgrif -- will do.

end
end
end
end
@@ -4,6 +4,7 @@
require "active_record/connection_adapters/sqlite3/quoting"
require "active_record/connection_adapters/sqlite3/schema_creation"
require "active_record/connection_adapters/sqlite3/schema_definitions"
require "active_record/connection_adapters/sqlite3/schema_dumper"
gem "sqlite3", "~> 1.3.6"
require "sqlite3"
@@ -53,6 +54,7 @@ class SQLite3Adapter < AbstractAdapter
ADAPTER_NAME = "SQLite".freeze
include SQLite3::Quoting
include SQLite3::ColumnDumper
NATIVE_DATABASE_TYPES = {
primary_key: "INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL",
@@ -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
@@ -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.

This comment has been minimized.

@matthewd

matthewd Dec 5, 2016

Member

My worry here is just that enumerating the adapter types implies not-so-great things about the compatibility story for out-of-tree adapters.

cc @yahonda @metaskills

@matthewd

matthewd Dec 5, 2016

Member

My worry here is just that enumerating the adapter types implies not-so-great things about the compatibility story for out-of-tree adapters.

cc @yahonda @metaskills

This comment has been minimized.

@jmccartie

jmccartie Dec 5, 2016

Contributor

@matthewd @yahonda @metaskills Is there a decent set of defaults that will work well?

In your previous comment, you suggested what I have in the else block here. Rather than moving PG and Sqlite logic to the adapters, they're here. While I'm not a massive fan of the case statement, I feel like moving it out is premature. Either way -- here or in the adapters -- we need a set of sane defaults, I think.

@jmccartie

jmccartie Dec 5, 2016

Contributor

@matthewd @yahonda @metaskills Is there a decent set of defaults that will work well?

In your previous comment, you suggested what I have in the else block here. Rather than moving PG and Sqlite logic to the adapters, they're here. While I'm not a massive fan of the case statement, I feel like moving it out is premature. Either way -- here or in the adapters -- we need a set of sane defaults, I think.

This comment has been minimized.

@matthewd

matthewd Dec 5, 2016

Member

That's where I was thinking we could define defaults that must work well, and teach our own adapters to support them.

I'm fine with forcing changes onto adapters* -- I just don't want them to have to reach outside their classes to patch this V5_0 class, for example.


* Ultimately we'd like to get to a point where we don't do that so much, but right now, it's par for the course. So while big-picture unfortunate, it's not a negative against a particular approach here & now.

@matthewd

matthewd Dec 5, 2016

Member

That's where I was thinking we could define defaults that must work well, and teach our own adapters to support them.

I'm fine with forcing changes onto adapters* -- I just don't want them to have to reach outside their classes to patch this V5_0 class, for example.


* Ultimately we'd like to get to a point where we don't do that so much, but right now, it's par for the course. So while big-picture unfortunate, it's not a negative against a particular approach here & now.

This comment has been minimized.

@jmccartie

jmccartie Dec 5, 2016

Contributor

I understand. If you think that's the best way to go, I'll make the change. Can you help me by pointing me to a good spot to place this logic? I want to get it right the first time...

@jmccartie

jmccartie Dec 5, 2016

Contributor

I understand. If you think that's the best way to go, I'll make the change. Can you help me by pointing me to a good spot to place this logic? I want to get it right the first time...

This comment has been minimized.

@sgrif

sgrif Dec 5, 2016

Member

I really do think that this is fine. I'm in favor of opting other adapters into the new behavior by default. I'm not aware of any adapter that won't behave the same as the MySQL case.

@sgrif

sgrif Dec 5, 2016

Member

I really do think that this is fine. I'm in favor of opting other adapters into the new behavior by default. I'm not aware of any adapter that won't behave the same as the MySQL case.

This comment has been minimized.

@jmccartie

jmccartie Dec 5, 2016

Contributor

@sgrif Thanks, Sean. Do you mean you're ok with the current case statement? Or want to move this into the adapters? If the latter, can you suggest a good place to put this logic?

Something like this?

self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS
{:id=>:integer}
@jmccartie

jmccartie Dec 5, 2016

Contributor

@sgrif Thanks, Sean. Do you mean you're ok with the current case statement? Or want to move this into the adapters? If the latter, can you suggest a good place to put this logic?

Something like this?

self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS
{:id=>:integer}

This comment has been minimized.

@sgrif

sgrif Dec 5, 2016

Member

The fact that this required code changes in every adapter makes me hesitate though. Ideally it would either "just work" for third party adapters or require opting in. We should avoid having the default behavior require code changes for them.

@sgrif

sgrif Dec 5, 2016

Member

The fact that this required code changes in every adapter makes me hesitate though. Ideally it would either "just work" for third party adapters or require opting in. We should avoid having the default behavior require code changes for them.

This comment has been minimized.

@jmccartie

jmccartie Dec 5, 2016

Contributor

@sgrif Agreed -- that's why I was hoping there was a good default. it sounds like the else block works here. So I'm suggesting something like this:

if options[:id].blank?
  if self.connection.class.const_defined?("Compatibility")
    options.merge(self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS)
  else
    options[:id] = :integer
    options[:auto_increment] = true
  end
end
@jmccartie

jmccartie Dec 5, 2016

Contributor

@sgrif Agreed -- that's why I was hoping there was a good default. it sounds like the else block works here. So I'm suggesting something like this:

if options[:id].blank?
  if self.connection.class.const_defined?("Compatibility")
    options.merge(self.connection.class::Compatibility::INTEGER_PRIMARY_KEY_OPTIONS)
  else
    options[:id] = :integer
    options[:auto_increment] = true
  end
end

This comment has been minimized.

@jmccartie

jmccartie Dec 5, 2016

Contributor

note: #27272 made it possible to set the single set of defaults. Change pushed.

@jmccartie

jmccartie Dec 5, 2016

Contributor

note: #27272 made it possible to set the single set of defaults. Change pushed.

This comment has been minimized.

@yahonda

yahonda Dec 6, 2016

Contributor

Falling back to options[:id] = :integer looks ok for Oracle enhanced adapter.

@yahonda

yahonda Dec 6, 2016

Contributor

Falling back to options[:id] = :integer looks ok for Oracle enhanced adapter.

This comment has been minimized.

@jmccartie

jmccartie Dec 6, 2016

Contributor

Thanks @yahonda

@jmccartie

jmccartie Dec 6, 2016

Contributor

Thanks @yahonda

if options[:id].blank?
options[:id] = :integer
options[:auto_increment] = true
end
super
end
end
@@ -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
@@ -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)
@@ -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
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.