feat(grpc_datasource): add ConnectRPC transport implementation#1509
feat(grpc_datasource): add ConnectRPC transport implementation#1509fengyuwusong wants to merge 20 commits into
Conversation
Replace the hand-rolled protoc invocation with a buf-driven pipeline that emits both gRPC and ConnectRPC stubs from product.proto. The new productv1connect package exposes the Connect handler/client interfaces alongside the existing gRPC code, and MockServiceConnect wires the existing MockService into the Connect handler so the same backing implementation can serve gRPC, gRPC-Web, and Connect from one HTTP endpoint. This is the fixture the follow-up Connect transport tests exercise. MockServiceConnect is intentionally hand-maintained rather than generated; it is a pure passthrough that only needs touch-ups when MockService method signatures change.
Implement RPCTransport on top of connect-go so the data source can dial
ConnectRPC subgraphs without changing the planner, compiler, or JSON
builder. NewConnectTransport accepts a base URL plus a wire format
(ConnectEncodingProtobuf "proto" or ConnectEncodingJSON "json"), an
optional HTTP client, and a slice of connect.Interceptor for tracing,
logging, retries, or auth header injection.
Each procedure is served by a cached connect.Client built around a
custom codec that binds dynamicpb.Message to the expected response
descriptor at unmarshal time; connect-go's default proto codec cannot
do this because it would allocate a descriptor-less *dynamicpb.Message.
The cache is intentionally unbounded because the set of procedures is
bounded by the data source schema.
Outgoing grpc/metadata is copied onto the Connect request headers using
the same conventions as the gRPC transport, with binary headers (the
"-bin" suffix) base64-encoded per the Connect spec. Errors are wrapped
with %w so callers can still errors.As the underlying *connect.Error
to inspect Code, Message, Details, and Metadata. The response is merged
into a reset output message to avoid accumulating repeated-field values
across invocations.
The RPCTransport interface contract is now documented in transport.go,
covering the methodFullName format ("/package.Service/Method") and the
input/output expectations that both gRPC and Connect implementations
share.
Tests cover protobuf and JSON wire formats, header passthrough
(including binary), error mapping, context cancellation, trailing-slash
BaseURL handling, and the HTTPClient: nil default-fallback path.
Dependencies: adds connectrpc.com/connect v1.19.2 and promotes
golang.org/x/net from indirect to direct.
Mirror the existing MockService end-to-end happy path, but route the call through NewConnectTransport against an httptest server backed by the buf-generated MockServiceConnect handler. Two tests cover the protobuf and JSON wire formats; both prove that the full data source pipeline (compiler -> JSON builder -> transport -> response unmarshal) behaves identically over Connect, using the same fixture, schema, and mapping as the gRPC path.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Connect-based gRPC transport with dynamic protobuf/JSON codecs, Buf codegen and Makefile updates, a Connect adapter for the mock service, GraphQL datasource wiring to accept a Connect RPCTransport, and unit plus end-to-end tests for protobuf and JSON encodings. ChangesConnect gRPC Transport Implementation
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConnectTransport
participant ConnectClient
participant RemoteService
Caller->>ConnectTransport: Invoke(ctx, methodFullName, input, output, md)
ConnectTransport->>ConnectClient: Build request (headers, codec)
ConnectClient->>RemoteService: HTTP POST (proto/json)
RemoteService-->>ConnectClient: Response body
ConnectClient-->>ConnectTransport: Decoded dynamicpb.Message
ConnectTransport->>ConnectTransport: Reset & merge into output
ConnectTransport-->>Caller: return (error/nil)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go (1)
180-181: ⚡ Quick winUse structured assertions for the JSON transport path too.
Substring checks are permissive and can pass on malformed or partially incorrect payloads. Unmarshal and assert typed fields (same as the protobuf test) to keep both e2e paths equally strict.
Suggested fix
- require.Contains(t, string(output), `"id":"test-id-123"`) - require.Contains(t, string(output), `"name":"Test Product"`) + type response struct { + Data struct { + ComplexFilterType []struct { + Id string `json:"id"` + Name string `json:"name"` + } `json:"complexFilterType"` + } `json:"data"` + } + var resp response + require.NoError(t, json.Unmarshal(output, &resp)) + require.NotEmpty(t, resp.Data.ComplexFilterType) + require.Equal(t, "test-id-123", resp.Data.ComplexFilterType[0].Id) + require.Equal(t, "Test Product", resp.Data.ComplexFilterType[0].Name)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go` around lines 180 - 181, Replace the permissive substring checks on the JSON transport output with structured JSON assertions: unmarshal the output bytes (variable output) into a typed struct or map (mirroring the protobuf test's fields), then use require.Equal/require.NotNil to assert the id equals "test-id-123" and the name equals "Test Product" (same typed fields used in the protobuf test) so the JSON transport path is validated strictly rather than via contains checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go`:
- Around line 116-117: The test currently indexes resp.Data.ComplexFilterType[0]
directly which can panic when the slice is empty; update the test in
grpc_datasource_connect_test.go to first assert the slice is non-empty (e.g.,
use require.NotEmpty or require.Greater(t, len(resp.Data.ComplexFilterType), 0)
on resp.Data.ComplexFilterType) and only then assert the element fields (Id and
Name) from resp.Data.ComplexFilterType[0], so failures report assertion errors
instead of panics.
---
Nitpick comments:
In `@v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go`:
- Around line 180-181: Replace the permissive substring checks on the JSON
transport output with structured JSON assertions: unmarshal the output bytes
(variable output) into a typed struct or map (mirroring the protobuf test's
fields), then use require.Equal/require.NotNil to assert the id equals
"test-id-123" and the name equals "Test Product" (same typed fields used in the
protobuf test) so the JSON transport path is validated strictly rather than via
contains checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 181c5c95-0f96-4e7d-96bb-9286bb23d180
⛔ Files ignored due to path filters (1)
v2/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
v2/go.modv2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.gov2/pkg/engine/datasource/grpc_datasource/transport.gov2/pkg/engine/datasource/grpc_datasource/transport_connect.gov2/pkg/engine/datasource/grpc_datasource/transport_connect_test.gov2/pkg/grpctest/Makefilev2/pkg/grpctest/buf.gen.yamlv2/pkg/grpctest/buf.yamlv2/pkg/grpctest/mockservice_connect.gov2/pkg/grpctest/productv1/productv1connect/product.connect.go
Assert resp.Data.ComplexFilterType is non-empty before indexing into it. A regression that returns an empty slice would otherwise surface as an opaque index-out-of-range panic instead of a clear assertion failure. Addresses coderabbitai review feedback on wundergraph#1509.
|
Hi @Noroth , Could you please take a look? Thanks! |
|
Hi @fengyuwusong can you let me know how you planned the next steps preferably in your own words please. This PR only adds support for the connect protocol but currently it cannot yet be wired up to the router because that part would be in the graphql_datasource. Are you going to raise a new PR then when this was merged because you wanted to keep the changes small or was it forgotten? I'm fine with both approaches just want to know how you planned the implementation process. Thanks for all the work so far! |
Hello @Noroth, I am sorry about that. Because my English is poor, so I use Claude to improve my words. Now I try to use my own words to explain the next steps about how to support the ConnectRPC protocol.
The above is my rough plan. Please see if it matches your expectations. Thanks for your patient review. If you have any questions, feel free to ask anytime~ |
Expose the ConnectRPC transport (added in this PR's earlier commits) on
the GraphQL data source planner so subgraphs declared with a Connect
endpoint dial over the Connect transport instead of a gRPC client
connection.
- configuration.go: new ConnectConfiguration{BaseURL, Encoding} field on
ConfigurationInput, plus a Connect != nil arm in the
"fetch or subscription or grpc or connect" validation. Connect reuses
the gRPC mapping/compiler for proto schema definitions, so the GRPC
configuration is required even when only Connect will be dialed at
runtime.
- graphql_datasource.go:
* Planner and Factory carry a connectTransport field (RPCTransport).
* The data source factory dispatches: connectTransport when set, else
NewGRPCTransport(p.grpcClient). The dispatch sits inside the
existing grpc != nil branch because both transports share the same
mapping/compiler path; selection is per-Factory.
* NewFactoryConnect constructor wires a Connect transport into the
planner without requiring a gRPC client.
* AlwaysFlattenFragments now also returns true when only a Connect
transport is configured.
* Error message switches from "failed to create gRPC datasource" to
the transport-agnostic "failed to create datasource".
Addresses @Noroth's review comment on wundergraph#1509 about the wire-up step
living in graphql_datasource. The router-side configuration plumbing
will land in the follow-up cosmo PR.
…ration Six unit tests pin the wiring contract introduced in the preceding feat commit: - NewFactoryConnect rejects a nil context or a nil transport. - A Connect-backed Factory propagates its RPCTransport to every Planner it constructs and reports AlwaysFlattenFragments=true so the planner emits inline fields. - NewConfiguration refuses a Connect-only ConfigurationInput, since Connect reuses the gRPC mapping/compiler. - NewConfiguration accepts Connect when paired with GRPC and the resulting Configuration reports both IsGRPC() and IsConnect(). - The empty-configuration error message lists "connect" alongside the existing transports. The full transport-level pipeline is already exercised in pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go, so these tests focus on the dispatch contract rather than duplicating the end-to-end path.
…ruct Bring the JSON-encoded happy-path test in line with the protobuf-encoded one by unmarshalling the response into a typed struct and asserting on the decoded fields, instead of using permissive substring checks against the serialised bytes. A regression that emits a malformed or partially correct payload would slip past the previous require.Contains assertions but now triggers a clear failure. Addresses coderabbitai nitpick on wundergraph#1509.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 364-373: The transport selection currently uses p.connectTransport
presence as a factory-wide override; change it to honor the datasource config by
checking p.config.IsConnect() first: if IsConnect() is true, require
p.connectTransport to be non-nil and use it or return a hard error; if
IsConnect() is false, require p.grpcClient to be non-nil and use
grpcdatasource.NewGRPCTransport(p.grpcClient) or return a hard error; update the
selection logic around p.connectTransport, p.grpcClient, and
grpcdatasource.NewGRPCTransport and ensure NewFactoryConnect/NewFactoryGRPC
callers still provide the matching transport/client so the error surfaces when
mismatched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ef81d55-df74-4003-aac6-1f70606536ec
📒 Files selected for processing (4)
v2/pkg/engine/datasource/graphql_datasource/configuration.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource_connect_test.gov2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go
The previous dispatch keyed off Planner.connectTransport != nil, which
made the choice of wire protocol a factory-wide override rather than a
per-subgraph decision. Two configurations could silently dial the wrong
protocol:
- NewFactoryGRPC + ConfigurationInput{Connect: ...}: factory has no
connect transport, so the planner falls back to gRPC even though
the data source asked for Connect.
- NewFactoryConnect + ConfigurationInput{GRPC only}: factory carries
a connect transport, so the planner uses it even though the data
source did not ask for Connect.
Both cases now read p.config.IsConnect() as the source of truth and
require the matching factory-side resource:
- IsConnect() true and no connect transport ->
"connect configuration requires a connect transport"
- IsConnect() false and no grpc client ->
"grpc configuration requires a grpc client"
A consistent factory + configuration pair behaves exactly as before;
mismatches surface a hard error at plan time rather than at the wire.
Addresses coderabbitai review on wundergraph#1509.
|
This PR is done, please feel free to take a look. Thank you @Noroth! |
…guration dispatch
The previous fix used p.config.IsConnect() as the sole discriminator,
but in practice the cosmo router (the primary consumer) wires
ConnectRPC subgraphs by constructing a Factory through NewFactoryConnect
and passing the transport in directly. The Configuration carried into
the planner only holds the gRPC mapping/compiler (which Connect reuses)
and never gets a populated Connect block, so the Configuration-driven
branch always fell through to "grpc configuration requires a grpc
client" and every Connect subgraph errored at plan time.
Reorder the switch so the factory-supplied transport wins first:
- p.connectTransport != nil -> use it; the Factory was built with
NewFactoryConnect, which is itself the explicit intent statement.
- p.config.IsConnect() -> the caller wanted Connect but did not
supply a transport; surface a hard error so the misconfiguration is
visible at plan time.
- p.grpcClient != nil -> native gRPC dial.
- otherwise -> neither resource available; error.
This keeps the misuse detection coderabbitai asked for on
NewFactoryGRPC + Configuration{Connect}, while unbreaking the common
NewFactoryConnect path used by callers that treat the Factory as the
transport-selection point.
|
Hi @fengyuwusong, I appreciate all the work you're doing! |
Two findings surfaced when running the upstream lint pipeline locally (golangci-lint v2.10.1, the same version v2-ci uses); they predate the upstream sync and were missed because v2-ci has not yet been authorized to run on this fork. - mockservice_connect.go: insert a blank line between the embedded productv1connect.UnimplementedProductServiceHandler and the inner field so embeddedstructfieldcheck accepts the layout. - graphql_datasource_connect_test.go: declare a typed `nilCtx` variable so the deliberate nil-context path is still exercised but staticcheck SA1012 (which only flags the literal `nil` expression) no longer fires; the variable name documents the intent better than a nolint directive would.
…atasource-impl # Conflicts: # v2/go.mod
|
Pushed two follow-up commits to resolve the merge conflict with master and address a couple of lint findings that surfaced along the way. Thanks for the review, @Noroth ! Happy to clarify or address any follow-up feedback. |
|
hi @Noroth , we have any progress about this? Feel free talk to me if have any question, thanks~ |
|
Hey @fengyuwusong |
Noroth
left a comment
There was a problem hiding this comment.
So far this looks pretty good. We should have a couple more tests for the actual execution to ensure that the implementation covers edge cases and those are also working properly.
Apart from that in parallel I'm working on this topic: #1474 where I have to make sure to properly integrate your implementation then.
|
thanks for your reply, i will finish today. |
…IsConnect() Per @Noroth's review on wundergraph#1509, simplify the transport selection to a single switch on p.config.IsConnect(): - Connect configuration -> require p.connectTransport, else hard error - otherwise (gRPC) -> require p.grpcClient, else hard error The Configuration is the declarative source of truth for which wire protocol a subgraph uses; the factory must supply the matching runtime resource. Mismatches now surface at plan time instead of silently dialing the wrong protocol. This replaces the prior factory-first ordering. Callers that historically relied on "construct NewFactoryConnect, leave Connect out of Configuration" must now populate ConnectConfiguration on the data source configuration as well; the cosmo router PR will be updated to do exactly that.
MockServiceConnect forwards Connect handler calls into the underlying MockService, which reads request metadata via metadata.FromIncomingContext. Connect carries headers on req.Header() rather than on the ctx, so until now the inner service saw an empty metadata.MD whenever a test driver attached X-User-ID / X-User-Prefix etc. on the way in — the fixture was silently dropping every header that arrived over ConnectRPC. Introduce a small connectCtxToGRPC helper that copies req.Header() into a metadata.MD on a fresh incoming-metadata context (lowercasing keys the way grpc/metadata does), and apply it at the head of every forward. The helper is a no-op when no headers are present, so the existing happy-path tests are unaffected. This unblocks the upcoming Connect e2e tests for headers and context-metadata propagation.
…ect e2e tests Pull the planner/transport/data-source scaffolding out of the existing WithMockServiceConnect happy-path tests into a small helper so the table-driven Connect tests that follow can focus on query, mapping, and validation: - connectE2E options struct collects the per-call knobs (BaseURL, Query, Vars, Ctx, Headers, Encoding, FederationConfigs). Zero-valued Ctx falls back to context.Background(), zero-valued Encoding falls back to ConnectEncodingProtobuf, and Headers/FederationConfigs to nil; callers only set what matters. - loadConnectQuery wires those inputs through NewConnectTransport, NewDataSource(testMapping()), and ds.Load, returning the raw GraphQL response bytes. - The original WithMockServiceConnect and WithMockServiceConnect_JSON tests collapse onto one table-driven function with proto and json subtests. They previously carried 30+ lines of hand-rolled GRPCMapping apiece; testMapping() is functionally a superset for these fields, so the assertions remain identical. No behaviour change, no new test coverage. Net: ~150 lines removed.
…-case scenarios Address @Noroth's request on wundergraph#1509 to broaden the ConnectRPC e2e coverage beyond the original happy path. The new cases target points where the gRPC and Connect transports diverge in code path, so the tests pin the boundary where new Connect-specific bugs would surface rather than re-test planner behaviour the gRPC suite already covers. - *UnionTypes inline fragments through a union (`... on Product | User | Category`), single and list. Pins that __typename narrowing still works when the wire is Connect. - *FieldResolvers a query that fans out into separate Resolve<X> RPCs after the parent Category loads. Pins that the data source still merges multiple Connect calls into one GraphQL response. - *Entities Apollo Federation `_entities` lookup with a mixed batch of Product/Storage representations. - *AnimalInterface interface types (proto oneof). Runs both protobuf and protojson encodings to pin that __typename narrowing works either way. - *NullableFields proto3 wrapper types (StringValue, Int32Value, ...). protojson serialises wrappers as bare values or null; protobuf keeps them as messages. Both encodings exercised. - *Headers metadata-to-HTTP-header translation: the data source attaches an http.Header on Load, the transport puts it on the wire, and the subgraph echoes it back. - *PreservesContextMetadata existing grpc/metadata in the caller's context must reach the subgraph alongside the per-call HTTP headers, otherwise nested RPCs that rely on the passed-through metadata silently break. - *Error Connect's HTTP-status-based error model must still surface as a GraphQL errors array, mirroring the gRPC-trailer case. - *NestedLists repeated-field encoding under both wires, including required-list-of- optional-items and optional-list-of- required-items shapes. All built on the loadConnectQuery helper introduced in the preceding refactor commit, so each test reads as query + validation rather than transport scaffolding.
Summary
Add a ConnectRPC implementation of the
RPCTransportinterface introduced in #1490, so the gRPC data source can dial Connect subgraphs without changing the planner, compiler, or JSON builder. The sameMockServicefixture is regenerated viabufto emit Connect handlers alongside the existing gRPC code, and a new end-to-end test exercises the data source over Connect against that fixture.This is the second of the two PRs that together replace #1480.
Related
RPCTransportinterface +GRPCTransportimplementation)What's changed
pkg/grpctest— buf-driven fixturebuf.yaml/buf.gen.yaml: driveprotoc-gen-go,protoc-gen-go-grpc, andprotoc-gen-connect-gofrom a singlebuf generateinvocation, keeping the existing import path (graphql-go-tools/v2/pkg/grpctest/productv1) viaM=overrides.Makefile:generate-protonow callsbuf generate;install-protocalso installsprotoc-gen-connect-go.productv1/productv1connect/product.connect.go: buf-generated Connect handler/client. Sibling of the existingproductv1/product_grpc.pb.go, so the package serves Connect, gRPC, and gRPC-Web from one HTTP endpoint.mockservice_connect.go: hand-maintained adapter (MockServiceConnect) wrapping the existingMockServiceontoproductv1connect.ProductServiceHandler. Same backing implementation, two transports.pkg/engine/datasource/grpc_datasource— ConnectRPC transporttransport_connect.go:NewConnectTransport(ConnectTransportConfig{BaseURL, Encoding, Interceptors, HTTPClient})implementsRPCTransport.Invokeon top ofconnectrpc.com/connect. SupportsConnectEncodingProtobuf("proto") andConnectEncodingJSON("json") wire formats. Translatesgrpc/metadatato and from Connect headers using the same conventions asGRPCTransport, with binary request headers (-binsuffix) base64-encoded per the Connect spec.Interceptorsfield exposes connect-go's interceptor chain so callers can plug in tracing, logging, retries, or auth header injection without wrapping the HTTPClient.dynamicProtoCodec/dynamicJSONCodec) that bind adynamicpb.Messageto the expected response descriptor at Unmarshal time — connect-go's default codec would otherwise produce a descriptor-less empty message.transport_connect_test.go: covers Invoke over both wire formats, header passthrough (including binary), error mapping (*connect.Erroris preserved through%wwrapping so callers canerrors.As), context cancellation, base-URL trailing-slash handling, and theHTTPClient: nildefault-fallback path.grpc_datasource_connect_test.go: end-to-end tests that mirror the existingTest_DataSource_Load_WithMockServicehappy path but route the call throughNewConnectTransportagainst anhttptest/H2C server backed byMockServiceConnect. One test per wire format. Proves the full pipeline (compiler → JSON builder → transport → response unmarshal) is transport-agnostic.pkg/engine/datasource/grpc_datasource/transport.go— contract docsThe
RPCTransportinterface now documents themethodFullNameformat (/package.Service/Method, leading slash required) and the input/output expectations, so both implementations agree on the shape.Backward compatibility
No changes to existing APIs.
NewDataSource(transport RPCTransport, config DataSourceConfig)continues to accept anyRPCTransport; this PR just adds a new implementation. Existing gRPC call sites are untouched.Testing
pkg/engine/datasource/grpc_datasource: full test suite passes (459 cases, including 11 new Connect cases —TestConnectTransport_*andTest_DataSource_Load_WithMockServiceConnect[_JSON]).pkg/engine/datasource/graphql_datasource: 1842 cases pass (no regression).pkg/grpctest: builds clean; fixtures regenerated end-to-end viabuf generate.Dependencies
connectrpc.com/connect v1.19.2golang.org/x/netpromoted from indirect to direct (the e2e test useshttp2/h2cto run the Connect server over a real HTTP/2 endpoint).Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.
@coderabbitai summary