refactor(proto): adopt the connect-go simple API - #352
Conversation
PR Environment - Torn DownThe environment for this PR has been removed. |
704cbab to
44dd045
Compare
jos-
left a comment
jos-
left a comment
There was a problem hiding this comment.
LGTM. See two small comments below.
| // scriptedCall serves the scripted steps from an in-process Connect server and | ||
| // returns a plain client call with the same signature as generated clients, so | ||
| // the idempotency headers travel through the real call-info plumbing. | ||
| func scriptedCall(t *testing.T, steps []scriptStep, gotKeys *[]string) func(context.Context, *authnv1.ExchangeTokenRequest) (*authnv1.ExchangeTokenResponse, error) { |
There was a problem hiding this comment.
Small comment from Claude:
idempotent_create_test.go replaced the local fakeReq/fakeResp with an unrelated auth proto because the test now needs real messages on the wire. It works, but an idempotency test keyed on a token-exchange message reads confusingly. Either add a one-line comment saying the type is arbitrary, or define a minimal test proto.
There was a problem hiding this comment.
Added a note to the scriptedCall doc comment that the ExchangeToken types are arbitrary and only the headers matter (59b5ae7).
| return c | ||
| } | ||
|
|
||
| // ensureToken ensures we have a valid JWT, exchanging the API key if necessary. |
There was a problem hiding this comment.
Suggestion from Claude: Add a guard comment to ensureToken
client.go:64-77 is safe only because the nested client
context never reaches next(). That invariant is invisible at the call site, and ensureToken
has two callers (the interceptor and the exported ExchangeToken). One line noting that creating
a client context inside an interceptor is prohibited by connect would stop a future edit from
breaking it in a way no unit test would catch.
There was a problem hiding this comment.
Added a guard comment on the NewClientContext in ensureToken: the nested client context is only for the ExchangeToken call and must never flow into next(), since connect fails requests whose call info changes mid-flight (59b5ae7).
Also bumps the protoc-gen-connect-go pin in mise.toml to match, and picks up routine genproto updates via go mod tidy.
Generate all five Go proto modules with the protoc-gen-connect-go "simple" option. Handlers and clients now use plain message signatures instead of connect.Request/connect.Response wrappers, matching the API that connect-go v2 will make the default. Per-call metadata moves to context: handlers read headers via connect.CallInfoForHandlerContext, clients set them via connect.NewClientContext. Interceptors are unchanged. Two test suites that invoked handlers directly with header-carrying requests now go through httptest servers, since connect exports no way to fabricate a handler-side call-info context.
44dd045 to
59b5ae7
Compare
Summary
Migrates all Go connect code to the
protoc-gen-connect-gosimpleoption (connectrpc/connect-go#851, shipped in v1.19). Handlers and clients now use plain message signatures —Method(ctx, *Req) (*Resp, error)— instead ofconnect.Request[T]/connect.Response[T]wrappers. This matches the API that connect-go v2 (connectrpc/connect-go#951) will make the default, so the eventual v2 bump becomes mostly an import change.Two commits:
protoc-gen-connect-gopin in mise.toml) plus routine genproto bumps.opt: paths=source_relative,simplein all five Go proto modules (authn-api, organization-api, dcim-api, plugin-proxy, plugin-sdk metadata), 24 regenerated files, and ~230 migrated hand-written files.Notable changes beyond the mechanical unwrapping
connect.CallInfoForHandlerContext(ctx)(authn-api JWT/Authorization handling); clients set per-request headers viaconnect.NewClientContext(ctx)(functlensureToken, terraformrefreshToken, plugin-publishwithAuth— the latter redesigned from a request mutator into a context producer).connect.AnyRequest, whichsimpledoesn't change.createIdempotentgenerics now take plain call funcs; a fresh client context per retry attempt carriesIdempotency-Keyand readsIdempotency-Statusfrom response headers.httptestservers (mint_plugin_token_test.go, terraformidempotent_create_test.go): connect exports no way to fabricate a handler-side call-info context, so header-carrying direct calls are no longer possible. Coverage is equivalent (the real wire path is now exercised).connect.Client.CallUnary) — that's the correct low-level plumbing, mirroring generated simple clients.*connect.ServerStream[T]parameter; only the request wrapper is dropped. Frontend codegen (connect-es) is unaffected.Test plan
go build ./...,go vet ./...— cleangolangci-lint run --new-from-rev master— 0 issuesgo test -count=1 ./...— all packages pass except the pre-existingcluster-worker/pkg/handler/clusterharness failure, which also fails on master and is fixed separately in fix(cluster-worker): use trust auth in embedded-postgres test harness #351