* [tarantool-patches] Re: [PATCH] httpc: allow only strings for opts.headers values
[not found] <20190729130517.61178-1-roman.habibov@tarantool.org>
@ 2019-07-30 14:03 ` Alexander Turenko
2019-07-31 10:11 ` Kirill Yukhin
0 siblings, 1 reply; 2+ messages in thread
From: Alexander Turenko @ 2019-07-30 14:03 UTC (permalink / raw)
To: Roman Khabibov; +Cc: tarantool-patches, 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.
^ permalink raw reply [flat|nested] 2+ messages in thread