validate_unique in Model class doesn't validate uniqueness for ImageField.

[Gaurav Ghildyal]

Steps for reproduction:

• Consider a model with an ImageField

@final
class ValidateUnique(models.Model):
    logo = models.ImageField(
          _('Official Logo'),
          help_text=_('The official logo..'),
          upload_to='logos/',
          blank=True,
          unique=True,
    )

• Integrate the above model with the admin interface.
• Create two instances of the model via the admin interface.
• Upload an image for a logo in the first one.
• Upload the same image for a logo in the second one.

Expected Result: The 'logo' field in the form shows an error saying the logo already exists (Fails uniqueness constraint)

Actual Result: An error page with a stacktrace (running with DEBUG=True) throwing an IntegrityError.

Possible cause:

In the Model class in django/db/models/base.py

def _perform_unique_checks(self, unique_checks):
  ...
  ...
           # some fields were skipped, no reason to do the check
            if len(unique_check) != len(lookup_kwargs):
                continue

            qs = model_class._default_manager.filter(**lookup_kwargs)
 ....

In the above code, the lookup_kwargs gets this value

lookup_kwargs = {'logo': <ImageFieldFile: temp.logo.png>}

The queryset fails to return the first model instance because the lookup for ImageField fails. So validation error isn't added to the form and instead IntegrityError exception is raised.

It would work if the lookup value was the the fields 'name' attribute, say if the lookup_kwargs looked like this:

lookup_kwargs = {'logo': '/logos/temp.logo.png/'}

[Carlton Gibson]

Test project

[Carlton Gibson]

I'm afraid I can't reproduce this with the given info.

Actual Result: An error page with a stacktrace (running with DEBUG=True) throwing an IntegrityError.

I don't see that.

Rather, uploaded files have a random string appended in order to create a unique name.

db.sqlite3> SELECT * from reproduce_validateunique;                                                                                        
+----+---------------------------------------------------------+
| id | logo                                                    |
+----+---------------------------------------------------------+
| 1  | logos/2020-03-19-View-from-inside-my-laptop.jpg         |
| 2  | logos/2020-03-19-View-from-inside-my-laptop_xB8SLzj.jpg |
| 3  | logos/2020-03-19-View-from-inside-my-laptop_YeNU9Rp.jpg |
+----+---------------------------------------------------------+
3 rows in set
Time: 0.016s

(See Storage.get_alternative_name())

Can you adjust the uploaded sample project to demonstrate this issue?

Possibly, there's a New Feature here. Originally unique was not supported for FileField. Support was added in 1.11, for #27188, supporting uniqueness of the name attribute.
Maybe additional work is needed to push that out to the form layer (but pending the reproduce...)

Checking the image contents is not (won't be) supported:

I think by definition ImageFields are unique; even if you upload the same image twice you'll still end up with two unique files.

Jacob on #1898.

[Gaurav Ghildyal]

The automatic random string is appended by the storage layer - in this case the default FileSystemStorage class.

I was storing the filles on AWS S3 using the S3Storage from the ​https://django-storages.readthedocs.io/en/latest/ library which doesn't do that, it just overwrites the file on the actual storage and returns the same name as set by the form, which the validate_unique function fails to catch but the database layer throws an exception due to name conflict.

Maybe asserting the uniqueness should become a part of the storage layer instead of the model layer, but in that case the validate_unique function should exclude ImageField and FileFields from uniqueness validation and documentation should be updated to reflect this?

[Gaurav Ghildyal]

Reproduce with s3 storage (or any other external storage)

[Carlton Gibson]

Hi Gaurav,

Thanks for the follow-up. (I wasn't sure about the attachment: it's seemed like the whole project was saved as binary in the manage.py file — how did you create that? How would one run it? — Not something I've seen before, but I was able to look at it.)

I'm going to move this to needsinfo — assuming a storage that didn't create unique names, I can see how this might come up.

The storage issue is a bit of a distraction. The correct validation should be at the form layer:

• Calling form.is_valid(), on a ModelForm using the image (or file field).
• In ​`_post_clean()` we WOULD validate the uniqueness of the field.
• In a test case ​`_get_validation_exclusions()` included the logo field — that's because of the blank=True on the model definition. That means _post_clean() will be skipped for it.

Removing the blank=True allows logo not be excluded but I didn't have enough time to verify whether the image/file field would then trigger the expected error in validate_unique(). I may be able to get back to it (it's interesting) but if you could dig down deeper that would help.

• I'd subclass Storage and override .get_alternative_name() to not do that properly.
• Then create a ModelForm with just the logo field to verify that _post_clean() triggers the uniqueness error.

If not then we can see why not, and whether it can be adjusted.

[Gaurav Ghildyal]

Oops, that's my mistake, wrong tar command! Will upload the correct one!

[Gaurav Ghildyal]

Hi Carlton,

Thanks for looking into this. I will dig deeper along the lines of your input.

Why do you think it should be in the form layer? Given that there is a function (_perform_unique_checks ) that catches all uniqueness errors and raises validation errors, it would be nice to fix the uniqueness check there? The added benefit is that Django admin and API validations can be in one common place.

[Carlton Gibson]

Yes, you're right: the form calls down to the model, but as I was looking, I'm wondering why the field was excluded from the check? Then, if we can make sure it's included, is it triggered? (If not, is that because the name isn't set yet by the storage... — and so on.) It's just a place to start looking, but from the view's point-of-view I need form.is_valid() to return False.
(Hopefully that makes sense.)