[tarantool-patches] Re: [PATCH] httpc: allow only strings for opts.headers values

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

Updated the branch according to the comments below. Expanded the commit
message with more info (it even does not state that the feature of the
question did not work -- this is the important detail :)).

I'm okay with the patch. Kirill, please, proceed.

WBR, Alexander Turenko.

On Mon, Jul 29, 2019 at 04:05:17PM +0300, Roman Khabibov wrote:
> Remove possibility to pass any tables as the opts.headers values.

It was about table with __tostring in a metatable, not any tables.

> 
> Closes (again) #3679

Better write it as 'Closes #3679 (again)' to let github close the issue
automatically:
https://help.github.com/en/articles/closing-issues-using-keywords

> ---
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-tostring
> Issue: https://github.com/tarantool/tarantool/issues/3679
> 
>  src/lua/httpc.c                   | 15 +++------------
>  test/app-tap/http_client.test.lua | 26 ++++++++++----------------
>  2 files changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index 4e78688fa..e02adb9fb 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -184,18 +184,9 @@ luaT_httpc_request(lua_State *L)
>  		while (lua_next(L, -2) != 0) {
>  			int header_type = lua_type(L, -1);
>  			if (header_type != LUA_TSTRING) {
> -				const char *err_msg =
> -					"opts.headers values should be strings "
> -					"or tables with \"__tostring\"";
> -				if (header_type != LUA_TTABLE) {
> -					httpc_request_delete(req);
> -					return luaL_error(L, err_msg);
> -				} else if (!luaL_getmetafield(L, -1,
> -							      "__tostring")) {
> -					httpc_request_delete(req);
> -					return luaL_error(L, err_msg);
> -				}
> -				lua_pop(L, 1);
> +				httpc_request_delete(req);
> +				return luaL_error(L, "opts.headers values "
> +						  "should be strings");

Looks ok.

>  			}
>  			if (lua_type(L, -2) != LUA_TSTRING) {
>  				httpc_request_delete(req);
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 25fab89ab..f7243c2fa 100755
> --- a/test/app-tap/http_client.test.lua
> +++ b/test/app-tap/http_client.test.lua
> @@ -273,15 +273,13 @@ local function test_errors(test)
>      local r = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru")
>  end
>  
> --- gh-3679 Check that opts.headers values can be strings or table
> --- convertible to string only.
> +-- gh-3679 Check that opts.headers values can be strings only.
>  -- gh-4281 Check that opts.headers can be a table and opts.headers
>  -- keys can be strings only.
>  local function test_request_headers(test, url, opts)
>      local exp_err_bad_opts_headers = 'opts.headers should be a table'
>      local exp_err_bad_key = 'opts.headers keys should be strings'
> -    local exp_err_bad_value = 'opts.headers values should be strings or ' ..
> -                              'tables with "__tostring"'
> +    local exp_err_bad_value = 'opts.headers values should be strings'
>      local cases = {
>          -- Verify opts.headers type checks.
>          {
> @@ -326,18 +324,6 @@ local function test_request_headers(test, url, opts)
>              opts = {headers = {aaa = 'aaa'}},
>              exp_err = nil,
>          },
> -        {
> -            'header value with __tostring() metamethod',
> -            opts = {headers = {aaa = setmetatable({}, {
> -                __tostring = function(self)
> -                    return 'aaa'
> -                end})}},
> -            exp_err = nil,
> -            postrequest_check = function(opts)
> -                assert(type(opts.headers.aaa) == 'table',
> -                    '"aaa" header was modified in http_client')
> -            end,
> -        },
>          {
>              'boolean header value',
>              opts = {headers = {aaa = true}},
> @@ -358,6 +344,14 @@ local function test_request_headers(test, url, opts)
>              opts = {headers = {aaa = 10ULL}},
>              exp_err = exp_err_bad_value,
>          },
> +        {
> +            'header value with __tostring() metamethod',
> +            opts = {headers = {aaa = setmetatable({}, {
> +                __tostring = function(self)
> +                    return 'aaa'
> +                end})}},
> +            exp_err = exp_err_bad_value,
> +        },

I don't see a reason to test tables with metatables anymore.