From e26c156e3c9883f40269faeb0c59e3b6a4500c2c Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Mon, 24 Jun 2024 19:26:58 +1000 Subject: [PATCH] Fix `add_reference_concurrently` to be idempotent when adding a foreign key (#56) Co-authored-by: fatkodima --- CHANGELOG.md | 1 + lib/online_migrations/schema_statements.rb | 3 +- test/command_checker/add_reference_test.rb | 12 ++++++++ test/command_checker/foreign_keys_test.rb | 9 ++++++ .../add_reference_concurrently_test.rb | 28 +++++++++++++++++++ 5 files changed, 52 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0338953..b013087 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## master (unreleased) +- Fix `add_reference_concurrently` to be idempotent when adding a foreign key - Fix `enqueue_background_data_migration` to be idempotent ## 0.19.1 (2024-05-24) diff --git a/lib/online_migrations/schema_statements.rb b/lib/online_migrations/schema_statements.rb index dc618ad..afddf9b 100644 --- a/lib/online_migrations/schema_statements.rb +++ b/lib/online_migrations/schema_statements.rb @@ -773,7 +773,8 @@ def index_name(table_name, options) # @see https://edgeapi.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-add_foreign_key # def add_foreign_key(from_table, to_table, **options) - if foreign_key_exists?(from_table, to_table, **options) + # Do not consider validation for idempotency. + if foreign_key_exists?(from_table, to_table, **options.except(:validate)) message = +"Foreign key was not created because it already exists " \ "(this can be due to an aborted migration or similar): from_table: #{from_table}, to_table: #{to_table}" message << ", #{options.inspect}" if options.any? diff --git a/test/command_checker/add_reference_test.rb b/test/command_checker/add_reference_test.rb index 2db919f..461257f 100644 --- a/test/command_checker/add_reference_test.rb +++ b/test/command_checker/add_reference_test.rb @@ -138,6 +138,18 @@ def test_add_reference_foreign_key_no_validate assert_safe AddReferenceForeignKeyNoValidate end + class AddReferenceForeignKeyNoValidatePluralizeName < TestMigration + disable_ddl_transaction! + + def change + add_reference :projects, :users, index: false, foreign_key: { validate: false } + end + end + + def test_add_reference_foreign_key_no_validate_with_pluralized_table_name + assert_safe AddReferenceForeignKeyNoValidatePluralizeName + end + class AddReferenceForeignKeyValidate < TestMigration def change add_reference :projects, :user, index: false, foreign_key: { validate: true } diff --git a/test/command_checker/foreign_keys_test.rb b/test/command_checker/foreign_keys_test.rb index 4b7a560..105f17f 100644 --- a/test/command_checker/foreign_keys_test.rb +++ b/test/command_checker/foreign_keys_test.rb @@ -69,6 +69,15 @@ def test_add_foreign_key_no_validate assert_safe AddForeignKeyNoValidate end + def test_add_foreign_key_no_validate_is_idempotent + assert_empty @connection.foreign_keys(:projects) + + migrate AddForeignKeyNoValidate + migrate AddForeignKeyNoValidate + + assert_equal 1, @connection.foreign_keys(:projects).size + end + class AddForeignKeyFromNewTable < TestMigration def change create_table :posts_new do |t| diff --git a/test/schema_statements/add_reference_concurrently_test.rb b/test/schema_statements/add_reference_concurrently_test.rb index 7f4cf8d..0cababf 100644 --- a/test/schema_statements/add_reference_concurrently_test.rb +++ b/test/schema_statements/add_reference_concurrently_test.rb @@ -71,6 +71,34 @@ def test_add_reference_concurrently_with_foreign_key end end + def test_add_reference_concurrently_with_foreign_key_is_idempotent + assert_empty connection.foreign_keys(:milestones) + + connection.add_reference_concurrently :milestones, :project, foreign_key: true + connection.add_reference_concurrently :milestones, :project, foreign_key: true + + assert_equal 1, connection.foreign_keys(:milestones).size + end + + def test_add_reference_concurrently_with_foreign_key_with_pluralized_table_name + assert_sql( + 'REFERENCES "projects" ("id") NOT VALID', + 'ALTER TABLE "milestones" VALIDATE CONSTRAINT' + ) do + connection.add_reference_concurrently :milestones, :projects, foreign_key: true + end + end + + def test_add_reference_concurrently_with_foreign_key_is_idempotent_with_pluralized_table_name + assert_empty connection.foreign_keys(:milestones) + + connection.add_reference_concurrently :milestones, :projects, foreign_key: true + connection.add_reference_concurrently :milestones, :projects, foreign_key: true + connection.add_reference_concurrently :milestones, :projects, foreign_key: true + + assert_equal 1, connection.foreign_keys(:milestones).size + end + def test_add_reference_concurrently_with_unvalidated_foreign_key refute_sql("VALIDATE CONSTRAINT") do connection.add_reference_concurrently :milestones, :project, foreign_key: { validate: false }