[Django] #30509: Various FileResponse fixes and changes
Django
-------------------------------------+-------------------------------------
Reporter: Piotr | Owner: nobody
Kunicki |
Type: | Status: new
Cleanup/optimization |
Component: HTTP | Version: master
handling |
Severity: Normal | Keywords: FileResponse
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
As I've mentioned in PR 11011 (of ticket #30196) I noticed a couple of
issues with the way FileResponse is created (and other minor things), and
in this ticket I'll try to list and fix them.
For initial clarity, this PR consists of 2 commits. The first is a set of
tests, most of which show invalid behavior in some quite specific cases
based on current code. The second contains my initial proposed fixes.
1. tests:
a. {{{test_file_from_buffer_absolute_name}}}:\\
test ends in an error, because {{{set_headers()}}} attempts to read the
'name' property if the filename (which can be overwritten by the custom
filename) is recognized as an absolute path by os.path.
b. {{{test_file_from_buffer_explicit_type1}}},
{{{test_file_from_buffer_explicit_type2}}}:\\
of these 2, first test shows the way to explicitly set {{{Content-Type}}}
header of the response, while the second shows that in a case that the set
value matches the beginning of the default value ({{{'text/html;
charset=%s' % self.charset}}}), it will consider that value as unset and
proceed to read the content type from the file.
c. {{{test_file_from_named_buffer1}}},
{{{test_file_from_named_buffer2}}}:\\
tests show how you can easily 'fool' FileResponse into thinking it's
dealing with an actual file, resulting in incorrect header values if such
file exists (1) or an error if it doesn't (2).
d. {{{test_file_from_disk_custom_name}}},
{{{test_file_from_disk_as_attachment_custom_name}}}:\\
these are just 2 missing tests to check whether giving a file a custom
name even works.
2. fixes:
a. ignored custom filename before setting {{{Content-Length}}} and
changed the following condition a bit to fix 1.a. and the error case of
1.c.
b. added a {{{self._default_content_type}}} flag to
{{{HttpResponseBase.__init__()}}} to mark that there was no
{{{content_type}}} explicitly set, so it should be figured out from the
file, if possible, to fix 1.b.
c.
* added a way to tell the filelike object's size if it's neither a file
nor has a {{{getbuffer}}} attribute, using {{{.seek()}}} and {{{.tell()}}}
if they're available, and otherwise {{{.read()}}}
* moved {{{os.path.getsize}}} path to be the worst-case-scenario, to
avoid reading the same file from disk again when possible
* that fixes the remaining problem found in 1.c..
d. moved {{{encoding_map}}} to initialize only when it's necessary.
e. changed {{{set_headers()}}} to {{{_set_headers()}}} to mark it as an
internal function.
f. simplified the - IMO - unnecessarily detailed docs of FileResponse to
just tell the user what do the parameters cause, instead of (almost)
explaining what each line of code does - if the user wants to know which
headers get what kind of values under which conditions they can just check
the code, Python is nearly pseudocode anyway.
One thing i'm not sure about is if FileResponse should support reading
filelikes from a different position than the beginning. If so, getting
{{{Content-Length}}} would require a tiny change to accomodate a non-zero
starting point, while otherwise it could use a {{{filelike.seek(0)}}}
somewhere to be sure we're reading it from the very beginning.
--
Ticket URL: <https://code.djangoproject.com/ticket/30509>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
Hi Piotr.
Thank you for the follow-up and the PR. The test cases look correct, so
lets accept this.
For interest, how do some of these cases come up in the wild?
1. When would you have a buffer passing an ''absolute'' value for
`filename`?
2. Equally, when would you have a buffer with an ''absolute'' `name`,
either pointing to an existing file or not, that wasn't e.g. a named pipe
itself?
My imagination is lacking slightly just now as to how you'd hit these
issues. :)
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:1>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* cc: Carlton Gibson (added)
* needs_better_patch: 0 => 1
Comment:
Left a few comments on the PR. Just ticking Patch needs improvement for
now. Please uncheck when it's ready for another look.
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:2>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Piotr Kunicki):
* needs_better_patch: 1 => 0
Comment:
1. Well, let's say you want to return a dynamically modified file, that
you have an absolute path to - you open and edit it using Pillow or
something similar, write result to a buffer, and then pass that buffer as
the {{{filelike}}}, with the path as the {{{filename}}}. Sure, you could
get the {{{basename}}} from the path and pass it instead, but... well now
you don't have to.
2. I'm not sure what you mean here, as the named pipe's {{{name}}} isn't
an absolute path, but a file descriptor integer.\\
However, regarding why would anyone pass an e.g. {{{BytesIO}}} buffer
with a {{{name}}} set - i'm not sure (alternative way to pass the filename
to the response?), but the point there was mostly to not assume that a
{{{name}}} attribute means it's a file (unless it's the last option to get
the {{{Content-Length}}} and such file actually exists?).
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:3>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:4>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Piotr Kunicki):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:5>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:6>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Piotr Kunicki):
* needs_better_patch: 1 => 0
Comment:
Sorry if i wasn't supposed to do it yet, but to make it more visible that
i did something...
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:7>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:8>
Django
Reporter: Piotr Kunicki | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: FileResponse | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"3ac476439751334138edcbb9abd73f783afcec4c" 3ac47643]:
{{{
#!CommitTicketReference repository=""
revision="3ac476439751334138edcbb9abd73f783afcec4c"
Refs #30509 -- Increased FileResponse test coverage.
Split tests by behavior, e.g. header, and added additional tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:11>
Django
-------------------------------------+-------------------------------------
Cleanup/optimization |
Component: HTTP handling | Version: dev
Keywords: FileResponse | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"cb8d7ca0ba0adc4fcefc069eae26f842a15755a1" cb8d7ca]:
{{{
#!CommitTicketReference repository=""
revision="cb8d7ca0ba0adc4fcefc069eae26f842a15755a1"
Refs #30509 -- Adjusted FileResponse test to close file earlier.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: HTTP handling | Version: dev
Severity: Normal | Resolution: fixed
Keywords: FileResponse | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"dc724c5bf9d3b8d59c9571aa751c3cd001cdeced" dc724c5]:
{{{
#!CommitTicketReference repository=""
revision="dc724c5bf9d3b8d59c9571aa751c3cd001cdeced"
Fixed #30509 -- Made FileResponse better handle buffers and non-zero file
offsets.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: HTTP handling | Version: dev
Severity: Normal | Resolution: fixed
Keywords: FileResponse | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"4a58dfd9dbbc1c62945e1cfcfe2b7da04b035ca0" 4a58dfd]:
{{{
#!CommitTicketReference repository=""
revision="4a58dfd9dbbc1c62945e1cfcfe2b7da04b035ca0"
Refs #30509 -- Adjusted internal FileResponse variable name.
Follow up to dc724c5bf9d3b8d59c9571aa751c3cd001cdeced.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30509#comment:14>