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

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

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.

On Fri, Dec 21, 2018 at 09:18:44PM +0300, Roman Khabibov wrote:
> Add function that checks the value of each header. It must be 'string' or 'table'
> with '__tostring' metamethod.
> 
> Closes #3679
> ---
> 
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-httpc-request
> Issue: https://github.com/tarantool/tarantool/issues/3679
> 
>  src/lua/httpc.lua                 | 23 +++++++++++++++++++++++
>  test/app-tap/http_client.test.lua | 17 +++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/src/lua/httpc.lua b/src/lua/httpc.lua
> index cd44b6054..ede976a3a 100644
> --- a/src/lua/httpc.lua
> +++ b/src/lua/httpc.lua
> @@ -216,6 +216,28 @@ local function process_cookies(cookies)
>      return result
>  end
>  
> +local function check_headers(opts)
> +    if opts ~= nil then
> +        local headers = opts["headers"]
> +        if headers ~= nil then
> +            for header, value in pairs(headers) do
> +                if type(value) ~= 'string' then
> +                    if  type(value) ~= 'table' then
> +                        error('Usage: httpc:request bad \''..header..'\' value')
> +                    end
> +                    local mt = getmetatable(value)
> +                    if mt == nil then
> +                        error('Usage: httpc:request \''..header..'\' has not \'__tostring\'')
> +                    end
> +                    if mt["__tostring"] ~= nil then
> +                        headers[header] = tostring(value)
> +                    end
> +                end
> +            end
> +        end
> +    end
> +end
> +

1. Too common name, IMHO.
2. Can be implemented with much less indentation level.
3. Changes user provided options.

>  local function process_headers(headers)
>      for header, value in pairs(headers) do
>          if type(value) == 'table' then
> @@ -296,6 +318,7 @@ curl_mt = {
>              if not method or not url then
>                  error('request(method, url [, options]])')
>              end
> +            check_headers(opts)
>              local resp = self.curl:request(method, url, body, opts or {})
>              if resp and resp.headers then
>                  if resp.headers['set-cookie'] ~= nil then
> diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua
> index 10a3728f8..d7e01d60f 100755
> --- a/test/app-tap/http_client.test.lua
> +++ b/test/app-tap/http_client.test.lua
> @@ -205,6 +205,23 @@ local function test_errors(test)
>      test:is(r.status, 595, "GET: response on bad url")
>  end
>  
> +--gh-3679 Check that client check that the httpc:request doesn't modify headers.
> +
> +local function test_request_headers(test)
> +    test:plan(5)
> +    httpc = require('http.client').new()
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = true}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = box.NULL}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = 10ULL}})),
> +            'expected error about bad value')
> +    test:ok(not pcall(httpc:request('GET', 'localhost:4444', nil, {headers = {aaa = {}}})),
> +            'expected error with no \'__tostring\'')
> +end
> +

1. Don't called.
2. Don't check error messages.
3. Too long lines.
4. Will not work in strict mode (require('strict').on() or Debug build).
5. pcall is called for the result, so it will not work as you expected;
   see below.
6. The message above the function is not about what the function does
   and is not what the issue states.

Re 5, compare:

pcall(error, 'hello')
pcall(error('hello'))

It is sad that you not even check whether your test was run.

>  local function test_headers(test, url, opts)
>      test:plan(19)
>      local http = client:new()
> -- 
> 2.17.1
>