Implement Three-State Boolean checker

[toydestroyer]

Hi there 👋,

I recently came across the new Rails/ThreeStateBooleanColumn rule that rubocop-rails introduced (PR, docs). When I applied it to my project, it flagged several old migrations that were violating the rule, which is exactly what it's supposed to do.

While rubocop is great at catching new violations, it doesn't help much in assessing or fixing existing issues. That's where I thought the DatabaseConsistency gem could come in handy!

In this PR, I've created a column checker called ThreeStateBooleanChecker that goes through all boolean columns and reports a failure if the column is missing both a NOT NULL constraint and a default value.

For more details on Three-State Booleans, check out the Rails Style Guide: https://rails.rubystyle.guide/#three-state-boolean

I'm also interested in implementing an autofix feature for this issue. We might be able to reuse NullConstraintMissing, but it would be safe to autofix only when a default value is already present for the column. Otherwise, determining the right default value for each specific case could be tricky. I'd love to hear your thoughts on this, @djezzzl.

[pyromaniac]

I believe this is an incorrect cop in Rubocop and it would be incorrect here. Default value for boolean is a business concern, and should not exist on the storage level. Ideally we should require null: false for boolean columns and no default value whatsoever.

[djezzzl]

Hi, @toydestroyer!

This is a good idea. However, I also tend to agree with @pyromaniac.

We shouldn't force people to have business logic within the databases, but adding a null constraint seems reasonable.

What do you think?

[pyromaniac]

Maybe we can make it configurable after all? Like require no default value by default (cause we want to promote right approaches) but add a config option to negate this logic (in case somebody strongly thinks otherwise)? I think it will match all the opinions.

[toydestroyer]

Thanks, @pyromaniac and @djezzzl, for the comments! I don't have a strong opinion on this. My original idea was to map the logic 1:1 to Rubocop since the problem has been discussed before here, here, here, and here.

Maybe we can make it configurable after all?

Making checkers configurable is something I proposed previously. And yes, I think it would be great.

The good thing about not enforcing the default value is that we can get autofix with no effort (by reusing the NullConstraintMissing writer).

I'm okay with enforcing NOT NULL without a default value and reusing existing autofix as a part of this PR. And work on configuration later (after #169).

It's your call @djezzzl.

[djezzzl]

I'm okay with enforcing NOT NULL without a default value and reusing existing autofix as a part of this PR. And work on configuration later (after #169).

Let's do exactly this. I would happily merge and release an updated version as soon as possible.

Please let me know if you need any help with that.

[toydestroyer]

@djezzzl done!

[djezzzl]

Hi @toydestroyer,

That was super quick!

I'm sorry for the bad/hidden architecture. I think your solution currently misses SimpleWriter, meaning it is not being output by the database_consistency command.

Could you please add it?

[djezzzl]

Unfortunately, no integration tests are yet to check that this is covered.

[toydestroyer]

@djezzzl and done ✅

Thanks for helping me!

[djezzzl]

Super cool!

[djezzzl]

Merged! I'm releasing it right now. Would you mind updating the wiki page? I can do that if you can't.

[djezzzl]

Released as part of 1.7.6. Thank you again!

Would you like to contribute something else?

I guess it would be the idea of the enum checker you shared. I will read it through today. But maybe you have some others too?

[toydestroyer]

Would you like to contribute something else?

Definitely! The enum one is in progress already. Meanwhile, I can help address open issues, look into checkers' configuration, and maybe look into tests to see what can be improved there.

[toydestroyer]

Would you mind updating the wiki page?

Sure, I'll do it tomorrow

[djezzzl]

Thank you a lot! Looking forward to it!

[toydestroyer]

@djezzzl I've updated the wiki page, but I was only able to come up with 2 small paragraphs 😅

Please take a look and feel free to add anything else if you want.

How about including these 2 links?

• https://rails.rubystyle.guide/#three-state-boolean
• https://thoughtbot.com/blog/avoid-the-threestate-boolean-problem

I also have an example of how a three-state boolean can complicate your SQL queries, but it might seem a bit far-fetched.

Let's say you have an active column in your users table. If active can be NULL, then to query all inactive users, you have to do either

SELECT * FROM users WHERE active = false OR active IS NULL

or

SELECT * FROM users WHERE active IS DISTINCT FROM true

Rails can produce the first query for you:

User.where(active: [false, nil])

[djezzzl]

How about including these 2 links?

I've just added those 👍

I also have an example of how a three-state boolean can complicate your SQL queries, but it might seem a bit far-fetched.

We can add it to give more context. Although the issue is more profound, so maybe we should add a link to some more extensive guide or write it ourselves (which we might not have the capacity to do, though).

[djezzzl]

In any case, thank you again for your contribution!