#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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019d1fcbea3d-3810585c-f692-4838-b173-de2d7249d7fe-000000%40eu-central-1.amazonses.com.
