Opened 9 years ago

Closed 5 years ago

Last modified 5 years ago

#5160 closed New feature (fixed)

get_FIELD_url should return a valid URL with spaces escaped

Reported by: Esaj Owned by:
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: get_FIELD_url escape urllib.quote
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Some of my RSS feeds aren't validating because calling get_FIELD_url for an ImageField isn't escaping spaces in the filename.

A simple patch is attached which uses urllib.quote to fix this problem.

Attachments (4)

base.py.diff (664 bytes) - added by Esaj 9 years ago.
Escape spaces in generated URL using urllib.quote
base.2.py.diff (664 bytes) - added by Esaj 9 years ago.
Better patch; the previous one erroneously escapes MEDIA_URL
base.3.py.diff (910 bytes) - added by Esaj 9 years ago.
Use django.util.http.urlquote for unicode safety
5160-test.diff (867 bytes) - added by Claude Paroz 5 years ago.
Test that issue is solved

Download all attachments as: .zip

Change History (15)

Changed 9 years ago by Esaj

Attachment: base.py.diff added

Escape spaces in generated URL using urllib.quote

Changed 9 years ago by Esaj

Attachment: base.2.py.diff added

Better patch; the previous one erroneously escapes MEDIA_URL

comment:1 Changed 9 years ago by Malcolm Tredinnick

Using urllib.urlquote is incorrect here because the value could contain non-ASCII characters. Should be using django.utils.http.urlquote instead.

comment:2 Changed 9 years ago by Simon G. <dev@…>

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Changed 9 years ago by Esaj

Attachment: base.3.py.diff added

Use django.util.http.urlquote for unicode safety

comment:3 Changed 9 years ago by Chris Beaven

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:4 Changed 9 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This isn't ready to commit yet. Firstly, because the existing tests fail, which is a bit of a showstopper (in this case, the failing test is probably wrong, but it still needs to be fixed at the same time). Secondly because it doesn't have any tests of its own. And, most importantly, because it causes some problems with serialisation -- things don't round-trip cleanly.

The last point is almost certainly a problem in the serialization framework, but it needs to be fixed first or at the same time. Moving back to accepted until I (or somebody else, preferably) can look at the issues.

comment:5 Changed 9 years ago by Jason Davies

Owner: changed from nobody to Jason Davies

comment:6 Changed 9 years ago by Jason Davies

Status: newassigned

comment:7 Changed 8 years ago by Jason Davies

Owner: Jason Davies deleted
Status: assignednew

comment:8 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: New feature

Changed 5 years ago by Claude Paroz

Attachment: 5160-test.diff added

Test that issue is solved

comment:9 Changed 5 years ago by Claude Paroz

Easy pickings: unset
Patch needs improvement: unset
UI/UX: unset

I guess this issue is solved nowadays. I've just added the test for the space char in the corresponding file_storage test.

comment:10 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

get_FIELD_url doesn't exist anymore. Now it's just an url attribute on the field and the problem appears to be fixed. I'll commit the test.

comment:11 Changed 5 years ago by Aymeric Augustin

In [17361]:

Tested that spaces are properly escaped in files URLs. Refs #5160.

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