Potential for errors when deprecating non-nullable fields

[LincolnPuzey]

Say in version N of my project I have this model and code that assumes foo_instance.text_field is always going to be a string.

class FooModel(models.Model):
    text_field = models.CharField(max_length=256)

Then in version N+1 I then deprecate it

class FooModel(models.Model):
    text_field = deprecate_field(models.CharField(max_length=256), "")

Then when version N and N+1 are running simultaneously:

1. version N+1 inserts a row, it will have text_field=NULL
2. version N selects this row, and assumes text_field is going to be a string, when it is actually None
3. Potentially raising a TypeError / ValueError

I'm not sure how this could be addressed by the django-deprecate-fields library. Maybe this just needs to be detailed in the docs.

For us the solution is to first use a AddDefaultValue operation on the field to add a database-level default. This is so the rows inserted by N+1 get this default value instead of NULL, so version N can still handle them.

[flixx]

Hello @LincolnPuzey,
wow, I have never thought about this. You are right.

I'll keep this issue open. we need some time to think about it I guess.

Is the workaround with AddDefaultValue working reliably for you?
Maybe this is something to add to the README?

@David-Wobrock FYI: Also an intersting case for the linter I guess. I guess right now, the linter fails when using deprecate_field() (because we are altering a field). The perfect treatment here would be to allow adding removing the NOTNULL constraint but only if there is a default value added.

[LincolnPuzey]

@flixx We have only just started trying to do backwards-compatible migrations, so can't say for sure if AddDefaultValue is working well.

I think this is mainly a problem when you have an existing project, start doing backwards compatible migrations, and then deprecate a non-nullable field.

If you start a project with backwards-compatible migrations enforced from the start, every non-nullable field should have a AddDefaultValue operation from when the field is added.

[David-Wobrock]

Hi @LincolnPuzey , hi @flixx

Indeed an interesting case we didn't give much thought about.

My first reflection is that in the case described is it not Django that crashes, but custom code I guess.
Even if the model class defines that the field cannot be null, but the database actually contains a null value, Django will just fill the field with a None.
However, the code would rightfully not expect a null value.

So the easy way to fixing this would be:

1. adapt your code to handle a null value in this field, even if it is currently impossible
2. deprecate the field, and make it nullable => the previous code will know how to fallback when encountering a null value
3. make sure the field is unused
4. drop the column and the field

The other solution, as discussed, would be to add a default value on the database level (and basically keep the NOT NULL constraint, since you never want to expect a null). You can still use deprecate_field, even if making the field nullable is not necessary.
Maybe we could add a feature to deprecate_field to set a default in the database (if not already set), instead of making it nullable. The main feature you want here it being notified when using the field that shouldn't be used anymore.

In all cases, for the linter it's interesting to have a look at cases where Django handles a peculiar case just fine, but the business code rightfully doesn't expect a certain value.