[Django] #27150: Wrong File object behavior if file object has no name attribute
Django
------------------------------+--------------------
Reporter: pacahon | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
------------------------------+--------------------
[https://docs.djangoproject.com/en/1.10/ref/files/file/#the-file-class
django.core.files.base.File] supports file-like objects (synonym to file
object in python glossary) and in fact can't rely on additional `name`
attribute, but in current implementation of `__bool__` method it does:
{{{
def __bool__(self):
return bool(self.name)
}}}
For example, until `tempfile.SpooledTemporaryFile` isn't large enough to
have been written to disk, his `name` attribute is `None` and `if`
statement will return False.
{{{
import tempfile
from django.core.files.base import File
python_file_like_object = tempfile.SpooledTemporaryFile()
file_obj = File(python_file_like_object) # name keyword is None by
default
if file_object: # Falsy, because python_file_like_object.name is None
print("42")
}}}
The same behavior with streams (BytesIO, StringIO).
Maybe `__bool__` method should rely on `self.file` attribute to avoid this
behavior?
--
Ticket URL: <https://code.djangoproject.com/ticket/27150>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: pacahon | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.10
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 bmispelon):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
Hi,
As far as I can tell, `File.__bool__` is not documented but it does seem
to be used internally and is somewhat tested (though maybe not directly):
changing it to `return False` yields quite a few errors and failures in
the test suite.
Because of this lack of documentation, it's hard to tell if the behavior
you're describing is a bug.
I think it'd be worth it to:
1) Document `File.__bool__` (both in the reference docs and in the
docstring)
2) Figure out if we can improve it by handling underlying files with no
names as you suggest.
What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:1>
Django
Reporter: pacahon | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by pacahon):
Hi.
Sure, it makes sense. I'll try to investigate failed tests ASAP. Think
it's the best place to begin.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:2>
Django
that don't have one with File
--------------------------------------+------------------------------------
Reporter: pacahon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Core (Other) => Documentation
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
See #26495 for a case where this came up. Unless investigation yields
otherwise, I think documenting that you should pass a name for file-like
objects that don't have one is the best course of action. I'm not sure if
raising an error for that case might be acceptable (there might be use
cases where providing `name` isn't needed) but if so, they would likely be
even more helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:3>
Django
that don't have one with File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by pacahon):
As I understood from #26495 python file-like objects works fine with
`requests` library, because in CPython file objects use `object.__bool__`
and always returns True if no `__bool__` and `__len__`.
> I think documenting that you should pass a name for file-like objects
that don't have one is the best course of action.
It's little bit strange for me to allow pass `name=None` to
`Storage.save`, while disallow use empty name for file-like objects. Maybe
we can restore default `__bool__` behavior in `Storage.save` method?
{{{
content.__bool__ = lambda _: True
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:4>
Django
that don't have one with File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by pacahon):
I've realized it's wrong, we should delete `__bool__` method, cause
default implementation depends on `__len__` method.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:5>
Django
that don't have one with File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I haven't looked into the issue so I can't propose an alternative,
however, I'm not sure that patching methods like that is a good practice.
At least, I'm not aware of anywhere else in Django that uses that pattern.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:6>
Django
that don't have one with File
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by pacahon):
You are right, I guess the main reason - it doesn't work :)
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:7>
Django
that don't have one with File
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Comment (by VIZZARD-X):
PR submitted: https://github.com/django/django/pull/20327
Updated the documentation to clarify that wrapping file-like objects
without a `name` attribute requires explicitly providing one. The docs
now build cleanly, including lint and spelling checks.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:8>
Django
that don't have one with File
--------------------------------------+------------------------------------
Reporter: Sergei Zh | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Changes (by VIZZARD-X):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:9>