From 3e147acebd2851404b8513e6833e91a026aa5715 Mon Sep 17 00:00:00 2001 From: Yasuo Honda Date: Tue, 24 Dec 2024 20:19:26 +0900 Subject: [PATCH] Deprecate the `validate: false` option for `check_constraint` `check_constraint` with `validate: false` option actually creates a valid check constraint because PostgreSQL ignores `NOT VALID` option used with `CREATE TABLE` statement. To create a check constraint that is `NOT VALID`, `add_check_constraint` is preferred that generates `ALTER TABLE` statement with `NOT VALID` option. - `check_constraint` with `validate: false` option creates a valid check constraint ```ruby create_table :posts, force: true do |t| t.check_constraint "id > 0", name: "foo", validate: false end ``` ```sql CREATE TABLE "posts" ("id" bigserial primary key, CONSTRAINT foo CHECK (id > 0) NOT VALID); SELECT conname, convalidated FROM pg_constraint WHERE contype = 'c' AND conname = 'foo'; /* conname | convalidated ---------+-------------- foo | t (1 row) */ ``` - `add_check_constraint` with `validate: false` option creates a invalid check constraint ```ruby create_table :posts, force: true do |t| end add_check_constraint :posts, "id > 0", name: "foo", validate: false ``` ```sql CREATE TABLE "posts" ("id" bigserial primary key); ALTER TABLE "posts" ADD CONSTRAINT foo CHECK (id > 0) NOT VALID; SELECT conname, convalidated FROM pg_constraint WHERE contype = 'c' AND conname = 'foo'; /* conname | convalidated ---------+-------------- foo | f (1 row) */ ``` Related to #53732 Refer to the following discussion at pgsql-hackers mailing list. https://www.postgresql.org/message-id/202412050936.bse4z5tbmze6%40alvherre.pgsql > Maybe it would have been wise to forbid NOT VALID when used with CREATE > TABLE. But we didn't. Should we do that now? Maybe we can just > document that you can specify it but it doesn't do anything. --- activerecord/CHANGELOG.md | 7 +++++++ .../postgresql/schema_definitions.rb | 11 ++++++++++ .../active_record/migration/compatibility.rb | 16 +++++++++++++++ .../cases/migration/check_constraint_test.rb | 12 +++++++++++ .../cases/migration/compatibility_test.rb | 20 +++++++++++++++++++ 5 files changed, 66 insertions(+) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3be4abdfedd15..141e176af558a 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,10 @@ +* Deprecate the `validate: false` option for `check_constraint`. + + PostgreSQL ignores the `NOT VALID` option for check constraints within a `CREATE TABLE` statement. + To enable the `NOT VALID` option, use `add_check_constraint` which generates an `ALTER TABLE` statement. + + *Yasuo Honda* + * Change the payload name of `sql.active_record` notification for eager loading from "SQL" to "#{model.name} Eager Load". diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb index ce7750fccdc25..265ecbac08fc7 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_definitions.rb @@ -261,6 +261,17 @@ def unique_constraint(column_name, **options) unique_constraints << new_unique_constraint_definition(column_name, options) end + def check_constraint(expression, **options) + if options[:validate] == false + ActiveRecord.deprecator.warn(<<~MSG) + Providing `validate: false` for `check_constraint` is deprecated because it is ignored by the PostgreSQL. + Use the `add_check_constraint` with `validate: false` to create a check constraint that is `NOT VALID`. + MSG + end + + super + end + def new_exclusion_constraint_definition(expression, options) # :nodoc: options = @conn.exclusion_constraint_options(name, expression, options) ExclusionConstraintDefinition.new(name, expression, options) diff --git a/activerecord/lib/active_record/migration/compatibility.rb b/activerecord/lib/active_record/migration/compatibility.rb index 664bfaeea081b..7f0639e545a09 100644 --- a/activerecord/lib/active_record/migration/compatibility.rb +++ b/activerecord/lib/active_record/migration/compatibility.rb @@ -32,6 +32,22 @@ def self.find(version) V8_1 = Current class V8_0 < V8_1 + if connection.adapter_name == "PostgreSQL" + module TableDefinition + def check_constraint(expression, **options) + options.delete(:validate) + super + end + end + end + + private + def compatible_table_definition(t) + class << t + prepend TableDefinition + end + super + end end class V7_2 < V8_0 diff --git a/activerecord/test/cases/migration/check_constraint_test.rb b/activerecord/test/cases/migration/check_constraint_test.rb index eb79644315404..11ea7ecdfcc7a 100644 --- a/activerecord/test/cases/migration/check_constraint_test.rb +++ b/activerecord/test/cases/migration/check_constraint_test.rb @@ -206,6 +206,18 @@ def test_schema_dumping_with_validate_true assert_match %r{\s+t.check_constraint "quantity > 0", name: "quantity_check"$}, output end + + def test_not_valid_check_constraint_in_create_table + assert_deprecated(ActiveRecord.deprecator) do + @connection.create_table :nonvalid_constraints, id: false, force: true do |t| + t.string :name + t.check_constraint "char_length(name) >= 5", name: "name_const", validate: false + end + ensure + @connection.drop_table(:nonvalid_constraints, if_exists: true) + ActiveRecord::Base.clear_cache! + end + end else # Check constraint should still be created, but should not be invalid def test_add_invalid_check_constraint diff --git a/activerecord/test/cases/migration/compatibility_test.rb b/activerecord/test/cases/migration/compatibility_test.rb index 835ea9400c710..a8aac43047d24 100644 --- a/activerecord/test/cases/migration/compatibility_test.rb +++ b/activerecord/test/cases/migration/compatibility_test.rb @@ -648,6 +648,26 @@ def migrate(x) connection.drop_table(:sub_testings, if_exists: true) ActiveRecord::Base.clear_cache! end + + def test_check_constraint_with_validate_false + migration = Class.new(ActiveRecord::Migration[8.0]) do + def migrate(x) + create_table :check_constraint_testings, force: true do |t| + t.string :name + + t.timestamps + t.check_constraint "char_length(name) >= 5", name: "name_const", validate: false + end + end + end.new + + assert_not_deprecated(ActiveRecord.deprecator) do + ActiveRecord::Migrator.new(:up, [migration], @schema_migration, @internal_metadata).migrate + end + ensure + connection.drop_table(:check_constraint_testings, if_exists: true) + ActiveRecord::Base.clear_cache! + end end if current_adapter?(:Mysql2Adapter, :TrilogyAdapter)