Opened 3 years ago

Closed 2 years ago

#24727 closed Cleanup/optimization (fixed)

ClearableFileInput masks useful exceptions in is_initial()

Reported by: Antonio García-Domínguez Owned by: Berker Peksag
Component: Forms Version: 1.8
Severity: Normal Keywords:
Cc: berker.peksag@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Antonio García-Domínguez)

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

Change History (11)

comment:1 Changed 3 years ago by Antonio García-Domínguez

Has patch: set

Here is a pull request with the proposed fix:

https://github.com/django/django/pull/4579

comment:2 Changed 3 years ago by Antonio García-Domínguez

Description: modified (diff)

comment:3 Changed 3 years ago by Tim Graham

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:4 Changed 3 years ago by Abhaya Agarwal

Needs tests: unset

comment:5 Changed 3 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:6 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5c412dd:

Fixes #24727 -- Prevented ClearableFileInput from masking exceptions on Python 2

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

In 7cb3a488:

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.

comment:8 Changed 3 years ago by Tim Graham

Easy pickings: unset
Has patch: unset
Resolution: fixed
Status: closednew
Triage Stage: Ready for checkinAccepted
Type: BugCleanup/optimization

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).

comment:9 Changed 2 years ago by Berker Peksag

Cc: berker.peksag@… added
Has patch: set
Owner: changed from nobody to Berker Peksag
Status: newassigned

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().

comment:10 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:11 Changed 2 years ago by Berker Peksag <berker.peksag@…>

Resolution: fixed
Status: assignedclosed

In 043383e3:

Fixed #24727 -- Prevented ClearableFileInput from masking exceptions on Python 2

Note: See TracTickets for help on using tickets.
Back to Top