change_column_null should raise if a non-boolean 3rd argument is provided
#45229
Conversation
ghiculescu
force-pushed
the
change-column-null-ux
branch
from
df856f3
to
eb9b076
Compare
activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb
Outdated
Show resolved
Hide resolved
ghiculescu
force-pushed
the
change-column-null-ux
branch
from
eb9b076
to
b337075
Compare
|
@fatkodima thanks for the great review ❤️ |
ghiculescu
force-pushed
the
change-column-null-ux
branch
from
b337075
to
0fac0aa
Compare
…ovided
Currently if you provide a non-boolean argument, `change_column_null` will treat it as truthy and make your column nullable. This might not be what you want. For example, I've noticed this happen a few times, where someone assumes that `change_column_null` and `change_column_default` work the same:
```ruby
change_column_default(:posts, :state, from: nil, to: "draft")
change_column_null(:posts, :state, from: true, to: false)
```
Reading the migration you would expect that the default is now "draft" and the column doesn't accept nulls. But actually, the "null" argument is `{ from: true, to: false }` which is truthy, so now the column accepts nulls. If you aren't paying close attention to the migration output you might miss this.
I think we can protect users here by having `change_column_null` require the 3rd argument to be `true` or `false` and raising an error otherwise. This PR implements that, for migrations created on Rails 7.1 onward.
ghiculescu
force-pushed
the
change-column-null-ux
branch
from
0fac0aa
to
08dc02b
Compare
|
LGTM Btw. what about other methods handling DB |
|
👍 that's a good call. We should do that too. I can open a followup PR for that, unless @byroot (or anyone else) feels strongly about it being in the same PR. |
In this case it's much more obvious that a boolean is expected. The reason I'm 👍 on this particular PR is that it's indeed very easy to make that mistake, but it doesn't mean that we should check the type of every params everywhere. |
Sorry for confusion, my intention wasn't to extend scope of this PR. I was just wondering if this is start of new pattern -> checking migration params. It seems random now. Something is checked, something not now. 🤷♂️ 🤔 Also looking at the code, currently it is needed to do validation in each adapter on its own. That means adapters maintained @byroot for the future, would it be welcomed to somehow wrap this into one shared entry for all adapters? I can try to prepare some initial PR to discuss. That way it would be much easier to bring similar changes. |
|
If you find a way to avoid that duplication and to support third-party adapter, then yes it's welcome. |
Currently if you provide a non-boolean argument,
change_column_nullwill treat it as truthy and make your column nullable. This might not be what you want. For example, I've noticed this happen a few times, where someone assumes thatchange_column_nullandchange_column_defaulthave the same signature:Reading the migration you would expect that the default is now "draft" and the column doesn't accept nulls. But actually, the "null" argument is
{ from: true, to: false }which is truthy, so now the column accepts nulls. If you aren't paying close attention to the migration output you might miss this.I think we can protect users here by having
change_column_nullrequire the 3rd argument to betrueorfalseand raising an error otherwise. This PR implements that, for migrations created on Rails 7.1 onward.