Idempotency bugs in add_reference_concurrently

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)

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?

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 }

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|

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 }