export some functions to let serveMux.protoErrorHandler interface work well #405

[jinleileiking]

#405

[jinleileiking]

I signed it!

[jinleileiking]

I signed it!

[jinleileiking]

ready to merge :)

[tmc]

What is the reason for this proposed change? Generally we should keep the exposed api as small as possible and it's not clear why this would be desirable. As is I would reject this change proposal.

[jinleileiking]

If you think

runtime.HandleForwardResponseServerMetadata
runtime.HandleForwardResponseTrailerHeader
runtime.HandleForwardResponseTrailer

is not commonly used.
You can close this issue.

But I think sometime, we need this 3 funcs.

[jinleileiking]

The standard DefaultHTTPError use these 3 funcs. see errors.go :81
the customized DefaultHTTPError will use these 3 funcs too. I think.

[jinleileiking]

If these 3 func is commonly used( every request). We should write a interface to let customize func run this 3 func automatically.

[yugui]

I am against these proposed commits themselves. But I understand the problem behind the proposal. So I'd like to propose a variant of the solution.

First of all, I basically agree with @tmc. I don't want to expose the functions as they are because they are very implementation details of the runtime and they are not designed from the perspective of the right granularity of APIs.

On the other hand, it have to say that it has been getting hard for users to write a right implementation of custom HTTPError handler because the header processing is getting better but complex.
Actually it was basically just

w.Header().Set("Content-Type", "application/json")
...
st := HTTPStatusFromCode(grpc.Code(err))
       
w.WriteHeader(st)
w.Write(buf)

in an earlier version. So it was easier to reproduce.
But now it requires the helper functions.

So I propose

• to extract a larger portion of response forwarding from DefaultHTPError
• to expose the extracted function.
  What do you think, @jinleileiking, @tmc?

The function would be, roughly

type ErrorResponse struct {
	Code int
	ContentType string
	MD ServerMetadata
	Body []byte
}

func WriteErrorResponse(ctx context.Context, w http.ResponseWriter, mux *ServeMux, resp ErrorResponse) error {
	w.Header().Del("Trailer")
	w.Header().Set("Content-Type", resp.ContentType)
	handleForwardResponseServerMetadata(w, mux, resp.MD)
	handleForwardResponseTrailerHeader(w, resp.MD)
	w.WriteHeader(resp.Code)
	if _, err := w.Write(resp.Body); err != nil {
		return err
	}

	handleForwardResponseTrailer(w, md)
	return nil
}

[jinleileiking]

@yugui What you said is just my want to say, lovely lady.

But I do not understand your codes

[yugui]

IIUC, if we had something like WriteErrorResponse you would be able to write your error handler by calling it instead of its implementation details (handleForwardResponseServerMetadata and others).

Is this understanding correct?

[jinleileiking]

Yes. Finally I understand your codes :)

[c4milo]

@yugui, @jinleileiking, any update on this issue? i'm willing to carry on any pending changes to make sure this is merged as soon as possible.

[jinleileiking]

I think we should wait @yugui codes :)

[jinleileiking]

I close this issue.