Proposal v2 - #951
Conversation
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
|
Wow, it's a shame we actually never discovered the simple API until this post now. Definitly looking forward to using simple API and also in-memory transport! |
|
skill or |
Simplify the dependencies by including the tool in the root module. It only adds golang.org/x packages. This keeps version semantics simple: one v2 tag releases the library and the tool together. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
|
Great job on this. Generics absolutely seemed like a real win and I was actually excited at how simple it seemed at the time, but the more I actually used ConnectRPC the more the generics got in the way, especially with testing. I really appreciate the bravery to switch back to the "standard" method when this experiment turned out to more pain than its worth. And the more I read through the changes in v2, the more I realize that I've built around most of the cited limitations from v1: I've built special interceptors to avoid non-connect errors leaking, helpers to test connect APIs locally, authentication as an HTTP middleware instead of an interceptor, etc. RE: pluggable transports: I know there's talk about Websocket support, but I have a WebTransport prototype cooking that I might have enough time soon to finish and contribute to connect-go and connect-es. But there's choices around how to frame messages and headers and the general connection strategy that should be aligned with WebSockets, so we can talk on the Buf slack about this soon. |
There was a problem hiding this comment.
When using connect-go-v2-migrate on large codebases it can take a little while before this script has any output. I think it might be nice to have a log message telling users that the script is started and is working. For a second, I thought this might be some kind of plugin that needed stdin or something.
There was a problem hiding this comment.
Thanks for trying this out, will update. Interested to hear about your WebTransport prototype too!
bufdev
left a comment
There was a problem hiding this comment.
This looks good to me, and seems like a large improvement.
| // ServerInterceptor wraps a [ServerFunc]. It may return an error without | ||
| // calling next to short-circuit the RPC. | ||
| // | ||
| // Server interceptors surround the full server invocation. To observe or | ||
| // modify individual messages, interceptors can pass a wrapped [ServerStream] | ||
| // to next. | ||
| type ServerInterceptor func(next ServerFunc) ServerFunc |
There was a problem hiding this comment.
In the PR description, it looks like this would be executed before unmarshaling the request body. Indeed, ServerFunc uses ServerStream which does not imply anything about an unmarshaled message.
However, the referenced migration guide uses protovalidate as an example which works upon a (somewhat?) unmarshaled message (see https://github.com/connectrpc/connect-go/blob/v2/docs/v2-migration.md#server-construction). I couldn't see a v2 PR/branch in https://github.com/connectrpc/validate-go, so I couldn't make much progress in figuring out what actually happens there.
I note that ServerStream says Receive cannot be called concurrently, but it doesn't say if it can/can't be called multiple times. I'm not sure if that's an oversight or working as intended. Assuming it's the latter, does that mean that interceptors that operate upon the message, i.e. validate, itself need to determine the appropriate message type (via Spec.Schema), then call ServerStream.Receive, then do whatever work they need to? If that's true, I further assume there's a helper somewhere that can be re-used by interceptors (as connect-go would presumably want said helper between calling interceptors and the eventual handler)?
Conversely, how does this work when you want to validate the outgoing response from the server?
There was a problem hiding this comment.
Hi matt, thanks for looking through. We will have v2 versions of all these ecosystem plugins. These APIs are based into some of the migration plan but these PRs are not active yet as they won't build until a v2.0.0-alpha.1 release of connect-go v2. I've pushed up the connect-go v2 version of validate here: https://github.com/connectrpc/validate-go/compare/ed/v2
The API is adapted to the v2 server and client interceptor model. It wraps the Send and Receive methods on the server or client stream.
func NewClientInterceptor(opts ...Option) connect.ClientInterceptor
func NewServerInterceptor(opts ...Option) connect.ServerInterceptorThere was a problem hiding this comment.
I've pushed up the connect-go v2 version of validate here: https://github.com/connectrpc/validate-go/compare/ed/v2
Thanks!
It wraps the Send and Receive methods on the server or client stream.
That's neat, I'm not sure I'd have thought to become the ServerStream interface myself. Thanks for clarifying.
This pull in the latest Go version for a HTTP/2 fix. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
pkwarren
left a comment
There was a problem hiding this comment.
Really nice to see the simplified code without generics and better separation of concerns (http, proto, compression packages). The inprocess module is going to be a great addition.
Only major comment is that we might consider making the migrator be its own go module (so it can be released separately). This would also prevent migrator dependencies (x/tools, x/mod) from making it into the top-level go.mod.
| } | ||
| var buf bytes.Buffer | ||
| buf.Grow(len(raw)) | ||
| if err := json.Compact(&buf, raw); err != nil { |
There was a problem hiding this comment.
Do we want to look at something like https://pkg.go.dev/encoding/json/v2@go1.27.0#Deterministic for go 1.27+?
There was a problem hiding this comment.
Yes I think so, but will do outside of this change.
| out: gen | ||
| opt: paths=source_relative | ||
| - - remote: buf.build/connectrpc/go:v1.18.1 | ||
| + - remote: buf.build/connectrpc/go:v2.0.0 |
There was a problem hiding this comment.
This may cause errors for users using the migration tool before a v2.0.0 is released and published as a plugin. Not sure if it is worth printing a warning for now and updating the tool when we have a remote plugin available.
There was a problem hiding this comment.
I agree. A warning asking the user to switch to a local plugin would be nice.
timostamm
left a comment
There was a problem hiding this comment.
Looks great to me. Left one comment about the API, and a couple of questions regarding the release.
| client := connect.NewClient( | ||
| connecthttp.NewTransport(http.DefaultClient, "http://localhost:8080"), | ||
| clientLoggingInterceptor, | ||
| ) |
There was a problem hiding this comment.
The layering makes sense, but would it be possible to have a connecthttp.NewClient() for convenience?
There was a problem hiding this comment.
Theres options for both the transport and client. These are passed as varargs to the constructors. The function signature of options for connecthttp.NewClient is not a clean replacement, but could be:
func NewClient(httpClient HTTPClient, baseURL string, interceptors []connect.ClientInterceptor, options ...Option) *connect.Client)
There was a problem hiding this comment.
The reason I'm asking is that the example - which demonstrates a simple call - grows the client construction from a single function call to three function calls.
Again, the layering makes sense, but I hope that we find a way to provide great ergonomics for the cases that do not require a non-http transport, h2c, etc. The idea is to provide sensible defaults for the simple case, and gradually configure with options or construction of the individual layers for more specialized cases.
For example, simple case:
pingClient := pingv1connect.NewPingServiceClient(
connecthttp.NewClient("http://localhost:8080"),
)With options:
pingClient := pingv1connect.NewPingServiceClient(
connecthttp.NewClient("http://localhost:8080",
connecthttp.WithInterceptors(clientLoggingInterceptor),
connecthttp.WithGRPC(),
connecthttp.WithHTTPClient(h2cClient),
),
)With the individual layers:
client := connect.NewClient(
connecthttp.NewTransport(h2cClient, "http://localhost:8080", connecthttp.WithGRPC()),
clientLoggingInterceptor,
)
pingClient := pingv1connect.NewPingServiceClient(client)
userClient := userv1connect.NewUserServiceClient(client)I'm probably skipping over several difficult questions with this, and it's very possible that it does not work out. I suggest doing nothing for now, cut an alpha release, and then investigate whether it's feasible with some more hands-on experience with the ergonomics.
| This module, `connectrpc.com/connect/v2`, is in beta. | ||
| The `v2` module will be published on the `main` branch of the repository when released. |
There was a problem hiding this comment.
It's a bit unclear to me what that means. Is the plan to merge this branch to main and maintain v1 in a v1 branch, or to keep the v2 branch going until a stable release? I believe that merging to main is more user-friendly because the docs and migration guide are easier to discover.
RELEASE.md still references v1.
There was a problem hiding this comment.
The intent is to merge v2 to main and create a v1 branch to maintain v1. The first release will be v2.0.0-alpha.1. It can be off this v2 branch, which would help users test this proposal, but the intent is to merge to main first and do the alpha release off main. The RELEASE.md needs updating.
Separates `connect-go-v2-migrate` into its own package and fixes related issues. Updates RELEASE.md docs to support nested module. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
Set a default ReadMaxBytes of 4 MiB for both clients and handlers. Both previously defaulted to no limit. This is a v2 behavior change. The migration tool is updated to apply the v1 unbounded behavoir when not set. It will also warn users to prompt setting a limit. Setting `connecthttp.WithReadMaxBytes(0)` restores the v1 behavior, and existing `WithReadMaxBytes(n)` calls are unaffected. Handlers that want to bound the total size of a request stream rather than individual messages should still use `http.MaxBytesHandler`. Supersedes #961 --------- Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
This PR introduces a proposal for a new major version of connect-go.
First, don't panic! A couple caveats:
v2ofconnect-godoesn't mean a v2 of ConnectRPC. This is a set of breaking changes to the Go API that we think set upconnect-gofor the future, not breaking changes to ConnectRPC the ecosystem, the wire protocol, or ConnectRPC's compatibility with gRPC. This is analogous to our v2 of connect-es a couple years ago.v1ofconnect-gois not going anywhere.v1has been widely adopted at some of the world's biggest orgs, and its support is indefinite. To make surev1keeps up with the latest changes, if this proposal is accepted, we intend to rewrite the internals ofv1in terms ofv2.OK, so why do this?
The ConnectRPC project has always been simplicity and compatibility. ConnectRPC strives to provide a gRPC-compatible RPC framework that also works with plain HTTP/JSON interactions, without importing any of the bloat that other RPC frameworks may have. Major versions are the opposite of simplicity and compatibility.
However, after a lot of consideration, we think it's worth it.
connect-gowas our first Connect product, released just over four years ago. We think that, while ConnectRPC's success has shown that a lot of the principles have stood the test of time, there's some key changes we think are necessary to set upconnect-gofor its next half-decade of success.The big changes:
Stop all the generics
connect-gocurrently uses generics extensively in its default API, for example by using aconnect.Request[T]andconnect.Response[T]in every unary function signature. The intent was to access metadata without reaching intocontext.Contextin an untyped manner, but this cut against the grain of standard Go RPC signatures, and caused a lot of consternation. Other RPC frameworks follow the standard(context.Context, *FooRequest) (*FooResponse, error)signature, which is well understood and unsurprising to most engineers.Additionally, the use of generics massively bloated binary sizes. The compiler stamps out a separate copy of the client and its methods for every RPC. As one example, in the Buf CLI, ConnectRPC-related entries account for nearly 1/3 of all function and type-name bytes. In total,
connectrpc.com/connectand its instantiations are 12–13% of the stripped binary.We introduced the simple API to address this, but it resulted in two parallel APIs, which is the opposite of simplicity.
v2effectively makes the simple API the default and only option. Almost all users, when made aware of the simple API, chose it over the existing generic-based default.Remove net/http as a core interface dependency
The
v1API heavily relies onnet/httpfor both its implementation and its interface.v2retains the former - ConnectRPC implementations always rely on the idiomatic HTTP implemenation in every language - but disconnects the API from usingnet/httptypes directly. This enables some key use cases. Some examples:To accomplish this, we modeled connect-es and introduced a
Transportinterface. The newconnecthttpsub-package implements this transport, speaking the Connect, gRPC, and gRPC-Web protocols overnet/http.Upgraded interceptor design
In
v1, interceptors cannot see the whole call. A unary interceptor receives an already-decoded message, pinning its execution to after decompression and unmarshaling. This forces packages likeconnectrpc.com/authnto be HTTP middleware rather than interceptors, to prevent unauthenticated clients from triggering decompression work. It also skews metrics inconnectrpc.com/otelconnect, as unary calls exclude unmarshal time while streams do not. The interface compounds this, asking for up to three implementations (WrapUnary,WrapStreamingClient,WrapStreamingHandler) with subtly different semantics for what is conceptually one job.In
v2, we replace the three-method interceptor interface with two function types,ClientInterceptorandServerInterceptor. A single implementation wraps the entire call, unary or streaming, and runs before any payload work. Authentication can now reject a call from its headers alone, and metrics can observe the full call, down to the bytes on the wire.Other improvements
While we're at it,
v2also tackles a few other issues unaddressable in a minor.CallInfo, reached through the context. This is the same pattern the simple API proved out, and it will feel familiar to developers coming from other RPC frameworks.v1default. In v1, an error returned from a handler is serialized straight to the client, a potential information leak.In v2, only an explicitly constructed
*connect.Errorcarries its message to the wire. Any other error reaches the client asCodeUnknownwith no message.Construction reflects the same split.
connect.NewErrortakes the public message, and.WithCause()attaches the private underlying error locally.Finally,
v1accumulated roughly 180 exported symbols in a single package.v2splits the module into focused packages (connect,connectproto,connectgzip,connecthttp), keeping the core small.The
v2guide expands on every decision above. See Why a new major version for details.Migration
You do not need to migrate, as
v1will continue to function indefinitely. However, in so far as you'd like to take advantage of the features inv2, we want to make it as easy as possible. We built a migration tool,connect-go-v2-migrate, to help automate the bulk of the mechanical translation.Users run the tool to scan their packages and config files. By default, it performs a dry run, providing a clear diff of the automated changes alongside warnings for anything that requires a human decision. Pass
-wto write the changes, or-jsonfor a machine-readable report.Read the v2 migration guide for more details.
Reviewer's guide
To help navigate this large diff, we recommend reviewing in the following order:
connect.gofor the new core types. The helper packagesconnectinprocess,connectgzip, andconnectprotoall show the use of these core types.connecthttppackage to see how thenet/httpboundary was extracted. This package uses the original v1 implementation wherever possible.cmd/protoc-gen-connect-go/for the updates to the code generator. The resulting generated output can be found in theinternaldirectory.cmd/connect-go-v2-migrateand its testdata scripts for the migration tooling.Let us know your thoughts
We've been working on this proposal for a while now as a maintainer group; It's time to hear from others: let us know what you think, we'd love to hear, and let's see if we can get this across the line.