Skip to content

feat(grpc_datasource): add ConnectRPC transport implementation#1509

Open
fengyuwusong wants to merge 20 commits into
wundergraph:masterfrom
fengyuwusong:feat/connectrpc-datasource-impl
Open

feat(grpc_datasource): add ConnectRPC transport implementation#1509
fengyuwusong wants to merge 20 commits into
wundergraph:masterfrom
fengyuwusong:feat/connectrpc-datasource-impl

Conversation

@fengyuwusong

Copy link
Copy Markdown
Contributor

Summary

Add a ConnectRPC implementation of the RPCTransport interface introduced in #1490, so the gRPC data source can dial Connect subgraphs without changing the planner, compiler, or JSON builder. The same MockService fixture is regenerated via buf to 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

What's changed

pkg/grpctest — buf-driven fixture

  • New buf.yaml / buf.gen.yaml: drive protoc-gen-go, protoc-gen-go-grpc, and protoc-gen-connect-go from a single buf generate invocation, keeping the existing import path (graphql-go-tools/v2/pkg/grpctest/productv1) via M= overrides.
  • Makefile: generate-proto now calls buf generate; install-protoc also installs protoc-gen-connect-go.
  • New productv1/productv1connect/product.connect.go: buf-generated Connect handler/client. Sibling of the existing productv1/product_grpc.pb.go, so the package serves Connect, gRPC, and gRPC-Web from one HTTP endpoint.
  • New mockservice_connect.go: hand-maintained adapter (MockServiceConnect) wrapping the existing MockService onto productv1connect.ProductServiceHandler. Same backing implementation, two transports.

pkg/engine/datasource/grpc_datasource — ConnectRPC transport

  • New transport_connect.go: NewConnectTransport(ConnectTransportConfig{BaseURL, Encoding, Interceptors, HTTPClient}) implements RPCTransport.Invoke on top of connectrpc.com/connect. Supports ConnectEncodingProtobuf ("proto") and ConnectEncodingJSON ("json") wire formats. Translates grpc/metadata to and from Connect headers using the same conventions as GRPCTransport, with binary request headers (-bin suffix) base64-encoded per the Connect spec.
  • The Interceptors field exposes connect-go's interceptor chain so callers can plug in tracing, logging, retries, or auth header injection without wrapping the HTTPClient.
  • The internal per-procedure client cache uses custom codecs (dynamicProtoCodec / dynamicJSONCodec) that bind a dynamicpb.Message to the expected response descriptor at Unmarshal time — connect-go's default codec would otherwise produce a descriptor-less empty message.
  • New transport_connect_test.go: covers Invoke over both wire formats, header passthrough (including binary), error mapping (*connect.Error is preserved through %w wrapping so callers can errors.As), context cancellation, base-URL trailing-slash handling, and the HTTPClient: nil default-fallback path.
  • New grpc_datasource_connect_test.go: end-to-end tests that mirror the existing Test_DataSource_Load_WithMockService happy path but route the call through NewConnectTransport against an httptest/H2C server backed by MockServiceConnect. 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 docs

The RPCTransport interface now documents the methodFullName format (/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 any RPCTransport; 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_* and Test_DataSource_Load_WithMockServiceConnect[_JSON]).
  • pkg/engine/datasource/graphql_datasource: 1842 cases pass (no regression).
  • pkg/grpctest: builds clean; fixtures regenerated end-to-end via buf generate.

Dependencies

  • Added: connectrpc.com/connect v1.19.2
  • golang.org/x/net promoted from indirect to direct (the e2e test uses http2/h2c to run the Connect server over a real HTTP/2 endpoint).

Checklist

  • I have discussed my proposed changes in an issue and have received approval to proceed.
  • I have followed the coding standards of the project.
  • Tests or benchmarks have been added or updated.
  • Documentation has been updated on https://github.com/wundergraph/docs-website — N/A: no public-facing API change worth surfacing in docs at this stage; user-facing docs land with the consuming cosmo PR.
  • I have read the Contributors Guide.

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

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.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds 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.

Changes

Connect gRPC Transport Implementation

Layer / File(s) Summary
Build tooling and dependency setup
v2/go.mod, v2/pkg/grpctest/Makefile, v2/pkg/grpctest/buf.yaml, v2/pkg/grpctest/buf.gen.yaml
Adds connectrpc.com/connect v1.19.2 and promotes golang.org/x/net to direct; updates Makefile to install Connect codegen and switches generation to buf generate with a Buf v2 generation config for Go/gRPC/Connect.
Test infra and Connect adapter
v2/pkg/engine/datasource/grpc_datasource/transport.go, v2/pkg/grpctest/mockservice_connect.go
Clarifies RPCTransport documentation and adds MockServiceConnect, an adapter implementing productv1connect.ProductServiceHandler by delegating to the existing gRPC MockService.
Connect transport implementation
v2/pkg/engine/datasource/grpc_datasource/transport_connect.go
Introduces ConnectEncoding, ConnectTransportConfig, NewConnectTransport, dynamic proto/json codecs that bind response descriptors, per-procedure client caching (10MB cap), header forwarding (including base64 for -bin), and unary Invoke with error wrapping.
Transport unit tests
v2/pkg/engine/datasource/grpc_datasource/transport_connect_test.go
Adds tests verifying protobuf/json request/response round-trips, Connect error handling, non-JSON upstream errors, string and binary metadata forwarding, URL joining with trailing slashes, context cancellation, and default HTTP client fallback.
GraphQL datasource wiring and config
v2/pkg/engine/datasource/graphql_datasource/configuration.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
Adds ConnectConfiguration, extends Configuration to track Connect, validates Connect requires GRPC mapping, adds NewFactoryConnect and a connectTransport field, and enables fragment flattening when Connect transport is present.
GraphQL datasource Connect tests
v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_connect_test.go
Adds unit tests for NewFactoryConnect input validation and propagation, and for NewConfiguration Connect validation and happy path; introduces a stubRPCTransport test double.
End-to-end datasource tests
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go
Adds end-to-end tests that run GraphQL compilation and DataSource.Load against an h2c httptest server using the new Connect transport with both Protobuf and JSON encodings.

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)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately and concisely describes the main change: adding a ConnectRPC transport implementation to the gRPC datasource.
Description check ✅ Passed The pull request description is comprehensive and directly related to the changeset, covering the summary, related issues, what's changed across all modified files, backward compatibility, testing, dependencies, and a checklist.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go (1)

180-181: ⚡ Quick win

Use 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

📥 Commits

Reviewing files that changed from the base of the PR and between 15a8132 and 3b92945.

⛔ Files ignored due to path filters (1)
  • v2/go.sum is excluded by !**/*.sum
📒 Files selected for processing (10)
  • v2/go.mod
  • v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go
  • v2/pkg/engine/datasource/grpc_datasource/transport.go
  • v2/pkg/engine/datasource/grpc_datasource/transport_connect.go
  • v2/pkg/engine/datasource/grpc_datasource/transport_connect_test.go
  • v2/pkg/grpctest/Makefile
  • v2/pkg/grpctest/buf.gen.yaml
  • v2/pkg/grpctest/buf.yaml
  • v2/pkg/grpctest/mockservice_connect.go
  • v2/pkg/grpctest/productv1/productv1connect/product.connect.go

Comment thread v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go Outdated
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.
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

Hi @Noroth , Could you please take a look? Thanks!

@Noroth

Noroth commented May 23, 2026

Copy link
Copy Markdown
Contributor

Hi @fengyuwusong can you let me know how you planned the next steps preferably in your own words please.
I cannot be 100% confident when I don't know if the response is coming from you or Claude. I just wanna make sure we both are aligned.

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!

@fengyuwusong

Copy link
Copy Markdown
Contributor Author

Hi @fengyuwusong can you let me know how you planned the next steps preferably in your own words please. I cannot be 100% confident when I don't know if the response is coming from you or Claude. I just wanna make sure we both are aligned.

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.

  • graphql-go-tools repo

  • cosmo repo

    • feat(controlplane): accept http(s) URLs for GRPC_SERVICE subgraphs cosmo#2841: because ConnectRPC can use http/https protocol, so controlplane should accept http(s) URLs for GRPC_SERVICE subgraphs
    • next new PR:
      • router
        • new grpc_protocol configuration block
        • wire the new transport into factoryresolver / graph_server
        • document ConnectRPC support
        • add e2e subgraph tests
      • demo
        • write the demo project code, support both gRPC and ConnectRPC
      • examples
        • new router-grpc-connect example, showing the grpc_protocol configuration and how to point the router at a ConnectRPC subgraph

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b2bf327 and a407431.

📒 Files selected for processing (4)
  • v2/pkg/engine/datasource/graphql_datasource/configuration.go
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
  • v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_connect_test.go
  • v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_connect_test.go

Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go Outdated
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.
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

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.
@Noroth

Noroth commented May 29, 2026

Copy link
Copy Markdown
Contributor

Hi @fengyuwusong,
thanks for your explanation, and sorry for the delay. I will take a look as soon as possible.
Also your English is absolutely fine and your plan makes sense to me, thanks!

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.
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

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.

@fengyuwusong fengyuwusong requested a review from a team as a code owner June 8, 2026 07:50
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

hi @Noroth , we have any progress about this? Feel free talk to me if have any question, thanks~

@Noroth

Noroth commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Hey @fengyuwusong
Sorry for the delay. I was a bit tight on time last week and I will take care of it in the upcoming days.

@Noroth Noroth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go Outdated
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

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.
@fengyuwusong

Copy link
Copy Markdown
Contributor Author

I've finished all necessary changes. Could you please take a look when you have time, @Noroth? Thanks a lot.

Also, I've reviewed the implementation of #1474. Our work can be largely integrated into it.
Please kindly assess this. Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants