[tarantool-patches] Re: [PATCH] http: add CURLOPT_ACCEPT_ENCODING option

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

I have no objections against the code, but I would describe it in more
details. See below.

WBR, Alexander Turenko.

On Mon, Aug 19, 2019 at 03:12:30PM +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

Nit: docbot request is needed.

> 
> Closes #4232
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4232-curlopt-accept-encoding
> Issue: https://github.com/tarantool/tarantool/issues/4232
> 
>  src/httpc.c       | 13 +++++++++++++
>  src/httpc.h       | 12 ++++++++++++
>  src/lua/httpc.c   |  5 +++++
>  src/lua/httpc.lua |  2 ++
>  4 files changed, 32 insertions(+)
> 
> diff --git a/src/httpc.c b/src/httpc.c
> index 54bc785e1..736184b6b 100644
> --- a/src/httpc.c
> +++ b/src/httpc.c
> @@ -337,6 +337,19 @@ httpc_set_follow_location(struct httpc_request *req, long follow)
>  			 follow);
>  }
>  
> +void
> +httpc_set_accept_encoding(struct httpc_request *req, const char *encoding)
> +{
> +#if LIBCURL_VERSION_NUM >= 0x071506 /* CURLOPT_ACCEPT_ENCODING was called
> + 	CURLOPT_ENCODING before libcurl 7.21.6 */

Nit: Move the comment to its own line(s), fit it to 66 symbols or carry
it, add a period at the end. It will look so:

 | /*
 |  * 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
> +}
> +

It is possible that a user will set something that bundled / system
libcurl does not support or a user will misspell one of values. What
will going on in the case? As I see from libcurl sources it will just
set passed value for the request header. An http server likely will
ignore unknown values and will choose an encoding from a value that is
spelled corectly (or will send plain http). So everything will work, no
error will be reported, but the transfer will be performed w/o requested
encoding. I think it worth to verify and mention such details.

>  int
>  httpc_execute(struct httpc_request *req, double timeout)
>  {
> diff --git a/src/httpc.h b/src/httpc.h
> index c6882a534..d5f4320c5 100644
> --- a/src/httpc.h
> +++ b/src/httpc.h
> @@ -312,6 +312,18 @@ 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 contents
> + * of the Accept-Encoding: header sent in an HTTP request,
> + * and enable decoding of a response
> + * when a Content-Encoding: header is received.

Nit: Fit the comment to 66 symbols; don't carry a line if it can be
continued.

I would remove colons after the header names, because it confuses at
reading.

> + * @param req request
> + * @param encoding - specify what encoding you'd like.

Let's describe the special value: empty string. Let's describe which
options are available with our bundled libcurl and which are possibly
availiable with system libcurl (it is for custom builds). Let's describe
how to set several options. Add this information to a docbot comment
too.

> + * @see https://curl.haxx.se/libcurl/c/CURLOPT_ACCEPT_ENCODING.html
> + */
> +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 e02adb9fb..a57f08c6c 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -295,6 +295,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 50ff91a36..4b73187c2 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -287,6 +287,8 @@ end
>  --          'Location' header that a server sends as part of an
>  --          3xx response;
>  --
> +--      accept_encoding - enables automatic decompression of HTTP downloads;
> +--
>  --  Returns:
>  --      {
>  --          status=NUMBER,
> -- 
> 2.17.1
> 

Can we test, say, gzip encoding with our test/app-tap/httpd.py server?
If so it would be good to add a test.