[Migrations] `null=True, unique=True` Raises `django.db.utils.IntegrityError: UNIQUE constraint failed` when running migration

[jjorissen52]

To reproduce, make sure some rows already exist for MyModel, then add a PhoneField to it.

class MyModel(models.Model):
    -pass
    +phone = PhoneField(null=True, blank=True, unique=True)

python manage.py makemigrations
python manage.py migrate
# django.db.utils.IntegrityError: UNIQUE constraint failed: new__myapp_mymodel.phone

[jjorissen52]

Happening with a SQLite database. When unique=False and the migration succeeds, all of the values for the column are an empty string instead of null.

[va-andrew]

Can you confirm whether this issue also affects a CharField with the same options and migration process? PhoneField is simply a thin wrapper around CharField, and I'm wondering whether this issue is specific to PhoneField.

[jjorissen52]

Yes, I can confirm that this same issue does not occur with a regular CharField (Django 3.1). The issue seems to be in the current implementation of PhoneField.get_prep_value. An empty PhoneNumber is ultimately stored as an empty string rather than as a SQL NULL. The issue does not occur when I override the method to return None instead of an empty string when not value.

class PhoneField(phone_models.PhoneField):
    def get_prep_value(self, value):
        if not value:
            # return ''
            return None

        if not isinstance(value, PhoneNumber):
            value = PhoneNumber(value)
        return value.cleaned

I can submit a PR if you like.

[jjorissen52]

@va-andrew any interest in this? I will close the issue if not.

[va-andrew]

Hey, thanks for the notice!

Overall, I wasn't expecting use of null=True with PhoneField, since it's somewhat of an antipattern with the CharField it's based on. However, null=True, unique=True is a valid scenario so I think we should support it.

The only concern I have is to make sure we're not upsetting the behavior of existing use cases. Is there a way we can make get_prep_value() return None only if null=True is set on the field?

[jjorissen52]

I don't know if I agree that it's an anti-pattern. In my typical usage of null=False (the default), I want an IntegrityError to be raised if someone attempts to save the instance without setting a value. I think that's pretty typical. This would also be keeping with the behavior of the underlying CharField, and, in my opinion, follows the "principal of least surprise".

Really what I want a specialty field (as with URLField, and EmailField) to do is if any value (and I consider the empty string to be a value) is going to be saved into a PhoneField column, I can be confident that it's a valid phone number, which of course, the empty string is not. It does seem that specialty fields in Django e.g. EmailField will allow you to save an empty string, so it would be consistent for PhoneField to do the same, but it's not consistent if PhoneField as a whole saves the empty string to the database when you didn't enter a value in on the form.

Regardless, this is how you would "return None only if null=True is set on the field":

class PhoneField(phone_models.PhoneField):
    def get_prep_value(self, value):
        if not value:
            return None if self.null else ''

        if not isinstance(value, PhoneNumber):
            value = PhoneNumber(value)
        return value.cleaned

[the-moog]

Just thought I'd add a me too. I think being able to prevent users entering the same phone number for different companies is a valid user case. unique=True is useful and only works with empy fields if they are null=True

[the-moog]

@jjorissen52 Your code snippet fixed the issue for me, thanks.