Tarantool development patches archive
 help / color / mirror / Atom feed
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
> 

  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