feat(grpc_datasource): introduce RPCTransport abstraction with gRPC implementation#1490
Conversation
…mplementation
Pull the gRPC dial path inside the data source out from a hard-coded
*grpc.ClientConnInterface dependency to a small RPCTransport interface.
This is a no-op for behaviour, but it lets the data source dispatch
through any concrete transport in follow-up work.
- new transport.go : RPCTransport interface + GRPCTransport
implementation that wraps grpc.ClientConnInterface.
- new transport_test.go : shared helpers (newTestCompiler, findMessageDesc)
plus a smoke test for GRPCTransport.Invoke.
- grpc_datasource.go : DataSource now keeps an RPCTransport rather than
a raw grpc.ClientConnInterface. NewDataSource
accepts the abstraction and a thin convenience
wrapper NewDataSourceGRPC preserves the existing
call shape for callers that already have a
grpc.ClientConnInterface.
- grpc_datasource{,_federation,_spy}_test.go : migrate the existing
NewDataSource(conn, ...) call sites onto
NewDataSourceGRPC(conn, ...) so the tests still
take a grpc connection directly.
- graphql_datasource.go : the planner constructed the gRPC data source
inline; switch its single NewDataSource(p.grpcClient,
...) site to NewDataSourceGRPC(p.grpcClient, ...)
so the package compiles against the new signature.
|
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 (5)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR introduces an RPCTransport interface and grpcTransport adapter, refactors grpcdatasource.DataSource to store and use an RPCTransport (constructor accepts RPCTransport), updates Load to call transport.Invoke, adds transport tests, and updates GraphQL wiring and all gRPC datasource tests to wrap connections with NewGRPCTransport. ChangesgRPC Datasource Transport Abstraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Per review feedback, remove the NewDataSourceGRPC convenience wrapper to avoid proliferating per-client constructors. Callers now compose explicitly: NewDataSource(NewGRPCTransport(conn), config).
🤖 I have created a release *beep* *boop* --- ## [2.4.0](v2.3.1...v2.4.0) (2026-05-21) ### Features * **grpc_datasource:** introduce RPCTransport abstraction with gRPC implementation ([#1490](#1490)) ([faffd81](faffd81)) ### Bug Fixes * use remapped variables in cost calculation ([#1505](#1505)) ([972ad0f](972ad0f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
@coderabbitai summary
Summary
Refactor the gRPC data source so the dial path goes through a small
RPCTransportinterface instead of a hard-codedgrpc.ClientConnInterfacedependency. This is a no-op for behaviour and a no-op for existing callers, but it lets follow-up work plug additional transports (Connect, gRPC-Web, in-memory mocks) into the same data source pipeline.This is the first of two PRs that together replace #1480.
Related
RPCTransport, regenerate thegrpctestfixture viabufto also emit Connect handlers, and exercise the data source end-to-end with both transports.What's changed
pkg/engine/datasource/grpc_datasourcetransport.go: defines theRPCTransportinterface and aGRPCTransportimplementation that wrapsgrpc.ClientConnInterface. The interface takes the same(ctx, methodFullName, input, output)shape the data source already passes togrpc.ClientConnInterface.Invoke, so swapping it in is mechanical.grpc_datasource.go:DataSourcenow keeps anRPCTransportrather than a rawgrpc.ClientConnInterface.NewDataSourceaccepts the abstraction; a thin convenience wrapperNewDataSourceGRPCpreserves the existing call shape for callers that already have agrpc.ClientConnInterface.transport_test.go: shared helpers (newTestCompiler,findMessageDesc) plus a smoke test forGRPCTransport.Invoke. Helpers live here so the follow-up Connect transport tests can reuse them without a circular dependency.grpc_datasource_test.go,grpc_datasource_federation_test.go,grpc_datasource_spy_test.go: every existingNewDataSource(conn, …)call site is migrated ontoNewDataSourceGRPC(conn, …). Tests still take agrpc.ClientConnInterfacedirectly.pkg/engine/datasource/graphql_datasourcegraphql_datasource.go: the planner constructed the gRPC data source inline viagrpcdatasource.NewDataSource(p.grpcClient, …). With the new signature this would no longer compile, so the call switches togrpcdatasource.NewDataSourceGRPC(p.grpcClient, …). No behavioural change, no change to public Factory APIs.Backward compatibility
NewDataSourceconsumers should switch toNewDataSourceGRPC. The old function name is kept but takes the abstraction, not agrpc.ClientConnInterface.GRPCTransport.Invokesimply forwards to the underlying client connection.Testing
pkg/engine/datasource/grpc_datasource: existing test suite (455+ cases) passes; newTestGRPCTransport_Invokecovers the wrapper directly.pkg/engine/datasource/graphql_datasource: existing tests pass against the migrated call site.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.