-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature Request] Provide a legit way to override HTTP Response. #4483
Comments
Hi, thanks for your well written issue. I'm not sure that this meets the threshold of utility for a new feature (though minor in cost). I usually like to hear from two separate users that are having the same issue before considering it. To rephrase your request (correct me if I'm wrong):
Now, as an alternative, have you tried just not returning an error from the response option? Since you've called |
Yup, you're absolutely right about my request. But it's not just me, our organization uses this approach as well. Additionally, two similar issues I mentioned earlier requested similar functionality but haven't reached a conclusion yet. Currently, the For example: message GetNameResponse {
string name = 1;
} Here's the relevant server code snippet: mux := runtime.NewServerMux(
runtime.WithHealthEndpointAt(grpc_health.NewHealthClient(conn), "/health"),
runtime.WithMarshalerOption(runtime.MIMEWildcard, &runtime.JSONBuiltin{}),
runtime.WithForwardResponseOption(forwardResponse),
)
func forwardResponse(ctx context.Context, w http.ResponseWriter, m protoreflect.ProtoMessage) error {
switch v := m.(type) {
case *pb.GetNameResponse:
resp := map[string]any{
"success": true,
"data": m,
}
return runtime.ErrResponseHandled // (1)
default:
return nil
}
} Issue with Returning Returning {"success":true,"data":{"name": "John Doe"}}{"name": "John Doe"} But if we return error at {"success":true,"data":{"name": "John Doe"}} |
Yep, I see the problem. I think the workflow of having ultimate control to override the HTTP response is something we do want to support. Let me propose some other options (not saying no to this feature yet, just exploring the problem space):
Let me know what you think! |
2.3. It鈥檚 okay to use these approaches, but my aim is to override HTTP responses. We want to maintain a structured response with gRPC calls. |
Do you think this is sufficient for now? If so, would you be willing to contribute an update to our docs that mentions this pattern for fully overriding responses? We already have a section for setting headers but it would be good to complement it with a custom marshaler example. Thanks! |
Yep, I agree, this is sufficient for now! I'll squeeze in some time to update the docs with that custom marshaler example. |
馃殌 Feature Request
Issue with error logs for ForwardResponseOptions
PR #4245 changed how errors are logged, which is cool. But it also started logging a bunch of stuff we already aware.
My situation:
I need to override the response sometimes. To do that, I use
ForwardResponseOptions
with a custom function that might return an error. This tells grpc-gateway to chill and not write anything else. Works great, but then it fills the logs with these messages afterv2.20.0
:Tried a few things:
GRPC_GO_LOG_SEVERITY_LEVEL=none
would stop all logs, but I still need to see other errors.grpc.Logger
is also an option, but that's kinda overkill just to omit the simple log.Proposal:
Add a new variable like
ErrResponseHandled
. This would tellgrpc-gateway
to:Similar issues but have been closed:
Code examples (in case they help):
Here's what the change in
grpc-gateway/runtime/handler.go
would look like:And here's how I use it in my code:
Let me know what you think!
The text was updated successfully, but these errors were encountered: