Add support for annotating check constraints

[lovro-bikic]

Rails added support for check constraints (rails/rails#31323), so I thought it would be nice if we could annotate them in models.

This PR adds annotation of check constraints with an option to disable/enable annotation. Most of the work done in this PR is based off of existing implementation for annotating indexes and foreign keys.

[Eusebius1920]

Thanks for sharing. This should get merged!

[deecewan]

@ctran any change we can get this merged?

[bruno-]

Here's a 2022 bump to this issue.

check_constraints are great and it would be great having them in the annotations.

[dtcristo]

Hi @lovro-bikic,

First of all, I love the new feature ❤️.

We've started using your implementation and noticed it has issues with check constraints that go across multiple lines.

For example, say I have the following constraint:

CONSTRAINT foo_not_null_when_active CHECK (
CASE
    WHEN ((status)::text = 'active'::text) THEN (foo IS NOT NULL)
    ELSE true
END)

Currently this results in the following annotation:

# Check Constraints
#
#  foo_not_null_when_active  ()
#

Not sure how we'd want this to be formatted in the annotation? Perhaps we can keep the wrapping. Maybe something like this?

# Check Constraints
#
#  foo_not_null_when_active  (CASE
#                                WHEN ((status)::text = 'active'::text) THEN (foo IS NOT NULL)
#                                ELSE true
#                            END)
#

Alternatively could .squish it and print:

# Check Constraints
#
#  foo_not_null_when_active  (CASE WHEN ((status)::text = 'active'::text) THEN (foo IS NOT NULL) ELSE true END)
#

[lovro-bikic]

@dtcristo, thanks for the feedback!

I would go with multiline expressions squished into a single line, to be consistent with the rest of the annotations. I pushed a commit for it: b850e9b.

Side note: CI is failing, though the failures are unrelated to this branch. develop branch currently has the same failures.

[dtcristo]

Hi @lovro-bikic 👋, I've had a play with your .squish changes in my Rails 6.1 application and running in to some issues.

So it turns out there is a bug in Rails 6.1 (since fixed here for Rails 7 only). The bug means that multi-line constraints are coming through as nil from the connection for PostgreSQL.

So your line:

check_constraint.expression.squish

Will error because check_constraint.expression is nil for these multi-line constraints. So at the very least we might want &.squish so we don't raise an exception in this case and keep the behaviour of printing nothing for users with older versions of Rails (like me).

However, something else I noticed with the change here, is that it will introduce an extra later of parenthesis around the check constraint for normal (one line) constraints.

So a constraint that was previously printing as:

alive    (age < 150)

Will now print

alive    ((age < 150))

However with this change a multi-line constraint will correctly print with only one layer of parenthesis.

Not sure if this is an issue, but I just wanted to point it out? Potentially if you see wrapping parenthesis you could strip them?

[lovro-bikic]

Thanks for pointing out the issue regarding missing expressions, it should be fixed with this commit: 6449e7d.

However, something else I noticed with the change rails/rails#43963, is that it will introduce an extra later of parenthesis around the check constraint for normal (one line) constraints.

I couldn't reproduce this. I tried it out on Rails versions 6.1.5.1 and 7.0.2.4, and Rails in both cases output the constraint expression without the wrapping parentheses, so the annotation would always correctly have a single pair of them.

[lparry]

It would be really helpful to get this merged. Is there anything we might to to help this along @ctran ?

[ctran]

I need to take a look to see why the CI is failing.

[rjherrera]

@ctran hi there! have you been able to look at this? We found ourselves needing to annotate constraints, and found out about this pull request, it looks like what we need 😄, anything we can do to help get this merged?

[ctran]

@ctran hi there! have you been able to look at this? We found ourselves needing to annotate constraints, and found out about this pull request, it looks like what we need 😄, anything we can do to help get this merged?

I just need help getting the CI build to pass :-)

[lovro-bikic]

@ctran I rebased the branch but the CI is still failing due to this error:

  1) AnnotateModels annotating a file frozen option should abort without existing annotation when frozen: true 
     Failure/Error: expect { annotate_one_file frozen: true }.to raise_error SystemExit, /user.rb needs to be updated, but annotate was run with `--frozen`./
       expected SystemExit with message matching /user.rb needs to be updated, but annotate was run with `--frozen`./ but nothing was raised
     # ./spec/lib/annotate/annotate_models_spec.rb:2914:in `block (4 levels) in <top (required)>'

It seems that the test suite is non-deterministic because when I run bundle exec rspec locally sometimes it passes and sometimes it fails (either with the above error or some other errors), depending on the RSpec seed. I found that running with the seed 43289 (bundle exec rspec --seed 43289) makes the suite pass.

I'm not sure how to proceed here because these errors fall outside the scope of this PR, please advise.

[lovro-bikic]

@ctran I invested a bit of time in researching the flaky test suite and found what causes these issues. Here is the PR which should fix all of them: #980. After we merge that, I will rebase this branch.

[ctran]

@lovro-bikic I merged #980

[lovro-bikic]

Thanks! I rebased the branch, but the CI failed because of Rubocop. I disabled these offenses but let me know if you want me to fix them in a different way.

[ctran]

LGTM (we'll deal with the rubocop issues later)