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 5554224E91 for ; Tue, 30 Jul 2019 10:03:28 -0400 (EDT) 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 kjlgUTjyc4jf for ; Tue, 30 Jul 2019 10:03:28 -0400 (EDT) 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 05A3124E6A for ; Tue, 30 Jul 2019 10:03:27 -0400 (EDT) Date: Tue, 30 Jul 2019 17:03:12 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] httpc: allow only strings for opts.headers values Message-ID: <20190730140312.vnedp2v5eedyk6yt@tkn_work_nb> References: <20190729130517.61178-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190729130517.61178-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, Kirill Yukhin Updated the branch according to the comments below. Expanded the commit message with more info (it even does not state that the feature of the question did not work -- this is the important detail :)). I'm okay with the patch. Kirill, please, proceed. WBR, Alexander Turenko. On Mon, Jul 29, 2019 at 04:05:17PM +0300, Roman Khabibov wrote: > Remove possibility to pass any tables as the opts.headers values. It was about table with __tostring in a metatable, not any tables. > > Closes (again) #3679 Better write it as 'Closes #3679 (again)' to let github close the issue automatically: https://help.github.com/en/articles/closing-issues-using-keywords > --- > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3679-tostring > Issue: https://github.com/tarantool/tarantool/issues/3679 > > src/lua/httpc.c | 15 +++------------ > test/app-tap/http_client.test.lua | 26 ++++++++++---------------- > 2 files changed, 13 insertions(+), 28 deletions(-) > > diff --git a/src/lua/httpc.c b/src/lua/httpc.c > index 4e78688fa..e02adb9fb 100644 > --- a/src/lua/httpc.c > +++ b/src/lua/httpc.c > @@ -184,18 +184,9 @@ luaT_httpc_request(lua_State *L) > while (lua_next(L, -2) != 0) { > int header_type = lua_type(L, -1); > if (header_type != LUA_TSTRING) { > - const char *err_msg = > - "opts.headers values should be strings " > - "or tables with \"__tostring\""; > - if (header_type != LUA_TTABLE) { > - httpc_request_delete(req); > - return luaL_error(L, err_msg); > - } else if (!luaL_getmetafield(L, -1, > - "__tostring")) { > - httpc_request_delete(req); > - return luaL_error(L, err_msg); > - } > - lua_pop(L, 1); > + httpc_request_delete(req); > + return luaL_error(L, "opts.headers values " > + "should be strings"); Looks ok. > } > if (lua_type(L, -2) != LUA_TSTRING) { > httpc_request_delete(req); > diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua > index 25fab89ab..f7243c2fa 100755 > --- a/test/app-tap/http_client.test.lua > +++ b/test/app-tap/http_client.test.lua > @@ -273,15 +273,13 @@ local function test_errors(test) > local r = http:get("http://do_not_exist_8ffad33e0cb01e6a01a03d00089e71e5b2b7e9930dfcba.ru") > end > > --- gh-3679 Check that opts.headers values can be strings or table > --- convertible to string only. > +-- gh-3679 Check that opts.headers values can be strings only. > -- gh-4281 Check that opts.headers can be a table and opts.headers > -- keys can be strings only. > local function test_request_headers(test, url, opts) > local exp_err_bad_opts_headers = 'opts.headers should be a table' > local exp_err_bad_key = 'opts.headers keys should be strings' > - local exp_err_bad_value = 'opts.headers values should be strings or ' .. > - 'tables with "__tostring"' > + local exp_err_bad_value = 'opts.headers values should be strings' > local cases = { > -- Verify opts.headers type checks. > { > @@ -326,18 +324,6 @@ local function test_request_headers(test, url, opts) > opts = {headers = {aaa = 'aaa'}}, > exp_err = nil, > }, > - { > - 'header value with __tostring() metamethod', > - opts = {headers = {aaa = setmetatable({}, { > - __tostring = function(self) > - return 'aaa' > - end})}}, > - exp_err = nil, > - postrequest_check = function(opts) > - assert(type(opts.headers.aaa) == 'table', > - '"aaa" header was modified in http_client') > - end, > - }, > { > 'boolean header value', > opts = {headers = {aaa = true}}, > @@ -358,6 +344,14 @@ local function test_request_headers(test, url, opts) > opts = {headers = {aaa = 10ULL}}, > exp_err = exp_err_bad_value, > }, > + { > + 'header value with __tostring() metamethod', > + opts = {headers = {aaa = setmetatable({}, { > + __tostring = function(self) > + return 'aaa' > + end})}}, > + exp_err = exp_err_bad_value, > + }, I don't see a reason to test tables with metatables anymore.