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

[Vladislav Shpilevoy]

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

On 26/12/2018 18:56, Roman wrote:
> 
> On 23.12.2018 6:19, Alexander Turenko wrote:
>> Hi!
>>
>> See comments below and proposed fixup on the branch
>> romanhabibov/gh-3679-httpc-request-fixups. Please, amend the patch and
>> proceed with the next reviewer (Vlad, added to CC).
>>
>> WBR, Alexander Turenko.
> 
> Done. Vlad, review, please.
> 
> 
> commit 253be70b02a700ef9936fbab98f80c79de3cf0fe
> 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 functions that preprocess the request headers. Each header must be nil, 'string' or 'table'
>      with '__tostring' metamethod.
> 
>      Closes #3679

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.

> 
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index cd44b6054..21a7cd2fd 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -216,6 +216,42 @@ local function process_cookies(cookies)
>       return result
>   end
> 
> +local function bad_header_error(func_name, header_name)
> +    error(('%s: cannot convert header "%s" to a string'):format(
> +        func_name, header_name))

2. Broken indentation. We never wrap a line right after '(',
and when we wrap parameters, they should be aligned by '(' of
the first line.

> +end
> +
> +-- Verify and preprocess outcoming headers before send a request.
> +--
> +-- Return the new headers table (with all string values) or nil
> +-- when no headers were provided.

3. Please, use @retval when describing return value.

> +local function preprocess_request_headers(headers)
> +    local func_name = 'preprocess_request_headers'

4. For a user this function name is useless. He called
:request(), but in logs doesn't see an appropriate message.

> +
> +    if headers == nil then
> +        return nil
> +    end
> +
> +    local res = {}
> +
> +    for name, value in pairs(headers) do
> +        local lua_type = type(value)
> +        if lua_type == 'string' then
> +            res[name] = value
> +        elseif lua_type == 'table' then
> +            local mt = getmetatable(value)
> +            if mt == nil or mt.__tostring == nil then
> +                bad_header_error(func_name, name)
> +            end
> +            res[name] = tostring(value)
> +        else
> +            bad_header_error(func_name, name)
> +        end
> +    end
> +
> +    return res
> +end
> +
>   local function process_headers(headers)
>       for header, value in pairs(headers) do
>           if type(value) == 'table' then
> @@ -296,6 +332,12 @@ curl_mt = {
>               if not method or not url then
>                   error('request(method, url [, options]])')
>               end
> +            local headers = preprocess_request_headers(opts and opts.headers)
> +            if headers ~= nil then
> +                -- prevent changing of user provided options
> +                opts = table.copy(opts)
> +                opts.headers = headers
> +            end

5. Already second table copying. This is why I do not like it in Lua.

>               local resp = self.curl:request(method, url, body, opts or {})
>               if resp and resp.headers then
>                   if resp.headers['set-cookie'] ~= nil then