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

Connect HTTP Get support #478

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Connect HTTP Get support #478

merged 13 commits into from
Apr 6, 2023

Conversation

jchadwick-buf
Copy link
Contributor

No description provided.

@jchadwick-buf
Add stable codec interface and implementations (#460)
5caa69e 
Unfortunately, protobuf offers no story whatsoever for canonicalization
of protobuf or protojson output. It seems the best we can get is to just
make it deterministic within each implementation.

Related issues:
* golang/protobuf#1121
* golang/protobuf#1373
@jchadwick-buf
Connect GET support (server) (#466)
af10789 
This is preliminary. It is split into parts:
- Plumbing of idempotency level from schema to handlers
- Implementation of method-based negotiation with Allow header
- Support for GET requests in Connect protocol
@jchadwick-buf
Connect GET support (client) (#473)
30fd7ec
@jchadwick-buf jchadwick-buf changed the title HTTP Get support Connect HTTP Get support Mar 17, 2023
@jchadwick-buf
Connect Get: Add serverside compression support (#475)
e39b053 
This was missing from the original server PR.

Q: Should we do anything for the accept header?
@jchadwick-buf
Rewrite HTTP Get options (#482)
439ad37 
* No longer a requirement to specify URL size limit
* Separate URL size limit option adds fallback parameter
akshayjshah
akshayjshah reviewed Mar 30, 2023
View reviewed changes
codec.go Show resolved Hide resolved
@akshayjshah
Copy link
Member

Overall, I really like the way this has shaped up! Connect's exported APIs don't change much:

  1. We introduce the IdempotencyLevel enum and WithIdempotency option. Even if we didn't want to support GETs, I think we'd want these changes to make automatic retry middleware work sanely.
  2. We introduce the WithHTTPGet() client option. I think this is as simple as opting into GETs can be.
  3. For users who run into URL size limits, we also add the WithHTTPGetMaxURLSize client option. I see the importance of this, but it's also a little esoteric; I wonder whether we ought to start by keeping it unexported and opening an issue for feedback.

@jchadwick-buf
Copy link
Contributor Author

  1. For users who run into URL size limits, we also add the WithHTTPGetMaxURLSize client option. I see the importance of this, but it's also a little esoteric; I wonder whether we ought to start by keeping it unexported and opening an issue for feedback.

Maybe we should re-think not having a default set here in that case; but it does put us in a hard spot, because as a user you wouldn't get to choose the fallback behavior. If we make it an error, the end user will probably need two clients just to have a graceful fallback. But if we make it fallback to POST, the end user can't even detect that its a GET or POST until after its finished.

Ultimately, most people would probably not have too much of an issue with either behavior, but neither seem particularly good as the only possible behavior.

@akshayjshah
Copy link
Member

Maybe we should re-think not having a default set here in that case; but it does put us in a hard spot, because as a user you wouldn't get to choose the fallback behavior. If we make it an error, the end user will probably need two clients just to have a graceful fallback. But if we make it fallback to POST, the end user can't even detect that its a GET or POST until after its finished.
Ultimately, most people would probably not have too much of an issue with either behavior, but neither seem particularly good as the only possible behavior.

Eh, I think the option shape we have is the best way to support the full breadth of options that I suspect we'll eventually need. However, people always surprise me with how not-picky they are; a small part of me wonders if WithHTTPGetMaxURLSize will be unused for years.

All that said, I'm fine keeping it exported. I feel more strongly about the consistent error handling for sure.

@jchadwick-buf jchadwick-buf marked this pull request as ready for review March 31, 2023 16:46
akshayjshah
akshayjshah approved these changes Mar 31, 2023
View reviewed changes
protocol.go Show resolved Hide resolved
@akshayjshah
Copy link
Member

Oh, please hold off on merging this until we chat next :)

I think we should (at least) cut a separate release with everything already on main, so that this is the only change in its release. We should probably also have a specification published and have the Envoy filter ready to PR.

jchadwick-buf
jchadwick-buf commented Apr 3, 2023
View reviewed changes
client_ext_test.go Outdated Show resolved Hide resolved
jchadwick-buf
jchadwick-buf commented Apr 3, 2023
View reviewed changes
option.go Outdated Show resolved Hide resolved
@akshayjshah
Copy link
Member

LGTM with the test added back.

@akshayjshah akshayjshah merged commit c80677b into main Apr 6, 2023
@akshayjshah akshayjshah deleted the get-support branch April 6, 2023 21:39
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>
akshayjshah added a commit that referenced this pull request Jul 26, 2023
@jchadwick-buf @akshayjshah
Connect HTTP Get support (#478)
019774d 
Co-authored-by: Akshay Shah <akshay@akshayshah.org>
@emcfarlane emcfarlane mentioned this pull request Dec 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akshayjshah akshayjshah akshayjshah approved these changes

@bufdev bufdev bufdev 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
@jchadwick-buf @akshayjshah @bufdev