Merge pull request rails#47659 from alpaca-tc/add_foreign_key_deferrable_must_be_false_or_symbol

Deprecate `deferrable: true` option of `add_foreign_key`

Author: yahonda

activerecord/CHANGELOG.md

@@ -1,3 +1,14 @@
+*   Deprecate `deferrable: true` option of `add_foreign_key`.
+
+    `deferrable: true` is deprecated in favor of `deferrable: :immediate`, and
+    will be removed in Rails 7.2.
+
+    Because `deferrable: true` and `deferrable: :deferred` are hard to understand.
+    Both true and :deferred are truthy values.
+    This behavior is the same as the deferrable option of the add_unique_key method, added in #46192.
+
+    *Hiroyuki Ishii*
+
 *   `AbstractAdapter#execute` and `#exec_query` now clear the query cache
 
     If you need to perform a read only SQL query without clearing the query

activerecord/lib/active_record/connection_adapters/postgresql/schema_creation.rb

@@ -18,11 +18,7 @@ def visit_AlterTable(o)
 
           def visit_AddForeignKey(o)
             super.dup.tap do |sql|
-              if o.deferrable
-                sql << " DEFERRABLE"
-                sql << " INITIALLY #{o.deferrable.to_s.upcase}" unless o.deferrable == true
-              end
-
+              sql << " DEFERRABLE INITIALLY #{o.options[:deferrable].to_s.upcase}" if o.deferrable
               sql << " NOT VALID" unless o.validate?
             end
           end

activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

@@ -518,6 +518,20 @@ def index_name(table_name, options) # :nodoc:
           super
         end
 
+        def add_foreign_key(from_table, to_table, **options)
+          if options[:deferrable] == true
+            ActiveRecord.deprecator.warn(<<~MSG)
+              `deferrable: true` is deprecated in favor of `deferrable: :immediate`, and will be removed in Rails 7.2.
+            MSG
+
+            options[:deferrable] = :immediate
+          end
+
+          assert_valid_deferrable(options[:deferrable])
+
+          super
+        end
+
         def foreign_keys(table_name)
           scope = quoted_scope(table_name)
           fk_info = internal_exec_query(<<~SQL, "SCHEMA", allow_retry: true, materialize_transactions: false)
@@ -543,7 +557,7 @@ def foreign_keys(table_name)
 
             options[:on_delete] = extract_foreign_key_action(row["on_delete"])
             options[:on_update] = extract_foreign_key_action(row["on_update"])
-            options[:deferrable] = extract_foreign_key_deferrable(row["deferrable"], row["deferred"])
+            options[:deferrable] = extract_constraint_deferrable(row["deferrable"], row["deferred"])
 
             options[:validate] = row["valid"]
             to_table = Utils.unquote_identifier(row["to_table"])
@@ -947,20 +961,16 @@ def extract_foreign_key_action(specifier)
             end
           end
 
-          def extract_foreign_key_deferrable(deferrable, deferred)
-            deferrable && (deferred ? :deferred : true)
+          def assert_valid_deferrable(deferrable)
+            return if !deferrable || %i(immediate deferred).include?(deferrable)
+
+            raise ArgumentError, "deferrable must be `:immediate` or `:deferred`, got: `#{deferrable.inspect}`"
           end
 
           def extract_constraint_deferrable(deferrable, deferred)
             deferrable && (deferred ? :deferred : :immediate)
           end
 
-          def assert_valid_deferrable(deferrable) # :nodoc:
-            return if !deferrable || %i(immediate deferred).include?(deferrable)
-
-            raise ArgumentError, "deferrable must be `:immediate` or `:deferred`, got: `#{deferrable.inspect}`"
-          end
-
           def reference_name_for_table(table_name)
             _schema, table_name = extract_schema_qualified_name(table_name.to_s)
             table_name.singularize

activerecord/lib/active_record/migration/compatibility.rb

@@ -138,6 +138,13 @@ def disable_extension(name, **options)
           super
         end
 
+        def add_foreign_key(from_table, to_table, **options)
+          if connection.adapter_name == "PostgreSQL" && options[:deferrable] == true
+            options[:deferrable] = :immediate
+          end
+          super
+        end
+
         private
           def compatible_table_definition(t)
             class << t

activerecord/test/cases/migration/compatibility_test.rb

@@ -756,6 +756,27 @@ def up
         ensure
           disable_extension!(:hstore, connection)
         end
+
+        def test_legacy_add_foreign_key_with_deferrable_true
+          migration = Class.new(ActiveRecord::Migration[7.0]) {
+            def migrate(x)
+              create_table :sub_testings do |t|
+                t.bigint :testing_id
+              end
+
+              add_foreign_key(:sub_testings, :testings, name: "deferrable_foreign_key", deferrable: true)
+            end
+          }.new
+
+          ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate
+
+          foreign_keys = Testing.connection.foreign_keys("sub_testings")
+          assert_equal 1, foreign_keys.size
+          assert_equal :immediate, foreign_keys.first.deferrable
+        ensure
+          connection.drop_table(:sub_testings, if_exists: true)
+          ActiveRecord::Base.clear_cache!
+        end
       end
 
       if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)

activerecord/test/cases/migration/foreign_key_test.rb

@@ -519,13 +519,13 @@ def test_add_invalid_foreign_key
 
         if ActiveRecord::Base.connection.supports_deferrable_constraints?
           def test_deferrable_foreign_key
-            @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", deferrable: true
+            @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", deferrable: :immediate
 
             foreign_keys = @connection.foreign_keys("astronauts")
             assert_equal 1, foreign_keys.size
 
             fk = foreign_keys.first
-            assert_equal true, fk.options[:deferrable]
+            assert_equal :immediate, fk.deferrable
           end
 
           def test_not_deferrable_foreign_key
@@ -535,7 +535,7 @@ def test_not_deferrable_foreign_key
             assert_equal 1, foreign_keys.size
 
             fk = foreign_keys.first
-            assert_equal false, fk.options[:deferrable]
+            assert_equal false, fk.deferrable
           end
 
           def test_deferrable_initially_deferred_foreign_key
@@ -545,7 +545,7 @@ def test_deferrable_initially_deferred_foreign_key
             assert_equal 1, foreign_keys.size
 
             fk = foreign_keys.first
-            assert_equal :deferred, fk.options[:deferrable]
+            assert_equal :deferred, fk.deferrable
           end
 
           def test_deferrable_initially_immediate_foreign_key
@@ -555,15 +555,15 @@ def test_deferrable_initially_immediate_foreign_key
             assert_equal 1, foreign_keys.size
 
             fk = foreign_keys.first
-            assert_equal true, fk.options[:deferrable]
+            assert_equal :immediate, fk.deferrable
           end
 
           def test_schema_dumping_with_defferable
-            @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", deferrable: true
+            @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", deferrable: :immediate
 
             output = dump_table_schema "astronauts"
 
-            assert_match %r{\s+add_foreign_key "astronauts", "rockets", deferrable: true$}, output
+            assert_match %r{\s+add_foreign_key "astronauts", "rockets", deferrable: :immediate$}, output
           end
 
           def test_schema_dumping_with_disabled_defferable
@@ -587,7 +587,19 @@ def test_schema_dumping_with_defferable_initially_immediate
 
             output = dump_table_schema "astronauts"
 
-            assert_match %r{\s+add_foreign_key "astronauts", "rockets", deferrable: true$}, output
+            assert_match %r{\s+add_foreign_key "astronauts", "rockets", deferrable: :immediate$}, output
+          end
+
+          def test_deferrable_true_foreign_key
+            assert_deprecated(ActiveRecord.deprecator) do
+              @connection.add_foreign_key :astronauts, :rockets, column: "rocket_id", deferrable: true
+            end
+
+            foreign_keys = @connection.foreign_keys("astronauts")
+            assert_equal 1, foreign_keys.size
+
+            fk = foreign_keys.first
+            assert_equal :immediate, fk.deferrable
           end
         end
 

guides/source/active_record_postgresql.md

@@ -618,7 +618,7 @@ ActiveRecord::Base.connection.transaction do
 end
 ```
 
-The `:deferrable` option can also be set to `true` or `:immediate`, which has the same effect. Both options let the foreign keys keep the default behavior of checking the constraint immediately, but allow manually deferring the checks using `SET CONSTRAINTS ALL DEFERRED` within a transaction. This will cause the foreign keys to be checked when the transaction is committed:
+When the `:deferrable` option is set to `:immediate`, let the foreign keys keep the default behavior of checking the constraint immediately, but allow manually deferring the checks using `SET CONSTRAINTS ALL DEFERRED` within a transaction. This will cause the foreign keys to be checked when the transaction is committed:
 
 ```ruby
 ActiveRecord::Base.transaction do