[Django] #24727: ClearableFileInput masks useful exceptions in is_initial()
Django
----------------------------+--------------------
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
----------------------------+--------------------
I have a Python 2.7 site with some special requirements on how to link to
uploaded files, so we defined a FileSystemStorage subclass with a custom
url() method that computed a reverse URL for them.
This morning I noticed that some files would not show up in our FileField
with a ClearableFileInput widget after being uploaded through the Django
admin app. After searching for a while, I found the issue: the
hasattr(value, "url") call in ClearableFileInput.is_initial() was simply
returning False for these files, masking an exception from the reverse()
call of our custom url() method.
I suggest changing ClearableFileInput.is_initial() so it will not mask
useful exceptions. Instead of simply using hasattr, it should use a
getattr call inside a try..except block and rethrow anything that is not
an AttributeError. In fact, this is the new behavior of hasattr in Python
3.2:
https://docs.python.org/3.2/library/functions.html#hasattr
--
Ticket URL: <https://code.djangoproject.com/ticket/24727>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by bluezio):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
Here is a pull request with the proposed fix:
https://github.com/django/django/pull/4579
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:1>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Description changed by bluezio:
Old description:
> I have a Python 2.7 site with some special requirements on how to link to
> uploaded files, so we defined a FileSystemStorage subclass with a custom
> url() method that computed a reverse URL for them.
>
> This morning I noticed that some files would not show up in our FileField
> with a ClearableFileInput widget after being uploaded through the Django
> admin app. After searching for a while, I found the issue: the
> hasattr(value, "url") call in ClearableFileInput.is_initial() was simply
> returning False for these files, masking an exception from the reverse()
> call of our custom url() method.
>
> I suggest changing ClearableFileInput.is_initial() so it will not mask
> useful exceptions. Instead of simply using hasattr, it should use a
> getattr call inside a try..except block and rethrow anything that is not
> an AttributeError. In fact, this is the new behavior of hasattr in Python
> 3.2:
>
> https://docs.python.org/3.2/library/functions.html#hasattr
New description:
I have a Python 2.7 site with some special requirements on how to link to
uploaded files, so I defined a FileSystemStorage subclass with a custom
url() method that computed a reverse URL for them.
This morning I noticed that some files would not show up in a FileField
with a ClearableFileInput widget after being uploaded through the Django
admin app. After searching for a while, I found the issue: the
hasattr(value, "url") call in ClearableFileInput.is_initial() was simply
returning False for these files, masking an exception from the reverse()
call of our custom url() method.
I suggest changing ClearableFileInput.is_initial() so it will not mask
useful exceptions. Instead of simply using hasattr, it should use a
getattr call inside a try..except block and rethrow anything that is not
an AttributeError. In fact, this is the new behavior of hasattr in Python
3.2:
https://docs.python.org/3.2/library/functions.html#hasattr
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:2>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:3>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by abhaga):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:4>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.8
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:5>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"5c412dd8a724b263489c1bd7a2fea381460665d7" 5c412dd]:
{{{
#!CommitTicketReference repository=""
revision="5c412dd8a724b263489c1bd7a2fea381460665d7"
Fixes #24727 -- Prevented ClearableFileInput from masking exceptions on
Python 2
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:6>
Django
Reporter: bluezio | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"7cb3a488436e8e7d0249280e95ba2ee945c96c87" 7cb3a488]:
{{{
#!CommitTicketReference repository=""
revision="7cb3a488436e8e7d0249280e95ba2ee945c96c87"
Fixed #25410 -- Fixed empty ClearableFileInput crash on Python 2.
Reverted "Fixes #24727 -- Prevented ClearableFileInput from masking
exceptions on Python 2" and added a regression test.
This reverts commit 5c412dd8a724b263489c1bd7a2fea381460665d7.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:7>
Django
Reporter: bluezio | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* status: closed => new
* resolution: fixed =>
* easy: 1 => 0
* has_patch: 1 => 0
* type: Bug => Cleanup/optimization
* stage: Ready for checkin => Accepted
Comment:
Sorry, we had to revert this as noted above because it caused a
regression.
It looks like we might have to address #13327 before we can add this back
(or find a patch that works differently).
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:8>
Django
Reporter: bluezio | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Component: Forms | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* owner: nobody => berkerpeksag
* status: new => assigned
* has_patch: 0 => 1
* cc: berker.peksag@… (added)
Comment:
Pull request: https://github.com/django/django/pull/6143
Actually, there is no reason to use `hasattr()` at all. `hasattr()` also
calls `PyObject_GetAttr` (like `getattr()`) in its implementation:
https://github.com/python/cpython/blob/d69ba698ea79bab7e6d14732d8bf6215b7b93f8b/Python/bltinmodule.c#L1061
and there is no significant speed difference between `hasattr()` and
`getattr()`.
I've also added a test for defining url as property. Previously, url was
defined as a class attribute in `FakeFieldFile()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:9>
Django
Reporter: bluezio | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:10>
Django
Reporter: bluezio | Owner:
Type: | berkerpeksag
Component: Forms | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"043383e3f31955c493517cb5b12dd538c02f5246" 043383e3]:
{{{
#!CommitTicketReference repository=""
revision="043383e3f31955c493517cb5b12dd538c02f5246"
Fixed #24727 -- Prevented ClearableFileInput from masking exceptions on
Python 2
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24727#comment:11>