* [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