Hi! Thanks for review.

On ²â, äÕÒ 5, 2019 at 2:42 PM, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
Hi! Thanks for the fixes! See 3 comments below.
4. In the commit message you said that 'nil' is ok, but here you did not check for that and return an error. Of course, real nil can not appear in a table, because it means absence of a value, but what about box.NULL?
Added test for this case, right?
1. Yes, you did, but the test fails. On my laptop http_client.test.lua is totally broken for already a year at least, but I run the test from console and got this: tarantool> client = require('http.client') --- ... tarantool> http = client:new() --- ... tarantool> opts = {headers = {aaa = box.NULL}} --- ... tarantool> http:get('127.0.0.1:12345', opts) --- - error: 'builtin/http.client.lua:299: can''t convert header to a string' So box.NULL is not converted to string despite the fact that box.NULL == nil and you said in the commit message "... Each header must be nil, 'string' or 'table' ..."
In a private chat, we decided that this result is ok. And I changed the commit message.

5. Out of 80 symbols.
2. Still out of 80.
My text editor shows 70 symbols.

+                } else if (luaL_getmetafield(L, -1, "__tostring") == LUA_TNIL) {
diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 10a3728f8..f231c2e90 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -205,6 +205,80 @@ local function test_errors(test)      test:is(r.status, 595, "GET: response on bad url")  end +-- gh-3679 allow only headers can be converted to string
3. I am not a linguist, but the sentence looks grammatically incorrect IMO. 'Allow only headers <something is missing> can be ... '.
+local function test_request_headers(test, url, opts) +    local exp_err = 'can\'t convert header to a string' +    local cases = { +        {
+ if (header_type != LUA_TTABLE) { + return luaL_error(L, + "headers must be string or table with \"__tostring\""); + } else if (!luaL_getmetafield(L, -1, "__tostring")) { + return luaL_error(L, + "headers must be string or table with \"__tostring\""); + }
Done like other error messages in this file. But I do not know how best to fit code in 80 symbols.


commit 104dbc419aaf3d9ec8f294513c73a98d2f103459 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Wed Dec 26 17:49:34 2018 +0300 httpc: add checking of headers in httpc:request Add preprocessing of the request headers. Each header must be 'string' or 'table' with '__tostring' metamethod. Closes #3679 diff --git a/src/lua/httpc.c b/src/lua/httpc.c index 5f4e2e912..bc89a2334 100644 --- a/src/lua/httpc.c +++ b/src/lua/httpc.c @@ -173,6 +173,17 @@ luaT_httpc_request(lua_State *L) if (!lua_isnil(L, -1)) { lua_pushnil(L); while (lua_next(L, -2) != 0) { + int header_type = lua_type(L, -1); + if (header_type != LUA_TSTRING) { + if (header_type != LUA_TTABLE) { + return luaL_error(L, + "headers must be string or table with \"__tostring\""); + } else if (!luaL_getmetafield(L, -1, "__tostring")) { + return luaL_error(L, + "headers must be string or table with \"__tostring\""); + } + lua_pop(L, 1); + } if (httpc_set_header(req, "%s: %s", lua_tostring(L, -2), lua_tostring(L, -1)) < 0) { diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua index 10a3728f8..8a98d1e92 100755 --- a/test/app-tap/http_client.test.lua +++ b/test/app-tap/http_client.test.lua @@ -205,6 +205,80 @@ local function test_errors(test) test:is(r.status, 595, "GET: response on bad url") end +-- gh-3679 allow only headers can be converted to string +local function test_request_headers(test, url, opts) + local exp_err = 'headers must be string or table with "__tostring"' + local cases = { + { + 'string header', + opts = {headers = {aaa = 'aaa'}}, + exp_err = nil, + }, + { + 'header 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', + opts = {headers = {aaa = true}}, + exp_err = exp_err, + }, + { + 'number header', + opts = {headers = {aaa = 10}}, + exp_err = exp_err, + }, + { + 'cdata header (box.NULL)', + opts = {headers = {aaa = box.NULL}}, + exp_err = exp_err, + }, + { + 'cdata<uint64_t> header', + opts = {headers = {aaa = 10ULL}}, + exp_err = exp_err, + }, + { + 'table header w/o metatable', + opts = {headers = {aaa = {}}}, + exp_err = exp_err, + }, + { + 'table header w/o __tostring() metamethod', + opts = {headers = {aaa = setmetatable({}, {})}}, + exp_err = exp_err, + }, + } + test:plan(#cases) + + local http = client:new() + + for _, case in ipairs(cases) do + local opts = merge(table.copy(opts), case.opts) + local ok, err = pcall(http.get, http, url, opts) + if case.postrequest_check ~= nil then + case.postrequest_check(opts) + end + if case.exp_err == nil then + -- expect success + test:ok(ok, case[1]) + else + -- expect fail + assert(type(err) == 'string') + err = err:gsub('^builtin/[a-z._]+.lua:[0-9]+: ', '') + test:is_deeply({ok, err}, {false, case.exp_err}, case[1]) + end + end +end + local function test_headers(test, url, opts) test:plan(19) local http = client:new() @@ -397,12 +471,13 @@ local function test_concurrent(test, url, opts) end function run_tests(test, sock_family, sock_addr) - test:plan(9) + test:plan(10) local server, url, opts = start_server(test, sock_family, sock_addr) test:test("http.client", test_http_client, url, opts) test:test("cancel and errinj", test_cancel_and_errinj, url .. 'long_query', opts) test:test("basic http post/get", test_post_and_get, url, opts) test:test("errors", test_errors) + test:test("request_headers", test_request_headers, url, opts) test:test("headers", test_headers, url, opts) test:test("special methods", test_special_methods, url, opts) if sock_family == 'AF_UNIX' and jit.os ~= "Linux" then