search

dakrone / clj-http

An idiomatic clojure http client wrapping the apache client. Officially supported version.
http://clojars.org/clj-http
MIT License
1.78k stars 408 forks source link

Disabling decompression requires specifying two sets of Options #510

Closed rymndhng closed 4 years ago

rymndhng commented 5 years ago

Motivation

When a user wants to disable decompression, they should only have to specify one option. Currently, a user has to do two things:

  1. Add :decompress-body false
  2. Add a custom :http-builder-fns to disable it in Apache HTTPClientBuilder. See [HTTPClientBuidler#disableContentDecompression](https://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/impl/client/HttpClientBuilder.html#disableContentCompression()).

Example, as discovered from #509:

(defn disable-decompression [^org.apache.http.impl.client.HttpClientBuilder builder request] 
  (.disableContentCompression builder))

(request {:url "...",
          :request-method :get

          ;; two options need to be specified          
          :decompress-body false
          :http-builder-fns [disable-decompression]
          })

Suggested Solution

We should remove the decompression implementation from clj-http since this functionality is delegated to Apache HTTPClient while respecting the option :decompress-body.

  1. In core.clj, detect the presence of :decompress-body and configure (.disableContentCompression builder).
  2. Remove wrap-decompression from the list of middlewares (this is completely handled by ApacheHTTPClient).

  3. Cleanup the README's section on Decompression

    HTTPClientBuidler automatically. We should remove the function decompression-request since this functionality is now deferred to Apache HTTPClient.

dakrone commented 5 years ago

Sounds good, thanks for the analysis :+1:

rymndhng commented 4 years ago

I did a bit further analysis and I've found some some new details, specifically HttpAsyncClientBuilder does not support .disableContentCompression. It also does not appear to be supported on 5.x.

I think it's important that a user's intuition of how a middleware works is consistent between sync and async requests. Therefore, I think the better fix is to set .disableContentCompression for all requests and instead rely on clj-http middleware to decompress.

I also took a peek at how the underlying library implements decompression and it has the same logic as clj-http's middleware: wrapping the InputStream with GzipInputStream/DeflateInputStream by matching on content-encoding.