Field uniqueness checks are broken by clean_isbn

[salomvary]

Hey @secnot,

First of all, thanks for creating and opensourcing ISBNField!

I've run into an issue with using unique=True. Apparently ISBNField breaks Django's pre-save uniquess check by doing the checks using the dirty ISBN instead of the cleaned one.

I've created a failing test case here: salomvary@306ab05

Instead of the validation error, it fails with django.db.utils.IntegrityError: UNIQUE constraint failed: tests_uniquecleanisbnmodel.isbn which is not what is expected for these cases.

The problem lies in Django getting the dirty value instead of the clean one here: https://github.com/django/django/blob/ca9872905559026af82000e46cde6f7dedc897b6/django/db/models/base.py#L1097

I am trying to come up with a workaround but as someone who has built zero custom fields so far, I might be slower than you spotting the problem. If you have any insights, let me know!

[secnot]

Hi

A solution could be to override to_python method to return the cleaned value instead of using pre-save. But I can't test it today.

def to_python(self, value):
    if self.clean_isbn:
        return value.replace(' ', '').replace('-', '').upper()
    else:
        return value

[salomvary]

Thanks for the quick response @secnot! I've tried to_python with no luck. I wonder if this is a Django bug/missing feature.. Or does Django expect such cleaning to happen in widgets and not in model fields?

[secnot]

Hi

Then the next candidate would be to_python method in the form field, if you look Datefield in uses the method to convert values to datetime.

[secnot]

There should be an easy fix to the model field, is just that model/field validation is so convoluted that it is not obvious sometimes.

If you look at db.models.fields.DateField it also uses to_python to convert values to datetimes, so it is probably a problem with clean_isbn, I will look into it more in depth tomorrow.

Probably this should work, but it will always clean the field

def to_python(self, value):
    return value.replace(' ', '').replace('-', '').upper()

[salomvary]

There should be an easy fix to the model field, is just that model/field validation is so convoluted that it is not obvious sometimes.

Yeah, I've read through the documentation for custom fields and it's quite intimidating..