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

Expose request method of unary requests to clients and server handlers #502

Merged
merged 5 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ func NewClient[Req, Res any](httpClient HTTPClient, url string, options ...Clien
unarySpec := config.newSpec(StreamTypeUnary)
unaryFunc := UnaryFunc(func(ctx context.Context, request AnyRequest) (AnyResponse, error) {
conn := client.protocolClient.NewConn(ctx, unarySpec, request.Header())
if hasRequestMethod, ok := conn.(clientConnWithRequestMethod); ok {
hasRequestMethod.onSetMethod(func(method string) {
request.setRequestMethod(method)
})
}
// Send always returns an io.EOF unless the error is from the client-side.
// We want the user to continue to call Receive in those cases to get the
// full error from the server-side.
Expand Down
31 changes: 31 additions & 0 deletions connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ type Request[T any] struct {
spec Spec
peer Peer
header http.Header
method string
}

// NewRequest wraps a generated request message.
Expand Down Expand Up @@ -172,9 +173,27 @@ func (r *Request[_]) Header() http.Header {
return r.header
}

// Method returns the HTTP method for this request. This is nearly always
// POST, but side-effect-free RPCs could be made via a GET.
//
// On a newly created request, via NewRequest, this will return "POST" and
// only changes to return to "GET" after it is actually sent to a server
// using GET as the method.
func (r *Request[_]) Method() string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do we expect clients to have much use for this in application code? I can imagine it being helpful in metrics and logging interceptors, but I'm having a hard time thinking beyond that. That's not a problem, I'm just wondering if I'm missing something.
  2. I'm unsure about the user-visible behavior from the client side - it feels misleading to return POST before we know what the method will be. Maybe we could return the empty string and then change to either POST or GET once we're sure? I think we could do that by explicitly setting the method to POST in NewClient.unaryFunc (just before the block you've already added) and in Client.CallServerStream. Server-side, we'd need to set the method explicitly in NewServerStreamHandler. Then we could remove the block below that's special-casing the zero value.
  3. I avoided Method in connect-go because it's so ambiguous - are we talking about a method on a protobuf RPC service, or are we talking about HTTP methods? I chose Procedure for the former. Maybe we could name this HTTPMethod()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect clients to have much use for this in application code?

Like you mention, could be useful for metrics or logs. It's just to provide some observability of under-the-hood mechanics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to HTTPMethod() and also updated the behavior and docs (and verified in tests) that it returns empty string until the method is actually known (after RPC is sent).

if r.method == "" {
return http.MethodPost
}
return r.method
}

// internalOnly implements AnyRequest.
func (r *Request[_]) internalOnly() {}

// setRequestMethod sets the request method to the given value.
func (r *Request[_]) setRequestMethod(method string) {
r.method = method
}

// AnyRequest is the common method set of every [Request], regardless of type
// parameter. It's used in unary interceptors.
//
Expand All @@ -190,8 +209,10 @@ type AnyRequest interface {
Spec() Spec
Peer() Peer
Header() http.Header
Method() string

internalOnly()
setRequestMethod(string)
}

// Response is a wrapper around a generated response message. It provides
Expand Down Expand Up @@ -315,6 +336,16 @@ type handlerConnCloser interface {
Close(error) error
}

type handlerConnWithRequestMethod interface {
StreamingHandlerConn
getMethod() string
}

type clientConnWithRequestMethod interface {
StreamingClientConn
onSetMethod(fn func(string))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these interfaces need to embed StreamingHandlerConn and StreamingClientConn...and do we need them at all? I think we could get away with anonymous foo.(interface { getMethod() string})-style casts instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I redid this. This is now using an anonymous interface as suggested for the handler side. For the client-side, to implement the new semantics (always blank until after RPC is sent, and set after), we need to know when the RPC is sent for all client conn impls. So now they all implement it, and there's no need for a type assertion.


// receiveUnaryResponse unmarshals a message from a StreamingClientConn, then
// envelopes the message and attaches headers and trailers. It attempts to
// consume the response stream and isn't appropriate when receiving multiple
Expand Down
4 changes: 4 additions & 0 deletions duplex_http_call.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type duplexHTTPCall struct {
ctx context.Context
httpClient HTTPClient
streamType StreamType
onSetMethod func(method string)
validateResponse func(*http.Response) *Error

// We'll use a pipe as the request body. We hand the read side of the pipe to
Expand Down Expand Up @@ -150,6 +151,9 @@ func (d *duplexHTTPCall) URL() *url.URL {
// SetMethod changes the method of the request before it is sent.
func (d *duplexHTTPCall) SetMethod(method string) {
d.request.Method = method
if d.onSetMethod != nil {
d.onSetMethod(method)
}
}

// Read from the response body. Returns the first error passed to SetError.
Expand Down
5 changes: 5 additions & 0 deletions handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,16 @@ func NewUnaryHandler[Req, Res any](
if err := conn.Receive(&msg); err != nil {
return err
}
method := http.MethodPost
if hasRequestMethod, ok := conn.(handlerConnWithRequestMethod); ok {
method = hasRequestMethod.getMethod()
}
request := &Request[Req]{
Msg: &msg,
spec: conn.Spec(),
peer: conn.Peer(),
header: conn.RequestHeader(),
method: method,
}
response, err := untyped(ctx, request)
if err != nil {
Expand Down
10 changes: 9 additions & 1 deletion protocol_connect.go
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,10 @@ func (cc *connectUnaryClientConn) CloseResponse() error {
return cc.duplexCall.CloseRead()
}

func (cc *connectUnaryClientConn) onSetMethod(fn func(string)) {
cc.duplexCall.onSetMethod = fn
}

func (cc *connectUnaryClientConn) validateResponse(response *http.Response) *Error {
for k, v := range response.Header {
if !strings.HasPrefix(k, connectUnaryTrailerPrefix) {
Expand Down Expand Up @@ -719,6 +723,10 @@ func (hc *connectUnaryHandlerConn) Close(err error) error {
return hc.request.Body.Close()
}

func (hc *connectUnaryHandlerConn) getMethod() string {
return hc.request.Method
}

func (hc *connectUnaryHandlerConn) writeResponseHeader(err error) {
header := hc.responseWriter.Header()
if hc.request.Method == http.MethodGet {
Expand Down Expand Up @@ -928,7 +936,7 @@ type connectUnaryRequestMarshaler struct {
func (m *connectUnaryRequestMarshaler) Marshal(message any) *Error {
if m.enableGet {
if m.stableCodec == nil && !m.getUseFallback {
return errorf(CodeInternal, "codec %s doesn't support stable marshal; cam't use get", m.codec.Name())
return errorf(CodeInternal, "codec %s doesn't support stable marshal; can't use get", m.codec.Name())
}
if m.stableCodec != nil {
return m.marshalWithGet(message)
Expand Down