* [tarantool-patches] [PATCH] httpc: set reason when status is more than 400 @ 2019-02-04 13:59 Roman Khabibov 2019-02-05 4:40 ` [tarantool-patches] " Alexander Turenko 2019-02-15 15:04 ` Kirill Yukhin 0 siblings, 2 replies; 5+ messages in thread From: Roman Khabibov @ 2019-02-04 13:59 UTC (permalink / raw) To: tarantool-patches; +Cc: alexander.turenko Set the reason "Unknown" when it is CURLE_OK and status is more than 400. 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) + 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 2019-02-04 13:59 [tarantool-patches] [PATCH] httpc: set reason when status is more than 400 Roman Khabibov @ 2019-02-05 4:40 ` Alexander Turenko 2019-02-05 22:16 ` Roman 2019-02-15 15:04 ` Kirill Yukhin 1 sibling, 1 reply; 5+ messages in thread From: Alexander Turenko @ 2019-02-05 4:40 UTC (permalink / raw) To: Roman Khabibov; +Cc: tarantool-patches 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 2019-02-05 4:40 ` [tarantool-patches] " Alexander Turenko @ 2019-02-05 22:16 ` Roman 2019-02-15 13:15 ` Vladislav Shpilevoy 0 siblings, 1 reply; 5+ messages in thread From: Roman @ 2019-02-05 22:16 UTC (permalink / raw) To: tarantool-patches, Alexander Turenko; +Cc: Vladislav Shpilevoy [-- Attachment #1: Type: text/plain, Size: 3638 bytes --] 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") [-- Attachment #2: Type: text/html, Size: 4759 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 2019-02-05 22:16 ` Roman @ 2019-02-15 13:15 ` Vladislav Shpilevoy 0 siblings, 0 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2019-02-15 13:15 UTC (permalink / raw) To: Roman, tarantool-patches, Alexander Turenko, Kirill Yukhin 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") > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 2019-02-04 13:59 [tarantool-patches] [PATCH] httpc: set reason when status is more than 400 Roman Khabibov 2019-02-05 4:40 ` [tarantool-patches] " Alexander Turenko @ 2019-02-15 15:04 ` Kirill Yukhin 1 sibling, 0 replies; 5+ messages in thread From: Kirill Yukhin @ 2019-02-15 15:04 UTC (permalink / raw) To: tarantool-patches; +Cc: alexander.turenko Hello, On 04 Feb 16:59, Roman Khabibov wrote: > Set the reason "Unknown" when it is CURLE_OK and status is more than 400. > > Closes #3681 > > Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-3681-httpc-reason > Issue: https://github.com/tarantool/tarantool/issues/3681 I've checked your patch into 2.1 branch. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-02-15 15:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-04 13:59 [tarantool-patches] [PATCH] httpc: set reason when status is more than 400 Roman Khabibov 2019-02-05 4:40 ` [tarantool-patches] " Alexander Turenko 2019-02-05 22:16 ` Roman 2019-02-15 13:15 ` Vladislav Shpilevoy 2019-02-15 15:04 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox