[Django] #27150: Wrong File object behavior if file object has no name attribute

[Django]

#27150: Wrong File object behavior if file object has no name attribute
------------------------------+--------------------
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]

#27150: Wrong File object behavior if file object has no name attribute

------------------------------+--------------------------------------

Reporter: pacahon | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.10

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 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]

#27150: Wrong File object behavior if file object has no name attribute

------------------------------+--------------------------------------

Reporter: pacahon | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 1.10

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

------------------------------+--------------------------------------

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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Needs tests: 0 | Patch needs improvement: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Needs tests: 0 | Patch needs improvement: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Needs tests: 0 | Patch needs improvement: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Needs tests: 0 | Patch needs improvement: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Needs tests: 0 | Patch needs improvement: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 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]

#27150: Document that a name should be provided when wrapping file-like objects
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

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by VIZZARD-X):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:9>

[Django]

#27150: Document that a name should be provided when wrapping file-like objects
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
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Comment (by Vijaya Lakshmi Pokala):

Hi, I'd like to work on this ticket and submit a patch soon
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:10>

[Django]

#27150: Document that a name should be provided when wrapping file-like objects
that don't have one with File

-------------------------------------+-------------------------------------

Reporter: Sergei Zh | Owner: nobody

Type: | Status: new
Cleanup/optimization |

Component: Documentation | Version: 1.10
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by blighj):

* stage: Accepted => Unreviewed

Comment:

A 10 year old ticket that never really reached consensus may not be a good
ticket to pick up and introduce new wording to the docs.

Reading between the lines it feels like this is one where someone needs to
do a bit of work investigating deeper. Defining a base File class and then
giving it a `__bool__` method that relies on a name seems like a break in
the abstraction.

I amended the `__bool__` call to log its usage and ran the test suite, and
then did some searching to see what other cases I could see. I think this
is pretty comprehensive. I've given the intent of the calling code, why
does it want to know if the file is true, and what type of file is it
working with currently.

|| '''Location''' || '''Calls''' || '''Intent''' || '''Instance(s)''' ||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L336
fields/files.py:336] `pre_save` || 847 || Skip committing the file if the
field is empty (name is the DB value; empty name means nothing to save) ||
FieldFile ||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L42
fields/files.py:42] `_require_file` || 85 || Guard: raise `ValueError` if
no file is associated with this field before attempting file operations ||
FieldFile ||
||
[https://github.com/django/django/blob/main/django/http/multipartparser.py#L374
http/multipartparser.py:374] `handle_file_complete` || 59 || Check whether
an upload handler returned a completed file object (vs `None` meaning not
yet done) || InMemoryUploadedFile, TemporaryUploadedFile ||
|| [https://github.com/django/django/blob/main/django/forms/fields.py#L696
forms/fields.py:696] `FileField.clean` || 37 || Fall back to the initial
value if no new file was uploaded || FieldFile, SimpleUploadedFile ||
||
[https://github.com/django/django/blob/main/django/forms/widgets.py#L552
forms/widgets.py:552] `ClearableFileInput.is_initial` || 24 || Determine
whether the current value is an existing stored file (to decide whether to
render the "currently" link) || FieldFile, SimpleUploadedFile ||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L368
fields/files.py:368] `save_form_data` || 11 || Use empty string if no file
was provided (`data or ""`) to avoid storing `False` in the DB column ||
FieldFile, SimpleUploadedFile ||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L109
fields/files.py:109] `delete` || 9 || Early return if no file is
associated with the field — nothing to delete || FieldFile ||
||
[https://github.com/django/django/blob/main/django/forms/widgets.py#L521
forms/widgets.py:521] `FileInput.use_required_attribute` || 5 || Suppress
the HTML `required` attribute if a file is already set as the initial
value || FieldFile ||
||
[https://github.com/django/django/blob/main/django/forms/widgets.py#L585
forms/widgets.py:585] `ClearableFileInput.value_from_datadict` || 2 ||
Detect contradiction: user uploaded a new file AND checked the clear
checkbox simultaneously || SimpleUploadedFile ||
||
[https://github.com/django/django/blob/main/django/contrib/admin/utils.py#L463
contrib/admin/utils.py:463] `display_for_field` || 2 || Only render a
hyperlink to the file if a file is actually set on the field || FieldFile
||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L500
fields/files.py:500] `update_dimension_fields` || 0 || Skip reading image
dimensions if no file is associated with the ImageField || FieldFile ||
||
[https://github.com/django/django/blob/main/django/db/models/fields/files.py#L518
fields/files.py:518] `update_dimension_fields` || 0 || Read width/height
from the image if a file is set; clear dimension fields to `None`
otherwise || FieldFile ||
||
[https://github.com/django/django/blob/main/django/core/files/base.py#L23
core/files/base.py:23] `File.__repr__` || 0 || Presentational: display the
file's name in repr output, or `"None"` if no name is set ||

In reality the cases are checking either a FieldFile or a subclass of
UploadedFile, in those cases it makes sense to check the name because if
you have no name, then the row in the db is empty or the uploadedfile
didn't work. We want to keep that behaviour.

But the default File looks like a thin wrapper around a python File, and
the docs support this
https://docs.djangoproject.com/en/6.0/ref/files/file/#django.core.files.File
and have examples of constructing a file without a name, which makes sense
for a thin wrapper, but results in a False file
https://docs.djangoproject.com/en/6.0/topics/files/#the-file-object and
you get bugs like #26495

So what I think we should do is have `__bool__` on File return True, which
would make sense for a wrapper class, files are kind of truthy by default
in python, it's what ContentFile does already. But then add `__bool__`
methods for FieldFile and UploadedFile that rely on the name. This
preserves the behaviour that most of the code is working with and makes
sense for those classes, no name = not a file = no need to check the
underlying file object.

That leaves that `__repr__` method, which should probably be updated to
`return "<%s: %s>" % (self.__class__.__name__, self.name or "None")`

The docs don't really then need updating at all, you could make a case for
documenting the `__bool__` behaviour of FieldFile and UploadedFile but I
think you'd be fine without it.

I think we should close and pause the current PR and instead move this
ticket back to unreviewed, so a member of the triage team can have a think
about it.

----
'''*''' The intent here is genuinely `is not None`, the upload handler API
[https://docs.djangoproject.com/en/6.0/ref/files/uploads/#django.core.files.uploadhandler.FileUploadHandler.file_complete
documents that handlers return None to signal "not done yet"]. This case
works incidentally, and would continue to work with the `__bool__`
changes. Maybe that line could be if `file_obj is not None:` as a
separate/additional cleanup.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:11>

[Django]

#27150: Wrong File object behavior if file object has no name attribute

-------------------------------------+-------------------------------------
Reporter: Sergei Zh | Owner: nobody
Type: | Status: new
Cleanup/optimization |

Component: Core (Other) | Version: 1.10

Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

-------------------------------------+-------------------------------------
Changes (by blighj):

* component: Documentation => Core (Other)
* summary:

Document that a name should be provided when wrapping file-like
objects that don't have one with File

=> Wrong File object behavior if file object has no name attribute

--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:12>

[Django]

#27150: Wrong File object behavior if file object has no name attribute

--------------------------------------+------------------------------------

Reporter: Sergei Zh | Owner: nobody

Type: Cleanup/optimization | Status: new
Component: File uploads/storage | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Jacob Walls):

* component: Core (Other) => File uploads/storage

* stage: Unreviewed => Accepted

Comment:

Marvelous work, James. I find your proposal in comment:11 convincing. I'm
not certain whether we should document the change as a minor backward-
incompatible change, on account of an application subclassing the thin
`File` wrapper. We would be changing the semantic.
--
Ticket URL: <https://code.djangoproject.com/ticket/27150#comment:13>