Re: [Django] #14787: Upload handler should pass errors on to forms.FileField
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: 1.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by anubhav9042):
* cc: anubhav9042@… (added)
* owner: nobody => anubhav9042
* status: new => assigned
Comment:
There have been some changes since past years. But I am in favour of this.
I hope/intend to do this in GSoC this year. I think the following can also
be added along with those suggested above:
File not uploaded completely
Write failure
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by anubhav9042):
* version: 1.2 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
for upload limit we now have `max_length`.
We can have a check `unicode error in filename` and also of possible the
two I mentioned above.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
In general we should be careful about exposing detailed error messages to
users. e.g. I believe the unicode error is a server configuration issue
(see #17686). Similarly, "file not uploaded completely" and "write error"
are not issues that the user is in a position to resolve so if anything,
those errors would need to be surfaced as something like "The file upload
was unsuccessful. Contact the site admin if the error persists". The issue
of upload size seems like a valid one though. Do you have a proposal for
propagating errors from upload handlers to forms?
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
I think checks for `exceeding_upload_size` and for `encoding_type`, we
could add a check in
[https://github.com/django/django/blob/master/django/forms/fields.py#L582
forms.FileField.to_python()]. We already have several checks there, so it
seems reasonable enough to add there.
Thought on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
Although the scope of this ticket isn't well defined, I think what you've
implemented is somewhat tangential to what the summary describes "Upload
handler should pass errors on to forms.FileField" -- although, I'm not
sure how/if file size limit errors would be part of the upload handler.
Btw, `max_upload_size` already exists as a third party package:
[https://pypi.python.org/pypi/django-validated-file/2.0 django-validated-
file]. We'd probably want a mailing list thread to figure out if we'd want
to bring it into core. I don't feel strongly either way on doing so.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
Here: https://groups.google.com/forum/#!topic/django-
developers/4fbcsPhuuR8
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
A couple comments on the post:
1. It's better to make a proposal than ask "what to do" or "how to solve a
ticket." What do you think should be done? If you don't care, then find
I'd find something else to work on.
2. A subject that includes a description of the item instead of "Decision
for ticket #14787" is more likely to get interested people to read it.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
I did mention what I want to do, i.e. including the new attribute
`max_size`, though I will pay more attention to the subject from next time
onwards.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
What's the file name encoding issue you mentioned in the mailing list
post? Is it different from #17686?
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
What I wanted to say is we can inform the user about the problem in a
better way than just adding in the docs by raising a proper error in
`to_python()` itself as we have file_name there so we could check the
encoding.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timo):
It's a server configuration issue that should be fixed by the person
deploying the site, not something an end user should get an error about.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
Component: File | Status: assigned
uploads/storage | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by anubhav9042):
OK. So we are down to that maximum size issue itself.
Can you please post your replies on both the points on ML, so that when
someone sees he knows about these things as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: intgr | Owner:
Type: New feature | anubhav9042
uploads/storage | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
After looking into this, Anubhav and I don't see any easy way to pass
errors from upload handlers to forms since at the time the handler
exception is raised, you aren't inside a form to catch it.
One thing I thought of would be to allow an exception raised during file
upload parsing to be stored in `request.FILES` so it would be re-raised
when you accessed the file's key later, but I'm wary of this complexity
and not even sure it would work.
Regarding `max_size` on `FileField`, it really should be the subject of a
separate ticket and given it's possible to implement this yourself and
available in 3rd party packages, I don't think there's an urgent need for
it.
--
Ticket URL: <https://code.djangoproject.com/ticket/14787#comment:18>