Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gzip support to FileSender #1426

Merged
merged 1 commit into from
Dec 7, 2016
Merged

Add gzip support to FileSender #1426

merged 1 commit into from
Dec 7, 2016

Conversation

balloob
Copy link
Contributor

@balloob balloob commented Nov 24, 2016

What do these changes do?

When a FileSender object will send a file, it will now check if the request supports gzip and if a gzipped version of the file exists (filepath + '.gz'). If both checks pass, it will send the gzipped version to the client.

This is something that we have been using in Home Assistant since we embraced aiohttp. I did not include tests/updated docs yet as I first wanted to know if it's something that would be accepted.

Are there changes in behavior for the user?

As an end user, you are now able to add gzipped versions of your files served through aiohttp.web.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

@codecov-io
Copy link

codecov-io commented Nov 24, 2016

Current coverage is 98.83% (diff: 100%)

Merging #1426 into master will increase coverage by 0.01%

@@             master      #1426   diff @@
==========================================
  Files            30         30          
  Lines          6931       6939     +8   
  Methods           0          0          
  Messages          0          0          
  Branches       1146       1149     +3   
==========================================
+ Hits           6849       6858     +9   
  Misses           40         40          
+ Partials         42         41     -1   

Powered by Codecov. Last update 15e71a0...13c7b70

@asvetlov
Copy link
Member

Relying on filename with .gz extension is too tricky for my feels.

If you really need it -- create a route with desired behavior in your code.

@balloob
Copy link
Contributor Author

balloob commented Nov 28, 2016

I think that it is a pretty common approach with servers.

NGINX gzip compression works like this and also Apache has support for this.

@Kentzo
Copy link

Kentzo commented Nov 28, 2016

@asvetlov maybe this project needs some kind of repo with various recipes for pluggable things like root? Would address certain amount of PR and provide a ground for testing features to be considered for later integration.

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2016

Hmm. Didn't know about NGINX option.

I'm ok with the PR but new functionality should be test covered and documented.

@asvetlov
Copy link
Member

asvetlov commented Dec 1, 2016

@Kentzo I don't follow what repo you are talking about.

@balloob
Copy link
Contributor Author

balloob commented Dec 1, 2016

Cool. Will add some tests and docs soon.

@balloob
Copy link
Contributor Author

balloob commented Dec 3, 2016

@asvetlov I have updated the PR with tests and docs. It is ready for review.

@balloob
Copy link
Contributor Author

balloob commented Dec 3, 2016

It looks like the test failures are in code unrelated to my change. Both are related to url component encoding. Could it be due to the Yarl 0.8.1 release?

** test_proxy_http_raw_path
>       assert proxy.request.path_qs == raw_url
E       assert 'http://aioht...eep?q=can:fly' == 'http://aiohtt...p?q=can%3Afly'
E         - http://aiohttp.io:2561/space%20sheep?q=can:fly
E         ?                                           ^
E         + http://aiohttp.io:2561/space%20sheep?q=can%3Afly
E         ? 
tests/test_proxy_functional.py:87: AssertionError
** test_str_params
  File "/home/travis/build/KeepSafe/aiohttp/tests/test_client_functional.py", line 340, in handler
    assert 'q=t+est' in request.query_string
AssertionError: assert 'q=t+est' in 'q=t est'
 +  where 'q=t est' = <Request GET / >.query_string

@asvetlov
Copy link
Member

asvetlov commented Dec 3, 2016

@balloob most likely yes.
I've restarted jobs, hope they are will be green

@balloob
Copy link
Contributor Author

balloob commented Dec 7, 2016

Rebased on the latest master

@balloob
Copy link
Contributor Author

balloob commented Dec 7, 2016

All tests pass 💃

@asvetlov asvetlov merged commit 103be5d into aio-libs:master Dec 7, 2016
@asvetlov
Copy link
Member

asvetlov commented Dec 7, 2016

Thanks!

@andrey-git
Copy link

@asvetlov would you be willing to accept a similar PR for Brotli format?

@fafhrd91
Copy link
Member

@andrey-git I think payload writer should support br first

@fafhrd91
Copy link
Member

btw I'd just add it everywhere.

@asvetlov
Copy link
Member

btw I'd just add it everywhere.

+1

@andrey-git
Copy link

Could you point me to where, except FileSender, support should be added?

Turns out that mimetypes.guess_type doesn't recognize .br. Maybe just hardcode it? (Sounds ugly, any other ideas?)

@asvetlov
Copy link
Member

It's not MIME type but Content-Encoding header.

Explained in #2518

@lock
Copy link

lock bot commented Oct 28, 2019

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

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants