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

Bypass GZipMiddleware when response includes Content-Encoding #1901

Merged
merged 2 commits into from Oct 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/middleware.md
Expand Up @@ -180,6 +180,8 @@ The following arguments are supported:

* `minimum_size` - Do not GZip responses that are smaller than this minimum size in bytes. Defaults to `500`.

The middleware won't GZip responses that already have a `Content-Encoding` set, to prevent them from being encoded twice.

## BaseHTTPMiddleware

An abstract class that allows you to write ASGI middleware against a request/response
Expand Down
8 changes: 8 additions & 0 deletions starlette/middleware/gzip.py
Expand Up @@ -33,6 +33,7 @@ def __init__(self, app: ASGIApp, minimum_size: int, compresslevel: int = 9) -> N
self.send: Send = unattached_send
self.initial_message: Message = {}
self.started = False
self.content_encoding_set = False
self.gzip_buffer = io.BytesIO()
self.gzip_file = gzip.GzipFile(
mode="wb", fileobj=self.gzip_buffer, compresslevel=compresslevel
Expand All @@ -48,6 +49,13 @@ async def send_with_gzip(self, message: Message) -> None:
# Don't send the initial message until we've determined how to
# modify the outgoing headers correctly.
self.initial_message = message
headers = Headers(raw=self.initial_message["headers"])
self.content_encoding_set = "content-encoding" in headers
elif message_type == "http.response.body" and self.content_encoding_set:
if not self.started:
self.started = True
await self.send(self.initial_message)
await self.send(message)
elif message_type == "http.response.body" and not self.started:
kklingenberg marked this conversation as resolved.
Show resolved Hide resolved
self.started = True
body = message.get("body", b"")
Expand Down
24 changes: 24 additions & 0 deletions tests/middleware/test_gzip.py
Expand Up @@ -76,3 +76,27 @@ async def generator(bytes, count):
assert response.text == "x" * 4000
assert response.headers["Content-Encoding"] == "gzip"
assert "Content-Length" not in response.headers


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
Comment on lines +81 to +102
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created what I think is a better test here. @Kludex If you're interested, I can submit a PR updating the test here as well.

Copy link
Sponsor Member

@Kludex Kludex Nov 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... It was my bad. This test was modified, you can see how it is on master.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh indeed, I found it