head method in health endpoint #688
Conversation
| @@ -104,7 +104,7 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co | |||
|
|
|||
| ui.Register(g, *vInfo, conf.Registration) | |||
|
|
|||
| g.GET("/health", healthHandler.Health) | |||
| g.Match([]string{"GET", "HEAD"}, "/health", healthHandler.Health) | |||
There was a problem hiding this comment.
This is not compliant with HTTP specs. Please refer to the MDN docs and not return a body. Thanks!
https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD
There was a problem hiding this comment.
@eternal-flame-AD I would merge this as is. The RFC states:
The server SHOULD send the same header fields in response to a HEAD request as it would have sent if the request method had been GET. However, a server MAY omit header fields for which a value is determined only while generating the content.
I'd see it as recommendation to include content-length, but you can/MAY optionally omit it in some circumstances. So the current implementation complies with the recommendation.
I tested this with socat, seems like gin internally strips the body if the verb was HEAD
I think it's actually the native go http server, and it makes sense, as you need the body to get the content-length and given HEAD requests must not have a body I'd see this as working as intended and not as error-catching feature.
There was a problem hiding this comment.
It's not clearly stated that how acceptable possible inconsistencies are but I agree it seems to imply it's not super important, and in the remote possibility they are doing it in C we still have an always correct length in the actual GET.
Hyper/axum seem to do it too it seems. I just think the semantics looks like GET and HEAD would produce similar results because you just matched string-wise on the method and I'm not sure it's well-accepted for an http library to internally handle this for you.
|
@moinologics I tested this with socat, seems like gin internally strips the body if the verb was HEAD, so this code produces a compliant behavior (except dynamically generated JSON should not have a content-length on HEAD). However I still think this is not ideal, we shouldn't rely on undocumented error-catching features and write code with a looks-wrong semantics. Feel free to @ jmattheis for a second opinion :) Everybody has different styles and preferences |
|
Here's a solution I think is robust, (maybe even a little overkill?) , but at the very least I think connection, content type and content-encoding must be the same, and content length must not be present. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #688 +/- ##
=======================================
Coverage 80.46% 80.46%
=======================================
Files 56 56
Lines 2191 2191
=======================================
Hits 1763 1763
Misses 337 337
Partials 91 91 ☔ View full report in Codecov by Sentry. |
| @@ -104,7 +104,7 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co | |||
|
|
|||
| ui.Register(g, *vInfo, conf.Registration) | |||
|
|
|||
| g.GET("/health", healthHandler.Health) | |||
| g.Match([]string{"GET", "HEAD"}, "/health", healthHandler.Health) | |||
There was a problem hiding this comment.
It's not clearly stated that how acceptable possible inconsistencies are but I agree it seems to imply it's not super important, and in the remote possibility they are doing it in C we still have an always correct length in the actual GET.
Hyper/axum seem to do it too it seems. I just think the semantics looks like GET and HEAD would produce similar results because you just matched string-wise on the method and I'm not sure it's well-accepted for an http library to internally handle this for you.
Co-authored-by: 饺子w (Yumechi) <yume@yumechi.jp>
Fixes #687