Deprecate `deferrable: true` option of `add_foreign_key` #47659

alpaca-tc · 2023-03-13T14:33:31Z

Motivation / Background

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.

Detail

As with add_unique_key method added in Add support for unique constraints (PostgreSQL-only). #46192, deferrable now should be accept only symbols.
- deferrable: true is deprecated now.
Extend ActiveRecord::Migration[7.0] so that deferrable: true is available without deprecation message in older migration files.

Additional information

The SQL generated will be slightly different, but unaffected.
#46192 confirms that DEFERRABLE and DEFERRABLE INITIALLY IMMEDIATE are equivalent.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

yahonda · 2023-03-14T14:33:47Z

I think it needs to deprecate deferrable: true first If the migration parent class is ActiveRecord::Migration[7.1]. The deprecation message should suggest like "deferrable: true is deprecated and will be removed in Rails 7.2. Use deferrable: immediate" (English grammar needs to be fixed).

ActiveRecord::Migration[7.0] deferrable: true should work as is without deprecation warnings. Changing the SQL statement is fine like deferrable: true works as deferrable: immediate because ADD CONSTRAINT ... DEFERRABLE is same as ADD CONSTRAINT ... INITIALLY IMMEDIATE

guides/source/active_record_postgresql.md

alpaca-tc · 2023-03-15T07:46:52Z

@yahonda Fixed 😄 🙏

activerecord/lib/active_record/migration/compatibility.rb

guides/source/active_record_postgresql.md

alpaca-tc · 2023-03-30T06:09:59Z

@yahonda Fixed 😄 👍

yahonda · 2023-04-18T11:00:04Z

Would you resolve the conflicts in the activerecord/CHANGELOG.md ?

alpaca-tc · 2023-04-18T11:18:45Z

@yahonda done 👍

guides/source/active_record_postgresql.md

yahonda

Agreed to deprecate :deferrable:true as it can also take :deferred and :immediate, both of which are truth values. I think it is clearer to just allow :deferred and :immediate.

I'd like hear from other committers/core members to get some more opinion/approvals.

ghiculescu · 2023-04-21T18:17:50Z

This PR looks great to me, could https://edgeguides.rubyonrails.org/active_record_postgresql.html#deferrable-foreign-keys get updated at the same time?

alpaca-tc · 2023-04-22T13:38:32Z

I've rebased and fixed conflict.

alpaca-tc · 2023-04-22T13:40:06Z

@ghiculescu The guide has also been modified in this PR 👍 Please check Files changed.

yahonda · 2023-04-24T11:40:22Z

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

@@ -1131,6 +1131,17 @@ def add_foreign_key(from_table, to_table, **options)
        return if options[:if_not_exists] == true && foreign_key_exists?(from_table, to_table, **options.slice(:column))

        options = foreign_key_options(from_table, to_table, options)
+
+        if options[:deferrable] == true


I should have commented this in advance, I think this requirement is PostgreSQL adapter specific.
So how about creating add_foreign_key(from_table, to_table, **options) method in activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb to run these warnings validations?

yahonda · 2023-04-24T11:40:43Z

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

@@ -1724,6 +1735,12 @@ def check_constraint_for!(table_name, expression: nil, **options)
            raise(ArgumentError, "Table '#{table_name}' has no check constraint for #{expression || options}")
        end

+        def assert_valid_deferrable(deferrable)


Let's move this method to activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb

activerecord/lib/active_record/migration/compatibility.rb

activerecord/test/cases/migration/compatibility_test.rb

alpaca-tc · 2023-04-25T11:08:30Z

@yahonda fixed 🤩

yahonda · 2023-04-25T23:46:23Z

Let's resolve the conflict at activerecord/CHANGELOG.md . other than it looks good.

`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 rails#46192. *Hiroyuki Ishii*

alpaca-tc · 2023-04-26T12:48:50Z

@yahonda rebased 👍

rails-bot bot added activerecord docs labels Mar 13, 2023

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from b0bc930 to 86a5756 Compare March 13, 2023 15:00

This was referenced Mar 14, 2023

deferrable foreign key can be passed to references #47671

Merged

Add support for unique constraints (PostgreSQL-only). #46192

Merged

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 86a5756 to 7b6ecbf Compare March 15, 2023 05:01

yahonda reviewed Mar 15, 2023

View reviewed changes

guides/source/active_record_postgresql.md Outdated Show resolved Hide resolved

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 7b6ecbf to ab07a79 Compare March 15, 2023 07:20

alpaca-tc changed the title ~~The deferrable option of add_foreign_key must now be false or symbols~~ Deprecate deferrable: true option of add_foreign_key Mar 15, 2023

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from b9be31a to 2bbcbca Compare March 15, 2023 07:32

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch 2 times, most recently from 4dae78e to 44ed638 Compare March 16, 2023 03:26

yahonda reviewed Mar 16, 2023

View reviewed changes

activerecord/lib/active_record/migration/compatibility.rb Outdated Show resolved Hide resolved

guides/source/active_record_postgresql.md Outdated Show resolved Hide resolved

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch 3 times, most recently from 4105ded to 1ce57d3 Compare March 30, 2023 06:09

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 1ce57d3 to 1bc8325 Compare April 17, 2023 14:01

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 1bc8325 to 7341df3 Compare April 18, 2023 11:18

yahonda reviewed Apr 18, 2023

View reviewed changes

guides/source/active_record_postgresql.md Outdated Show resolved Hide resolved

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 7341df3 to 48d255b Compare April 18, 2023 12:04

yahonda self-requested a review April 18, 2023 12:08

yahonda approved these changes Apr 18, 2023

View reviewed changes

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 48d255b to bf79e1b Compare April 22, 2023 13:37

alpaca-tc mentioned this pull request Apr 22, 2023

Add a section on exclusion constraints to the AR PostgreSQL guide #48019

Merged

yahonda reviewed Apr 24, 2023

View reviewed changes

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch 4 times, most recently from 1002dd4 to 62b260d Compare April 25, 2023 08:28

alpaca-tc force-pushed the add_foreign_key_deferrable_must_be_false_or_symbol branch from 62b260d to 896d359 Compare April 26, 2023 12:48

yahonda approved these changes Apr 26, 2023

View reviewed changes

yahonda merged commit 992d689 into rails:main Apr 26, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `deferrable: true` option of `add_foreign_key` #47659

Deprecate `deferrable: true` option of `add_foreign_key` #47659

alpaca-tc commented Mar 13, 2023 •

edited

yahonda commented Mar 14, 2023

alpaca-tc commented Mar 15, 2023

alpaca-tc commented Mar 30, 2023

yahonda commented Apr 18, 2023

alpaca-tc commented Apr 18, 2023

yahonda left a comment

ghiculescu commented Apr 21, 2023

alpaca-tc commented Apr 22, 2023

alpaca-tc commented Apr 22, 2023

yahonda Apr 24, 2023

yahonda Apr 24, 2023

alpaca-tc commented Apr 25, 2023

yahonda commented Apr 25, 2023

alpaca-tc commented Apr 26, 2023

Deprecate deferrable: true option of add_foreign_key #47659

Deprecate deferrable: true option of add_foreign_key #47659

Conversation

alpaca-tc commented Mar 13, 2023 • edited

Motivation / Background

Detail

Additional information

Checklist

yahonda commented Mar 14, 2023

alpaca-tc commented Mar 15, 2023

alpaca-tc commented Mar 30, 2023

yahonda commented Apr 18, 2023

alpaca-tc commented Apr 18, 2023

yahonda left a comment

Choose a reason for hiding this comment

ghiculescu commented Apr 21, 2023

alpaca-tc commented Apr 22, 2023

alpaca-tc commented Apr 22, 2023

yahonda Apr 24, 2023

Choose a reason for hiding this comment

yahonda Apr 24, 2023

Choose a reason for hiding this comment

alpaca-tc commented Apr 25, 2023

yahonda commented Apr 25, 2023

alpaca-tc commented Apr 26, 2023

Deprecate `deferrable: true` option of `add_foreign_key` #47659

Deprecate `deferrable: true` option of `add_foreign_key` #47659

alpaca-tc commented Mar 13, 2023 •

edited