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 Brotli support #75

Closed
wants to merge 1 commit into from
Closed

Add Brotli support #75

wants to merge 1 commit into from

Conversation

pmouawad
Copy link

This PR adds support of Brotli decoding to HC.
It relies on https://github.com/google/brotli/tree/master/java/org/brotli which is under MIT license.

Regards
@philmdot

@ok2c
Copy link
Member

ok2c commented May 1, 2017

@pmouawad I am not sure it is a good idea to introduce a hard dependency on an external library for a single decompression codec, especially given it is quite easy to plug it in with HttpClientBuilder. If the brotli codec were supported by Commons Codec it would be easier to add it to the default configuration.

LinkedHashMap<String, InputStreamFactory> contentDecoderMap = new LinkedHashMap<>();
contentDecoderMap.put("gzip", new InputStreamFactory() {

    @Override
    public InputStream create(final InputStream instream) throws IOException {
        return new GZIPInputStream(instream);
    }

});
contentDecoderMap.put("br", new InputStreamFactory() {

    @Override
    public InputStream create(final InputStream instream) throws IOException {
        return new BrotliInputStream(instream);
    }

});
CloseableHttpClient httpclient = HttpClients.custom()
        .setContentDecoderRegistry(contentDecoderMap)
        .build();

@pmouawad
Copy link
Author

pmouawad commented May 1, 2017

Hello @ok2c ,
It appears to me gzip, deflate are standards and as such included in HC, as brotli will soon become the 3rd standard (I notice main browsers send it in Accept-Encoding) , if we follow current policy brotli should be included.

I doubt commons-compress will contain it alone soon, it will be a bridge as Gary suggested, so dec will be a dependency anyway.

I think the most built-in behaviour you provide the better it is for users , but as you like.

Regards
Philippe M.
@philmdot

@ok2c
Copy link
Member

ok2c commented May 1, 2017

What we can do is to add org.brotli:dec as an optional dependency (mandatory at compile and optional at runtime) and wire it up only if it is actually on the classpath. Users would need to explicitly add the library to the POM in order to enable it. This technique is widely used, for instance, by Spring to reduce its runtime footprint.

@garydgregory
Copy link
Member

Commons Codec is alive and well and released fairly regularly. Disclosure: I am the project chair over at Apache Commons. Feel free to create a Jira or PR in Common Compress ;-)

@pmouawad
Copy link
Author

pmouawad commented May 1, 2017

Hi @garydgregory ,
Don't get me wrong, I never intended to say commons-codec was not alive :-)

If you give me some tips on how I should proceed to integrate feature in commons-codec, I'll be happy to submit a PR.

Thanks

@garydgregory
Copy link
Member

We're all good :-) I only intended to mention the project is alive and well :-) And I meant to say Apache Commons Compress (https://commons.apache.org/proper/commons-compress/), not Codec. Doh!

WRT PR, the process is the same as here.

You can create a JIRA here https://issues.apache.org/jira/browse/COMPRESS/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel

And push PRs here https://github.com/apache/commons-compress

The difference with HC is that Commons Compress has made the move to Git within the Apache infrastructure.

Gary

@pmouawad
Copy link
Author

pmouawad commented May 1, 2017

Hi @garydgregory ,
See apache/commons-codec#6

Regards

@garydgregory
Copy link
Member

Oops, wrong project, can you please patch against Apache Commons Compress? https://github.com/apache/commons-compress

Thank you,
Gary

@pmouawad
Copy link
Author

pmouawad commented May 1, 2017

Hi Gary,
Oleg mentioned commons-codec.

By the way, why does HC not rely on deflate decompressor in commons-compress?

Thanks

@asfbot
Copy link

asfbot commented May 1, 2017

Gary Gregory on dev@hc.apache.org replies:
In general, we do not want to create additional dependencies. I think it
would be nice to have an optional dependency on Apache Commons Compress to
deflate/compress whatever Apache Commons Compress supports.

@pmouawad
Copy link
Author

pmouawad commented May 2, 2017

Hello,
As advised by Oleg and Gary, I opened a PR towards commons-compress.

Regards

@pmouawad pmouawad closed this May 2, 2017
@garydgregory
Copy link
Member

Tracking through a new ticket here: https://issues.apache.org/jira/browse/HTTPCLIENT-1843

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