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

jhump · 2023-05-04T20:01:24Z

In order for #494 (sentinel error to return "304 not modified" status) to be safe to use, the handler actually needs to know whether the incoming request used a POST vs GET. It's only safe to use that error for conditional GET requests.

I haven't added tests yet because I wanted to maybe get a little feedback first on whether these additions are acceptable. The primary concern is about adding another unexported method to AnyRequest. Also, this is very targeted to only unary RPCs (nothing in the streaming flows ever attempts to expose this since they should always be POST anyway). But I didn't know if we should try to push more of this down further into the abstractions (though that's a bit tricky since the abstractions are exported interfaces StreamingClientConn and StreamingHandlerConn...).

Anyhow, if this idea seems work-able, then I'll finish this PR out with some tests.

In addition to making the request method available to server handlers, this also makes it available to clients -- when the unary RPC completes, the caller can inspect the request's method to see if it was sent via GET or POST.

WDYT?

jchadwick-buf

I think the general approach will work. I'm not thrilled by it, but when I tried to do this for the client I ran into the same thing. It's not great since obviously the earliest point you can know a request was going to be a GET request is after it's sent, and there's really no elegant way to implement this with how connect-go is currently architected. Therefore, I don't think there's much reasonable I can do other than give my general approval, albeit with the note that it is a bit begrudged :) At the very least, I think the API portion of this is good and what I think most people would expect, and that's the part we can't ever break, so that's the important bit.
I hope we can clean this up a bit later down the road by reorganizing the code to make it a bit less awkward, but there's no sense in trying to do it right now in my opinion. So yes. LGTM, with some caveats in the comments.

protocol_connect.go

client.go

connect.go

jhump · 2023-05-04T21:50:12Z

I didn't realize that Query was added to Peer (vs. adding it to Request, alongside Header() which seems like the closest attribute we expose).

So I guess the question -- potentially for @akshayjshah -- is whether this attribute should be added to Peer or to Request. I personally think it makes more sense in Request (and the fact that it's irrelevant for streaming RPCs, which are always POST, means we don't need any analog in the stream interfaces). But, perhaps it could be viewed as more consistent to put it next to the query params (though I see that is only ever populated for handlers, so putting it there might be more confusing...).

timostamm

Seems obvious to me that users need a way to distinguish GET requests in order to implement conditional requests. They shouldn't just look at If-* request headers.

The tests suggest:

https://github.com/bufbuild/connect-go/blob/e04e4bd94c1d91031cdb2de84a50be3c003b275f/error_not_modified_example_test.go#L48

It does not seem very intuitive. Method() string looks more discoverable to me, but I am wondering whether a Cacheable() bool might be a better fit since we are in RPC land, not HTTP land.

jchadwick-buf · 2023-05-05T11:31:43Z

I think I'm in favor of Method. Even though we're in RPC land, the abstraction is somewhat necessarily leaky here; we expose the concept of HTTP headers and trailers inside handlers, and obviously when setting up the RPC server there's no question that it is an HTTP handler. The reason we want to know the Method in handlers is primarily so that we can apply logic that is specific to HTTP GET anyways, and I think leaking the abstraction is the best option for that, since it gives the user the most flexibility and least confusion.
Certainly this PR is better than the if len(req.Peer().Query) > 0 hack, because at least with the way Connect handlers are generated, you could easily formulate a request that gets the wrong answer.

As for Peer vs Request - My knee-jerk thought is that it probably belongs in Request. To be honest, I'm not sure that Query really belongs in Peer either. I think I get what we were going for, but honestly, to me, the word "peer" implies that it's details about the peer itself, and to me the method/query are details about the request specifically. The way that we wound up with other details is because before a connection is made, a Client might have details in Peer that are taken from the URL rather than being representative of details about the peer itself, since there isn't a connection yet. But that's just my personal gut feeling.

From a user ergonomics standpoint, I definitely think calling req.Method() would be a more comfortable and obvious result than req.Peer().Method and even req.Cacheable(). Frankly, I think req.Query() would be more comfortable and obvious, too. Even if we didn't have a great justification for it, I think it would be nice to prioritize giving the user an obvious API that they can figure out without needing an example, when possible.

Another less-valid reason for why not to use Peer is one I experienced while trying to do it: it's very easy to wind up with data races when adding things to Peer that aren't known right away.

jhump · 2023-05-05T18:49:09Z

I'm not opposed to a more semantic method, such as Cacheable. My only concern is if there are (now or in the future) other implications of GET. Then it might be awkward to ask if the request is cacheable when we are actually intending to ask about some other property that happens to be true when method == "GET". And it would also be awkward to expand the API surface area with more accessors for those other properties. ¯\_(ツ)_/¯

I'd like to defer to @akshayjshah who can hopefully take a look at this early next week.

jchadwick-buf · 2023-05-09T14:11:17Z

This might become more urgent to figure out if we're going to want to resolve the TODO in connectrpc/connect-es#624 before merging, for similar functionality in connect-es. It'd be nice to have functional parity and unity on the API design front.

akshayjshah

I'm on board with exposing this on Request rather than Peer. I'd prefer to directly expose the HTTP verb rather than something semantic like Cacheable - let's stick as close to HTTP as we can.

In retrospect, I should probably have put the query params onto Request too. We can add a passthrough Request.Query() later on if there's demand - it doesn't seem likely to be used enough to double the API surface immediately.

@jhump When you add tests, can you please make sure to nail down how the new method behaves in interceptors? I'm particularly concerned that the current behavior will be a little misleading for client-side interceptors. Also make sure to update the example that Timo pointed out - it's part of the GoDoc.

akshayjshah · 2023-05-10T04:51:43Z

connect.go

+// 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 {


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.

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.

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()?

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.

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).

akshayjshah · 2023-05-10T04:53:41Z

connect.go

+type clientConnWithRequestMethod interface {
+	StreamingClientConn
+	onSetMethod(fn func(string))
+}


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.

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.

jhump · 2023-05-10T16:13:55Z

@akshayjshah, I think this is ready for another pass. I'm not sure what's up with CI. make test lint works locally. But in CI, the jobs aren't even getting that far, like they're failing due to something wonky going on in GitHub's infra...

jchadwick-buf · 2023-05-10T16:23:44Z

Yeah, don't worry about that, GitHub Actions is having trouble again today. I noticed the same. https://www.githubstatus.com/

jhump · 2023-05-15T17:51:10Z

@akshayjshah, ping, I was hoping for an LGTM from you before merging.

akshayjshah · 2023-05-15T17:52:46Z

Will look from the plane today! I'm in airport purgatory RN.

akshayjshah

LGTM - some small nits and suggestions, but no need for more review from me.

akshayjshah · 2023-05-15T21:48:23Z

interceptor_ext_test.go

+	assert.Equal(t, int32(2), client3.Load())
+	assert.Equal(t, int32(2), handler1.Load())
+	assert.Equal(t, int32(2), handler2.Load())
+	assert.Equal(t, int32(2), handler3.Load())


👍 appreciate the improvement here!

interceptor_ext_test.go

protocol.go

Fixes #502 to expose the HTTP method in handlers.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/bufbuild/connect-go](https://github.com/bufbuild/connect-go) | require | minor | `v1.7.0` -> `v1.8.0` | --- ### Release Notes <details> <summary>bufbuild/connect-go</summary> ### [`v1.8.0`](https://github.com/bufbuild/connect-go/releases/tag/v1.8.0) [Compare Source](https://github.com/bufbuild/connect-go/compare/v1.7.0...v1.8.0) #### What's Changed ##### Enhancements - Expose HTTP method of unary requests (see [documentation][http-method-docs]) by [@jhump](https://github.com/jhump) and [@akshayjshah](https://github.com/akshayjshah) in [https://github.com/bufbuild/connect-go/pull/502](https://github.com/bufbuild/connect-go/pull/502) and [https://github.com/bufbuild/connect-go/pull/509](https://github.com/bufbuild/connect-go/pull/509) - Implement `fmt.Stringer` interface for StreamType to improve debug logging by [@svrana](https://github.com/svrana) in [https://github.com/bufbuild/connect-go/pull/495](https://github.com/bufbuild/connect-go/pull/495) ##### Bugfixes - Fix documentation comments to use correct identifier name by [@cuishuang](https://github.com/cuishuang) in [https://github.com/bufbuild/connect-go/pull/498](https://github.com/bufbuild/connect-go/pull/498) - Fix Makefile quotes to handle paths with spaces by [@bufdev](https://github.com/bufdev) in [https://github.com/bufbuild/connect-go/pull/508](https://github.com/bufbuild/connect-go/pull/508) - Always close response body in `CloseRead` by [@pkwarren](https://github.com/pkwarren) in [https://github.com/bufbuild/connect-go/pull/515](https://github.com/bufbuild/connect-go/pull/515) #### New Contributors - [@cuishuang](https://github.com/cuishuang) made their first contribution in [https://github.com/bufbuild/connect-go/pull/498](https://github.com/bufbuild/connect-go/pull/498) - [@svrana](https://github.com/svrana) made their first contribution in [https://github.com/bufbuild/connect-go/pull/495](https://github.com/bufbuild/connect-go/pull/495) **Full Changelog**: bufbuild/connect-go@v1.7.0...v1.8.0 [http-method-docs]: https://connect.build/docs/go/get-requests-and-caching#distinguishing-get-requests </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/open-feature/flagd).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

#502) Add ability for clients and servers to inspect the HTTP method used for unary RPCs. Combined with #494, this enables support for conditional GET requests.

expose request method of unary requests to client and server handler

35e3b32

jhump requested a review from akshayjshah May 4, 2023 20:01

jchadwick-buf approved these changes May 4, 2023

View reviewed changes

protocol_connect.go Show resolved Hide resolved

client.go Outdated Show resolved Hide resolved

connect.go Outdated Show resolved Hide resolved

no need for anon func

198c746

timostamm reviewed May 4, 2023

View reviewed changes

timostamm mentioned this pull request May 9, 2023

Add server support for Connect HTTP GET requests connectrpc/connect-es#624

Merged

akshayjshah reviewed May 10, 2023

View reviewed changes

jhump added 2 commits May 10, 2023 11:23

addressing feedback, adding tests

19d0e12

more tests

cc96335

jhump marked this pull request as ready for review May 10, 2023 15:51

akshayjshah approved these changes May 15, 2023

View reviewed changes

address latest feedback

bc5d7db

jhump merged commit 17eb88d into main May 17, 2023

jhump deleted the jh/expose-method branch May 17, 2023 13:14

akshayjshah added a commit that referenced this pull request May 17, 2023

Expose HTTP method in unary handlers

b6ff498

Fixes #502 to expose the HTTP method in handlers.

akshayjshah mentioned this pull request May 17, 2023

Expose HTTP method in unary handlers #509

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jhump commented May 4, 2023 •

edited

Loading

jchadwick-buf left a comment

jhump commented May 4, 2023 •

edited

Loading

timostamm left a comment

jchadwick-buf commented May 5, 2023

jhump commented May 5, 2023

jchadwick-buf commented May 9, 2023

akshayjshah left a comment •

edited

Loading

akshayjshah May 10, 2023

jhump May 10, 2023

jhump May 10, 2023

akshayjshah May 10, 2023

jhump May 10, 2023

jhump commented May 10, 2023

jchadwick-buf commented May 10, 2023

jhump commented May 15, 2023

akshayjshah commented May 15, 2023

akshayjshah left a comment

akshayjshah May 15, 2023

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

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

Conversation

jhump commented May 4, 2023 • edited Loading

jchadwick-buf left a comment

Choose a reason for hiding this comment

jhump commented May 4, 2023 • edited Loading

timostamm left a comment

Choose a reason for hiding this comment

jchadwick-buf commented May 5, 2023

jhump commented May 5, 2023

jchadwick-buf commented May 9, 2023

akshayjshah left a comment • edited Loading

Choose a reason for hiding this comment

akshayjshah May 10, 2023

Choose a reason for hiding this comment

jhump May 10, 2023

Choose a reason for hiding this comment

jhump May 10, 2023

Choose a reason for hiding this comment

akshayjshah May 10, 2023

Choose a reason for hiding this comment

jhump May 10, 2023

Choose a reason for hiding this comment

jhump commented May 10, 2023

jchadwick-buf commented May 10, 2023

jhump commented May 15, 2023

akshayjshah commented May 15, 2023

akshayjshah left a comment

Choose a reason for hiding this comment

akshayjshah May 15, 2023

Choose a reason for hiding this comment

jhump commented May 4, 2023 •

edited

Loading

jhump commented May 4, 2023 •

edited

Loading

akshayjshah left a comment •

edited

Loading