feat(grpc_datasource): add ConnectRPC transport implementation#1509
Open
fengyuwusong wants to merge 5 commits into
Open
feat(grpc_datasource): add ConnectRPC transport implementation#1509fengyuwusong wants to merge 5 commits into
fengyuwusong wants to merge 5 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.
fengyuwusong
requested review from
Noroth,
StarpTech,
devsergiy,
dkorittki,
jensneuse and
ysmolski
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a Connect-based gRPC transport for GraphQL datasources, configures Buf code generation for Connect/gRPC/proto, provides a Connect adapter for the mock service, and includes unit and end-to-end tests for Protobuf and JSON wire encodings. ChangesConnect gRPC Transport Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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.
Contributor
Author
|
Hi @Noroth , Could you please take a look? Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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