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.

Sign up for GitHub

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

Jump to bottom

head method in health endpoint #688

Merged
merged 2 commits into from
Sep 22, 2024
Merged

head method in health endpoint #688

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d13cc40
head method in health endpoint
moinologics Sep 18, 2024
2e0210d
Merge branch 'master' into head-method-healthcheck
eternal-flame-AD Sep 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion router/router.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

g.GET("/swagger", docs.Serve)
g.StaticFS("/image", &onlyImageFS{inner: gin.Dir(conf.UploadedImagesDir, false)})

Expand Down
Loading