Add basic support for check constraints to database migrations by fatkodima · Pull Request #31323 · rails/rails

fatkodima · 2017-12-03T23:34:17Z

This patch adds basic support for adding and removing check constraints using the migration DSL:

create_table :distributors do |t|
  t.string :zipcode
  t.check_constraint "zipchk", "char_length(zipcode) = 5"   
end

remove_check_constraint :distributors, "zipchk"

Check constraints are dumped inside create_table sections to schema.rb.

add_check_constraint and remove_check_constraint are both reversible.

Considerations

Maybe remove the _constraint part from new methods names? Seems a little redundant
Add additional options, such as NO INHERIT(PostgreSQL)?

Related #23083.

rails-bot · 2017-12-03T23:34:20Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

sgrif · 2017-12-03T23:46:32Z

activerecord/lib/active_record/migration/command_recorder.rb

Why was every line in this hash changed when you only added a single line?

@sgrif because it's very clearly [already] aligned to the longest key?

(though it does seem to have ended up one space further over than it should be.)

Sorry, I should have been more clear on my intent:

Do you think we could drop the visual alignment change here so we aren't changing every line in this hash? I think having the one line that is not aligned the same is fine.

sgrif

I don't think the tests here are sufficient. There are no tests which check that the constraint has actually been added to the table (which can only really be tested by attempting an insertion that should fail).

In particular, I don't think you've handled SQLite correctly, where check constraints are supported, but can only be added inline with the column definition. Unless I'm missing something, there's no way to add a check constraint as part of a column definition in this PR at all.

lostapathy · 2017-12-04T14:39:59Z

+1 to @sgrif's thoughts on sqlite. I also recently discovered that add_reference doesn't create foreign keys on sqlite, even though sqlite could support it. I'm working on a PR for that, and it'd great to not see sqlite fall behind farther.

matthewd · 2017-12-04T15:01:53Z

Unless I'm missing something, there's no way to add a check constraint as part of a column definition in this PR at all.

That sounds more like an additional feature than a flaw in this PR, per se.

(i.e., if the PR were implementing column-level constraints and had omitted SQLite, that would be more of a problem; whereas I think only implementing table-level constraints is a perfectly reasonable self-contained feature, particularly as someone's first substantial contribution.)

sgrif · 2017-12-04T17:22:19Z

I'm fine with omitting SQLite support entirely for this feature to start, but I still think we need tests which actually exercise the added constraint.

lostapathy · 2017-12-04T17:39:18Z

@matthewd you're right, that makes sense. This API makes sense in general, just not for sqlite. Totally fair to skip it. Much like how add_foreign_key can't work on sqlite, but add_reference should be doable.

Would it be possible/tolerable to emit some kind of warning if a constraint (or foreign key) migration is treated as a no-op sqlite3? I spent a ton of time tracing that behavior out and I'm not sure what we do now is intuitive. The sqlite docs say they support foreign keys and foreign key migrations are "successful", so it one does not expect to end up without a foreign key in their sqlite db.

fatkodima · 2017-12-05T21:09:44Z

Updated PR. Added missing test (suggested by @sgrif ) and support for sqlite.

pkoch · 2018-09-05T17:52:13Z

What's missing to move forward on this branch atm?

NikolayS · 2019-04-23T15:00:36Z

This is a good feature. It's 2019 already, and SQL-92 standard had CHECK. Good time to start supporting it, both for MySQL and Rails.

fatkodima · 2019-04-23T15:34:46Z

If core team will accept this PR, I will update it as needed.
Btw, right now I think it is a bad idea to support constraints, because currently migrations are not far from being bloated and there will be people who wants to support other not so popular features like triggers and other db stuff.

NikolayS · 2019-04-23T18:09:09Z

Constraints enforcement is a database system's job.

In general, only DBMS is able to reliably enforce constraints in 100% of cases. If you implement checks in your app logic, there are significant chances that after one year or so they will be silently broken, because some human accessed DB directly, changing data, or because you've launched, say, API using a different language/framework, or because you've used some DB migrations bypassing your abstraction layer which tried to enforce constraints.

CHECK is a very good thing, underestimated due to lack of its support in MySQL (until 8.0.16, not yet released) and lack of its support in frameworks. This is a classic "chicken vs egg" problem: without support in frameworks, people won't start using it, saying that it's not so popular (but it is a very popular for decades already; not among Ruby developers).

fatkodima · 2019-12-04T20:48:07Z

@matthewd Wdyt about moving this forward?

rails-bot · 2020-03-03T21:37:56Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

pkoch · 2020-03-03T21:46:20Z

@matthewd @sgrif I'd love to not let this die. What would it take to get a ship it?

jeremy

🙌🏼

Method naming

Maybe remove the _constraint part from new methods names? Seems a little redundant

Comparing,

create_table :distributors do |t|
  t.string :zipcode
  t.check_constraint "zipchk", "char_length(zipcode) = 5"   
end

remove_check_constraint :distributors, "zipchk"

create_table :distributors do |t|
  t.string :zipcode
  t.check "zipchk", "char_length(zipcode) = 5"   
end

remove_check :distributors, "zipchk"

I prefer the clarity of seeing constraint mentioned. If it were a constraint on a column definition, check: "expression" would be nicer.

Naming constraints
Having to name constraints is a hassle, too. Can we name them automatically, like we do with foreign keys and indexes? For example, we could transform the check expression into name like chk_rails_#{hashed_identifier}.

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

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

activerecord/test/cases/migration/check_constraint_test.rb

fatkodima · 2020-03-04T19:12:26Z

Updated this with current master branch.
fixed @jeremy suggestions (including optional constraint name feature)
added missing implementation for mysql

rails-bot · 2020-06-02T20:03:02Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

jeremy

Ready to merge pending rebase. Thanks for your work on this @fatkodima.

fatkodima · 2020-06-03T01:29:30Z

@jeremy Rebased.

NikolayS · 2020-06-03T08:28:18Z

Wow. Great news 👍

Active Record already supports adding `NOT VALID` foreign key constraints in PostgreSQL, but back in rails#31323 when check constraint support was initially added, the ability to add a check constraint and validate it separately was not included. This adds that functionality!

Supports annotating model check constraints, credit goes to folks who worked on ctran/annotate_models#868. Adds new option `show_check_constraints` that defaults to `false`. When enabled, it will add check constraints annotations rails/rails#31323 to the model annotations. It can be enabled also through the command line with options `-c` or `--show-check-constraints`. Resolves #104

rails-bot assigned schneems Dec 3, 2017

sgrif reviewed Dec 3, 2017

View reviewed changes

sgrif suggested changes Dec 3, 2017

View reviewed changes

fatkodima force-pushed the check_constraints branch 2 times, most recently from 4845759 to ab123b4 Compare December 5, 2017 21:07

gmcgibbon added activerecord docs labels Sep 30, 2018

rails-bot bot added the stale label Mar 3, 2020

rails-bot bot removed the stale label Mar 3, 2020

jeremy reviewed Mar 3, 2020

View reviewed changes

fatkodima force-pushed the check_constraints branch from 14b1e36 to c19da90 Compare March 4, 2020 19:08

rails-bot bot added the stale label Jun 2, 2020

jeremy added this to the 6.1.0 milestone Jun 2, 2020

rails-bot bot removed the stale label Jun 2, 2020

jeremy unassigned schneems Jun 2, 2020

jeremy self-assigned this Jun 2, 2020

jeremy approved these changes Jun 2, 2020

View reviewed changes

fatkodima force-pushed the check_constraints branch 5 times, most recently from b43feb3 to ac01c4d Compare June 3, 2020 00:55

Add basic support for check constraints to database migrations

c3ee7da

fatkodima force-pushed the check_constraints branch from ac01c4d to c3ee7da Compare June 3, 2020 01:11

jeremy merged commit 1944a7e into rails:master Jun 3, 2020

koic mentioned this pull request Jun 3, 2020

Add basic support for check constraints to database migrations rsim/oracle-enhanced#2025

Open

agrobbin mentioned this pull request Sep 7, 2020

Add support for NOT VALID check constraints in PostgreSQL #40192

Merged

agrobbin mentioned this pull request Sep 13, 2020

Add support for exclusion constraints (PostgreSQL-only) #40224

Merged

coreyja mentioned this pull request Dec 22, 2020

Rails 6.1 support? betesh/active_record-postgres-constraints#37

Closed

andrewpbrett mentioned this pull request Feb 19, 2021

Add more robust variant unit migration openfoodfoundation/openfoodnetwork#6872

Closed

lovro-bikic mentioned this pull request Apr 23, 2021

Add support for annotating check constraints ctran/annotate_models#868

Merged

kmasuda-aiming mentioned this pull request Dec 7, 2021

レビュー用 kmasuda-aiming/ridgepole#1

Closed

4 tasks

natt-eyre mentioned this pull request Jun 14, 2022

Add documentation on check constraints in Active Record Migrations to Rails Guides #45356

Closed

eudaimonious mentioned this pull request Jul 21, 2022

Check if database constraint exists before attempting to remove #45634

Closed

aidanharan mentioned this pull request Nov 20, 2023

Added support for check constraints rails-sqlserver/activerecord-sqlserver-adapter#1141

Merged

drwl mentioned this pull request Apr 12, 2024

Annotate model check constraints drwl/annotaterb#105

Merged

Comments

Conversation

fatkodima commented Dec 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Considerations

Uh oh!

rails-bot commented Dec 3, 2017

Uh oh!

sgrif Dec 3, 2017

Choose a reason for hiding this comment

Uh oh!

matthewd Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

sgrif Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgrif left a comment

Choose a reason for hiding this comment

Uh oh!

lostapathy commented Dec 4, 2017

Uh oh!

matthewd commented Dec 4, 2017

Uh oh!

sgrif commented Dec 4, 2017

Uh oh!

lostapathy commented Dec 4, 2017

Uh oh!

fatkodima commented Dec 5, 2017

Uh oh!

pkoch commented Sep 5, 2018

Uh oh!

NikolayS commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fatkodima commented Apr 23, 2019

Uh oh!

NikolayS commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fatkodima commented Dec 4, 2019

Uh oh!

rails-bot bot commented Mar 3, 2020

Uh oh!

pkoch commented Mar 3, 2020

Uh oh!

jeremy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fatkodima commented Mar 4, 2020

Uh oh!

rails-bot bot commented Jun 2, 2020

Uh oh!

jeremy left a comment

Choose a reason for hiding this comment

Uh oh!

fatkodima commented Jun 3, 2020

Uh oh!

NikolayS commented Jun 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

fatkodima commented Dec 3, 2017 •

edited

Loading

sgrif Dec 4, 2017 •

edited

Loading

NikolayS commented Apr 23, 2019 •

edited

Loading

NikolayS commented Apr 23, 2019 •

edited

Loading

jeremy left a comment •

edited

Loading

NikolayS commented Jun 3, 2020 •

edited

Loading