Skip to content
New issue

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

#

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # 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