From 8768305f20d12c40241396092a63e0d56269fefe Mon Sep 17 00:00:00 2001 From: Yves Senn Date: Fri, 20 Jun 2014 11:54:17 +0200 Subject: [PATCH] fk: use random digest names The name of the foreign key is not relevant from a users perspective. Using random names resolves the urge to rename the foreign key when the respective table or column is renamed. --- .../abstract/schema_creation.rb | 2 +- .../abstract/schema_definitions.rb | 11 ++++++- .../abstract/schema_statements.rb | 20 +++++++----- .../lib/active_record/schema_dumper.rb | 16 ++++++++-- .../test/cases/migration/foreign_key_test.rb | 31 ++++++++++--------- activerecord/test/fixtures/fk_test_has_pk.yml | 2 +- activerecord/test/schema/schema.rb | 4 +-- .../test/schema/sqlite_specific_schema.rb | 6 ++-- 8 files changed, 59 insertions(+), 33 deletions(-) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb index e495304376..5d13ee3633 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_creation.rb @@ -30,7 +30,7 @@ FOREIGN KEY (#{quote_column_name(o.column)}) end def visit_DropForeignKey(name) - "DROP CONSTRAINT #{name}" + "DROP CONSTRAINT #{quote_column_name(name)}" end private diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 2e47e68754..785cfd9dbc 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -35,7 +35,7 @@ module ActiveRecord end def primary_key - options[:primary_key] + options[:primary_key] || default_primary_key end def on_delete @@ -45,6 +45,15 @@ module ActiveRecord def on_update options[:on_update] end + + def custom_primary_key? + options[:primary_key] != default_primary_key + end + + private + def default_primary_key + "id" + end end # Represents the schema of an SQL table in an abstract way. This class diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 4da42717c1..e203767992 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -650,11 +650,10 @@ module ActiveRecord return unless supports_foreign_keys? options[:column] ||= foreign_key_column_for(to_table) - primary_key = options.fetch(:primary_key, "id") options = { column: options[:column], - primary_key: primary_key, + primary_key: options[:primary_key], name: foreign_key_name(from_table, options), on_delete: options[:on_delete], on_update: options[:on_update] @@ -674,8 +673,17 @@ module ActiveRecord options = { column: foreign_key_column_for(options_or_to_table) } end + fk_name_to_delete = options.fetch(:name) do + fk_to_delete = foreign_keys(from_table).detect {|fk| fk.column == options[:column] } + if fk_to_delete + fk_to_delete.name + else + raise ArgumentError, "Table '#{from_table}' has no foreign key on column '#{options[:column]}'" + end + end + at = create_alter_table from_table - at.drop_foreign_key foreign_key_name(from_table, options) + at.drop_foreign_key fk_name_to_delete execute schema_creation.accept at end @@ -686,11 +694,7 @@ module ActiveRecord def foreign_key_name(table_name, options) # :nodoc: options.fetch(:name) do - identifier = "#{table_name}_#{options.fetch(:column)}_fk" - if identifier.length > allowed_index_name_length - raise ArgumentError, "Foreign key name '#{identifier}' is too long; the limit is #{allowed_index_name_length} characters" - end - identifier + "fk_rails_#{SecureRandom.hex(5)}" end end diff --git a/activerecord/lib/active_record/schema_dumper.rb b/activerecord/lib/active_record/schema_dumper.rb index 84ce725409..64bc68eefd 100644 --- a/activerecord/lib/active_record/schema_dumper.rb +++ b/activerecord/lib/active_record/schema_dumper.rb @@ -226,10 +226,20 @@ HEADER parts = [ 'add_foreign_key ' + remove_prefix_and_suffix(foreign_key.from_table).inspect, remove_prefix_and_suffix(foreign_key.to_table).inspect, - 'column: ' + foreign_key.column.inspect, - 'primary_key: ' + foreign_key.primary_key.inspect, - 'name: ' + foreign_key.name.inspect ] + + if foreign_key.column != @connection.foreign_key_column_for(foreign_key.to_table) + parts << ('column: ' + foreign_key.column.inspect) + end + + if foreign_key.custom_primary_key? + parts << ('primary_key: ' + foreign_key.primary_key.inspect) + end + + if foreign_key.name !~ /^fk_rails_[0-9a-f]{10}$/ + parts << ('name: ' + foreign_key.name.inspect) + end + parts << ('on_update: ' + foreign_key.on_update.inspect) if foreign_key.on_update parts << ('on_delete: ' + foreign_key.on_delete.inspect) if foreign_key.on_delete diff --git a/activerecord/test/cases/migration/foreign_key_test.rb b/activerecord/test/cases/migration/foreign_key_test.rb index 6a24df076d..c985092b4c 100644 --- a/activerecord/test/cases/migration/foreign_key_test.rb +++ b/activerecord/test/cases/migration/foreign_key_test.rb @@ -42,7 +42,7 @@ module ActiveRecord assert_equal "fk_test_has_fk", fk.from_table assert_equal "fk_test_has_pk", fk.to_table assert_equal "fk_id", fk.column - assert_equal "id", fk.primary_key + assert_equal "pk_id", fk.primary_key assert_equal "fk_name", fk.name end @@ -57,7 +57,7 @@ module ActiveRecord assert_equal "rockets", fk.to_table assert_equal "rocket_id", fk.column assert_equal "id", fk.primary_key - assert_equal "astronauts_rocket_id_fk", fk.name + assert_match(/^fk_rails_.{10}$/, fk.name) end def test_add_foreign_key_with_column @@ -71,7 +71,7 @@ module ActiveRecord assert_equal "rockets", fk.to_table assert_equal "rocket_id", fk.column assert_equal "id", fk.primary_key - assert_equal "astronauts_rocket_id_fk", fk.name + assert_match(/^fk_rails_.{10}$/, fk.name) end def test_add_foreign_key_with_non_standard_primary_key @@ -146,15 +146,6 @@ module ActiveRecord assert_equal :nullify, fk.on_update end - def test_add_foreign_key_with_too_long_identifier - with_example_table @connection, "long_table_name_will_result_in_a_long_foreign_key_name", "rocket_id integer" do - e = assert_raises(ArgumentError) do - @connection.add_foreign_key "long_table_name_will_result_in_a_long_foreign_key_name", "rockets" - end - assert_match(/^Foreign key name 'long_table_name_will_result_in_a_long_foreign_key_name_rocket_id_fk' is too long;/, e.message) - end - end - def test_remove_foreign_key_inferes_column @connection.add_foreign_key :astronauts, :rockets @@ -179,9 +170,21 @@ module ActiveRecord assert_equal [], @connection.foreign_keys("astronauts") end + def test_remove_foreign_non_existing_foreign_key_raises + assert_raises ArgumentError do + @connection.remove_foreign_key :astronauts, :rockets + end + end + def test_schema_dumping + @connection.add_foreign_key :astronauts, :rockets + output = dump_table_schema "astronauts" + assert_match %r{\s+add_foreign_key "astronauts", "rockets"$}, output + end + + def test_schema_dumping_with_options output = dump_table_schema "fk_test_has_fk" - assert_match %r{\s+add_foreign_key "fk_test_has_fk", "fk_test_has_pk", column: "fk_id", primary_key: "id", name: "fk_name"$}, output + assert_match %r{\s+add_foreign_key "fk_test_has_fk", "fk_test_has_pk", column: "fk_id", primary_key: "pk_id", name: "fk_name"$}, output end def test_schema_dumping_on_delete_and_on_update_options @@ -205,7 +208,7 @@ module ActiveRecord def test_add_foreign_key_is_reversible migration = CreateCitiesAndHousesMigration.new silence_stream($stdout) { migration.migrate(:up) } - assert_equal ["houses_city_id_fk"], @connection.foreign_keys("houses").map(&:name) + assert_equal 1, @connection.foreign_keys("houses").size ensure silence_stream($stdout) { migration.migrate(:down) } end diff --git a/activerecord/test/fixtures/fk_test_has_pk.yml b/activerecord/test/fixtures/fk_test_has_pk.yml index c93952180b..73882bac41 100644 --- a/activerecord/test/fixtures/fk_test_has_pk.yml +++ b/activerecord/test/fixtures/fk_test_has_pk.yml @@ -1,2 +1,2 @@ first: - id: 1 \ No newline at end of file + pk_id: 1 \ No newline at end of file diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 6ce82c71a8..03d33c151a 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -855,10 +855,10 @@ ActiveRecord::Schema.define do t.integer :fk_id, null: false end - create_table :fk_test_has_pk, force: true do |t| + create_table :fk_test_has_pk, force: true, primary_key: "pk_id" do |t| end - execute "ALTER TABLE fk_test_has_fk ADD CONSTRAINT fk_name FOREIGN KEY (#{quote_column_name 'fk_id'}) REFERENCES #{quote_table_name 'fk_test_has_pk'} (#{quote_column_name 'id'})" + execute "ALTER TABLE fk_test_has_fk ADD CONSTRAINT fk_name FOREIGN KEY (#{quote_column_name 'fk_id'}) REFERENCES #{quote_table_name 'fk_test_has_pk'} (#{quote_column_name 'pk_id'})" execute "ALTER TABLE lessons_students ADD CONSTRAINT student_id_fk FOREIGN KEY (#{quote_column_name 'student_id'}) REFERENCES #{quote_table_name 'students'} (#{quote_column_name 'id'})" end diff --git a/activerecord/test/schema/sqlite_specific_schema.rb b/activerecord/test/schema/sqlite_specific_schema.rb index b7aff4f47d..b5552c2755 100644 --- a/activerecord/test/schema/sqlite_specific_schema.rb +++ b/activerecord/test/schema/sqlite_specific_schema.rb @@ -7,7 +7,7 @@ ActiveRecord::Schema.define do execute "DROP TABLE fk_test_has_pk" rescue nil execute <<_SQL CREATE TABLE 'fk_test_has_pk' ( - 'id' INTEGER NOT NULL PRIMARY KEY + 'pk_id' INTEGER NOT NULL PRIMARY KEY ); _SQL @@ -16,7 +16,7 @@ _SQL 'id' INTEGER NOT NULL PRIMARY KEY, 'fk_id' INTEGER NOT NULL, - FOREIGN KEY ('fk_id') REFERENCES 'fk_test_has_pk'('id') + FOREIGN KEY ('fk_id') REFERENCES 'fk_test_has_pk'('pk_id') ); _SQL -end \ No newline at end of file +end