From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4246921953 for ; Tue, 5 Feb 2019 17:16:33 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GPK89WQ2sKMp for ; Tue, 5 Feb 2019 17:16:33 -0500 (EST) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 50A8020F3A for ; Tue, 5 Feb 2019 17:16:32 -0500 (EST) Date: Wed, 06 Feb 2019 01:16:28 +0300 From: Roman Subject: [tarantool-patches] Re: [PATCH] httpc: set reason when status is more than 400 Message-Id: <1549404989.27135.0@smtp.mail.ru> In-Reply-To: <20190205044052.jroiypatjf4asxtf@tkn_work_nb> References: <20190204135927.10777-1-roman.habibov@tarantool.org> <20190205044052.jroiypatjf4asxtf@tkn_work_nb> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=-M+FnmYhuX0xFxvTMVawt" Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Alexander Turenko Cc: Vladislav Shpilevoy --=-M+FnmYhuX0xFxvTMVawt Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Hi all! Alexander, thanks for review. Vlad, can you do a second review,=20 please? On 05.02.2019 7:40, Alexander Turenko wrote: > The commit message header: >=20 >> httpc: set reason when status is more than 400 > More or equal. >=20 >> Set the reason "Unknown" when it is CURLE_OK and status is more than=20 >> 400. =E2=96=B2 =E2=96=B2 =E2=96=B2 Done. > The >=3D 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: >=20 > ``` > httpc: set 'Unknown' reason for 0, 4xx, 5xx codes > ``` =E2=96=B2 =E2=96=B2 =E2=96=B2 Done. > Why this order? req->status >=3D 100 && req->status < 400 reads more > easily. Done. commit a39ede25caef257fdc99b7896abb31557773e7f5 Author: Roman Khabibov 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=20 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=20 timeout) case CURLE_OK: curl_easy_getinfo(req->curl_request.easy, CURLINFO_RESPONSE_CODE,=20 &longval); req->status =3D (int) longval; - /* TODO: get real response string from resp->headers */ - req->reason =3D "Ok"; + + if (req->status >=3D 100 && req->status < 400) + req->reason =3D "Ok"; + else + req->reason =3D "Unknown"; + if (req->status =3D=3D 200) { ++env->stat.http_200_responses; } else { diff --git a/test/app-tap/http_client.test.lua=20 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=20 polluted"); local r =3D 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") ~=3D nil, "body") @@ -104,7 +105,7 @@ local function test_cancel_and_errinj(test, url,=20 opts) end local function test_post_and_get(test, url, opts) - test:plan(19) + test:plan(21) local http =3D client.new() test:ok(http ~=3D nil, "client is created") @@ -159,6 +160,7 @@ local function test_post_and_get(test, url, opts) r =3D 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 =3D responses.empty_post @@ -180,6 +182,7 @@ local function test_post_and_get(test, url, opts) r =3D 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") = --=-M+FnmYhuX0xFxvTMVawt Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
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 mess= age 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 >=3D 100= condition is matter too. http= s://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 >= ;=3D 100 && req->status < 400 reads more easily.
Done.

commit a39ede25caef257fdc99= b7896abb31557773e7f5 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 =20 Set the reason "Unknown" when it is CURLE_OK and status is more than or= equal to 400. =20 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 timeou= t) case CURLE_OK: curl_easy_getinfo(req->curl_request.easy, CURLINFO_RESPONSE_CODE, &am= p;longval); req->status =3D (int) longval; - /* TODO: get real response string from resp->headers */ - req->reason =3D "Ok"; + + if (req->status >=3D 100 && req->status < 400) + req->reason =3D "Ok"; + else + req->reason =3D "Unknown"; + if (req->status =3D=3D 200) { ++env->stat.http_200_responses; } else { diff --git a/test/app-tap/http_client.test.lua b/test/app-tap/http_client.t= est.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 =20 local function test_http_client(test, url, opts) - test:plan(9) + test:plan(10) =20 test:isnil(rawget(_G, 'http'), "global namespace is not polluted"); test:isnil(rawget(_G, 'http.client'), "global namespace is not pollute= d"); local r =3D 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") ~=3D nil, "body") @@ -104,7 +105,7 @@ local function test_cancel_and_errinj(test, url, opts) end =20 local function test_post_and_get(test, url, opts) - test:plan(19) + test:plan(21) =20 local http =3D client.new() test:ok(http ~=3D nil, "client is created") @@ -159,6 +160,7 @@ local function test_post_and_get(test, url, opts) =20 r =3D 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") =20 r =3D responses.empty_post @@ -180,6 +182,7 @@ local function test_post_and_get(test, url, opts) =20 r =3D 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")

<= /div>= --=-M+FnmYhuX0xFxvTMVawt--