[tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 7 comments below.

On 09/01/2019 16:29, Roman wrote:
> 
>> 1. Firstly, I think, that the patch should be in C in
>> lua/httpc.c and affect only function luaT_httpc_request. It
>> will prevent creation of additional table 'res', maybe
>> will look even shorter, and evidently will be more efficient.
>> It is not so complex logic so as to write it in Lua, IMO.
>>
> Done.
> 
> commit beb0562425dd5062af5d18ae202f0f4434cb5108
> Author: Roman Khabibov <roman.habibov at tarantool.org>
> Date:   Wed Dec 26 17:49:34 2018 +0300
> 
>      httpc: add checking of headers in httpc:request
> 
>      Add preprocessing of the request headers. Each header must be nil, 'string' or 'table'
>      with '__tostring' metamethod.
> 
>      Closes #3679
> 
> diff --git a/src/lua/httpc.c b/src/lua/httpc.c
> index 5f4e2e912..16a435ef1 100644
> --- a/src/lua/httpc.c
> +++ b/src/lua/httpc.c
> @@ -173,6 +173,15 @@ luaT_httpc_request(lua_State *L)
>       if (!lua_isnil(L, -1)) {
>           lua_pushnil(L);

1. I see, that you didn't use format-patch + send-email, since here we
have white-spaces, but in the real diff there are tabs. Please, use
format-patch + send-email. Or if you used them, then fix you text editor.
Or stop using web client - use a specialized one like Thunderbird.

>           while (lua_next(L, -2) != 0) {
> +            int header_type = lua_type(L, -1);
> +            if (header_type != LUA_TSTRING) {

2. Why so complex instead of just lua_isstring()?

> +                if (header_type != LUA_TTABLE) {
> +                    return luaL_error(L, "cannot convert header to a string");

3. Word 'cannot' does not exist. Please, use "can't" or "can not".

4. In the commit message you said that 'nil' is ok, but here you did not
check for that and return an error. Of course, real nil can not appear in
a table, because it means absence of a value, but what about box.NULL?

5. Out of 80 symbols.

> +                } else if (luaL_getmetafield(L, -1, "__tostring") == LUA_TNIL) {

6. Why do you compare with LUA_TNIL instead of direct checking of 0/1?
I see, that luaL_getmetafield does return exactly 0 or 1, not LUA_TNIL. Maybe
they accidentally matched, but it is not a reason to use LUA_TNIL here.

> +                    return luaL_error(L, "cannot convert header to a string");
> +                }
> +                lua_pop(L, 1);

7. Why do you pop? Now on a stack we have key (L[-2]) and value (L[-1]).
Here you pops one, and we have key (L[-1]). So the function below does a
mess. And maybe this is a reason why the test below fails.

> +            }
>               if (httpc_set_header(req, "%s: %s",
>                            lua_tostring(L, -2),
>                            lua_tostring(L, -1)) < 0) {
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 10a3728f8..d1fde5009 100755
> --- a/test/app-tap/http_client.test.lua
> +++ b/test/app-tap/http_client.test.lua