Fix response headers for compressed file requests

[steverep]

What do these changes do?

Currently the server responds to requests for compressed files with a Content-Encoding header based on the extension. This is not correct as the encoding is supposed to be a transformation applied to the content type, and the type should be a representation of the request (i.e. if the request is for a compressed file, then the type should represent a compressed file). The reasoning is further explained in #4462 (comment).

Note this also addresses problems when non-standard or unsupported encodings are involved. The server will currently respond to "/hello.txt.bz2" with encoding "bzip2" to an aiohttp client, despite the fact that it's not supported or sent in the Accept-Encoding header. That header check is completely bypassed ATM.

Are there changes in behavior for the user?

Requests for static compressed files will no longer be decompressed by most clients (when the encoding is supported). However this is not a server behavior change so much as an expected reaction to the server correction.

Is it a substantial burden for the maintainers to support this?

No

Related issue number

Fixes #4462 and helps advance #8104 (by eliminating some ignorance of Accept-Encoding)

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• 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.
• Add a new news fragment into the CHANGES/ folder

  • name it <issue_or_pr_num>.<type>.rst (e.g. 588.bugfix.rst)

  • if you don't have an issue number, change it to the pull request
    number after creating the PR

    • .bugfix: A bug fix for something the maintainers deemed an
      improper undesired behavior that got corrected to match
      pre-agreed expectations.
    • .feature: A new behavior, public APIs. That sort of stuff.
    • .deprecation: A declaration of future API removals and breaking
      changes in behavior.
    • .breaking: When something public is removed in a breaking way.
      Could be deprecated in an earlier release.
    • .doc: Notable updates to the documentation structure or build
      process.
    • .packaging: Notes for downstreams about unobvious side effects
      and tooling. Changes in the test invocation considerations and
      runtime assumptions.
    • .contrib: Stuff that affects the contributor experience. e.g.
      Running tests, building the docs, setting up the development
      environment.
    • .misc: Changes that are hard to assign to any of the above
      categories.

  • Make sure to use full sentences with correct case and punctuation,
    for example:

    Fixed issue with non-ascii contents in doctest text files
    -- by :user:`contributor-gh-handle`.

    Use the past tense or the present tense a non-imperative mood,
    referring to what's changed compared to the last released version
    of this project.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.61%. Comparing base (98eec45) to head (dd78dbb).
Report is 918 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8485      +/-   ##
==========================================
- Coverage   97.61%   97.61%   -0.01%     
==========================================
  Files         107      107              
  Lines       33103    33104       +1     
  Branches     3894     3892       -2     
==========================================
  Hits        32314    32314              
- Misses        570      571       +1     
  Partials      219      219              

Flag	Coverage Δ
CI-GHA	97.52% <100.00%> (-0.01%)	⬇️
OS-Linux	97.19% <100.00%> (-0.01%)	⬇️
OS-Windows	95.64% <100.00%> (+<0.01%)	⬆️
OS-macOS	96.85% <93.75%> (-0.01%)	⬇️
Py-3.10.11	97.00% <93.75%> (-0.01%)	⬇️
Py-3.10.14	96.95% <93.75%> (-0.01%)	⬇️
Py-3.11.9	97.17% <93.75%> (-0.01%)	⬇️
Py-3.12.4	97.30% <93.75%> (-0.01%)	⬇️
Py-3.8.10	95.41% <100.00%> (+<0.01%)	⬆️
Py-3.8.18	96.84% <100.00%> (-0.01%)	⬇️
Py-3.9.13	96.98% <93.75%> (-0.01%)	⬇️
Py-3.9.19	96.94% <93.75%> (-0.01%)	⬇️
Py-pypy7.3.16	96.51% <93.75%> (-0.01%)	⬇️
VM-macos	96.85% <93.75%> (-0.01%)	⬇️
VM-ubuntu	97.19% <100.00%> (-0.01%)	⬇️
VM-windows	95.64% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Dreamsorcerer]

I believe this looks correct. @bdraco Think you could double check it?

[bdraco]

Will try to take a look soon. Hurricane Beryl is causing tons of problems in Houston right now so may not be today

[steverep]

I added a couple extra sentences to the change notes just in case users need to adjust clients to the probable behavior of no longer decompressing the files.

Also, does this need to backport to 3.9 as well or are patches done for 3.9?

[bdraco]

I don’t think we are doing any more 3.9s so no need to backport

[bdraco]

The code looks good but I still haven’t been able to test it as all my equipment in Houston is on emergency power. Grid power is expected to come back tomorrow

[steverep]

Thanks and no rush. The rain is currently headed my way for the next few days, albeit with much less intensity now, so hopefully I won't be saying the same.

I'm trying to work on #8182 in the meantime, so any advice there would be appreciated.

[bdraco]

Going to give this a test now

[bdraco]

backported to 3.10 for testing as bdraco@db1eaec

[bdraco]

I tested this with a binary file by gziping it and than downloading it. All went ok

When I tested with a .svg (compressed to .svg.gz), it failed to download or render in both Chrome and Firefox

[bdraco]

Downgrading back to 3.9.x and running the same test now

[bdraco]

same problem happens on 3.9

[bdraco]

Tried with a bzip2ed jpeg on 3.9

The content-type that comes back is image/jpeg and the browser renders a broken image

Trying with this patch now

[bdraco]

Tried with a bzip2ed jpeg on 3.10 with this patch

The content-type that comes back is image/jpeg and the browser renders a broken image

The behavior is the same as 3.9 baseline

[steverep]

If you request "file.svg.gz", then I would not expect it to render since the content type should be "application/gzip". That case is really no different than downloading "file.tar.gz". Images would only render if you request it without the compression extension.

[steverep]

Tried with a bzip2ed jpeg on 3.10 with this patch

The content-type that comes back is image/jpeg and the browser renders a broken image

I'm confused by this one. Was the request with or without the .bz2 extension?

• With: content type should be "application/x-bzip2"
• Without: aiohttp wouldn't serve it since it doesn't look for .bz2

Either way, browser wouldn't render it since they don't support bzip2 encoding.

[bdraco]

I'm confused by this one. Was the request with or without the .bz2 extension?

Request was with the bz2 extension

• With: content type should be "application/x-bzip2"

That was what I had expected as well but it was image/jpeg

[steverep]

That was what I had expected as well but it was image/jpeg

Can you share that test for me to repro?

[bdraco]

• Checkout bdraco@db1eaec
• Install it in a homeassistant container docker exec -it homeassistant /bin/bash
• Copy a jpeg file into /config/www/something.jpeg
• cd /config/www ; bzip2 something.jpeg
• navigate to http://xxxx:2083/local/something.jpeg.bz2

[steverep]

Tests with HA won't work for compressed files because HA incorrectly overrides the content type:

                content_type = (mimetypes.guess_type(rel_url))[
                    0
                ] or "application/octet-stream"

[bdraco]

I'll investigate the issue in HA this weekend (or first thing Monday) and see if I can come up with a solution so I can test this.

[steverep]

I can make a quick fix PR for HA by just removing the code to cache and override the Content-Type. It's just repeating the same code in FileResponse.prepare() that this PR is trying to fix.

But I wasn't going to bother with it because I wanted to fix #8182 too, which may end up rendering much more of that code unnecessary.

[steverep]

HA change to allow this to take effect: home-assistant/core#121831

[steverep]

In addition to @bdraco's suggestion that should improve performance, I also moved the lookup down (after any possible 416 responses) and avoided the string conversion (mimetypes supports PathLike after 3.8).

Another potential performance tweak would be to put the type lookup in parallel with opening the file.

[steverep]

Mypy is complaining add_types() isn't on the class, but it certainly is. 😕

[bdraco]

Mypy is complaining add_types() isn't on the class, but it certainly is. 😕

Its definitely defined and mimetypes.add_type is implemented as a wrapper around MimeTypes.add_type but it is missing from the pyi file. I think its unlikely to change so an ignore is fine.

[bdraco]

LGTM once https://github.com/aio-libs/aiohttp/pull/8485/files#r1676667798 is worked out 👍

Thanks @steverep

[steverep]

FWIW... python/cpython#121658

[Dreamsorcerer]

Mypy is complaining add_types() isn't on the class, but it certainly is. 😕

Its definitely defined and mimetypes.add_type is implemented as a wrapper around MimeTypes.add_type but it is missing from the pyi file. I think its unlikely to change so an ignore is fine.

I don't know if either you had anything to do with it, but it was just added to typeshed:
python/typeshed#12329

Not sure if the change will get synced into the 1.11 release or not.

[Dreamsorcerer]

Ah, I see, it was result of the docs bug report.

[patchback]

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply c086795 on top of patchback/backports/3.10/c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c/pr-8485

Backporting merged PR #8485 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.10/c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c/pr-8485 upstream/3.10

4. Now, cherry-pick PR Fix response headers for compressed file requests #8485 contents into that branch:

   $ git cherry-pick -x c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c

   If it'll yell at you with something like fatal: Commit c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Fix response headers for compressed file requests #8485 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.10/c086795452bc8fe9c5a476dc1d6b9d5a3120dc7c/pr-8485

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[steverep]

@bdraco FYI, the use of MimeTypes() here now guarantees that any mime.types files from the OS are read on module load. Previously, that would not have happened until the first call to mimetypes.guess_type(), potentially significantly blocking that first response.

[bdraco]

@bdraco FYI, the use of MimeTypes() here now guarantees that any mime.types files from the OS are read on module load. Previously, that would not have happened until the first call to mimetypes.guess_type(), potentially significantly blocking that first response.

This is much nicer for async. In HA we preload mine types at bootstrap in the executor for this reason

[steverep]

I mentioned above potentially another small improvement by putting that large table lookup concurrent with opening the file, e.g.:

guess_type_fut = loop.run_in_executor(None, CONTENT_TYPES.guess_type, self._path)
# later on...
open_fut = loop.run_in_executor(None, file_path.open, "rb")
fobj,ct = await asyncio.gather([open_fut, guess_type_fut])

Worth it? Is there a benchmark in place to test it?