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.

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

Fetching request body in the customErrorHandler middleware #3780

Open
lsgndln opened this issue Dec 4, 2023 · 7 comments
Open

Fetching request body in the customErrorHandler middleware #3780

lsgndln opened this issue Dec 4, 2023 · 7 comments

Comments

@lsgndln
Copy link

lsgndln commented Dec 4, 2023

🐛 Bug Report

Inside a custom error handler middleware with the WithErrorHandler option, I can't access the request body payload (for logging purpose for example). It seems empty (because it was probably already read) How could I do achieve this ?

To Reproduce

  1. In the NewServeMux function, pass a WithErrorHandler custom error handler function
	return grpcgw.NewServeMux(
                ...
		grpcgw.WithErrorHandler(customErrorHandler(logger)),
	)
  1. In the customErrorHandler function, try to log the request body. It is empty, probably because it has been read before.
return func(ctx context.Context, mux *grpcgw.ServeMux, marshaler grpcgw.Marshaler, writer http.ResponseWriter, request *http.Request, err error) {
    b, err := ioutil.ReadAll(req.Body)
    // b is empty

Sorry if I misreported it as a bug, I would appreciate some help.
Thanks a lot!

Expected behavior

I would expect to be able to read the request body.

Actual Behavior

I am not able to read the request body.

Your Environment

osx
go 1.21

@johanbrandhorst
Copy link
Collaborator

Hi, thanks for your issue. Yeah this is unfortunate, but I don't know that there's much we can do here. As you say, the body has already been consumed in this case. Note that it shouldn't universally be the case that the body has been consumed, it's likely your specific use case. For example, here the body should still be available: https://github.com/grpc-ecosystem/grpc-gateway/blob/main/runtime/mux.go#L348. If you track down the case that you're hitting, it might be possible to add something that would buffer the request body, but in all honest you're best off doing this at the middleware level where you have access to the request before it's being handled by the gateway runtime.

@richzw
Copy link
Contributor

richzw commented Mar 4, 2024

We met the same issue and we added one outer wrapper to clone the request body, since the body of the request has been consumed in the customErrorHandler.

type logResponseWriter struct {
	http.ResponseWriter
	statusCode int
}

func (rsp *logResponseWriter) WriteHeader(code int) {
	rsp.statusCode = code
	rsp.ResponseWriter.WriteHeader(code)
}

func NewLogResponseWriter(w http.ResponseWriter) *logResponseWriter {
	return &logResponseWriter{w, http.StatusOK}
}

func LogRequestBody(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		lw := NewLogResponseWriter(w)
		body, err := io.ReadAll(r.Body)
		if err != nil {
			// handle error
		}
		clonedR := r.Clone(r.Context())
		r.Body = io.NopCloser(bytes.NewReader(body))
		clonedR.Body = io.NopCloser(bytes.NewReader(body))

		h.ServeHTTP(lw, clonedR)

		if lw.statusCode != http.StatusOK {
			log.Printf("http error %+v, body %+v", w, string(body))
		}
	})
}
	mux := runtime.NewServeMux()

	return http.ListenAndServe(":8081", LogRequestBody(mux))

@richzw
Copy link
Contributor

richzw commented Mar 4, 2024

Hi, thanks for your issue. Yeah this is unfortunate, but I don't know that there's much we can do here. As you say, the body has already been consumed in this case. Note that it shouldn't universally be the case that the body has been consumed, it's likely your specific use case. For example, here the body should still be available: https://github.com/grpc-ecosystem/grpc-gateway/blob/main/runtime/mux.go#L348. If you track down the case that you're hitting, it might be possible to add something that would buffer the request body, but in all honest you're best off doing this at the middleware level where you have access to the request before it's being handled by the gateway runtime.

Maybe one default middleware to clone the request body on the flight, and log this request body when the status code of response is non-OK (200) or leave to the developers to custom this behavior. It just one immature thought.

@johanbrandhorst
Copy link
Collaborator

I think the workaround you shared here is probably the best solution to this, thank you!

@richzw
Copy link
Contributor

richzw commented Mar 8, 2024

I think the workaround you shared here is probably the best solution to this, thank you!

May I add this workaround to the samples through one PR? @johanbrandhorst

@johanbrandhorst
Copy link
Collaborator

Of course, please do

@richzw
Copy link
Contributor

richzw commented Mar 14, 2024

Of course, please do

Could you please review this PR? @johanbrandhorst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants