From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Roman <roman.habibov@tarantool.org>,
tarantool-patches@freelists.org,
Alexander Turenko <alexander.turenko@tarantool.org>,
Kirill Yukhin <kyukhin@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400
Date: Fri, 15 Feb 2019 14:15:17 +0100 [thread overview]
Message-ID: <31616ebe-0c04-7748-f104-9401303f34c6@tarantool.org> (raw)
In-Reply-To: <1549404989.27135.0@smtp.mail.ru>
LGTM.
On 05/02/2019 23:16, Roman wrote:
> Hi all! Alexander, thanks for review. Vlad, can you do a second review, please? On 05.02.2019 7:40, Alexander Turenko 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.
>
> ▲ ▲ ▲
> Done.
>
> 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 ```
>
> ▲ ▲ ▲
> Done.
>
> Why this order? req->status >= 100 && req->status < 400 reads more easily.
>
> Done.
>
> commit a39ede25caef257fdc99b7896abb31557773e7f5 Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Mon Feb 4 16:28:30 2019 +0300 httpc: set 'Unknown' reason for 0, 4xx, 5xx codes Set the reason "Unknown" when it is CURLE_OK and status is more than or equal to 400. Closes #3681 diff --git a/src/httpc.c b/src/httpc.c index 950f8b32f..1f9366982 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 >= 100 && req->status < 400) + 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")
>
next prev parent reply other threads:[~2019-02-15 13:15 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 ` [tarantool-patches] " Alexander Turenko
2019-02-05 22:16 ` Roman
2019-02-15 13:15 ` Vladislav Shpilevoy [this message]
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=31616ebe-0c04-7748-f104-9401303f34c6@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=alexander.turenko@tarantool.org \
--cc=kyukhin@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