fileresponse content-range header #2847

spcharc · 2018-03-18T19:28:41Z

Feel free to close this request if it is not properly written. I'm still learning how to use github

What do these changes do?

Now aiohttp provides Content-Range header for Range requests
Accept-Ranges:bytes for files
Out-of-range start-range requests now cause HTTP 416 Request Range Not Satisfiable according to rfc7233
for instance, file.size() == 200
but client gives this header request.header['Range'] == 'bytes=99999-'
Out-of-range tail-range requests would not break FileResponse now
for instance, file.size() == 200
but client gives this header request.header['Range'] == 'bytes=-99999'
Two reify(s): (in web_request.py)
```
Request.if_unmodified_since
Request.if_range
```
Why not one function to handle them all (including Request.if_modified_since)?
It seems that reify class uses function.__name__ as the key to store results ...
Their code is just the same with if_modified_since actually
Maybe I should create a _private_func (of course not a reify decorated one) and use that. Copy and paste the same piece of code is not interesting.
Some more test cases. They send requests with If-Range or If-Unmodified-Since now
Besides, some existing test functions get updated. They check Content-Range header now.

Are there changes in behavior for the user?

New headers returned
Out-of-range start-range requests result in HTTP 416
these are behavior changes, nothing more

Related issue number

#2844 However it won't be fully solved, as multipart/byteranges is still not supported.
multipart/byteranges support involves:

changing the behavior of request.http_range function.
It should be able to handle multiple ranges and return a tuple of slices
This will break existing code.
It seems that MultipartWriter in multipart.py need to be updated to handle multipart/byteranges

Checklist

[?] I think the code is well written

I think this question is for you. Everbody considers him/her-self good code writers
I added a few comments though. I think the comments are well written

[Y] Unit tests for the changes exist

Old test cases modified + new ones

[Y] Documentation reflects the changes

Request.if_range and Request.if_unmodified_since are now reflected in Server->reference page

[Y] If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.

I didn't know about this until everything is ready for a pull request. Went back and updated it

[Y] Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

OK. I went back and added 2844.feature
file content:"Provide Content-Range header for Range requests"
actually it's an improvement for an existing feature, not a new one

Question

In web_fileresponse.py, line 170
self._length_check = False
What is this for? I failed to figure out what it is.
_length_check is in StreamResponse(web_response.py). I commented out this line and everything still worked. That's strange
line 170 is returning an http error.
Returning other http errors in FileResponse do not involve setting _length_check to False.

Edit: Question solved. Actually I had made a grep -r _length_check search and found it in web_response.py before I made this PR. But I failed to understand that users could also set the Content-Length header at that time.

Update CHANGES

codecov-io · 2018-03-18T19:57:47Z

Codecov Report

Merging #2847 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2847      +/-   ##
==========================================
+ Coverage   97.97%   97.97%   +<.01%     
==========================================
  Files          39       39              
  Lines        7444     7461      +17     
  Branches     1307     1308       +1     
==========================================
+ Hits         7293     7310      +17     
  Misses         48       48              
  Partials      103      103

Impacted Files	Coverage Δ
aiohttp/web_request.py	`99.69% <100%> (ø)`	⬆️
aiohttp/web_fileresponse.py	`98.01% <100%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f8ea94...be63235. Read the comment docs.

spcharc · 2018-03-18T20:22:22Z

According to codecov, in web_fileresponse.py:
line 260:
if count:
this line is always true so the return line following it does not do anything ... oh no!
I should have noticed it
actually make cov failed to run on my computer. It kept telling me that it requires python version >= 3.5.3 to run but I'm using 3.6.3. It also raised ImportError 'No module async_generator found' but it is already installed. Weird.

spcharc · 2018-03-18T21:17:41Z

Sorry about the commits
Now the coverage would be a little better

asvetlov · 2018-03-19T01:48:28Z

@spcharc thank you for contribution.
I'll review in my first free time window.

spcharc · 2018-03-19T02:20:37Z

Thank you!
Feel free to tell me if there's anything wrong. I'll try fixing it ASAP.

…eturned

spcharc · 2018-03-19T17:03:13Z

Now Content-Range is also provided when HTTP 416 is returned.
file.size() == 200
client request:headers['Range'] = 'bytes=500-'
server response:headers['Content-Range'] == 'bytes */200' <- tell client the actual size/allowed range
It seems that this is necessary as indicated in rfc7233
However I investegated several sites around the web, some belongs to large companies, and found out they usually do not provide Content-Range with HTTP 416. (Neither do many of them support multipart/byteranges. Some return 416 some even return 403 in this case. 403 Forbidden! what a simple solution. I like it.)

No test case change for this commit (577c81a).

…f-unmodified-since

…eturned

…f-unmodified-since

…into fileresp-header

spcharc · 2018-03-20T08:42:56Z

No more changes of this PR beyond this point. ETag not supported yet (seems unnecessary)
Tested multiple kinds of stream video players / browsers / download managers, but none of them sends multi-range requests. multipart/byteranges support seems unnecessary too

asvetlov · 2018-03-20T13:06:33Z

Why not one function to handle them all (including Request.if_modified_since)?
It seems that reify class uses function.name as the key to store results ...
Their code is just the same with if_modified_since actually
Maybe I should create a _private_func (of course not a reify decorated one) and use that. Copy and paste the same piece of code is not interesting.

It's up to you. I'm ok with _private_func for the case (we already have an exception for Content-Type handling.

asvetlov · 2018-03-20T13:07:34Z

I'm ok with not supporting multi-range requests.

asvetlov

Looks good, just small improvements needed.

asvetlov · 2018-03-20T13:13:41Z

aiohttp/web_fileresponse.py

@@ -166,7 +167,12 @@ def __init__(self, path, chunk_size=256*1024, *args, **kwargs):
        modsince = request.if_modified_since
        if modsince is not None and st.st_mtime <= modsince.timestamp():
            self.set_status(HTTPNotModified.status_code)
-            self._length_check = False
+            # self._length_check = False     <- why this line is needed?


See https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_response.py#L340
Looks like is is important for chunked encoding, I don't recall details honestly.
Maybe @fafhrd91 can shed a light?

asvetlov · 2018-03-20T13:21:41Z

docs/web_reference.rst

+      Returns :class:`datetime.datetime` or ``None`` if
+      *If-Unmodified-Since* header is absent or is not a valid
+      HTTP date.
+


Here should be ..versionadded:: 3.1 tag.

asvetlov · 2018-03-20T13:21:59Z

docs/web_reference.rst

+      Returns :class:`datetime.datetime` or ``None`` if
+      *If-Range* header is absent or is not a valid
+      HTTP date.
+


.. versionadded::

spcharc · 2018-03-20T14:34:58Z

Thanks for review.
I'll adding version 3.1 tag right now
for content _length_check, if headers['Content-Length'] is set, the flag deletes it. because returning 304 should not contain any body
but content-length header is set after that line. (line 269 in web_fileresponse.py)

spcharc · 2018-03-20T14:42:29Z

I suddenly realized that content-length could be set by a user!
If you do not use add_static('/static_path/', static_folder) but directly return web.FileResponse(path, headers={'Content-Length':'100'}), this would cause problem.
You are right. I'll modify it

…p_date method for BaseRequest

…into fileresp-header

asvetlov · 2018-03-21T01:08:51Z

@spcharc thank you, very high quality work!

lock · 2019-10-28T07:04:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

spcharc added 3 commits March 19, 2018 02:06

Provide Content-Range header for Range requests

5b6c379

Upload

4742616

Update CHANGES

Update Contributors.txt

acbf7e6

spcharc changed the title ~~Fileresp header~~ fileresponse content-range header Mar 18, 2018

spcharc added 3 commits March 19, 2018 04:42

Coverage improvements?

e962b05

Remove Assert

f76cf86

Remove an empty file add by mistake

41bdf8c

spcharc added 2 commits March 19, 2018 20:31

fix an error in test

e751ebe

Added an probably unnecessay header: Content-Range when HTTP 416 is r…

577c81a

…eturned

spcharc added 11 commits March 20, 2018 01:34

Document Server->Reference changed for Request.if-range and Request.i…

278b388

…f-unmodified-since

Provide Content-Range header for Range requests

c6ba2e7

Update Contributors.txt

fe0c171

Coverage improvements?

4d10219

Remove Assert

c258bdb

Remove an empty file add by mistake

9e2960e

fix an error in test

f3eeaca

Added an probably unnecessay header: Content-Range when HTTP 416 is r…

b18bef4

…eturned

Document Server->Reference changed for Request.if-range and Request.i…

127309a

…f-unmodified-since

Merge branch 'fileresp-header' of https://github.com/spcharc/aiohttp …

9a9dea3

…into fileresp-header

Merge branch 'master' into fileresp-header

4d3e806

asvetlov requested changes Mar 20, 2018

View reviewed changes

spcharc added 3 commits March 20, 2018 22:42

Merge branch 'master' into fileresp-header

f04aac7

Added New in Version 3.1 tag. Solved the question in PR. Added a _htt…

e2c71b1

…p_date method for BaseRequest

Merge branch 'fileresp-header' of https://github.com/spcharc/aiohttp …

be63235

…into fileresp-header

asvetlov approved these changes Mar 21, 2018

View reviewed changes

asvetlov merged commit 037ccbc into aio-libs:master Mar 21, 2018

asvetlov mentioned this pull request Mar 21, 2018

FileResponse failed to prepare proper headers for Range requests #2844

Closed

spcharc deleted the fileresp-header branch March 21, 2018 02:00

This was referenced Mar 22, 2018

Bump aiohttp from 3.0.6 to 3.1.0 djetelina/aiocrypto_prices#16

Merged

Bump aiohttp from 2.3.9 to 3.1.0 andriyko/aiociscospark#34

Closed

Bump aiohttp from 3.0.5 to 3.1.0 thalesmorandi/MeguminBot#11

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fileresponse content-range header #2847

fileresponse content-range header #2847

spcharc commented Mar 18, 2018 •

edited

Loading

codecov-io commented Mar 18, 2018 •

edited

Loading

spcharc commented Mar 18, 2018

spcharc commented Mar 18, 2018

asvetlov commented Mar 19, 2018

spcharc commented Mar 19, 2018

spcharc commented Mar 19, 2018 •

edited

Loading

spcharc commented Mar 20, 2018

asvetlov commented Mar 20, 2018

asvetlov commented Mar 20, 2018 •

edited

Loading

asvetlov left a comment

asvetlov Mar 20, 2018

asvetlov Mar 20, 2018

asvetlov Mar 20, 2018

spcharc commented Mar 20, 2018

spcharc commented Mar 20, 2018

asvetlov commented Mar 21, 2018

lock bot commented Oct 28, 2019

fileresponse content-range header #2847

fileresponse content-range header #2847

Conversation

spcharc commented Mar 18, 2018 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Question

codecov-io commented Mar 18, 2018 • edited Loading

Codecov Report

spcharc commented Mar 18, 2018

spcharc commented Mar 18, 2018

asvetlov commented Mar 19, 2018

spcharc commented Mar 19, 2018

spcharc commented Mar 19, 2018 • edited Loading

spcharc commented Mar 20, 2018

asvetlov commented Mar 20, 2018

asvetlov commented Mar 20, 2018 • edited Loading

asvetlov left a comment

Choose a reason for hiding this comment

asvetlov Mar 20, 2018

Choose a reason for hiding this comment

asvetlov Mar 20, 2018

Choose a reason for hiding this comment

asvetlov Mar 20, 2018

Choose a reason for hiding this comment

spcharc commented Mar 20, 2018

spcharc commented Mar 20, 2018

asvetlov commented Mar 21, 2018

lock bot commented Oct 28, 2019

spcharc commented Mar 18, 2018 •

edited

Loading

codecov-io commented Mar 18, 2018 •

edited

Loading

spcharc commented Mar 19, 2018 •

edited

Loading

asvetlov commented Mar 20, 2018 •

edited

Loading