Re: [Tarantool-patches] [PATCH v5 2/3] httpc: add curl accept_encoding option

[Alexander Turenko <alexander.turenko@tarantool.org>]

LGTM.

I change wording a bit before push, commented below.

Pushed to master.

WBR, Alexander Turenko.

On Thu, Nov 07, 2019 at 03:07:15PM +0300, Ilya Kosarev wrote:
> CURLOPT_ACCEPT_ENCODING option is now supported.
> This option enables automatic decompression of HTTP downloads.
> See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

To be honest, I dislike 'HTTP downloads' wording of libcurl
documentation, because for me it suggests that application/octet-stream
mime type is attached to a response. The wording however technically
correct, just confusing for me and others who work more with a browser,
then HTTP directly. I changed it to 'HTTP responses'.

> Closes #4232
> 
> @TarantoolBot document
> Title: httpc: add curl accept_encoding option
> Update the documentation for client_object:request() parameters to
> reflect new accept_encoding option.
> It enables automatic decompression of HTTP downloads by setting the

Same here.

> contents of the Accept-Encoding header sent in a HTTP request and
> enabling decoding of a response when a Content-Encoding header is
> received.
> This is a request, not an order; the server may or may not do it.
> Servers might respond with Content-Encoding even without getting
> an Accept-Encoding in the request. Servers might respond with a
> different Content-Encoding than what was asked for in the request.
> @param accept_encoding specifies what encoding you'd like.
> This param can be an empty string which means Accept-Encoding header
> will contain all built-in supported encodings. This param can be
> comma-separated list of accepted encodings, like: "br, gzip, deflate".
> Bundled libcurl supports "identity", meaning non-compressed, "deflate"
> which requests the server to compress its response using the zlib
> algorithm and "gzip" which requests the gzip algorithm. System libcurl
> also possibly supports "br" which is brotli.
> See https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Separated paragraphs with empty lines for readability.

> ---
>  src/httpc.c       | 16 ++++++++++++++++
>  src/httpc.h       | 26 ++++++++++++++++++++++++++
>  src/lua/httpc.c   |  5 +++++
>  src/lua/httpc.lua |  2 ++
>  4 files changed, 49 insertions(+)
> 
> diff --git a/src/httpc.c b/src/httpc.c
> index 212064080..22b54d16a 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -361,6 +361,22 @@ httpc_set_follow_location(struct httpc_request *req, long follow)
>  			 follow);
>  }
>  
> +void
> +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding)
> +{
> +/*
> +* CURLOPT_ACCEPT_ENCODING was called CURLOPT_ENCODING before
> +* libcurl 7.21.6.
> +*/
> +#if LIBCURL_VERSION_NUM >= 0x071506
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_ACCEPT_ENCODING,
> +			 encoding);
> +#else
> +	curl_easy_setopt(req->curl_request.easy, CURLOPT_ENCODING,
> +			 encoding);
> +#endif
> +}
> +
>  int
>  httpc_execute(struct httpc_request *req, double timeout)
>  {
> diff --git a/src/httpc.h b/src/httpc.h
> index 99fd8fbd4..2fc557107 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -372,6 +372,32 @@ httpc_set_interface(struct httpc_request *req, const char *interface);
>  void
>  httpc_set_follow_location(struct httpc_request *req, long follow);
>  
> +/**
> + * Enable automatic decompression of HTTP downloads: set the

Same here.

> + * contents of the Accept-Encoding header sent in a HTTP request
> + * and enable decoding of a response when a Content-Encoding
> + * header is received.
> + * This is a request, not an order; the server may or may not do
> + * it. Servers might respond with Content-Encoding even without
> + * getting an Accept-Encoding in the request. Servers might
> + * respond with a different Content-Encoding than what was asked
> + * for in the request.
> + * @param req request
> + * @param encoding - specify what encoding you'd like.
> + * This param can be an empty string which means Accept-Encoding
> + * header will contain all built-in supported encodings. This
> + * param can be comma-separated list of accepted encodings, like:
> + * "br, gzip, deflate".
> + * Bundled libcurl supports "identity", meaning non-compressed,
> + * "deflate" which requests the server to compress its response
> + * using the zlib algorithm and "gzip" which requests the gzip
> + * algorithm. System libcurl also possibly supports "br" which
> + * is brotli.
> + * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html

Separated paragraphs with empty lines too.

> + */
> +void
> +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding);
> +
>  /**
>   * This function does async HTTP request
>   * @param request - reference to request object with filled fields
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index a8e3e2525..6ed4eb788 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -315,6 +315,11 @@ luaT_httpc_request(lua_State *L)
>  		httpc_set_follow_location(req, lua_toboolean(L, -1));
>  	lua_pop(L, 1);
>  
> +	lua_getfield(L, 5, "accept_encoding");
> +	if (!lua_isnil(L, -1))
> +		httpc_set_accept_encoding(req, lua_tostring(L, -1));
> +	lua_pop(L, 1);
> +
>  	if (httpc_execute(req, timeout) != 0) {
>  		httpc_request_delete(req);
>  		return luaT_error(L);
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index ce9bb9771..3224d8c10 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -296,6 +296,8 @@ end
>  --          'Location' header that a server sends as part of an
>  --          3xx response;
>  --
> +--      accept_encoding - enables automatic decompression of HTTP downloads;

Same here.

Carried the comment to fit 66 symbols limit.

> +--
>  --  Returns:
>  --      {
>  --          status=NUMBER,
> -- 
> 2.17.1
>