From: Alexander Turenko <alexander.turenko@tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org> Cc: tarantool-patches@freelists.org Subject: [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 Date: Tue, 5 Feb 2019 07:40:56 +0300 [thread overview] Message-ID: <20190205044052.jroiypatjf4asxtf@tkn_work_nb> (raw) In-Reply-To: <20190204135927.10777-1-roman.habibov@tarantool.org> Hi! It is ok except minor points. Please, fix them and proceed further with Vlad. WBR, Alexander Turenko. On Mon, Feb 04, 2019 at 04:59:27PM +0300, Roman Khabibov wrote: The commit message header: > httpc: set reason when status is more than 400 More or equal. > Set the reason "Unknown" when it is CURLE_OK and status is more than 400. The >= 100 condition is matter too. https://curl.haxx.se/libcurl/c/CURLINFO_RESPONSE_CODE.html states you can get zero as a code if 'no server response code has been received'. Don't sure how to reproduce this case, but anyway I think the commit message (and the header) should be either describe the whole change or be a bit more common. Say: ``` httpc: set 'Unknown' reason for 0, 4xx, 5xx codes ``` > Closes #3681 > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3681-httpc-reason > Issue: https://github.com/tarantool/tarantool/issues/3681 > --- > src/httpc.c | 8 ++++++-- > test/app-tap/http_client.test.lua | 7 +++++-- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/src/httpc.c b/src/httpc.c > index 950f8b32f..eb7a5334f 100644 > --- a/src/httpc.c > +++ b/src/httpc.c > @@ -325,8 +325,12 @@ httpc_execute(struct httpc_request *req, double timeout) > case CURLE_OK: > curl_easy_getinfo(req->curl_request.easy, CURLINFO_RESPONSE_CODE, &longval); > req->status = (int) longval; > - /* TODO: get real response string from resp->headers */ > - req->reason = "Ok"; > + > + if (req->status < 400 && req->status >= 100) Why this order? req->status >= 100 && req->status < 400 reads more easily. > + req->reason = "Ok"; > + else > + req->reason = "Unknown"; > + > if (req->status == 200) { > ++env->stat.http_200_responses; > } else { > diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.test.lua > index 10a3728f8..d4f3a9f45 100755 > --- a/test/app-tap/http_client.test.lua > +++ b/test/app-tap/http_client.test.lua > @@ -62,12 +62,13 @@ local function stop_server(test, server) > end > > local function test_http_client(test, url, opts) > - test:plan(9) > + test:plan(10) > > test:isnil(rawget(_G, 'http'), "global namespace is not polluted"); > test:isnil(rawget(_G, 'http.client'), "global namespace is not polluted"); > local r = client.get(url, opts) > test:is(r.status, 200, 'simple 200') > + test:is(r.reason, 'Ok', '200 - Ok') > test:is(r.proto[1], 1, 'proto major http 1.1') > test:is(r.proto[2], 1, 'proto major http 1.1') > test:ok(r.body:match("hello") ~= nil, "body") > @@ -104,7 +105,7 @@ local function test_cancel_and_errinj(test, url, opts) > end > > local function test_post_and_get(test, url, opts) > - test:plan(19) > + test:plan(21) > > local http = client.new() > test:ok(http ~= nil, "client is created") > @@ -159,6 +160,7 @@ local function test_post_and_get(test, url, opts) > > r = responses.absent_get > test:is(r.status, 500, "GET: absent method http code page exists") > + test:is(r.reason, 'Unknown', '500 - Unknown') > test:is(r.body, "No such method", "GET: absent method right body") > > r = responses.empty_post > @@ -180,6 +182,7 @@ local function test_post_and_get(test, url, opts) > > r = responses.bad_get > test:is(r.status, 404, "GET: http page not exists") > + test:is(r.reason, 'Unknown', '404 - Unknown') > test:isnt(r.body:len(), 0, "GET: not empty body page not exists") > test:ok(string.find(r.body, "Not Found"), > "GET: right body page not exists") > -- > 2.17.1 >
next prev parent reply other threads:[~2019-02-05 4:41 UTC|newest] Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-04 13:59 [tarantool-patches] " Roman Khabibov 2019-02-05 4:40 ` Alexander Turenko [this message] 2019-02-05 22:16 ` [tarantool-patches] " Roman 2019-02-15 13:15 ` Vladislav Shpilevoy 2019-02-15 15:04 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190205044052.jroiypatjf4asxtf@tkn_work_nb \ --to=alexander.turenko@tarantool.org \ --cc=roman.habibov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox