adding a null constraint is unsafe

[abcm1989]

Fix for #29 https://apidock.com/rails/ActiveRecord/ConnectionAdapters/SchemaStatements/change_column_null

[ankane]

Hey @abcm1989, thanks for the PR 👍 Can you explain more about what happened and why this should fix it?

[abcm1989]

@ankane Ah, I got my logic flipped in the code, which I've fixed. Adding the constraint should be unsafe, rather than removing it.

We had a migration that added a null constraint using change_column_null that wouldn't run in some environments because there were rows with null values. I had it raising the error only if no default value is passed based on this from the docs

The method accepts an optional fourth argument to replace existing +NULL+s with some other value. Use that one when enabling the constraint if needed, since otherwise those rows would not be valid.

[ankane]

Hey @abcm1989, I don't think adding a default value to replace NULLs is a safe operation.

[abcm1989]

@ankane alrighty, updated so that adding a null constraint always is unsafe

[ankane]

Hey @abcm1989, I'm not sure it's unsafe if all values are filled in.

[abcm1989]

@ankane sorry can you rephrase, I'm not sure what you mean

[ankane]

@abcm1989 Your initial report is about an error if you try to add a not null constraint and have rows with null values. If you don't have rows with null values, I believe this operation is safe.

[abcm1989]

@ankane that's right, do you think it should not raise a warning at all then?

[ankane]

Yeah, that's my current thinking.

[abcm1989]

@ankane okay...just to make sure I understand, your position is that because the migration is safe sometimes, we shouldn't wrap it in a safety assured block?

It's a bit inconsistent that change_column_null would not require a safety assured block
while change_column :my_table, :my_column, :null => false would.

LMK your thoughts

[ankane]

Hey @abcm1989, all change_column calls require a safety_assured block right now (doesn't have to do with changing null value). The one case that seems dangerous to me would be changing to null: false with a default, but I haven't tested it to see if it locks the table for an extended period if there are many null values (I suspect it would).

[abcm1989]

@ankane okay, I see your point. I'll update this so it's only dangerous when setting a default value. It likely makes sense to catch change_column_default migrations too in that case

[ankane]

@abcm1989 I haven't run into this, so would need a proof of concept. Also, changing the default by itself should be fine.

[abcm1989]

gotcha, okay, well thanks for reviewing