Skip to content
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

Implement HTTP HEAD method #19

Closed
ilatif opened this issue Dec 2, 2015 · 16 comments
Closed

Implement HTTP HEAD method #19

ilatif opened this issue Dec 2, 2015 · 16 comments

Comments

@ilatif
Copy link
Contributor

ilatif commented Dec 2, 2015

I' am thinking of implementing HTTP HEAD method to complete kemal's supported HTTP methods set.

@sdogruyol
Copy link
Member

Actually i don't know why i missed that in here https://github.com/sdogruyol/kemal/blob/master/src/kemal/dsl.cr#L1

@ilatif
Copy link
Contributor Author

ilatif commented Dec 2, 2015

Yeah, I have explored the code and I have got a bit idea on where to make this change :-). I think it would be nice to support HEAD.

@sdogruyol
Copy link
Member

Yeah indeed 👍

@sdogruyol
Copy link
Member

You can implement and open a PR for it (with specs)

@ilatif
Copy link
Contributor Author

ilatif commented Dec 2, 2015

That's great. I just checked Sinatra's HEAD implementation and it is returning proper response headers for every valid route even if it's type is GET, POST, PUT etc. Do you want HEAD to be implemented like this or do you want to support HEAD to only work with GET based routes?

@sdogruyol
Copy link
Member

Actually i'm not sure which one is better

On Wednesday, 2 December 2015, Imran Latif notifications@github.com wrote:

That's great. I just checked Sinatra's HEAD implementation and it is
returning proper response headers for every valid route even if it's type
is GET, POST, PUT etc. Do you want HEAD to be implemented like this or do
you want to support HEAD to only work with GET based routes?


Reply to this email directly or view it on GitHub
#19 (comment).

@ilatif
Copy link
Contributor Author

ilatif commented Dec 2, 2015

I think we should support HEAD implementation for GET based routes only since it is the one most commonly used. I have seen first time in my life that HEAD is working for POST routes :-).

@sdogruyol
Copy link
Member

Yeah i'm actually surprised to hear that it works with all routes

@sdogruyol
Copy link
Member

Only for GET is 👍

@ilatif
Copy link
Contributor Author

ilatif commented Dec 2, 2015

Awesome. Thanks. 👍.

@ilatif
Copy link
Contributor Author

ilatif commented Dec 2, 2015

I' am thinking that we could create a static site generator from kemal. I' am impressed by it's speed and elegance and I hope it will be quite beneficial for developers which are always looking for better static site generators.

@sdogruyol
Copy link
Member

Thanks for your comments. I also think that Crystal thus Kemal is pretty fast 👍

ilatif added a commit to ilatif/kemal that referenced this issue Dec 3, 2015
First I tried implementing this solution in such a way that it
explicitly clears body and set `Content-Length` header to body's size.
But for some reason, if I call the URL from cURL then `Content-Length`
header was blank which defeats the very purpose of `HEAD` requests.

I then later anticipated that since `HEAD` would be by-default
implemented by `HTTP::Server` module, there is no need to explicit
clears body and setting `Content-Length` but the way we have written
our previous specs were returning body as well. We could have used some
TestServer kind of thing but if we go to that route we explicitly need
to test non-existent route which I thought would create some
inconsistency among specs.

Crystal has clearly written specs for HEAD requests to make sure body
is not read for them. See
crystal-lang/crystal@acd0b6afb5af438a30529c36b11b
e7954336f23f. I decided to write simple specs which are easy to
maintain in long-run.

We are adding identical HEAD route for every GET route which will make
HEAD requests available for all defined GET requests.

kemalcr#19
@ilatif
Copy link
Contributor Author

ilatif commented Dec 3, 2015

@sdogruyol: I submitted a pull request. Please see my comment which describes the steps I went through.

Thanks.

@sdogruyol
Copy link
Member

Hey @ilatif currently i'm preparing a presentation for Crystal i'll take a look at your PR as soon as i can 👍 Thanks a lot

@ilatif
Copy link
Contributor Author

ilatif commented Dec 3, 2015

Hi @sdogruyol, thanks for the prompt reply. Yeah sure, please take your time :-). Please share your presentation with me as well after you have presented it :-). I would love to see it :-).

Thanks.

ilatif added a commit to ilatif/kemal that referenced this issue Dec 4, 2015
First I tried implementing this solution in such a way that it
explicitly clears body and set `Content-Length` header to body's size.
But for some reason, if I call the URL from cURL then `Content-Length`
header was blank which defeats the very purpose of `HEAD` requests.

I then later anticipated that since `HEAD` would be by-default
implemented by `HTTP::Server` module, there is no need to explicit
clears body and setting `Content-Length` but the way we have written
our previous specs were returning body as well. We could have used some
TestServer kind of thing but if we go to that route we explicitly need
to test non-existent route which I thought would create some
inconsistency among specs.

Crystal has clearly written specs for HEAD requests to make sure body
is not read for them. See
crystal-lang/crystal@acd0b6afb5af438a30529c36b11b
e7954336f23f. I decided to write simple specs which are easy to
maintain in long-run.

We are adding identical HEAD route for every GET route which will make
HEAD requests available for all defined GET requests.

kemalcr#19

Added comment for code line which is adding HEAD routes for defined GET routes.
@ilatif
Copy link
Contributor Author

ilatif commented Dec 4, 2015

Closing this issue as this is implemented and merged.

@ilatif ilatif closed this as completed Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants