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

Fix DecompressResponse Interceptor skipping decompression when Accept-Encoding is set manually #383

Open
wants to merge 1 commit into
base: 5.x
Choose a base branch
from

Conversation

luzrain
Copy link
Contributor

@luzrain luzrain commented Mar 5, 2025

This PR fixes the DecompressResponse Interceptor.
Previously, if the Accept-Encoding header was manually set on a request, the interceptor would skip decompression, resulting in an encoded response body.
With this change, the DecompressResponse Interceptor determines whether to decompress the response based only on the Content-Encoding response header. It will decompress the body if this header contains a supported algorithm, regardless of the request headers.

@kelunik
Copy link
Member

kelunik commented Mar 5, 2025

This is actually by-design, e.g. if you build a reverse proxy, you don't want to decompress and compress again.

If someone else sets the header, that code should also handle the decompression.

@luzrain
Copy link
Contributor Author

luzrain commented Mar 5, 2025

This is actually by-design, e.g. if you build a reverse proxy, you don't want to decompress and compress again.

Not sure if I get you. Can we make it configurable?

If someone else sets the header, that code should also handle the decompression.

It actually doesn't handle decompression if I set Accept-Encoding manually, despite the registered Interceptor.

Please check this example below.
What should be var dumped? I expect the decompressed output because of the registered DecompressResponse Interceptor, but it will actually be the raw gzipped string. Try commenting out the setHeader line, and you'll get the decompressed output.

<?php

use Amp\Http\Client\HttpClientBuilder;
use Amp\Http\Client\Request;

require dirname(__DIR__, 1) . '/vendor/autoload.php';

$client = HttpClientBuilder::buildDefault();

$request = new Request('https://httpbin.org/gzip');
$request->setHeader('Accept-Encoding', 'gzip'); // <- This supresses DecompressResponse Interceptor

$response = $client->request($request);

var_dump($response->getBody()->buffer());

@@ -29,30 +29,20 @@ public function requestViaNetwork(
Cancellation $cancellation,
Stream $stream
): Response {
// If a header is manually set, we won't interfere
Copy link
Member

Choose a reason for hiding this comment

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

See here, there's even a comment explaining that it will do nothing if the header is manually set.

@kelunik
Copy link
Member

kelunik commented Mar 6, 2025

Why do you need it to be configurable? Why do you set the header manually?

I expect the compressed body to be var dumped, because as I said, if it's not the interceptor that sets the header, it does nothing by design.

However, I get that the current naming of the interceptor is misleading, because it sounds like it only modifies the response and does so always, but it also modifies the request.

@luzrain
Copy link
Contributor Author

luzrain commented Mar 6, 2025

I got you.

I get that the current naming of the interceptor is misleading, because it sounds like it only modifies the response

Yes, I think I misunderstood the purpose of this interceptor because its name.

Why do you set the header manually?

I would like to control compression manually for each request, but the current interceptor is not suitable for this.
I found out this particular issue while trying to add decode-content option support in http-client-guzzle-adapter.
For this, we need an interceptor that automatically decompresses responses based only on the response header.

What do you think about adding another interceptor with the logic from this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants