[Django] #26495: Storage save method wraps StringIO in File object which is identified as False.
Django
False.
--------------------------------------+--------------------
Reporter: m-novikov | Owner: nobody
Type: Uncategorized | Status: new
Component: File uploads/storage | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
https://github.com/django/django/blob/master/django/core/files/storage.py#L50
{{{
#!python
class Storage(object):
def save(self, name, content, max_length=None):
...
if not hasattr(content, 'chunks'):
content = File(content)
}}}
Underlying libraries may use following pattern of code:
{{{
#!python
def store(data, name):
data = data or ''
}}}
But File object without a name resolved into False which can produce bugs.
Possibly better to pass a name to File object. To ensure bool(content) is
True
{{{
#!python
if not hasattr(content, 'chunks'):
content = File(content, name=name)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26495>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
False.
Reporter: m-novikov | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
It's not clear to me how to reproduce the issue. In particular I'm not
sure what the `store` method refers to (I don't see anything in Django
itself with that name). Please try to include more details such as a test
for Django's test suite (ideally) or a sample project.
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:1>
Django
False.
Reporter: m-novikov | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by m-novikov:
Old description:
> https://github.com/django/django/blob/master/django/core/files/storage.py#L50
> {{{
> #!python
> class Storage(object):
>
> def save(self, name, content, max_length=None):
> ...
> if not hasattr(content, 'chunks'):
> content = File(content)
> }}}
> Underlying libraries may use following pattern of code:
> {{{
> #!python
> def store(data, name):
> data = data or ''
> }}}
> But File object without a name resolved into False which can produce
> bugs.
> Possibly better to pass a name to File object. To ensure bool(content) is
> True
> {{{
> #!python
> if not hasattr(content, 'chunks'):
> content = File(content, name=name)
> }}}
New description:
https://github.com/django/django/blob/master/django/core/files/storage.py#L50
{{{
#!python
class Storage(object):
def save(self, name, content, max_length=None):
...
if not hasattr(content, 'chunks'):
content = File(content)
}}}
For example if custom storage uses requests with post query.
{{{
#!python
import requests
class CustomStorage(object):
def _save(self, name, content, max_length=None):
requests.post('http://testhost.org/upload', data=content)
}}}
But it will result in empty upload as bool(content) is False.
Possibly better to pass a name to File object. To ensure bool(content) is
True
{{{
#!python
if not hasattr(content, 'chunks'):
content = File(content, name=name)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:2>
Django
False.
Reporter: m-novikov | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Thanks for updating the description. It's still not entirely clear to me
where the problem lies or how the proposed change will fix it. Any chance
you could offer a tested patch? Otherwise, I'm not too sure how to proceed
here.
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:3>
Django
written to file
-------------------------------------+-------------------------------------
Reporter: m-novikov | Owner: nobody
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by m-novikov:
Old description:
> https://github.com/django/django/blob/master/django/core/files/storage.py#L50
> {{{
> #!python
> class Storage(object):
>
> def save(self, name, content, max_length=None):
> ...
> if not hasattr(content, 'chunks'):
> content = File(content)
> }}}
> For example if custom storage uses requests with post query.
> {{{
> #!python
> import requests
> class CustomStorage(object):
> def _save(self, name, content, max_length=None):
> requests.post('http://testhost.org/upload', data=content)
> }}}
> But it will result in empty upload as bool(content) is False.
> Possibly better to pass a name to File object. To ensure bool(content) is
> True
> {{{
> #!python
> if not hasattr(content, 'chunks'):
> content = File(content, name=name)
> }}}
New description:
https://github.com/django/django/blob/master/django/core/files/storage.py#L50
{{{
#!python
class Storage(object):
def save(self, name, content, max_length=None):
...
if not hasattr(content, 'chunks'):
content = File(content)
}}}
For example if custom storage uses requests with post query and we are
saving StringIO object.
{{{
#!python
from StringIO import StringIO
from django.core.files.storage import Storage
import requests
class CustomStorage(Storage):
def _save(self, name, content, max_length=None):
requests.post('http://testhost.org/upload', data=content)
custom_storage = CustomStorage()
custom_storage.save('new_name', StringIO('content'))
}}}
But it will result in empty upload as bool(content) is False.
Possibly better to pass a name to File object. To ensure bool(content) is
True
{{{
#!python
if not hasattr(content, 'chunks'):
content = File(content, name=name)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:4>
Django
written to file
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by m-novikov):
Patch https://github.com/django/django/pull/6448
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:5>
Django
written to file
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
uploads/storage |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by m-novikov):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:6>
Django
written to file
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: File | Version: master
uploads/storage |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Ready for checkin
Comment:
It's a bit odd to me that `bool(file)` is based on the file's name, but in
light of that, I guess this makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:7>
Django
written to file
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: File | Version: master
uploads/storage |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"4d1c229ee5cb210e8b592a8d9c87d4a66864328e" 4d1c229e]:
{{{
#!CommitTicketReference repository=""
revision="4d1c229ee5cb210e8b592a8d9c87d4a66864328e"
Fixed #26495 -- Added name arg to Storage.save()'s File wrapping.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26495#comment:8>