[Django] #31774: validate_unique in Model class doesn't validate uniqueness for ImageField.
Django
-------------------------------------+-------------------------------------
Reporter: gghildyal | Owner: nobody
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
'''Steps for reproduction:'''
* Consider a model with an ImageField
{{{#!python
@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
{{{#!python
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/'} }}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31774>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "ticket_31774.zip" added.
Test project
Django
-------------------------------------+-------------------------------------
Component: Database layer | Version: 3.0
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* type: Uncategorized => New feature
* resolution: => worksforme
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) | Resolution:
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) | Resolution:
Severity: Normal | worksforme
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "ticket_31774_s3.tgz" added.
Reproduce with s3 storage (or any other external storage)
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: worksforme => needsinfo
Comment:
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
[https://github.com/django/django/blob/156a2138db20abc89933121e4ff2ee2ce56a173a/django/forms/models.py#L383
`_post_clean()`] we WOULD validate the uniqueness of the field.
* In a test case
[https://github.com/django/django/blob/156a2138db20abc89933121e4ff2ee2ce56a173a/django/forms/models.py#L310
`_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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Gaurav Ghildyal):
Oops, that's my mistake, wrong tar command! Will upload the correct one!
Replying to [comment:3 Carlton Gibson]:
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:4>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "ticket_31774_s3_corrected.tgz" added.
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:5>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Database layer | Version: 3.0
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31774#comment:6>