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

WithHealthzEndpoint should answer with application/json Content-Type #2611

Closed
GreyXor opened this issue Mar 25, 2022 · 4 comments · Fixed by #2634
Closed

WithHealthzEndpoint should answer with application/json Content-Type #2611

GreyXor opened this issue Mar 25, 2022 · 4 comments · Fixed by #2634
Labels
bug good first issue help wanted

Comments

@GreyXor
Copy link
Contributor

GreyXor commented Mar 25, 2022 •
edited
Loading

🐛 Bug Report

WithHealthzEndpoint didn't set Content-Type to anything but text/plain; charset=utf-8. Shouldn't it be application/json?

To Reproduce

  1. Enable WithHealthzEndpoint
  2. GET to the /healthz endpoint

Expected behavior

Content-Type: application/json
Grpc-Metadata-Content-Type: application/grpc

Actual Behavior

Content-Type: text/plain; charset=utf-8

Your Environment

Go 1.18 with grpc-gateway v2.10.0
Linux 5.17.0-arch1-1 #1 SMP PREEMPT Tue, 22 Mar 2022 23:36:09 +0000 x86_64 GNU/Linux
@johanbrandhorst
Copy link
Collaborator

Yeah this definitely seems reasonable. Would you like to contribute this fix?

@johanbrandhorst
Copy link
Collaborator

CC @brumhard @antonioiubatti93

@GreyXor
Copy link
Contributor Author

GreyXor commented Mar 26, 2022 •
edited
Loading

Yeah this definitely seems reasonable. Would you like to contribute this fix?

Sure, but i don't know where to define headers. I never looked at the code of grpc-gateway. I'll be looking at

@GreyXor
Copy link
Contributor Author

GreyXor commented Apr 9, 2022 •
edited
Loading

Since we stay in the WithHealthEndpointAt() method and we use the outgoing marshaler directly in it, we can't take advantage of the Content-Type deduction (except in the errorHandler). In my opinion it is easier in this case to directly add the headers before writing the response writer.

I invite you to review my PR with the proposed changes. I'm listening to your comments

@GreyXor GreyXor mentioned this issue Apr 9, 2022
Merged
johanbrandhorst pushed a commit that referenced this issue May 4, 2022
@GreyXor
fix: Add Content-Type header to the health endpoint (#2634)
61facaa 
* fix: Content-Type and Grpc-Metadata-Content-Type headers with the health endpoint #2611

* fix: no need to set grpc content type header

* fix: remove unnecessary grpc header comment

* test: lighten the Content-Type header test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good first issue help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Content-Type and Grpc-Metadata-Content-Type headers with the health endpoint
2 participants
@johanbrandhorst @GreyXor