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 EBBF420FBE for ; Sat, 22 Dec 2018 22:19:30 -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 H8vCY61yAWT8 for ; Sat, 22 Dec 2018 22:19:30 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5239920F37 for ; Sat, 22 Dec 2018 22:19:30 -0500 (EST) Date: Sun, 23 Dec 2018 06:19:29 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] httpc: add checking of headers in httpc:request Message-ID: <20181223031929.hhmxdlsox2aon5ih@tkn_work_nb> References: <20181221181844.4417-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181221181844.4417-1-roman.habibov@tarantool.org> 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 Khabibov Cc: tarantool-patches@freelists.org, Vladislav Shpilevoy 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 >