Bypass GZipMiddleware when response includes Content-Encoding
#1901
Conversation
Suggestions by Kludex Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
|
Great suggestions, they reduce the diff size and make it more readable. Thanks :) |
Closes #4 Refs encode/starlette#1901 Credit for this change goes to https://github.com/Kludex and https://github.com/kklingenberg
|
I was about to start working on a PR for this, thankfully I noticed that you already did so I just have to update my Starlette version. Thanks @kklingenberg!! Edit: actually I'm using FastAPI and the latest version (0.87.0) still uses Starlette 0.21.0, so I will have to wait... Thankfully there is already a PR to update it to 0.22.0 so it will probably happen soon... Sorry for the noise. |
| def test_gzip_ignored_for_responses_with_encoding_set(test_client_factory): | ||
| def homepage(request): | ||
| async def generator(bytes, count): | ||
| for index in range(count): | ||
| yield bytes | ||
|
|
||
| streaming = generator(bytes=b"x" * 400, count=10) | ||
| return StreamingResponse( | ||
| streaming, status_code=200, headers={"Content-Encoding": "br"} | ||
| ) | ||
|
|
||
| app = Starlette( | ||
| routes=[Route("/", endpoint=homepage)], | ||
| middleware=[Middleware(GZipMiddleware)], | ||
| ) | ||
|
|
||
| client = test_client_factory(app) | ||
| response = client.get("/", headers={"accept-encoding": "gzip, br"}) | ||
| assert response.status_code == 200 | ||
| assert response.text == "x" * 4000 | ||
| assert response.headers["Content-Encoding"] == "br" | ||
| assert "Content-Length" not in response.headers |
There was a problem hiding this comment.
Wait, this test creates a StreamingResponse that claims to be encoded as Brotli but actually isn't, since its body is just b"x" * 4000, right? How is it possible that this test passes? Doesn't the client try to process brotli automatically on the response and fails?
>>> import brotli
>>> decompressor = brotli.Decompressor()
>>> decompressor.process(b"x" * 4000)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
brotli.error: BrotliDecoderDecompressStream failed while processing the stream
There was a problem hiding this comment.
Yeah... It was my bad. This test was modified, you can see how it is on master.
|
Gratitude is rare, and I don't consider it as noise. Thanks for the message :) |
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
This is a suggestion for
GZipMiddlewareto not encode responses that already include theContent-Encodingheader. It is a minor issue for me when using the middleware in an application that includes regular routes and proxies based on this:https://gist.github.com/tomchristie/5765e10a90a41c7e57470e2dc700f9db
If the proxied response already has an encoding set, starlette's middleware double-encodes the response.
I can get around it by writing the middleware on my own, such as shown in this PR, but I thought you might want to consider it also. I'm basically imitating Django's GZipMiddleware which claims to have this behaviour.