[Django] #22717: FileField.url returns a wrong URL
Django
--------------------------------------+-----------------------------------
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Keywords: storage,filefield,url
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+-----------------------------------
models.py:
{{{
...
class Media(models.Model):
....
file = models.FileField(upload_to=storages.upload_to,
storage=storages.media_storage)
...
}}}
storages.py:
{{{
...
def upload_to(instance, filename):
return '%s/%s' % (instance.pk, filename)
media_storage = FileSystemStorage(location='/test/media',
base_url='/test/media')
}}}
Django shell:
{{{
...
>>> m = Media.objects.get(pk=27)
>>> m.file.storage.base_url
'/test/media'
>>> m.file.name
'salut.txt'
>>> m.file.url
'/test/salut.txt'
}}}
Expected -> '/test/media/salut.txt'
Investigating:
{{{
>>> from django.utils.six.moves.urllib.parse import urljoin
>>> urljoin('/test/media', 'salut.txt')
'/test/salut.txt'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22717>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
storage,filefield,url | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by david.fischer.ch@…):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Hello,
Using '/test/media/' instead of '/test/media' fix the issue.
This is an unexpected behavior, I would like that the default
implementation of FileSystemStorage takes care of this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:1>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* easy: 0 => 1
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:2>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by claudep):
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/2731
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:3>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by timo):
We require `MEDIA_URL` to end with a trailing slash (see #6198 for reasons
why it won't be changed). I am not sure if the same reasoning holds for
this, but I thought it was worth mentioning.
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:4>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mardini):
`urljoin()` is implemented like this in Python to be consistent with RFC
3986, section 5.2.3. (http://tools.ietf.org/html/rfc3986#section-5.2.3)
which states how merging a relative-path with the base URI should work. It
*might* not be a good idea to override that standard for the convince.
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:5>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by claudep):
IMHO, RFC 3986 is aimed at "generic" merging, where you can't assume that
`/url` point to a path, but can also be an arbitrary resource. In our
case, `base_url` can be considered as a path in all cases (AFAIK), that's
why I don't see the problem of auto-adding the ending slash. But if anyone
can come with a use case where this is not wanted behavior, I'll happily
change my mind.
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:6>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
storage,filefield,url | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mardini):
That's a solid reasoning I guess.
I can't think of any use case where adding a trailing slash to `base_url`
would cause undesirable behavior either. And since `base_url` isn't
necessarily `MEDIA_URL`, I see no harm in implementing this feature, even
though `MEDIA_URL` must always end in a slash.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:7>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: new
uploads/storage | Resolution:
Keywords: | checkin
storage,filefield,url | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by timo):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:8>
Django
Reporter: david.fischer.ch@… | Owner: nobody
Type: Bug | Status: closed
Component: File | Version: master
uploads/storage | Resolution: fixed
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
storage,filefield,url | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"fb9d8f06520f495d0c36236f7534dbe660c7e164"]:
{{{
#!CommitTicketReference repository=""
revision="fb9d8f06520f495d0c36236f7534dbe660c7e164"
Fixed #22717 -- Auto-corrected missing ending slash in FileSystemStorage
Thanks David Fischer for the report and Moayad Mardini for the
review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22717#comment:9>