Skip to content

Strip the result body when creating default HEAD responses #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aturon opened this issue Nov 17, 2018 · 10 comments
Closed

Strip the result body when creating default HEAD responses #45

aturon opened this issue Nov 17, 2018 · 10 comments
Labels
good first issue Good for newcomers needs test believed to be fixed, but needs a regression test

Comments

@aturon
Copy link
Collaborator

aturon commented Nov 17, 2018

If you don't specify a HEAD behavior but you do provide GET, Tide will automatically use the GET behavior. But it retains the body, which should be stripped.

@aturon aturon added bug Something isn't working good first issue Good for newcomers labels Nov 17, 2018
@tzilist
Copy link
Contributor

tzilist commented Nov 19, 2018

@aturon did a little diggin into this, not sure if I am reading this right either, but it seems like hyper handles this for us. In the current code on master here are some example responses from curl
I'm running the named_path example.

The get request:

url --get --url http://127.0.0.1:8000/add_two/2 --header 'content-type: application/json' -v

Returns

*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> GET /add_two/2 HTTP/1.1
> Host: 127.0.0.1:8000
> User-Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Trident/5.0)
> Accept: */*
> Referer:
> content-type: application/json
>
< HTTP/1.1 200 OK
< content-type: text/plain
< content-length: 15
< date: Mon, 19 Nov 2018 04:53:55 GMT
<
* Connection #0 to host 127.0.0.1 left intact
2 plus two is 4

and the head request:

curl --head --url http://127.0.0.1:8000/add_two/2 --header 'content-type: application/json' -v

returns

*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to 127.0.0.1 (127.0.0.1) port 8000 (#0)
> HEAD /add_two/2 HTTP/1.1
> Host: 127.0.0.1:8000
> User-Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; Trident/5.0)
> Accept: */*
> Referer:
> content-type: application/json
>
< HTTP/1.1 200 OK
HTTP/1.1 200 OK
< content-type: text/plain
content-type: text/plain
< content-length: 15
content-length: 15
< date: Mon, 19 Nov 2018 04:53:33 GMT
date: Mon, 19 Nov 2018 04:53:33 GMT

<
* Connection #0 to host 127.0.0.1 left intact

Perhaps I am missing something though. Thoughts?

@DeltaManiac
Copy link
Contributor

@tzilist

I had observed the same behavior while testing #31.
Guess i missed something too :)

@aturon
Copy link
Collaborator Author

aturon commented Nov 19, 2018

That's very interesting. I haven't been able to track down where this is happening in Hyper...

@aturon
Copy link
Collaborator Author

aturon commented Nov 24, 2018

@seanmonstar maybe you can provide a quick answer: does Hyper have built-in treatment for HEAD requests and if so, what is it?

@seanmonstar
Copy link

hyper will try to prevent illegal HTTP semantics, like sending a body in response to HEAD requests, or in 204/304 status codes.

It may still be useful to recognize a HEAD request is different from a GET in cases where preparing the body may have been expensive.

@tzilist
Copy link
Contributor

tzilist commented Nov 27, 2018

@aturon if this is the case, should we just write a test case to ensure that HEAD requests always strip the body? We can just rely on hyper to handle this and the test case to make sure it doesn't break.

@aturon
Copy link
Collaborator Author

aturon commented Nov 27, 2018

@tzilist Yep, that seems reasonable for now! I'd also suggest adding a comment to make clear that's what we're doing.

@aturon aturon added needs test believed to be fixed, but needs a regression test and removed bug Something isn't working labels Feb 21, 2019
@fairingrey
Copy link
Contributor

I attempted to write a test for this given the mock service provided through http_service_mock but I'm running into something strange... There's more details in my PR: #179

@WSeegers
Copy link

Has this issue been fully resolved?

@yoshuawuyts
Copy link
Member

@WSeegers I think it may have been in #179. Going to go ahead and close this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers needs test believed to be fixed, but needs a regression test
Projects
None yet
Development

No branches or pull requests

7 participants