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.

Sign up for GitHub

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

Jump to bottom

Add APIs to make and handle conditional GETs #494

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Add APIs to make and handle conditional GETs #494

merged 2 commits into from
Apr 18, 2023

Conversation

akshayjshah
Copy link
Member

If servers want clients to cache responses to HTTP GETs, they'll also
need a mechanism to return HTTP 304s. Unfortunately, we can't introduce
a new status code without breaking our promise that clients can switch
protocols with just a configuration flag, so we need a sentinel error
instead.

@akshayjshah
Add APIs to make and handle conditional GETs
2b0e1a2 
If servers want clients to cache responses to HTTP GETs, they'll also
need a mechanism to return HTTP 304s. Unfortunately, we can't introduce
a new status code without breaking our promise that clients can switch
protocols with just a configuration flag, so we need a sentinel error
instead.
jhump
jhump reviewed Apr 10, 2023
View reviewed changes
protocol_connect.go
@@ -518,7 +518,12 @@ func (cc *connectUnaryClientConn) validateResponse(response *http.Response) *Err
cc.compressionPools.CommaSeparatedNames(),
)
}
if response.StatusCode != http.StatusOK {
if response.StatusCode == http.StatusNotModified && cc.Spec().IdempotencyLevel == IdempotencyNoSideEffects {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible for the handler to detect whether the request was a GET vs a POST? Would that possibly be a better check here than just looking if the method is side-effect-free?

Copy link
Member

Choose a reason for hiding this comment

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

@jchadwick-buf, do you know the answer to above? I think either way, we can probably go ahead and merge this PR. If the answer is "no, there isn't yet a way for a handler to detect if current request was GET vs POST", then that is something that can be tacked on later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, no - currently, there's no way for clients to easily detect whether the request was a GET or a POST. (The GET/POST decision happens after interceptors run, so there wasn't any need for such an API before.) We could add a way to detect GETs directly in a later PR - it's feasible, but would be a bit gross.

Copy link
Member

Choose a reason for hiding this comment

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

I was just talking about handlers, not clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's not a great way to tell with handlers, though as a hack it is possible to inspect the Query value on Peer.

jhump
jhump approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

One question, otherwise LGTM.

@akshayjshah akshayjshah merged commit 6b6a0b8 into main Apr 18, 2023
@akshayjshah akshayjshah deleted the ajs/304 branch April 18, 2023 18:28
renovate bot referenced this pull request in open-feature/flagd Apr 20, 2023
@renovate
fix(deps): update module github.com/bufbuild/connect-go to v1.7.0 (#625)
1b24fc9 
[![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://togithub.com/bufbuild/connect-go)
| require | minor | `v1.6.0` -> `v1.7.0` |

---

### Release Notes

<details>
<summary>bufbuild/connect-go</summary>

###
[`v1.7.0`](https://togithub.com/bufbuild/connect-go/releases/tag/v1.7.0)

[Compare
Source](https://togithub.com/bufbuild/connect-go/compare/v1.6.0...v1.7.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

As of this release, the Connect protocol supports performing idempotent,
side-effect free requests using HTTP GETs. This makes it easier to cache
responses in the browser, on your CDN, or in proxies and other
middleboxes.

> **Note**
> This functionality is *only* supported when using the Connect
protocol—using a Connect client with a Connect server. When using
`grpc-go` clients, or `connect-go` clients configured with the
`WithGRPC` or `WithGRPCWeb` options, all requests will continue to be
HTTP POSTs.

To opt into GET support for a given Protobuf RPC, you must mark it as
being side-effect free using the
[MethodOptions.IdempotencyLevel](https://togithub.com/protocolbuffers/protobuf/blob/e5679c01e8f47e8a5e7172444676bda1c2ada875/src/google/protobuf/descriptor.proto#L795)
option:

```proto
service ElizaService {
  rpc Say(stream SayRequest) returns (SayResponse) {
    option idempotency_level = NO_SIDE_EFFECTS;
  }
}
```

With this schema change, handlers will automatically support both GET
and POST requests for this RPC. However, clients will continue to use
POST requests by default, even when GETs are possible. To make clients
use GETs for side effect free RPCs, use the `WithHTTPGet` option:

```go
client := elizav1connect.NewElizaServiceClient(
    http.DefaultClient,
    connect.WithHTTPGet(),
)
```

This functionality is *not* yet supported by other Connect
implementations (including `connect-es`), but hang tight! We're working
on it. For more information, please check the [full
documentation](https://connect.build/docs/go/get-requests-and-caching).

##### Enhancements

- Connect HTTP Get support by
[@&#8203;jchadwick-buf](https://togithub.com/jchadwick-buf) in
[https://github.com/bufbuild/connect-go/pull/478](https://togithub.com/bufbuild/connect-go/pull/478)
- Add APIs to make and handle conditional GETs by
[@&#8203;akshayjshah](https://togithub.com/akshayjshah) in
[https://github.com/bufbuild/connect-go/pull/494](https://togithub.com/bufbuild/connect-go/pull/494)

##### Bugfixes

- Fix `WithCompression` to match docs by
[@&#8203;jhump](https://togithub.com/jhump) in
[https://github.com/bufbuild/connect-go/pull/493](https://togithub.com/bufbuild/connect-go/pull/493)

**Full Changelog**:
bufbuild/connect-go@v1.6.0...v1.7.0

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

---

- [ ] <!-- rebase-check -->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).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS40OS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNDkuMCJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@jhump jhump mentioned this pull request May 4, 2023
Merged
jhump added a commit that referenced this pull request May 17, 2023
@jhump
Expose request method of unary requests to clients and server handlers (
17eb88d 
#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.
akshayjshah added a commit that referenced this pull request Jul 26, 2023
@akshayjshah
Add APIs to make and handle conditional GETs (#494)
75880e4 
If servers want clients to cache responses to HTTP GETs, they'll also
need a mechanism to return HTTP 304s. Unfortunately, we can't introduce
a new status code without breaking our promise that clients can switch
protocols with just a configuration flag, so we need a sentinel error
instead.
akshayjshah pushed a commit that referenced this pull request Jul 26, 2023
@jhump
Expose request method of unary requests to clients and server handlers (
ef2d883 
#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchadwick-buf jchadwick-buf jchadwick-buf approved these changes

@jhump jhump jhump approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@akshayjshah @jhump @jchadwick-buf