From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 01E6322A79 for ; Sat, 29 Dec 2018 05:30:07 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ctTByZ2aNs3Z for ; Sat, 29 Dec 2018 05:30:06 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id B04ED205AF for ; Sat, 29 Dec 2018 05:30:06 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request References: <20181221181844.4417-1-roman.habibov@tarantool.org> <20181223031929.hhmxdlsox2aon5ih@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: <3b2626ce-b887-70dd-38b0-ef38b37a983c@tarantool.org> Date: Sat, 29 Dec 2018 13:30:03 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Roman , tarantool-patches@freelists.org, Alexander Turenko 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 > 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