Fixed #27150 -- Made base File objects truthy by default.#20327
Fixed #27150 -- Made base File objects truthy by default.#20327sarahboyce merged 1 commit intodjango:mainfrom
Conversation
5f83965 to
b6ede76
Compare
|
I had a look at this ticket, and I think the docs approach is more of a bandaid, I've updated the ticket with my investigation and recommendation for a code cleanup rather than documenting the odd behaviour. I've moved the ticket to unreviewed, so a triage team member can weigh in in time. The doc change you came up with in itself is good, but I think a more thorogh approach is better. If the triagers do agree that a code change is the right approach feel free to be the one to submit that PR if you are still interested. |
|
That ticket has been triaged now based on the investigation, if you pick it back up I'll rereview |
07c12a6 to
fc9c9fb
Compare
fc9c9fb to
f83ed76
Compare
|
Many thanks to James for taking a deep dive into this, and to Jacob for his thoughtful insights as always. The PR is ready for another review, please let me know if any changes are needed 👍 |
f83ed76 to
54608c5
Compare
| File uploads/storage | ||
| -------------------- |
There was a problem hiding this comment.
I'm not an expert in this area, but I've had a look at previous release note docs, and I don't see headings introduced like this very often. Far more typical to see it in the Miscellaneous section below. I think I'd put it there, but at this stage you are more experience contributor to django than me. So just my 2 cents.
There was a problem hiding this comment.
TBH I think it's the right thing to do, having an entire dedicated subheading for a single point doesn't make much sense. Also since it's a really old ticket adding it to miscellaneous would be better since most users wouldn't be affected by this change, given that it's coming from Django 1.10.
54608c5 to
e0200c1
Compare
blighj
left a comment
There was a problem hiding this comment.
I think this is as good as my reviews can make it now. The final wording may well get changed slightly by a merger, but anything I might suggest is as likely to be changed.
sarahboyce
left a comment
There was a problem hiding this comment.
Thank you for the work and reviews on this ticket 💜
As we have a test class files.tests.NoNameFileTestCase, I was wondering if we would like to add a few explicit test cases such as:
--- a/tests/files/tests.py
+++ b/tests/files/tests.py
@@ -233,6 +233,15 @@ class NoNameFileTestCase(unittest.TestCase):
def test_noname_file_get_size(self):
self.assertEqual(File(BytesIO(b"A file with no name")).size, 19)
+ def test_noname_bool(self):
+ self.assertTrue(bool(File(BytesIO(b"A file with no name"))))
+
+ def test_noname_repr(self):
+ self.assertEqual(repr(File(BytesIO(b"A file with no name"))), "<File: None>")
+
+ def test_noname_bool_uploaded(self):
+ self.assertFalse(bool(UploadedFile(file=BytesIO(b"A file with no name"))))
+
class ContentFileTestCase(unittest.TestCase):What do we think? Perhaps just the test test_noname_bool adds the behavior change explicitly to the test suite?
| * The base :class:`~django.core.files.File` class now evaluates to | ||
| ``True`` by default, rather than relying on the file's ``name`` attribute. | ||
| Subclasses like :class:`~django.db.models.fields.files.FieldFile` and | ||
| :class:`~django.core.files.uploadedfile.UploadedFile` retain their | ||
| previous behavior of evaluating based on this attribute. |
There was a problem hiding this comment.
My only suggestion here is perhaps "Subclasses like" needs to be more precise as not all subclasses retain the previous behavior (such as ContentFile). This also doesn't currently list all built-in subclasses which retain the previous behavior. Maybe:
| * The base :class:`~django.core.files.File` class now evaluates to | |
| ``True`` by default, rather than relying on the file's ``name`` attribute. | |
| Subclasses like :class:`~django.db.models.fields.files.FieldFile` and | |
| :class:`~django.core.files.uploadedfile.UploadedFile` retain their | |
| previous behavior of evaluating based on this attribute. | |
| * The :class:`~django.core.files.File` class now always evaluates to ``True`` | |
| in boolean contexts, rather than relying on the ``name`` attribute. The | |
| built-in subclasses ``FieldFile``, ``UploadedFile``, | |
| ``TemporaryUploadedFile``, ``InMemoryUploadedFile``, and | |
| ``SimpleUploadedFile`` retain the previous behavior of evaluating based on | |
| the ``name`` attribute. |
e0200c1 to
8b67987
Compare
8b67987 to
90786db
Compare
blighj
left a comment
There was a problem hiding this comment.
Those changes look good. Adding just the one extra suggested test, test_noname_bool seems fair and the wording proposed by Sarah is added and reads well.
Trac ticket number
ticket-27150
Branch description
Originally, this PR proposed a documentation update. However, following the consensus reached here.
The base File class previously fell back to
__len__ for booleanevaluation because it lacked a__bool__ method. This caused thin wrappers around valid but nameless or empty file-like objects to unexpectedly evaluate to False.This PR makes the base File class true by default to fix the buggy boolean evaluation of nameless file wrappers like
BytesIO. It strictly preserves the existing name-based evaluation logic inFieldFileandUploadedFileto protect framework behavior, and includes a backward-incompatible release note for Django 6.1.Checklist
mainbranch.