feat(router): add connectrpc handler for graphql-to-grpc bridge #2379
Conversation
asoorm
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR introduces comprehensive ConnectRPC (gRPC/Connect/GRPC-Web) protocol support to the router. It adds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
asoorm
changed the title
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (23)
router/pkg/connectrpc/proto_loader.go (1)
322-326: Document that GetServices returns the internal map.The comment at line 323 mentions treating the returned map as read-only, but this is not enforced. Consider returning a copy of the map or documenting this more prominently in the method's godoc.
Apply this diff to make the read-only contract clearer:
-// GetServices returns all loaded service definitions. -// The returned map should be treated as read-only to prevent accidental mutation. +// GetServices returns all loaded service definitions. +// IMPORTANT: The returned map is the internal map and must NOT be modified. +// Modifying the returned map will cause data races and undefined behavior. func (pl *ProtoLoader) GetServices() map[string]*ServiceDefinition { return pl.services }router/pkg/connectrpc/proto_loader_test.go (1)
20-107: Comprehensive test coverage for proto loading.Tests properly verify service metadata, method properties, and expected method names. Consider adding
t.Parallel()to subtests for faster execution, though this is optional.router/pkg/connectrpc/operation_registry_test.go (1)
438-447: Consider handling or logging errors from LoadOperationsForService in concurrent test.The concurrent clearer goroutines discard the error from
LoadOperationsForService. While this is acceptable for stress-testing thread safety, consider at minimum logging unexpected errors to aid debugging if the test fails.go func() { defer wg.Done() for j := 0; j < 50; j++ { registry.Clear() - _ = registry.LoadOperationsForService(serviceName, []string{opFile}) + if err := registry.LoadOperationsForService(serviceName, []string{opFile}); err != nil { + t.Logf("LoadOperationsForService error (expected during stress): %v", err) + } } }()router/core/router.go (3)
966-972: Consider using consistent URL construction pattern.Using
path.Join(r.listenAddr, r.graphqlPath)produces a path likelocalhost:3002/graphqlwithout a scheme. While the downstreamNewServernormalizes this by addinghttp://if missing, usingurl.JoinPath(as done at line 1362) would be more semantically correct and consistent.// Determine the router GraphQL endpoint var routerGraphQLEndpoint string if r.connectRPC.GraphQLEndpoint != "" { routerGraphQLEndpoint = r.connectRPC.GraphQLEndpoint } else { - routerGraphQLEndpoint = path.Join(r.listenAddr, r.graphqlPath) + routerGraphQLEndpoint = fmt.Sprintf("http://%s%s", r.listenAddr, r.graphqlPath) }Note: The same pattern appears in MCP initialization at line 907.
954-964: Service discovery result is underutilized.The
discoveredServicesslice is computed here but only used for logging the count. The actual service loading is performed again insideconnectrpc.NewServer. Consider either passing the discovered services toNewServerto avoid redundant discovery, or removing this discovery call if it's only for early validation/logging.
1527-1538: Perfect! I have completed the verification. Here's my final analysis:Verification Results:
- ✅
connectRPCServeris declared asinterface{}(line 89 in router_config.go)- ✅ It's assigned
crpcServerwhich has concrete type*connectrpc.Server(line 1005)- ✅
connectrpc.ServerDOES have aStop(context.Context) errormethod (line 201 in router/pkg/connectrpc/server.go)- ✅ The type assertion at line 1532 works correctly but defeats compile-time type safety
- ✅
mcpServerdemonstrates the better pattern: concrete type*mcpserver.GraphQLSchemaServerwith direct method calls (line 1521) - no type assertion needed- ✅
connectRPCServeris only assigned once with a known concrete type
Type assertion for Stop method is fragile and inconsistent with mcpServer pattern.
The
connectRPCServerfield is declared asinterface{}but always assigned the concrete type*connectrpc.Server, which has aStop(context.Context) errormethod. The type assertion works but sacrifices compile-time safety. For consistency withmcpServer(which uses concrete type*mcpserver.GraphQLSchemaServerand callsStop()directly at line 1521), declareconnectRPCServeras*connectrpc.Serverinstead. This eliminates the type assertion and provides type safety at compile time.router/pkg/connectrpc/vanguard_service_test.go (5)
253-260: Direct map access bypasses OperationRegistry encapsulation.Directly manipulating
opRegistry.operationscouples tests to internal implementation details. If the registry's internal structure changes, these tests will break. Consider adding a test helper method to the OperationRegistry or using the public API if available.- if opRegistry.operations[serviceName] == nil { - opRegistry.operations[serviceName] = make(map[string]*schemaloader.Operation) - } - opRegistry.operations[serviceName]["QueryGetEmployeeById"] = &schemaloader.Operation{ + // Use a public method or test helper to register operations + opRegistry.RegisterOperation(serviceName, &schemaloader.Operation{ Name: "QueryGetEmployeeById", OperationType: "query", OperationString: "query QueryGetEmployeeById($id: Int!) { employee(id: $id) { id name } }", - } + })
273-277: Inefficient pattern to retrieve a single service definition.This pattern iterates through a map just to get one element. Consider exposing a method like
GetFirstService()orGetService(name)on ProtoLoader for cleaner test code.
316-356: Repeated service extraction pattern across multiple sub-tests.The same 8-line pattern for extracting a service definition is repeated in lines 317-324, 338-345, 359-366, and 380-387. Extract this into a helper function to reduce duplication and improve maintainability.
func getFirstServiceDef(t *testing.T, protoLoader *ProtoLoader) *ServiceDefinition { t.Helper() services := protoLoader.GetServices() require.NotEmpty(t, services) for _, svc := range services { return svc } return nil }
240-244: Ignoring error fromw.Write.While this is test code and unlikely to fail, ignoring the error return value is not idiomatic. Consider using a helper or explicitly discarding with
_.- w.Write([]byte(`{"data":{"employee":{"id":1,"name":"Test Employee"}}}`)) + _, _ = w.Write([]byte(`{"data":{"employee":{"id":1,"name":"Test Employee"}}}`))
474-504: Helper function also directly accesses internal map.Same encapsulation concern as noted earlier. This helper sets up the operation registry by directly manipulating internal state.
router-tests/connectrpc_test.go (2)
23-27: Ignoring error fromw.Writein mock server.- w.Write([]byte(`{"data":{"employees":[{"id":1}]}}`)) + _, _ = w.Write([]byte(`{"data":{"employees":[{"id":1}]}}`))
70-73: Same issue: ignoringw.Writeerror in mock server.- w.Write([]byte(`{"data":{}}`)) + _, _ = w.Write([]byte(`{"data":{}}`))router/pkg/connectrpc/server_lifecycle_test.go (3)
46-50: Ignored error return fromw.Write.The error returned by
w.Writeis not checked. While this is test code, it's good practice to handle or explicitly ignore errors.- w.Write([]byte(`{"data":{}}`)) + _, _ = w.Write([]byte(`{"data":{}}`))
108-111: Ignored error return fromserver.Stop.For test hygiene, consider checking the error from
server.Stopor explicitly ignoring it.ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - server.Stop(ctx) + _ = server.Stop(ctx) })
154-157: Ignored error return fromserver.Stop.Same pattern as above - consider explicitly ignoring the error for consistency.
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - server.Stop(ctx) + _ = server.Stop(ctx) })router/pkg/connectrpc/operation_registry.go (1)
218-230: Consider pre-allocating slice forGetAllOperations.The slice could be pre-allocated for a minor performance improvement, similar to
GetAllOperationsForService.func (r *OperationRegistry) GetAllOperations() []schemaloader.Operation { r.mu.RLock() defer r.mu.RUnlock() - var operations []schemaloader.Operation + // Count total operations for pre-allocation + total := 0 + for _, serviceOps := range r.operations { + total += len(serviceOps) + } + operations := make([]schemaloader.Operation, 0, total) for _, serviceOps := range r.operations { for _, op := range serviceOps { operations = append(operations, *op) } } return operations }router/pkg/connectrpc/server.go (3)
71-74: Protocol prefix check may have edge cases.Using
strings.Contains(config.GraphQLEndpoint, "://")could match URLs where://appears in the path or query string (though unlikely). Consider usingstrings.HasPrefixwith known protocols.// Add protocol if missing - if !strings.Contains(config.GraphQLEndpoint, "://") { + if !strings.HasPrefix(config.GraphQLEndpoint, "http://") && !strings.HasPrefix(config.GraphQLEndpoint, "https://") { config.GraphQLEndpoint = "http://" + config.GraphQLEndpoint }
176-182: HTTP server timeouts are hardcoded.The server timeouts are hardcoded to 30s/30s/60s while
config.RequestTimeoutexists but is only used for the HTTP client. Consider using the configured timeout or adding separate server timeout configuration.s.httpServer = &http.Server{ Addr: s.config.ListenAddr, Handler: h2cHandler, - ReadTimeout: 30 * time.Second, - WriteTimeout: 30 * time.Second, - IdleTimeout: 60 * time.Second, + ReadTimeout: s.config.RequestTimeout, + WriteTimeout: s.config.RequestTimeout, + IdleTimeout: s.config.RequestTimeout * 2, }
344-349: CapturedstatusCodeis unused.The
statusCodefield is captured but never read. If this is for future observability/logging, consider adding a comment. Otherwise, it could be removed.Either add usage (e.g., logging) or remove the unused field:
type responseWriter struct { http.ResponseWriter - statusCode int + statusCode int // Captured for future observability/metrics }router/pkg/connectrpc/samples/services/employee.v1/employee.proto (1)
50-58: Unusual field number gaps may indicate removed fields.Field numbers jump from 3 to 53-54-57-58, which is valid but unusual. This pattern often indicates fields were removed. If this is sample/test data, consider using sequential numbering for clarity.
router/pkg/connectrpc/testdata/employee_only/employee.proto (1)
50-58: Field number gaps present in test data.Field numbers jump from 3 to 53-54-57-58. For test data, sequential numbering improves readability and maintenance.
router/pkg/connectrpc/handler.go (1)
436-444: hasData check could be more robust.The check for partial data uses string comparisons that may miss edge cases:
- Empty array
[]might represent valid partial data- Whitespace variations like
"{ }"would be considered as having data- No trimming of whitespace before comparison
Consider a more robust check:
- hasData := len(graphqlResponse.Data) > 0 && string(graphqlResponse.Data) != "null" && string(graphqlResponse.Data) != "{}" + // Check if data is meaningful (not null, empty object, or empty array) + dataStr := strings.TrimSpace(string(graphqlResponse.Data)) + hasData := len(dataStr) > 0 && + dataStr != "null" && + dataStr != "{}" && + dataStr != "[]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
router-tests/connectrpc_test.go(1 hunks)router/connect.config.yaml(1 hunks)router/core/router.go(4 hunks)router/core/router_config.go(3 hunks)router/core/supervisor_instance.go(1 hunks)router/go.mod(3 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/connectrpc_test.go(1 hunks)router/pkg/connectrpc/handler.go(1 hunks)router/pkg/connectrpc/handler_error_test.go(1 hunks)router/pkg/connectrpc/handler_test.go(1 hunks)router/pkg/connectrpc/operation_registry.go(1 hunks)router/pkg/connectrpc/operation_registry_test.go(1 hunks)router/pkg/connectrpc/proto_loader.go(1 hunks)router/pkg/connectrpc/proto_loader_test.go(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeTag.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetProducts.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/employee.proto(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/server_lifecycle_test.go(1 hunks)router/pkg/connectrpc/server_test.go(1 hunks)router/pkg/connectrpc/service_discovery.go(1 hunks)router/pkg/connectrpc/service_discovery_test.go(1 hunks)router/pkg/connectrpc/testdata/employee_only/employee.proto(1 hunks)router/pkg/connectrpc/testdata/examples/product.v1/product_service.proto(1 hunks)router/pkg/connectrpc/testdata/examples/user.v1/user_service.proto(1 hunks)router/pkg/connectrpc/vanguard_service.go(1 hunks)router/pkg/connectrpc/vanguard_service_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/vanguard_service_test.gorouter/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/handler_error_test.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/handler_test.gorouter-tests/connectrpc_test.gorouter/pkg/connectrpc/handler_error_test.gorouter/pkg/connectrpc/operation_registry_test.gorouter/pkg/config/connectrpc_test.gorouter/pkg/connectrpc/service_discovery_test.gorouter/pkg/connectrpc/server_lifecycle_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/pkg/config/config.gorouter/core/router.gorouter/core/supervisor_instance.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/go.modrouter/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.mod
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.mod
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.json
🧬 Code graph analysis (14)
router/pkg/connectrpc/server_test.go (1)
router/pkg/connectrpc/server.go (2)
NewServer(49-89)ServerConfig(21-33)
router/pkg/connectrpc/vanguard_service_test.go (4)
router/pkg/connectrpc/vanguard_service.go (2)
NewVanguardService(31-56)VanguardServiceConfig(16-20)router/pkg/connectrpc/proto_loader.go (3)
ProtoLoader(53-59)NewProtoLoader(62-72)ServiceDefinition(17-30)router/pkg/connectrpc/handler.go (3)
RPCHandler(138-143)NewRPCHandler(154-182)HandlerConfig(146-151)router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)
router/pkg/connectrpc/handler_test.go (2)
router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)router/pkg/connectrpc/handler.go (2)
NewRPCHandler(154-182)HandlerConfig(146-151)
router-tests/connectrpc_test.go (3)
router-tests/testenv/testenv.go (2)
Run(105-122)Environment(1733-1769)router/pkg/connectrpc/server.go (2)
NewServer(49-89)ServerConfig(21-33)router/pkg/connectrpc/handler.go (1)
GraphQLRequest(112-115)
router/pkg/connectrpc/operation_registry_test.go (1)
router/pkg/connectrpc/operation_registry.go (1)
NewOperationRegistry(32-41)
router/pkg/config/connectrpc_test.go (1)
router/pkg/config/config.go (3)
ConnectRPCConfiguration(995-1000)LoadConfig(1129-1241)Config(1022-1098)
router/pkg/connectrpc/service_discovery_test.go (1)
router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-158)ServiceDiscoveryConfig(29-34)
router/pkg/connectrpc/operation_registry.go (1)
router/core/context.go (3)
OperationTypeQuery(506-506)OperationTypeMutation(507-507)OperationTypeSubscription(508-508)
router/pkg/connectrpc/server_lifecycle_test.go (2)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)router/pkg/connectrpc/server.go (3)
Server(36-46)NewServer(49-89)ServerConfig(21-33)
router/core/router.go (3)
router/pkg/connectrpc/server.go (3)
Server(36-46)ServerConfig(21-33)NewServer(49-89)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-158)ServiceDiscoveryConfig(29-34)router/pkg/config/config.go (1)
ConnectRPCConfiguration(995-1000)
router/core/supervisor_instance.go (1)
router/core/router.go (1)
WithConnectRPC(2212-2216)
router/pkg/connectrpc/handler.go (1)
router/pkg/connectrpc/operation_registry.go (1)
OperationRegistry(24-29)
router/core/router_config.go (1)
router/pkg/config/config.go (1)
ConnectRPCConfiguration(995-1000)
router/pkg/connectrpc/vanguard_service.go (2)
router/pkg/connectrpc/handler.go (1)
RPCHandler(138-143)router/pkg/connectrpc/proto_loader.go (3)
ProtoLoader(53-59)ServiceDefinition(17-30)MethodDefinition(33-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build-router
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (40)
router/pkg/connectrpc/samples/services/employee.v1/QueryGetProducts.graphql (1)
1-16: ✓ Sample query demonstrates proper inline fragment usage.The GraphQL query is syntactically correct and effectively demonstrates querying a union/interface type (
productTypes) with type-specific fields. The pattern of using inline fragments for different product types is appropriate and showcases how the operation registry handles multi-type queries.router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql (1)
1-20: LGTM!The query is syntactically valid and properly follows the operation naming convention (
Query<OperationName>) expected by the service registry. The nested field selection is correctly structured, and the parameterized query with$id: Int!is properly defined for the operation registry integration.router/pkg/connectrpc/handler_test.go (1)
1-344: LGTM! Comprehensive test coverage.The test file provides excellent coverage of the RPC handler functionality, including:
- Constructor validation with various configurations
- RPC request handling (success and error cases)
- Header forwarding from context
- HTTP transport error handling
- Reload, operation counting, and validation
The use of mock implementations and test helpers follows Go testing best practices.
router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql (1)
1-19: LGTM! Valid sample GraphQL query.The GraphQL query is well-formed and appropriate for testing the ConnectRPC-to-GraphQL bridge functionality.
router/core/supervisor_instance.go (1)
271-271: LGTM! Consistent ConnectRPC option integration.The addition of
WithConnectRPC(config.ConnectRPC)follows the established pattern for router options and properly integrates the ConnectRPC configuration.router/core/router_config.go (1)
89-89: LGTM! ConnectRPC configuration properly integrated.The additions properly integrate ConnectRPC configuration into the router:
- Field for the ConnectRPC server instance
- Configuration field matching the config package structure
- Usage tracking following the established pattern
Also applies to: 121-121, 310-310
router/connect.config.yaml (1)
1-20: LGTM! Well-structured sample configuration.The configuration file provides a clear example of ConnectRPC setup with:
- Proper schema hint for editor support
- Standard router settings
- ConnectRPC server configuration with local addresses
- File system storage provider for service discovery
This aligns well with the ConnectRPC integration introduced in the PR.
router/pkg/connectrpc/server_test.go (1)
1-113: LGTM! Thorough server lifecycle testing.The test suite comprehensively validates:
- Server construction with various configurations
- Default value application (listen address, timeout)
- Protocol auto-completion for GraphQL endpoint
- Error handling for missing required fields
- Service information consistency across start/stop lifecycle
The tests follow Go best practices and provide good coverage of the server's public API.
router/go.mod (1)
60-60: All dependency versions are secure; golang.org/x/net v0.46.0 is current with all known vulnerabilities patched.Verification results:
- connectrpc.com/vanguard v0.3.0: No security advisories
- github.com/jhump/protoreflect v1.17.0: No security advisories
- golang.org/x/net v0.46.0: Multiple historical vulnerabilities exist, but all are in versions below v0.46.0. The current version includes all patches and is safe.
The three dependencies are compatible and secure as specified in the PR.
router/pkg/config/config.go (1)
995-1005: LGTM! Configuration follows established patterns.The new
ConnectRPCConfigurationandConnectRPCServerstructs mirror the existing MCP configuration pattern, with appropriate env variable bindings and sensible defaults (e.g.,localhost:5026avoids port conflicts with MCP on5025).router/pkg/connectrpc/proto_loader_test.go (1)
11-18: Well-designed test helper.Good use of
t.Helper()for proper stack trace attribution. The shared helper reduces duplication across test files.router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeTag.graphql (1)
1-11: LGTM! Valid GraphQL mutation following naming conventions.The mutation properly declares required variables (
Int!,String!), and the operation name matches the filename, aligning with the ConnectRPC service discovery conventions.router/pkg/connectrpc/operation_registry_test.go (3)
14-36: Good coverage of constructor behavior.Tests properly verify both logger and nil logger paths, ensuring the defensive nil check in
NewOperationRegistryworks correctly.
385-452: Solid thread-safety testing approach.The concurrent read and read-with-clear patterns properly exercise the RWMutex implementation. Good use of
sync.WaitGroupfor coordination.
454-499: Excellent isolation test for service-scoped operations.This test correctly verifies that operations with the same name in different service namespaces remain isolated, which is critical for multi-service scenarios.
router/core/router.go (2)
926-929: Good validation of required configuration.The early validation of
ServicesProviderIDprovides a clear error message when the required configuration is missing.
2212-2216: LGTM! Option function follows established pattern.The
WithConnectRPCoption follows the same pattern as other configuration options in this file.router/pkg/connectrpc/handler_error_test.go (3)
17-48: Comprehensive HTTP status to Connect code mapping tests.Good coverage of both 4xx and 5xx status codes with appropriate Connect error code mappings. The table-driven approach makes it easy to add new cases.
134-255: Well-structured critical error tests with inline snapshots.The tests properly validate error classification, Connect error codes, and GraphQL error metadata. Using
JSONEqfor semantic comparison is the right approach.
257-397: Good coverage of partial data (non-critical) error scenarios.Tests correctly verify that partial data is captured in metadata alongside error information, which is important for the Relay-inspired error classification mentioned in the PR objectives.
router/pkg/config/connectrpc_test.go (3)
12-20: Good test for zero-value safety invariant.Testing that the zero value of
ConnectRPCConfigurationrepresents a disabled state is an important safety check that protects against accidental enablement.
22-118: Comprehensive YAML loading tests with table-driven approach.Good coverage of different configuration scenarios including minimal config, full overrides, environment variable expansion, and disabled state. The use of
t.Setenvensures proper cleanup.
171-203: Multi-file config merge test validates important behavior.This test ensures that ConnectRPC configuration can be split across multiple files with proper override semantics, which is useful for environment-specific configurations.
router-tests/connectrpc_test.go (3)
40-44: Good shutdown pattern with context timeout.The deferred cleanup with a timeout context ensures the server is properly stopped even if tests fail, preventing resource leaks.
98-115: Integration test validates router environment setup.This test ensures the router testenv works correctly with GraphQL requests, providing a basic smoke test for the integration.
30-35: Relative path concern is unfounded—this is standard practice in the repository.The path
../router/pkg/connectrpc/testdatais appropriate for this monorepo structure whererouter/androuter-tests/are sibling directories. Go tests execute from their package directory by default, making the relative path predictable and reliable. This pattern is consistently used throughout other test files inrouter-tests/(e.g.,router_plugin_test.gowith../router/plugins,testexec.gowithconst routerDir = "../router"). No refactoring is needed.Likely an incorrect or invalid review comment.
router/pkg/connectrpc/service_discovery_test.go (6)
13-56: Well-structured test for single service discovery.Good test that validates the complete service discovery flow including package extraction, service name extraction, and file associations.
134-176: Important ADR compliance test for nested proto handling.This test ensures that the discovery logic stops at the first proto file and doesn't descend into subdirectories, which is critical for the expected directory structure. The test name and comment clearly document this behavior.
218-251: Enforces single proto per directory constraint.This constraint prevents ambiguity in service definitions. The test properly verifies the error message to ensure users get clear feedback.
253-283: Good test for duplicate service detection.Validates that the discovery logic prevents the same
package.servicecombination from appearing in multiple directories, which would cause routing conflicts.
423-460: Thorough edge case coverage for package extraction.Tests cover normal cases, whitespace variations, nested packages, and missing declarations. This ensures robust parsing of proto files.
462-502: Good coverage for service name extraction.Tests handle variations like presence/absence of braces, multiple services (first-wins behavior), and missing declarations.
router/pkg/connectrpc/testdata/examples/user.v1/user_service.proto (1)
1-130: Well-structured proto definition for testing.The proto file follows the documented naming conventions (Query/Mutation prefixes) and provides comprehensive message definitions for testing the ConnectRPC integration. The service definition and message types are appropriately designed for the test scenarios.
router/pkg/connectrpc/server_lifecycle_test.go (1)
63-307: Comprehensive test coverage for server lifecycle.The test suite covers key lifecycle scenarios including start/stop/reload sequences, error handling, component initialization, and graceful shutdown. The use of
sync.Oncefor shared proto loading is a good pattern to avoid registration conflicts.router/pkg/config/config.schema.json (1)
2110-2167: Well-structured ConnectRPC configuration schema.The new
connect_rpcconfiguration block follows the established patterns in the schema (similar tomcp). The structure includes appropriate properties for server configuration, service discovery, and GraphQL endpoint integration.One minor observation: The
servicesarray items could benefit fromrequiredfields to enforce that at least one ofproto_provider_idoroperations_provider_idis specified when a service is defined.Consider whether the
servicesarray items should have required properties. If a service entry is provided without any provider IDs, it may lead to runtime confusion.router/pkg/connectrpc/operation_registry.go (2)
46-133: Robust operation loading with appropriate error handling.The
LoadOperationsForServicemethod handles individual file errors gracefully by logging and continuing, which is appropriate for resilience. The duplicate detection behavior (last one wins) is documented in the warning log.One consideration: The method returns
nilerror even when all files fail to load. This could mask scenarios where no operations were successfully loaded.Consider whether callers need to distinguish between "0 files provided" vs "all files failed to parse" scenarios. Currently both result in success with 0 operations loaded.
24-29: Thread-safe design with appropriate locking strategy.The use of
sync.RWMutexwithRLockfor read operations andLockfor write operations is correct. The documentation accurately describes the thread-safety guarantees.router/pkg/connectrpc/server.go (1)
91-198: Server startup flow is well-structured.The
Start()method follows a clear initialization pattern: discover services → initialize components → load protos/operations → create vanguard service → create transcoder → start HTTP server. Error handling is appropriate with wrapped errors for context.router/pkg/connectrpc/testdata/examples/product.v1/product_service.proto (1)
1-213: Well-structured test proto definition.This proto file demonstrates good practices:
- Clear documentation comments for service and methods
- Sequential field numbering for maintainability
- Dedicated request/response messages for each RPC
- Appropriate go_package option for testdata
- Comprehensive coverage of query and mutation patterns
router/pkg/connectrpc/service_discovery.go (1)
225-258: Let me create a final test to definitively prove the review comment's example is incorrect:
<function_calls>
#!/bin/bashFinal verification test with the exact scenario from review comment
python3 << 'EOF'
def extractPackageFromProto(content):
"""Exact Go implementation simulation"""
lines = content.split("\n")
for line in lines:
line = line.strip()
if line.startswith("package "): # This is the key check
pkg = line[len("package "):]
pkg = pkg.rstrip(";")
pkg = pkg.strip()
return pkg
return ""EXACT scenario from review comment
example_from_review = """// Old package: package employee.v1;
package company.v2;"""result = extractPackageFromProto(example_from_review)
print("=" * 60)
print("TESTING EXACT REVIEW COMMENT SCENARIO")
print("=" * 60)
print(f"Proto content:\n{repr(example_from_review)}\n")
print(f"Result extracted: '{result}'")
print(f"Review claims: would extract 'employee.v1' (WRONG)")
print(f"Actual result: '{result}' (CORRECT)")
print(f"\nReview comment assertion: INCORRECT")
print(f"Reason: Line '// Old package: package employee.v1;' starts with '//'")
print(f" so it does NOT match the check: line.startswith('package ')")
EOF
</function_calls>
router/pkg/connectrpc/samples/services/employee.v1/employee.proto
Outdated
Show resolved
Hide resolved
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
4 times, most recently
from
016c128 to
3059f5e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2379 +/- ##
==========================================
+ Coverage 1.49% 60.88% +59.38%
==========================================
Files 292 237 -55
Lines 46968 25104 -21864
Branches 431 0 -431
==========================================
+ Hits 703 15285 +14582
+ Misses 45982 8474 -37508
- Partials 283 1345 +1062
🚀 New features to boost your workflow:
|
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
4d210e1 to
50c0cf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
router/pkg/connectrpc/vanguard_service.go (1)
242-286: Improvement from past review, but message sanitization still needed.The error response is now properly formatted as Connect JSON, which is an improvement. However, the
connectErr.Message()at line 258 may still contain internal details (service names, internal URLs, implementation specifics) from theHandleRPCerror chain.The past review comment's concern about error sanitization remains partially valid. Consider implementing a sanitization layer for error messages returned to clients:
func sanitizeErrorMessage(err *connect.Error) string { // For internal errors, return a generic message if err.Code() == connect.CodeInternal { return "internal server error" } // For other errors, the message is typically safe return err.Message() }Then use:
"message": sanitizeErrorMessage(connectErr)This ensures internal infrastructure details don't leak while preserving useful client-facing error messages for validation errors, not-found, etc.
router/pkg/connectrpc/server.go (1)
283-284: Reload does not wrap handler in h2c, breaking HTTP/2 support.This issue was raised in a previous review. In
Start()(line 177), the handler is wrapped withh2c.NewHandler(handler, &http2.Server{}), butReload()sets the handler directly without the h2c wrapper, which will break gRPC/HTTP/2 compatibility after a reload.// Update HTTP server handler - s.httpServer.Handler = s.createHandler() + handler := s.createHandler() + s.httpServer.Handler = h2c.NewHandler(handler, &http2.Server{})
🧹 Nitpick comments (22)
protographic/tests/operations/operation-validation.test.ts (1)
201-361: PascalCase validation tests are solid; consider a couple of explicit edge casesThis suite exercises the new rule well across success and failure paths (queries, mutations, subscriptions, and error messaging). To make the contract even clearer, you might optionally add tests for names that:
- Start uppercase but contain underscores (e.g.
Get_user)- Start with an underscore (e.g.
_GetUser), if those are intentionally disallowedThese are already rejected by the regex, but dedicated tests would document that behavior.
protographic/src/operation-to-proto.ts (1)
259-300: PascalCase validation is correct; tweak wording to match behavior (esp. withprefixOperationType)The regex-based PascalCase check is straightforward and matches the new tests (rejects camelCase, snake_case, ALL‑CAPS, etc.), which is good.
Two minor polish points:
- Error message vs.
prefixOperationTypebehaviorThe error text says:
“This ensures the RPC method name exactly matches the GraphQL operation name.”
But when
prefixOperationTypeistrue,methodNamebecomes e.g.QueryGetUser, so the RPC method no longer “exactly matches” the GraphQL operation name. You can avoid that mismatch with a small wording change while keeping the tests intact. For example:- `This ensures the RPC method name exactly matches the GraphQL operation name.`, + `This helps ensure RPC method names derived from GraphQL operations remain predictable.`,This still satisfies the existing regex expectations in the tests.
- Comment above method name construction
Similarly, this comment is slightly stronger than the actual behavior once the prefix is applied:
// 4. Create method name from operation name // Use operation name as-is to ensure exact matching (no transformation) let methodName = operationName;Consider softening it so it’s accurate both with and without
prefixOperationType:- // 4. Create method name from operation name - // Use operation name as-is to ensure exact matching (no transformation) + // 4. Create method name from operation name + // Use the operation name as the base (no additional casing transformation)This keeps the intent clear (no more automatic camelCase / capitalization magic) while reflecting that an optional type prefix may still be applied.
router/pkg/config/config.go (1)
1012-1022: ConnectRPCConfiguration wiring looks correct; consider validation for required fieldsThe
ConnectRPCConfiguration/ConnectRPCServerstructs use sensible YAML keys and env var names and line up with the defaults JSON. To avoid hard-to-debug runtime failures, it would be good to ensure (viaconfig.schema.jsonand/or Go-level post-processing) thatconnect_rpc.enabled: trueis not allowed with an emptyconnect_rpc.graphql_endpoint(and any other fields you consider mandatory when enabled). If the schema already enforces this, no further change needed here.router/pkg/connectrpc/samples/services/employee.v1/service.proto (1)
1-191: Consider consolidating duplicate proto files.This proto file is duplicated between
router/pkg/connectrpc/samples/services/employee.v1/androuter-tests/testdata/connectrpc/services/employee.v1/. While the duplication might be intentional (samples for documentation, testdata for tests), it creates maintenance burden and risk of divergence.Consider:
- Using symlinks if both locations need the same file
- Generating one from the other during build
- Documenting why duplication is necessary if it serves different purposes
router-tests/connectrpc/README.md (1)
47-56: Add a language to the directory tree fenced block to satisfy markdownlint (MD040).The directory structure block lacks a language identifier. To keep markdownlint happy and be consistent with the earlier Go snippet, add a language like
text:-``` +```text router-tests/ ├── connectrpc/ │ ├── server.go │ └── README.md ├── connectrpc_test.go └── testdata/ └── connectrpc/ └── services/router/pkg/connectrpc/proto_loader_test.go (1)
20-109: ProtoLoader tests cover key metadata paths; only a minor naming nit.The subtests thoroughly exercise service discovery, method enumeration, and method metadata (names, full names, input/output types, streaming flags) against the employee.v1 sample; this is solid coverage.
One tiny optional polish: the length assertion message mentions
employee_only directorywhile the path under test is"samples/services/employee.v1". Renaming the message for consistency would avoid future confusion:- assert.Len(t, services, 1, "Should load exactly one service from employee_only directory") + assert.Len(t, services, 1, "Should load exactly one service from samples/services/employee.v1")Otherwise, the test logic looks good.
router-tests/testdata/connectrpc/README.md (1)
7-27: Add language specification to fenced code block.The directory structure code block should include a language specification to satisfy markdown linting rules and improve rendering.
-``` +```text router-tests/testdata/connectrpc/router-tests/connectrpc/connectrpc_test.go (1)
44-53: Test name doesn't match behavior.The subtest is named "reloads operations on schema change" but it only verifies that
Reload()doesn't return an error. Consider either renaming it to something like "reload completes without error" or expanding the test to verify that operations are actually reloaded by comparing operation counts/contents before and after reload.router/pkg/connectrpc/vanguard_service_test.go (1)
249-260: Direct access to internal map is fragile.The test directly manipulates the internal
operationsmap of the registry (opRegistry.operations[serviceName]). This couples the test to the implementation details and will break if the internal structure changes. Consider adding a test helper method or using the public API to register operations.If
OperationRegistrydoesn't expose a public method for adding operations in tests, consider adding one:// In operation_registry.go or test_helpers.go func (r *OperationRegistry) RegisterOperation(serviceName, methodName string, op *schemaloader.Operation) { r.mu.Lock() defer r.mu.Unlock() if r.operations[serviceName] == nil { r.operations[serviceName] = make(map[string]*schemaloader.Operation) } r.operations[serviceName][methodName] = op }router-tests/connectrpc/connectrpc_server_lifecycle_test.go (1)
53-78: Clarify expected behavior for concurrent starts.The test verifies "at least one start should succeed" but doesn't specify what should happen to the other concurrent start attempts. Should they return an error (e.g., "server already started") or also succeed (idempotent)?
Consider adding assertions to verify the expected behavior:
// At least one should succeed successCount := 0 +alreadyStartedCount := 0 for _, err := range errors { if err == nil { successCount++ + } else if strings.Contains(err.Error(), "already") { + alreadyStartedCount++ } } -assert.GreaterOrEqual(t, successCount, 1, "at least one start should succeed") +assert.Equal(t, 1, successCount, "exactly one start should succeed") +assert.Equal(t, 2, alreadyStartedCount, "other starts should fail with 'already started'")If the server is designed to be idempotent (multiple starts succeed), document this expectation and adjust the assertion accordingly.
router/core/router.go (1)
1560-1571: Unnecessary type assertion for shutdown.The
connectRPCServerfield is assigned fromconnectrpc.NewServer()which returns*connectrpc.Server. The type assertion to check forStop(context.Context) errorinterface is redundant since the concrete type is known.Consider simplifying by using the concrete type directly:
if r.connectRPCServer != nil { wg.Add(1) go func() { defer wg.Done() - // Type assert to access Stop method - if server, ok := r.connectRPCServer.(interface{ Stop(context.Context) error }); ok { - if subErr := server.Stop(ctx); subErr != nil { - err.Append(fmt.Errorf("failed to shutdown connect_rpc server: %w", subErr)) - } + if subErr := r.connectRPCServer.Stop(ctx); subErr != nil { + err.Append(fmt.Errorf("failed to shutdown connect_rpc server: %w", subErr)) } }() }This requires changing the field type in
router_config.gofrominterface{}oranyto*connectrpc.Serverif it isn't already.router-tests/connectrpc/connectrpc_client_test.go (1)
239-295: Concurrency test could benefit from timeout context.The concurrency test is good but uses unbounded
context.Background()which could hang indefinitely if there's an issue.Consider adding a timeout to prevent test hangs:
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + for i := 0; i < numRequests; i++ { go func() { req := connect.NewRequest(&employeev1.GetEmployeeByIdRequest{ EmployeeId: 1, }) - _, err := client.GetEmployeeById(context.Background(), req) + _, err := client.GetEmployeeById(ctx, req) results <- err }() }You'd also need to add
"time"to the imports.router/pkg/connectrpc/constructor_validation_test.go (1)
134-141: Consider usingt.Parallel()for subtests.The table-driven test loop doesn't run subtests in parallel. Since these are validation tests without shared mutable state, they could run in parallel for faster execution.
for _, tt := range tests { + tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { + t.Parallel() result, err := tt.constructor() assert.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), tt.wantErr) }) }router/pkg/connectrpc/test_helpers.go (2)
78-84: Mock response missing required HTTP fields.The
mockRoundTripper.RoundTripreturns a response without settingProto,ProtoMajor,ProtoMinor, andRequestfields. While this works for basic tests, it may cause issues if code inspects these fields.func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: m.statusCode, Body: io.NopCloser(strings.NewReader(m.responseBody)), Header: make(http.Header), + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Request: req, }, nil }
94-102: Direct access to unexported fieldoperationsbreaks encapsulation.The test helper directly accesses
opRegistry.operations, which is an unexported field. This works because the test helper is in the same package, but it tightly couples tests to implementation details.Consider adding a test-only method to
OperationRegistryor using the publicRegistermethod if one exists:- // Manually add test operations to the registry using service-scoped approach - serviceName := "employee.v1.EmployeeService" - if opRegistry.operations[serviceName] == nil { - opRegistry.operations[serviceName] = make(map[string]*schemaloader.Operation) - } - opRegistry.operations[serviceName]["GetEmployeeById"] = &schemaloader.Operation{ + // Use public API to register test operations + opRegistry.Register("employee.v1.EmployeeService", &schemaloader.Operation{ Name: "GetEmployeeById", OperationType: "query", OperationString: "query GetEmployeeById($id: Int!) { employee(id: $id) { id name } }", - } + })If no public registration method exists, consider adding one like
RegisterForTestwith a clear name indicating it's for testing purposes.router/pkg/connectrpc/handler_test.go (1)
84-94: Missing test case for ProtoLoader validation.According to the
NewRPCHandlervalidation logic inhandler.go,ProtoLoaderis a required dependency. Consider adding a test case to verify the error whenProtoLoaderis nil:t.Run("returns error when proto loader is missing", func(t *testing.T) { operationRegistry := NewOperationRegistry(logger) handler, err := NewRPCHandler(HandlerConfig{ GraphQLEndpoint: "http://localhost:4000/graphql", HTTPClient: httpClient, Logger: logger, OperationRegistry: operationRegistry, }) assert.Error(t, err) assert.Nil(t, handler) assert.Contains(t, err.Error(), "proto loader is required") })router/pkg/connectrpc/validator.go (3)
63-78: Consider removing or repurposing these helper functions.
getKeysandgetFieldNamesare currently only used by the debug print statements. If the debug statements are removed, these functions become dead code. Either remove them or repurpose them for structured logging.
80-115: Consider validating for unknown fields in JSON.The current implementation only validates that required proto fields exist in JSON but doesn't reject unknown JSON keys that aren't defined in the proto schema. Depending on your requirements, you may want to add validation for unexpected fields:
// After validating all proto fields, check for unknown keys in JSON for key := range data { found := false for _, field := range fields { if field.GetName() == key || field.GetJSONName() == key { found = true break } } if !found { return &ValidationError{ Field: fieldPath + key, Message: "unknown field", } } }This would help catch typos and invalid field names early. However, if you prefer lenient validation (ignoring unknown fields), the current behavior is acceptable.
294-305: Consider validating enum values against allowed members.The current enum validation only checks that the value is a string or number, but doesn't verify the value is a valid enum member. This could allow invalid enum values through:
case descriptorpb.FieldDescriptorProto_TYPE_ENUM: enumDesc := field.GetEnumType() switch v := value.(type) { case string: // Validate enum name exists if enumDesc.FindValueByName(v) == nil { return &ValidationError{ Field: fieldPath, Message: fmt.Sprintf("invalid enum value: %s", v), } } case float64: // Validate enum number exists if enumDesc.FindValueByNumber(protoreflect.EnumNumber(int32(v))) == nil { return &ValidationError{ Field: fieldPath, Message: fmt.Sprintf("invalid enum value: %v", v), } } // ... }This is optional as lenient validation may be acceptable for your use case.
router-tests/connectrpc/connectrpc_test_helpers.go (1)
230-237: Potential JSON injection in ErrorGraphQLHandler.The
messageparameter is directly interpolated into JSON without escaping, which could produce invalid JSON if the message contains quotes or other special characters:w.Write([]byte(fmt.Sprintf(`{"errors": [{"message": "%s"}]}`, message)))For test helpers this is low risk, but consider using proper JSON encoding:
-func ErrorGraphQLHandler(message string) http.HandlerFunc { - return func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - w.Write([]byte(fmt.Sprintf(`{"errors": [{"message": "%s"}]}`, message))) - } -} +func ErrorGraphQLHandler(message string) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + response := map[string]interface{}{ + "errors": []map[string]string{{"message": message}}, + } + json.NewEncoder(w).Encode(response) + } +}router/pkg/connectrpc/vanguard_service.go (1)
109-153: Consider reducing log verbosity for production.Multiple Info-level log statements during service registration could be noisy in production environments with many services:
- Lines 110-113: Service registration info
- Lines 117-121: Each method logged individually
- Lines 127-133: Service discovery info
- Lines 148-152: Registration success
Consider using Debug level for the per-method logging (lines 117-121) and consolidating the Info-level logs:
- vs.logger.Info("service method", + vs.logger.Debug("service method", zap.String("service", serviceName), zap.String("method", method.Name),router/pkg/connectrpc/proto_loader.go (1)
323-327: Consider returning a copy to prevent accidental mutation.The method returns the internal map directly. While the comment advises treating it as read-only, callers could accidentally mutate the map. For defensive coding, consider returning a shallow copy.
func (pl *ProtoLoader) GetServices() map[string]*ServiceDefinition { - return pl.services + result := make(map[string]*ServiceDefinition, len(pl.services)) + for k, v := range pl.services { + result[k] = v + } + return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
router-tests/go.sumis excluded by!**/*.sumrouter-tests/testdata/connectrpc/client/employee.v1/service.pb.gois excluded by!**/*.pb.gorouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (53)
cli/src/commands/grpc-service/commands/generate.ts(0 hunks)protographic/src/operation-to-proto.ts(2 hunks)protographic/tests/operations/operation-validation.test.ts(1 hunks)router-tests/connectrpc/README.md(1 hunks)router-tests/connectrpc/connectrpc_client_test.go(1 hunks)router-tests/connectrpc/connectrpc_server_lifecycle_test.go(1 hunks)router-tests/connectrpc/connectrpc_test.go(1 hunks)router-tests/connectrpc/connectrpc_test_helpers.go(1 hunks)router-tests/go.mod(3 hunks)router-tests/testdata/connectrpc/README.md(1 hunks)router-tests/testdata/connectrpc/buf.gen.yaml(1 hunks)router-tests/testdata/connectrpc/buf.yaml(1 hunks)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/service.proto(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.json(1 hunks)router/core/router.go(4 hunks)router/core/router_config.go(3 hunks)router/core/supervisor_instance.go(1 hunks)router/go.mod(3 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)router/pkg/connectrpc/constructor_validation_test.go(1 hunks)router/pkg/connectrpc/error_handling_test.go(1 hunks)router/pkg/connectrpc/handler.go(1 hunks)router/pkg/connectrpc/handler_test.go(1 hunks)router/pkg/connectrpc/proto_loader.go(1 hunks)router/pkg/connectrpc/proto_loader_test.go(1 hunks)router/pkg/connectrpc/samples/service.graphqls(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/service.proto(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.json(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/server_test.go(1 hunks)router/pkg/connectrpc/service_discovery.go(1 hunks)router/pkg/connectrpc/test_helpers.go(1 hunks)router/pkg/connectrpc/validator.go(1 hunks)router/pkg/connectrpc/vanguard_service.go(1 hunks)router/pkg/connectrpc/vanguard_service_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cli/src/commands/grpc-service/commands/generate.ts
✅ Files skipped from review due to trivial changes (2)
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql
🚧 Files skipped from review as they are similar to previous changes (4)
- router/pkg/connectrpc/server_test.go
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql
- router/core/router_config.go
- router/pkg/config/config.schema.json
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/connectrpc/connectrpc_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter-tests/connectrpc/connectrpc_client_test.gorouter-tests/go.modrouter/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/constructor_validation_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router-tests/connectrpc/connectrpc_test.gorouter/pkg/connectrpc/test_helpers.gorouter/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/error_handling_test.gorouter/pkg/connectrpc/vanguard_service_test.gorouter/pkg/connectrpc/vanguard_service.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router-tests/connectrpc/connectrpc_test.gorouter-tests/testdata/connectrpc/buf.gen.yamlrouter-tests/go.modrouter/core/router.gorouter/go.mod
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/connectrpc/validator.go
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/connectrpc/validator.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/router.gorouter/core/supervisor_instance.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
protographic/src/operation-to-proto.tsprotographic/tests/operations/operation-validation.test.ts
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.mod
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/go.mod
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/pkg/connectrpc/handler.go
🧬 Code graph analysis (15)
router-tests/connectrpc/connectrpc_test.go (2)
router-tests/testenv/testenv.go (2)
Run(105-122)Environment(1763-1799)router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)
router-tests/connectrpc/connectrpc_server_lifecycle_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)router/pkg/connectrpc/server.go (1)
ServerConfig(21-33)
router-tests/connectrpc/connectrpc_client_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (5)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)EmployeeGraphQLHandler(222-228)ErrorGraphQLHandler(231-237)HTTPErrorHandler(240-245)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
NewEmployeeServiceClient(77-130)
router/pkg/connectrpc/validator.go (1)
router/pkg/connectrpc/proto_loader.go (1)
ProtoLoader(53-59)
router/pkg/config/config.go (3)
router/pkg/connectrpc/server.go (1)
Server(36-47)router/core/graph_server.go (1)
Server(69-72)router-tests/jwks/jwks.go (1)
Server(24-29)
router/core/router.go (3)
router/pkg/connectrpc/server.go (3)
Server(36-47)ServerConfig(21-33)NewServer(50-90)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)router/pkg/config/config.go (1)
ConnectRPCConfiguration(1012-1017)
router/pkg/connectrpc/test_helpers.go (3)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)router/pkg/connectrpc/handler.go (2)
RPCHandler(139-145)HandlerConfig(148-154)router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)
router/pkg/connectrpc/handler_test.go (4)
router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)router/pkg/connectrpc/proto_loader.go (2)
NewProtoLoader(62-72)ProtoLoader(53-59)router/pkg/connectrpc/handler.go (2)
NewRPCHandler(157-190)HandlerConfig(148-154)router/pkg/connectrpc/test_helpers.go (1)
MockHTTPClient(64-71)
router/pkg/connectrpc/constructor_validation_test.go (3)
router/pkg/connectrpc/handler.go (3)
NewRPCHandler(157-190)HandlerConfig(148-154)RPCHandler(139-145)router/pkg/connectrpc/server.go (1)
ServerConfig(21-33)router/pkg/connectrpc/vanguard_service.go (2)
NewVanguardService(68-93)VanguardServiceConfig(53-57)
router/pkg/connectrpc/server.go (5)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)router/pkg/connectrpc/operation_registry.go (2)
OperationRegistry(24-29)NewOperationRegistry(32-41)router/pkg/connectrpc/handler.go (3)
RPCHandler(139-145)NewRPCHandler(157-190)HandlerConfig(148-154)router/pkg/connectrpc/vanguard_service.go (3)
VanguardService(60-65)NewVanguardService(68-93)VanguardServiceConfig(53-57)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)
router/pkg/connectrpc/vanguard_service.go (2)
router/pkg/connectrpc/handler.go (2)
MetaKeyGraphQLErrors(101-101)GraphQLError(125-130)router/pkg/connectrpc/proto_loader.go (2)
ServiceDefinition(17-30)MethodDefinition(33-50)
router/pkg/connectrpc/proto_loader_test.go (3)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)router-tests/testenv/testenv.go (1)
Run(105-122)demo/pkg/subgraphs/test1/subgraph/model/models_gen.go (1)
InputType(346-348)
router-tests/connectrpc/connectrpc_test_helpers.go (1)
router/pkg/connectrpc/server.go (1)
ServerConfig(21-33)
router/pkg/connectrpc/handler.go (3)
router/pkg/connectrpc/operation_registry.go (1)
OperationRegistry(24-29)router/pkg/connectrpc/validator.go (3)
MessageValidator(13-15)NewMessageValidator(18-22)ValidationError(25-28)router/pkg/connectrpc/proto_loader.go (1)
ProtoLoader(53-59)
router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.go (22)
GetEmployeeByIdRequest(222-227)GetEmployeeByIdRequest(240-240)GetEmployeeByIdRequest(255-257)GetEmployeeByIdResponse(266-271)GetEmployeeByIdResponse(284-284)GetEmployeeByIdResponse(299-301)GetEmployeesRequest(479-483)GetEmployeesRequest(496-496)GetEmployeesRequest(511-513)GetEmployeesResponse(515-520)GetEmployeesResponse(533-533)GetEmployeesResponse(548-550)GetEmployeesWithMoodResponse(435-440)GetEmployeesWithMoodResponse(453-453)GetEmployeesWithMoodResponse(468-470)UpdateEmployeeMoodRequest(125-131)UpdateEmployeeMoodRequest(144-144)UpdateEmployeeMoodRequest(159-161)UpdateEmployeeMoodResponse(177-183)UpdateEmployeeMoodResponse(196-196)UpdateEmployeeMoodResponse(211-213)File_employee_v1_service_proto(1853-1853)
🪛 markdownlint-cli2 (0.18.1)
router-tests/testdata/connectrpc/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
router-tests/connectrpc/README.md
47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
router-tests/testdata/connectrpc/services/employee.v1/service.proto
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
router/pkg/connectrpc/server.go (1)
288-289: Critical: Reload does not wrap handler in h2c, breaking HTTP/2 support.In
Start()at lines 176-177, the handler is wrapped withh2c.NewHandler(handler, &http2.Server{})to enable HTTP/2 over cleartext. However,Reload()sets the handler directly without the h2c wrapper, which will break gRPC compatibility after a reload.Apply this diff to fix:
// Update HTTP server handler - s.httpServer.Handler = s.createHandler() + handler := s.createHandler() + s.httpServer.Handler = h2c.NewHandler(handler, &http2.Server{})
🧹 Nitpick comments (1)
router/pkg/connectrpc/server.go (1)
330-336: Optional: Consider removing unused statusCode field.The
responseWritercaptures the HTTP status code inWriteHeader()(line 369), but the capturedstatusCodefield is never read or used elsewhere in the codebase. If this is intentional for future logging or metrics, consider adding a comment. Otherwise, you can simplify by removing the unused field.If you decide to remove it, apply this diff:
type responseWriter struct { http.ResponseWriter - statusCode int } func (rw *responseWriter) WriteHeader(code int) { - rw.statusCode = code rw.ResponseWriter.WriteHeader(code) }And remove the initialization in
createHandler:- rw := &responseWriter{ResponseWriter: w, statusCode: 200} + rw := &responseWriter{ResponseWriter: w}Also applies to: 365-371
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
router/pkg/connectrpc/constructor_validation_test.go(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/validator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/connectrpc/constructor_validation_test.go
- router/pkg/connectrpc/validator.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
router/pkg/connectrpc/server.go (7)
20-47: LGTM!The configuration structure and server type definitions are well-organized and include all necessary components for lifecycle management, service discovery, proto loading, operation registry, and HTTP/2 support.
49-90: LGTM!The constructor properly validates required configuration, sets sensible defaults, and creates a resilient HTTP client with retry capabilities. The protocol normalization for GraphQL endpoint is appropriate.
92-208: LGTM!The Start method correctly orchestrates service discovery, component initialization, and server startup. The ProtoLoader is created before initializeComponents() (ensuring the RPCHandler gets the fresh loader), and the handler is properly wrapped with h2c for HTTP/2 support.
210-227: LGTM!The Stop method implements proper graceful shutdown with a reasonable timeout and appropriate error handling.
295-315: LGTM!The component initialization method correctly creates the operation registry and RPC handler, with clear documentation about the ProtoLoader dependency.
317-323: LGTM!The deprecated LoadOperations method is appropriately maintained for backward compatibility with a clear warning message.
344-410: LGTM!The accessor methods properly handle nil checks, and the responseWriter's streaming interface implementations (Flush, Push, Hijack) correctly delegate to the underlying ResponseWriter when supported, which is essential for gRPC streaming compatibility.
router-tests/testdata/connectrpc/services/employee.v1/service.proto
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
router-tests/testdata/connectrpc/README.md (1)
24-33: Scope concern: Verify whether "Regenerating Client Code" section aligns with feedback.The past review feedback requested cutting the README down to "just explaining how to write new tests." The current "Regenerating Client Code" section (lines 24+) addresses client code management rather than test writing. If the intent is to focus strictly on test authoring, consider moving or removing this section; otherwise, clarify the README's scope in the intro.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router-tests/testdata/connectrpc/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
🪛 markdownlint-cli2 (0.18.1)
router-tests/testdata/connectrpc/README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
router/pkg/connectrpc/handler_test.go (1)
84-94: Add missing test case for nil ProtoLoader.The constructor validates that
ProtoLoaderis not nil (handler.go line 177-179), but there's no test case covering this validation path.Add a test case:
+ t.Run("should return error when proto loader is missing", func(t *testing.T) { + operationRegistry := NewOperationRegistry(logger) + handler, err := NewRPCHandler(HandlerConfig{ + GraphQLEndpoint: "http://localhost:4000/graphql", + HTTPClient: httpClient, + Logger: logger, + OperationRegistry: operationRegistry, + }) + + assert.Error(t, err) + assert.Nil(t, handler) + assert.ErrorContains(t, err, "proto loader is required") + })router/pkg/connectrpc/server.go (1)
109-134: Consider using LoadFromDirectories for consistency.The code calls
LoadFromDirectoryseparately for each service (line 118), which bypasses the package uniqueness validation inLoadFromDirectories. While service discovery already validatespackage.serviceuniqueness, usingLoadFromDirectorieswould provide additional validation and be more efficient.Consider collecting all service directories and loading them in one call:
+ // Collect all service directories + serviceDirs := make([]string, len(discoveredServices)) + for i, service := range discoveredServices { + serviceDirs[i] = service.ServiceDir + } + + // Load all proto files at once (validates package uniqueness) + if err := server.protoLoader.LoadFromDirectories(serviceDirs); err != nil { + return nil, fmt.Errorf("failed to load proto files: %w", err) + } + // Load proto files and operations for each discovered service for _, service := range discoveredServices { server.logger.Info("loading service", zap.String("service", service.FullName), zap.String("dir", service.ServiceDir), zap.Int("proto_files", len(service.ProtoFiles)), zap.Int("operation_files", len(service.OperationFiles))) - // Load proto files for this service - if err := server.protoLoader.LoadFromDirectory(service.ServiceDir); err != nil { - return nil, fmt.Errorf("failed to load proto files for service %s: %w", service.FullName, err) - } - // Load operations for this service if len(service.OperationFiles) > 0 {router/pkg/config/config.schema.json (1)
2198-2202: Usehttp-urlformat for consistency with other endpoint configurations.Line 2201 specifies
graphql_endpointwithformat: "url", but other endpoint fields in the schema (e.g.,cdn.url,mcp.router_url) use the more restrictivehttp-urlformat. For consistency and validation clarity, consider updating this tohttp-url."graphql_endpoint": { "type": "string", - "description": "Custom URL to use for the router GraphQL endpoint in ConnectRPC. Use this when your router is behind a proxy. This URL is used to forward RPC requests as GraphQL operations.", + "description": "Custom URL to use for the router GraphQL endpoint in ConnectRPC. Use this when your router is behind a proxy. This URL is used to forward RPC requests as GraphQL operations.", - "format": "url" + "format": "http-url" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
router-tests/testdata/connectrpc/buf.gen.yaml(1 hunks)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go(1 hunks)router/core/router.go(5 hunks)router/core/router_config.go(4 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/connectrpc/error_handling_test.go(1 hunks)router/pkg/connectrpc/handler.go(1 hunks)router/pkg/connectrpc/handler_test.go(1 hunks)router/pkg/connectrpc/proto_loader.go(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/server_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- router/core/router_config.go
- router/pkg/config/config.go
- router/pkg/connectrpc/server_test.go
- router-tests/testdata/connectrpc/buf.gen.yaml
- router/core/router.go
- router/pkg/connectrpc/error_handling_test.go
- router/pkg/connectrpc/handler.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
🧬 Code graph analysis (2)
router/pkg/connectrpc/handler_test.go (4)
router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)router/pkg/connectrpc/proto_loader.go (2)
NewProtoLoader(62-72)ProtoLoader(53-59)router/pkg/connectrpc/handler.go (2)
NewRPCHandler(161-194)HandlerConfig(152-158)router/pkg/connectrpc/test_helpers.go (1)
MockHTTPClient(64-71)
router/pkg/connectrpc/server.go (4)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)router/pkg/connectrpc/handler.go (3)
RPCHandler(143-149)NewRPCHandler(161-194)HandlerConfig(152-158)router/pkg/connectrpc/vanguard_service.go (3)
VanguardService(60-65)NewVanguardService(68-93)VanguardServiceConfig(53-57)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
router/pkg/connectrpc/handler_test.go (7)
112-178: LGTM!The tests properly cover both success and error paths for RPC handling, including service-scoped operation registry usage.
184-220: LGTM - Test structure improved.The nested test structure follows the previous review feedback and clearly describes expected behavior ("forwarding headers from context should forward listed headers").
222-249: LGTM!The test properly verifies HTTP transport error handling using a custom error round tripper.
251-280: LGTM!The test correctly verifies operation counting across service-scoped registries.
282-311: LGTM!The test properly validates both positive and negative cases for operation existence.
313-496: LGTM - Comprehensive coverage.The test suite thoroughly covers all edge cases for protobuf JSON to GraphQL variable conversion, including nested structures, arrays, nulls, and empty values.
498-577: LGTM!Both test functions provide comprehensive coverage with clean table-driven tests for
snakeToCameland thorough case coverage forconvertKeysRecursive.router/pkg/connectrpc/proto_loader.go (9)
16-72: LGTM!The struct definitions are well-documented, and the constructor properly initializes the local
filesregistry, avoiding global registry conflicts.
74-104: LGTM!The directory loading logic is clear and handles errors appropriately.
138-175: LGTM - Package uniqueness validation fixed.The validation logic has been corrected per previous review feedback. By tracking existing services before loading each file (lines 139-142) and only validating newly added services (lines 155-159), duplicate proto packages from different directories will now be properly detected.
185-206: LGTM!Standard recursive file discovery implementation.
208-236: LGTM!The proto file parsing logic is standard and handles errors appropriately.
238-278: LGTM - Race condition resolved.The global registry race condition has been fixed by using a local
pl.filesregistry instance (line 255). Each ProtoLoader instance now has its own registry, eliminating cross-instance conflicts mentioned in previous reviews.
280-322: LGTM!The service definition extraction logic properly converts descriptors and extracts all method metadata.
324-334: LGTM!The getter methods are straightforward. The read-only comment on
GetServicesis helpful guidance for callers.
336-356: LGTM!The method lookup and files registry accessor are correctly implemented.
router/pkg/connectrpc/server.go (10)
20-47: LGTM!The configuration and server structs are well-defined with appropriate fields for lifecycle management and component wiring.
168-213: LGTM!The server start logic properly wraps the handler with h2c for HTTP/2 support and starts the server with appropriate timeouts.
215-232: LGTM!Standard graceful shutdown implementation with appropriate timeout.
234-299: LGTM - Previous issues resolved.Both critical issues from previous reviews have been fixed:
- The h2c wrapper is now applied during reload (line 295)
- ProtoLoader is created before
initializeComponents(line 249), ensuring the handler gets fresh proto definitions
301-321: LGTM!Component initialization is clear, and the comment about ProtoLoader requirement is helpful.
323-340: LGTM!The handler creation logic properly wraps the transcoder to capture response status.
342-356: LGTM!The getter methods properly handle nil checks and delegate to the vanguard service.
358-391: LGTM!The response writer wrapper properly implements all required streaming interfaces and checks for underlying support before delegating.
393-399: LGTM!The operation count getter properly handles nil checks.
401-407: LGTM!The address accessor properly handles the nil listener case.
router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
1-307: Generated code - LGTM.This file is auto-generated by
protoc-gen-connect-go(line 1). The generated client and handler scaffolding follows standard Connect RPC patterns and should not be manually modified.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
6836b37 to
c1b1b30
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
router-tests/testdata/connectrpc/README.md (1)
16-22: Add a language identifier to the fenced code block.Markdown lint (MD040) expects a language on fenced blocks; use a generic language like
textfor the directory tree example.-``` +```text services/ └── myservice.v1/ ├── service.proto # Proto service definition ├── QueryGetItem.graphql # GraphQL query operation └── MutationCreateItem.graphql # GraphQL mutation operation -``` +```router/pkg/config/config.schema.json (1)
2137-2204: Clarify schema semantics for global vs per-service providers inconnect_rpc.The
connect_rpcblock enforcesservices_provider_idwhenenabled: true, but theservicesitems’proto_provider_id/operations_provider_idremain unconstrained (optional, nominItems, no relationship with the global provider). This makes it easy to express configs the runtime may not actually support (e.g., mixed or partially-specified overrides).Consider tightening the schema and/or comments here to describe the intended model more explicitly (global-only vs per-service overrides vs required both), and only expose per-service provider fields once they’re fully wired through the Go configuration if that’s not already the case.
router-tests/testdata/connectrpc/services/employee.v1/service.proto (1)
39-52: Fix minor grammar in mood mutation comment.The comment reads
This mutation update the mood of an employee.— consider changing toThis mutation updates the mood of an employee.(or similar) so generated docs/readers don’t stumble on it.router/pkg/connectrpc/handler_test.go (1)
112-178: Consider nested subtests for better organization.The test cases in
TestHandleRPCcould benefit from nested subtests similar toTestExecuteGraphQL(lines 184-220), which uses clear hierarchical naming like "forwarding headers from context" → "should forward listed headers". This improves readability and makes test failures easier to diagnose.router/pkg/connectrpc/validator.go (1)
217-237: Add parsing validation for int64 string representation.When
int64values are provided as strings (per protobuf JSON mapping spec), the validation only checks the type but doesn't verify the string contains a valid integer. This would accept invalid values like"abc"or"12.34".Consider adding parsing validation:
case string: - // String representation is valid for int64 + // Validate string can be parsed as int64 + if _, err := strconv.ParseInt(v, 10, 64); err != nil { + return &ValidationError{ + Field: fieldPath, + Message: fmt.Sprintf("Int64 cannot represent value: %v", value), + } + }Apply similar validation for
uint64at lines 255-272.router/pkg/connectrpc/vanguard_service.go (1)
218-231: Ensure Connect error messages exposed in JSON don’t leak internal details.This new Connect-style JSON error handling is a big improvement over writing
err.Error()directly, butwriteConnectErrorstill surfacesconnectErr.Message()verbatim to clients for all error codes:
- Any
connect.NewErrorcreated insideRPCHandler.HandleRPC(or deeper) that embeds low-level details (service/method names, internal URLs, backend messages, etc.) will have those details echoed back in the"message"field.- For internal/server-side failures (e.g.
CodeInternal,CodeUnknown,CodeDataLoss, possibly someCodeUnavailable/CodeDeadlineExceededcases), this can still expose implementation details that should ideally live only in logs.Consider tightening this in one of these ways:
Differentiate “public” vs “internal” codes.
For clearly client-facing errors (e.g.InvalidArgument,Unauthenticated,PermissionDenied,NotFound,FailedPrecondition,OutOfRange,Aborted,ResourceExhausted), keep usingconnectErr.Message()as-is (assuming upstream uses user-oriented messages).
For internal-ish codes (e.g.Internal,Unknown,DataLoss, and maybe someUnavailable/DeadlineExceeded), return a fixed generic message like"internal server error"while:
- Logging the full
connectErr(including cause and metadata) withvs.logger.Error.- Optionally preserving structured fields like
graphql_errorsin the JSON if they’re already curated/safe.Align mapping & tests.
Since this path usesconnectCodeToHTTPStatus, it might be worth a small table-driven test that asserts yourconnectCodeToHTTPStatusandhttpStatusToConnectCodestay in sync for all codes you intend to support and that “internal” codes are the ones that get generic messages.This keeps the improved Connect error shape and GraphQL metadata while reducing the chance of accidental information leakage from lower layers.
Also applies to: 244-285
router/pkg/connectrpc/samples/services/employee.v1/service.proto (1)
50-51: Fix grammatical error in comment onupdate_mood.The comment still reads
This mutation update the mood of an employee.; the verb should agree with the subject.- // This mutation update the mood of an employee. + // This mutation updates the mood of an employee.
🧹 Nitpick comments (6)
protographic/tests/operations/operation-validation.test.ts (2)
232-245: Slight duplication in camelCase scenariosYou have two tests exercising camelCase (
getUser/getUserById) with very similar setup; one checks rejection and the other focuses on the helpful message. Consider consolidating into a single test that asserts both that an error is thrown and that the message contains the examples, or change the second test to reuse a shared helper to avoid repetition.Also applies to: 292-305
306-360: Add positive-path tests for PascalCase mutation/subscription namesYou validate that non-PascalCase names for
mutationandsubscriptionare rejected, but there are no corresponding tests proving that valid PascalCase names (e.g.,CreateUser,OnMessageAdded) are accepted. Adding those would close the loop on behavior across all operation types and guard against over‑restrictive changes to the validator.protographic/src/operation-to-proto.ts (1)
304-304: Nit: Duplicate step number in comments.Line 294 and line 304 both use "4." as the step number. Consider renumbering for consistency.
- // 4. Create request message from variables + // 5. Create request message from variablesrouter/pkg/connectrpc/proto_loader_test.go (1)
11-107: Good coverage of ProtoLoader behavior; only tiny polish possible.The tests nicely cover service discovery, method metadata, and non-streaming guarantees for query methods. If you want to tighten things further later, you could:
- Factor
"samples/services/employee.v1"into a small helper/const to avoid repetition.- Add a negative-case test for unknown service or method to lock down the error contract.
Both are optional; current tests are already solid.
router/pkg/config/connectrpc_test.go (1)
12-203: Comprehensive ConnectRPC config tests; behavior is well-anchored.Zero-value, YAML + env, integration, and multi-file merge paths are all exercised cleanly, which should make future refactors to
ConnectRPCConfigurationorLoadConfigsafer. An optional future enhancement could be a test that bootstraps ConnectRPC purely fromCONNECT_RPC_*env vars (no YAML), but the current suite already covers the critical semantics.router/pkg/connectrpc/vanguard_service.go (1)
124-127: Clarify expected request path format between handler comment andextractMethodName.The comments and implementation disagree on the path shape:
- Lines 124–126:
createServiceHandlersays the handler will receive requests at paths like/Method(without the service prefix).- Lines 288–307:
extractMethodName’s comment and logic expect[/]package.Service/Methodand verify thatparts[0] == serviceName.If Vanguard actually passes paths including the fully-qualified service name (e.g.
/employee.v1.EmployeeService/GetEmployeeById), then the comment on lines 124–126 is stale and should be updated. If instead the handler really sees/Method,extractMethodNamewill always return"", and every call will be treated asCodeNotFound.Please confirm the actual
r.URL.Pathformat as seen by this handler under Vanguard and either:
- Update the comment to reflect the
package.Service/Methodformat (likely case), or- Adjust
extractMethodNameto support the/Methodformat if that’s what Vanguard supplies.Also applies to: 288-307
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
router-tests/go.sumis excluded by!**/*.sumrouter-tests/testdata/connectrpc/client/employee.v1/service.pb.gois excluded by!**/*.pb.gorouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (57)
cli/src/commands/grpc-service/commands/generate.ts(0 hunks)protographic/src/operation-to-proto.ts(2 hunks)protographic/tests/operations/operation-validation.test.ts(1 hunks)router-tests/connectrpc/connectrpc_client_test.go(1 hunks)router-tests/connectrpc/connectrpc_server_lifecycle_test.go(1 hunks)router-tests/connectrpc/connectrpc_test.go(1 hunks)router-tests/connectrpc/connectrpc_test_helpers.go(1 hunks)router-tests/go.mod(3 hunks)router-tests/testdata/connectrpc/README.md(1 hunks)router-tests/testdata/connectrpc/buf.gen.yaml(1 hunks)router-tests/testdata/connectrpc/buf.yaml(1 hunks)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/service.proto(1 hunks)router-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.json(1 hunks)router/connect.config.yaml(1 hunks)router/core/router.go(5 hunks)router/core/router_config.go(4 hunks)router/core/supervisor_instance.go(1 hunks)router/go.mod(3 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/config/connectrpc_test.go(1 hunks)router/pkg/config/testdata/config_defaults.json(1 hunks)router/pkg/config/testdata/config_full.json(1 hunks)router/pkg/connectrpc/constructor_validation_test.go(1 hunks)router/pkg/connectrpc/error_handling_test.go(1 hunks)router/pkg/connectrpc/handler.go(1 hunks)router/pkg/connectrpc/handler_test.go(1 hunks)router/pkg/connectrpc/operation_registry.go(1 hunks)router/pkg/connectrpc/operation_registry_test.go(1 hunks)router/pkg/connectrpc/proto_loader.go(1 hunks)router/pkg/connectrpc/proto_loader_test.go(1 hunks)router/pkg/connectrpc/samples/service.graphqls(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/service.proto(1 hunks)router/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.json(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/server_test.go(1 hunks)router/pkg/connectrpc/service_discovery.go(1 hunks)router/pkg/connectrpc/service_discovery_test.go(1 hunks)router/pkg/connectrpc/test_helpers.go(1 hunks)router/pkg/connectrpc/validator.go(1 hunks)router/pkg/connectrpc/vanguard_service.go(1 hunks)router/pkg/connectrpc/vanguard_service_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cli/src/commands/grpc-service/commands/generate.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql
- router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql
- router/pkg/config/config.go
- router/core/router_config.go
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router/pkg/config/testdata/config_defaults.json
- router-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphql
- router/pkg/config/testdata/config_full.json
- router/pkg/connectrpc/operation_registry_test.go
- router-tests/connectrpc/connectrpc_test.go
- router/connect.config.yaml
- router/core/supervisor_instance.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql
- router/pkg/connectrpc/constructor_validation_test.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql
- router/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.json
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql
- router/pkg/connectrpc/test_helpers.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphql
- router/pkg/connectrpc/error_handling_test.go
- router-tests/testdata/connectrpc/buf.gen.yaml
- router/pkg/connectrpc/vanguard_service_test.go
- router/go.mod
- router/pkg/connectrpc/operation_registry.go
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/server_test.gorouter/pkg/config/connectrpc_test.gorouter-tests/connectrpc/connectrpc_client_test.gorouter-tests/go.modrouter/pkg/connectrpc/handler_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/pkg/connectrpc/server_test.gorouter-tests/go.modrouter/core/router.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/handler_test.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter/pkg/connectrpc/vanguard_service.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
protographic/tests/operations/operation-validation.test.tsprotographic/src/operation-to-proto.ts
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/connectrpc/validator.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/connectrpc/validator.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-07-30T09:29:46.660Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-07T12:05:06.775Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2079
File: proto/wg/cosmo/platform/v1/platform.proto:39-45
Timestamp: 2025-08-07T12:05:06.775Z
Learning: In the Cosmo project, the proto fields for schema, mappings, and lock in ProtoInput are intentionally kept as string types rather than bytes because the team works with text data and wants it to be UTF-8 encoded for readability and text processing purposes.
Applied to files:
router/pkg/connectrpc/handler.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/pkg/connectrpc/handler.go
🧬 Code graph analysis (9)
router/pkg/connectrpc/server_test.go (1)
router/pkg/connectrpc/server.go (2)
NewServer(50-166)ServerConfig(21-33)
router/pkg/config/connectrpc_test.go (1)
router/pkg/config/config.go (3)
ConnectRPCConfiguration(1012-1017)LoadConfig(1146-1258)Config(1039-1115)
router-tests/connectrpc/connectrpc_client_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (5)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)EmployeeGraphQLHandler(222-228)ErrorGraphQLHandler(231-237)HTTPErrorHandler(240-245)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
NewEmployeeServiceClient(89-141)
router/pkg/connectrpc/handler_test.go (4)
router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(32-41)OperationRegistry(24-29)router/pkg/connectrpc/proto_loader.go (2)
NewProtoLoader(62-72)ProtoLoader(53-59)router/pkg/connectrpc/handler.go (2)
NewRPCHandler(161-194)HandlerConfig(152-158)router/pkg/connectrpc/test_helpers.go (1)
MockHTTPClient(64-71)
router-tests/connectrpc/connectrpc_server_lifecycle_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)router/pkg/connectrpc/server.go (1)
ServerConfig(21-33)
router/pkg/connectrpc/proto_loader_test.go (1)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(53-59)NewProtoLoader(62-72)
router-tests/connectrpc/connectrpc_test_helpers.go (1)
router/pkg/connectrpc/server.go (1)
ServerConfig(21-33)
router/pkg/connectrpc/vanguard_service.go (2)
router/pkg/connectrpc/handler.go (2)
MetaKeyGraphQLErrors(105-105)GraphQLError(129-134)router/pkg/connectrpc/proto_loader.go (3)
ProtoLoader(53-59)ServiceDefinition(17-30)MethodDefinition(33-50)
router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.go (24)
FindEmployeesByPetsRequest(321-327)FindEmployeesByPetsRequest(342-342)FindEmployeesByPetsRequest(357-359)FindEmployeesByPetsResponse(368-375)FindEmployeesByPetsResponse(390-390)FindEmployeesByPetsResponse(405-407)GetEmployeeByIdRequest(227-233)GetEmployeeByIdRequest(248-248)GetEmployeeByIdRequest(263-265)GetEmployeeByIdResponse(274-280)GetEmployeeByIdResponse(295-295)GetEmployeeByIdResponse(310-312)GetEmployeesRequest(501-505)GetEmployeesRequest(520-520)GetEmployeesRequest(535-537)GetEmployeesResponse(539-545)GetEmployeesResponse(560-560)GetEmployeesResponse(575-577)UpdateEmployeeMoodRequest(124-131)UpdateEmployeeMoodRequest(146-146)UpdateEmployeeMoodRequest(161-163)UpdateEmployeeMoodResponse(179-186)UpdateEmployeeMoodResponse(201-201)UpdateEmployeeMoodResponse(216-218)
🪛 markdownlint-cli2 (0.18.1)
router-tests/testdata/connectrpc/README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: build_test
🔇 Additional comments (31)
router-tests/testdata/connectrpc/buf.yaml (1)
1-9: Configuration is well-formed and appropriate for testdata.The Buf v2 configuration is syntactically valid and semantically correct. The module path, lint rules (STANDARD), and breaking change detection (FILE) are suitable for proto file validation in the ConnectRPC testdata context.
protographic/tests/operations/operation-validation.test.ts (1)
201-361: PascalCase validation tests look comprehensive and aligned with behaviorThe new suite covers the key accepted/rejected patterns and the specific error messaging; structure and expectations all look consistent with a strict PascalCase rule for operation names.
router/pkg/connectrpc/samples/service.graphqls (1)
1-346: Schema looks structurally sound and well‑suited as a ConnectRPC sample surfaceThe SDL is internally consistent: all referenced types are defined, interface implementations satisfy their contracts (e.g.,
Pet/Animal,Hobby,Experience), and theProductsunion has valid object members. The mix of queries, mutations (including uploads), subscriptions, unions, interfaces, enums, and nested inputs should give good coverage for exercising the new ConnectRPC bridge and service discovery.As long as this file is wired into the new samples/tests as intended elsewhere in the PR, I don’t see any blocking issues here.
protographic/src/operation-to-proto.ts (2)
267-279: LGTM! PascalCase validation with helpful error messaging.The regex correctly enforces PascalCase, and the suggested name using
upperFirst(camelCase(operationName))addresses previous feedback. The error message is clear with practical examples.Note: All-uppercase names like
"ID"or"GETUSER"will be rejected—this is correct behavior for enforcing PascalCase conventions.
294-296: LGTM! Direct use of operationName aligns with PascalCase validation.Since the operation name is already validated as PascalCase, using it directly for
methodNameensures exact matching between GraphQL operation names and RPC method names.router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql (1)
1-20: Well-structured fragment-based query.Fragment definitions and reuse in
FindEmployeesByPetsNamedFragmentlook consistent and idiomatic; no changes needed.router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql (1)
1-10: LGTM for GetEmployees sample.The operation shape is clear and minimal, suitable as a canonical example; nothing to adjust.
router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphql (1)
1-12: Query shape for mood + nested pets looks appropriate.The selection set exercises both scalar (
currentMood) and nested list (details.pets) fields, which is useful for the ConnectRPC E2E tests.router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql (1)
1-14: Inline-fragment variant looks correct.The inline
... on Detailsfragment is well-formed and mirrors the named-fragment variant; good complementary sample.router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
1-307: Generated EmployeeService client/handler wiring looks consistent.The connect-go stub (procedure constants, descriptors, client methods, handler routing, unimplemented handler) is internally consistent and matches the proto surface; no manual changes needed here.
router/pkg/connectrpc/server_test.go (1)
12-85: NewServer config/defaulting tests match server behavior.The tests exercise the key NewServer invariants (required fields, protocol prefixing, listen addr and timeout defaults, nil logger handling) and align with the current implementation; they should give good protection against regressions in the constructor.
router/pkg/connectrpc/service_discovery_test.go (1)
13-352: Service discovery tests comprehensively exercise the intended conventions.These tests nicely pin down the discovery rules (one proto per dir, no nested protos, unique package.service, recursive operation discovery, and the parsing helpers). They align with the documented ADR-style layout and should make future refactors to
DiscoverServicesmuch safer.router-tests/connectrpc/connectrpc_client_test.go (1)
19-295: Client protocol/error/concurrency tests look solid and race-free.The tests validate Connect/gRPC/gRPC‑Web behavior, GraphQL/HTTP error mapping to
connect.Errorcodes, and basic concurrency with an atomic request counter. The patterns (h2c client, buffered channel fan‑out/fan‑in) are sound and shouldn’t introduce data races.router-tests/go.mod (1)
6-6: Verify alignment of connectrpc.com/connect versions between connect-go and router-tests modules.The router-tests module pins
connectrpc.com/connect v1.16.2, but connect-go/go.mod uses v1.17.0. The vanguard and protoreflect versions are consistent across modules, and mcp-go is correctly at v0.36.0. Confirm whether the connect version difference (v1.16.2 vs v1.17.0) is intentional or requires alignment for multi-module compatibility.router/pkg/connectrpc/handler_test.go (2)
25-110: LGTM: Constructor validation is comprehensive.The test coverage for
NewRPCHandlervalidation paths is thorough, including error cases, defaults, and protocol normalization.
313-577: LGTM: Variable conversion tests are comprehensive.The test coverage for proto JSON to GraphQL variable conversion is excellent, covering nested structures, arrays, null handling, empty inputs, and edge cases for the
snakeToCamelhelper.router/core/router.go (3)
960-1034: LGTM: ConnectRPC bootstrap follows established patterns.The ConnectRPC bootstrap flow is well-structured and consistent with the MCP bootstrap pattern. Service discovery, provider resolution, endpoint determination, and error handling are all appropriately implemented.
1551-1557: LGTM: Proper shutdown integration.The ConnectRPC server shutdown is correctly integrated into the graceful shutdown flow using
wg.Go()(available in Go 1.25+), with appropriate error handling and wrapping.
2208-2212: LGTM: Standard option implementation.The
WithConnectRPCoption follows the established pattern for router configuration options.router/pkg/connectrpc/validator.go (1)
54-62: LGTM: Debug logging properly structured.The validation logging now uses structured logging with appropriate debug level and field names, addressing previous feedback about debug print statements.
router/pkg/connectrpc/proto_loader.go (2)
238-278: LGTM: Local registry addresses previous race condition concerns.The proto loader now uses a local registry (
pl.files) instead of the global registry, eliminating the concurrent registration issues flagged in previous reviews. EachProtoLoaderinstance maintains its own registry, making the check-then-register pattern safe.
138-175: LGTM: Package uniqueness validation correctly implemented.The package uniqueness logic now properly tracks newly added services using an
existingServicesmap, addressing previous feedback about broken validation. The implementation correctly enforces unique proto package names across all service directories.router/pkg/connectrpc/service_discovery.go (1)
46-153: LGTM: Service discovery is well-designed.The convention-based service discovery correctly handles both flat and nested directory structures, enforces the one-proto-per-directory rule, validates package.service uniqueness, and prevents duplicate discovery with
filepath.SkipDir. Error handling and logging are comprehensive.router-tests/connectrpc/connectrpc_test_helpers.go (1)
18-245: LGTM: Test helpers are well-designed.The test infrastructure provides comprehensive helpers with proper resource management. The use of
t.Cleanup()for automatic teardown, idempotentClose()via thecleanupDoneflag, and dynamic port allocation vialocalhost:0are all best practices for test reliability.router/pkg/connectrpc/server.go (3)
49-166: LGTM: Server initialization is well-structured.The constructor properly validates configuration, applies sensible defaults, and initializes components in the correct order. The ProtoLoader is created before
initializeComponents(), addressing previous feedback about initialization ordering.
234-299: LGTM: Reload implementation addresses previous feedback.The
Reload()method now correctly:
- Creates a fresh
ProtoLoaderbefore callinginitializeComponents()(lines 249-257), ensuring the handler receives fresh proto definitions- Wraps the handler with
h2c.NewHandler()(line 295), preserving HTTP/2 support after reloadBoth issues flagged in previous reviews have been resolved.
358-391: LGTM: Response writer properly implements required interfaces.The
responseWriterwrapper correctly implementshttp.Flusher,http.Pusher, andhttp.Hijackerinterfaces required for gRPC streaming, while safely capturing the response status code with appropriate fallbacks when the underlying writer doesn't support these interfaces.router/pkg/connectrpc/handler.go (4)
17-54: LGTM: HTTP status to Connect code mapping is appropriate.The status code mapping follows Connect RPC conventions and covers all common HTTP status codes with a safe default to
CodeUnknown.
290-371: LGTM: Variable conversion properly implements recursive transformation.The conversion logic now correctly handles nested structures using
convertKeysRecursive(), andsnakeToCamel()usesstrings.Builderwith pre-allocation for efficiency. Previous feedback about recursive conversion and performance has been addressed.
373-434: LGTM: Error classification follows GraphQL best practices.The CRITICAL vs PARTIAL error classification correctly distinguishes between complete failures and partial successes, following Relay-inspired patterns. Using
CodeUnknownfor GraphQL errors (rather thanCodeInternal) is appropriate, and the metadata embedding enables clients to access both error details and partial data.
436-542: LGTM: GraphQL execution is comprehensive and well-designed.The execution flow properly handles header propagation (filtering sensitive headers), HTTP error mapping, GraphQL error classification, and edge cases (empty responses). The logic correctly distinguishes between CRITICAL (no data) and PARTIAL (data + errors) scenarios.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
c1b1b30 to
92ba712
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
router-tests/testdata/connectrpc/README.md (1)
16-22: Add language identifier to fenced code block.The fenced code block is missing a language specifier. Add
textorplaintextto comply with Markdown linting rules.-``` +```text services/ └── myservice.v1/ ├── service.proto # Proto service definition ├── QueryGetItem.graphql # GraphQL query operation └── MutationCreateItem.graphql # GraphQL mutation operation</blockquote></details> <details> <summary>router/pkg/connectrpc/constructor_validation_test.go (1)</summary><blockquote> `89-89`: **Tests use relative paths that may not resolve correctly from the test working directory.** The paths `"samples/services"` and `"samples/services/employee.v1"` are relative to the test file location. When CI runs tests from the `router/` directory (per `router-ci.yaml`), these paths should be `"pkg/connectrpc/samples/services"` instead. However, if tests are run from `router/pkg/connectrpc/`, they would work. Verify which working directory is used during CI test execution. ```shell #!/bin/bash # Check how tests are run in CI and verify the samples directory exists echo "=== Checking test working directory in CI ===" grep -A10 "go test\|gotestsum" .github/workflows/router-ci.yaml | head -20 echo -e "\n=== Verifying samples directory structure ===" ls -la router/pkg/connectrpc/samples/services/ 2>/dev/null || echo "Path not found from repo root" echo -e "\n=== Check if there's a go.mod in the connectrpc package ===" cat router/pkg/connectrpc/go.mod 2>/dev/null || echo "No separate go.mod"Also applies to: 100-100, 165-165, 175-175
router/pkg/config/config.schema.json (1)
2137-2204: Remove dead code schema fields for per-service provider overrides.The
servicesarray (lines 2179–2197) and its nestedproto_provider_idandoperations_provider_idfields are defined in the schema but have no corresponding Go struct fields inConnectRPCConfiguration. The implementation uses convention-based auto-discovery viaDiscoverServices(), which scans the filesystem—it never reads or parses per-service provider configuration. Users who configure these fields will have them silently discarded during unmarshalling, creating a confusing schema that misrepresents what the router actually supports.Either remove these unused fields from the schema, or add a comment documenting that they are reserved for future per-service provider support and clarify in the
servicesarray description that services are currently auto-discovered by convention.
🧹 Nitpick comments (13)
protographic/src/operation-to-proto.ts (1)
294-304: Fix duplicate step number in comments.Lines 294 and 304 both have step "4." in their comments. The second occurrence should be "5." for consistency.
- // 4. Create method name from operation name + // 5. Create method name from operation name // Use operation name as-is to ensure exact matching (no transformation) let methodName = operationName; // Add operation type prefix if requested if (this.prefixOperationType) { const operationTypePrefix = upperFirst(node.operation.toLowerCase()); methodName = `${operationTypePrefix}${methodName}` as any; } - // 4. Create request message from variables + // 6. Create request message from variablesrouter-tests/connectrpc/connectrpc_server_lifecycle_test.go (1)
121-137: Consider adding a test with an already-expired context.The graceful shutdown test verifies completion within a reasonable timeout, but doesn't test behavior when the context deadline is already exceeded. Consider adding a case:
t.Run("stop with already cancelled context", func(t *testing.T) { ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{}) require.NoError(t, ts.Start()) ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately err := ts.Server.Stop(ctx) assert.Error(t, err) // or NoError if shutdown proceeds anyway })router/pkg/connectrpc/error_handling_test.go (1)
305-376: Consider adding edge case tests for successful responses.The success path tests cover basic scenarios. Consider adding cases for:
- Empty data object:
{"data": {}}- Null values in successful response:
{"data": {"user": null}}- Response with extensions:
{"data": {...}, "extensions": {...}}{ name: "Successful query with null field", graphqlResponse: `{"data": {"user": null}}`, expectedData: `{"user": null}`, },router/pkg/connectrpc/constructor_validation_test.go (2)
12-13: Comment could be more informative.The comment states these tests are "consolidated" from other files, but those source files aren't mentioned. If they were removed, consider updating to clarify this is now the canonical location.
137-144: Subtests don't run in parallel.The outer test uses
t.Parallel(), but individual table-driven subtests don't. For consistency and faster execution, consider addingt.Parallel()to subtests.for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() result, err := tt.constructor() assert.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), tt.wantErr) }) }router/pkg/connectrpc/handler_test.go (1)
125-129: Direct manipulation of internal map bypasses encapsulation.The test directly assigns to
operationRegistry.operations, accessing an unexported field. While this works for same-package tests, consider adding a test helper method likeAddTestOperation(service, name string, op *Operation)onOperationRegistryto make tests more maintainable if the internal structure changes.router/core/router.go (3)
982-992: Discovered services are not passed to the server.
DiscoverServicesis called and logged, but the result (discoveredServices) is only used to log the count. TheServerConfigdoesn't receive the discovered services—it only getsServicesDirand rediscovers internally.This creates redundant work. Consider either:
- Passing
discoveredServicestoNewServerto avoid rediscovery- Removing the
DiscoverServicescall here if the server handles discovery internally
994-1000: Duplicate endpoint resolution logic with MCP.The GraphQL endpoint resolution pattern is duplicated between MCP (lines 935-942) and ConnectRPC (lines 994-1000). Consider extracting a helper function.
+func (r *Router) resolveGraphQLEndpoint(customURL string) string { + if customURL != "" { + return customURL + } + return path.Join(r.listenAddr, r.graphqlPath) +} + // In MCP bootstrap: -var routerGraphQLEndpoint string -if r.mcp.RouterURL != "" { - routerGraphQLEndpoint = r.mcp.RouterURL -} else { - routerGraphQLEndpoint = path.Join(r.listenAddr, r.graphqlPath) -} +routerGraphQLEndpoint := r.resolveGraphQLEndpoint(r.mcp.RouterURL) // In ConnectRPC bootstrap: -var routerGraphQLEndpoint string -if r.connectRPC.GraphQLEndpoint != "" { - routerGraphQLEndpoint = r.connectRPC.GraphQLEndpoint -} else { - routerGraphQLEndpoint = path.Join(r.listenAddr, r.graphqlPath) -} +routerGraphQLEndpoint := r.resolveGraphQLEndpoint(r.connectRPC.GraphQLEndpoint)Also applies to: 935-942
1010-1013: Consider reducing log verbosity.Multiple Info-level logs for the same operation (creating, created, starting, started) may clutter production logs. Consider using Debug level for intermediate steps and keeping only the final success message at Info level.
- r.logger.Info("Creating ConnectRPC server", + r.logger.Debug("Creating ConnectRPC server", zap.String("services_dir", servicesDir), zap.String("graphql_endpoint", routerGraphQLEndpoint), zap.String("listen_addr", r.connectRPC.Server.ListenAddr))router/pkg/connectrpc/validator.go (1)
299-309: Enum validation could verify the value exists in the enum definition.Currently, any string or number is accepted for enum fields. For stricter validation, consider checking if the value matches a defined enum value or number using
field.GetEnumType().FindValueByName()orFindValueByNumber().router/pkg/connectrpc/proto_loader.go (1)
324-328: GetServices exposes internal map without defensive copy.The comment says the map "should be treated as read-only," but callers could still mutate it. Consider returning a defensive copy if mutation would cause issues, or document more explicitly that this is intentional for performance.
// GetServices returns all loaded service definitions. -// The returned map should be treated as read-only to prevent accidental mutation. +// WARNING: Returns the internal map directly for performance. Callers MUST NOT +// modify the returned map or its values. Use GetService() for safe single lookups. func (pl *ProtoLoader) GetServices() map[string]*ServiceDefinition { return pl.services }Alternatively, return a copy:
func (pl *ProtoLoader) GetServices() map[string]*ServiceDefinition { result := make(map[string]*ServiceDefinition, len(pl.services)) for k, v := range pl.services { result[k] = v } return result }router/pkg/connectrpc/server.go (2)
184-190: Consider making server timeouts configurable.The
ReadTimeout,WriteTimeout, andIdleTimeoutare hardcoded. While the defaults are reasonable, consider exposing these inServerConfigfor consistency withRequestTimeout, especially if users need to tune for long-running gRPC streams.
328-334:statusCodeis captured but never used.The
responseWritercaptures the status code at line 330, but it's never read after being set. Either utilize it for logging/metrics or simplify by removing the unused field.If intended for future use, consider adding a comment. Otherwise, simplify:
- wrappedTranscoder := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Create a response writer that captures the status code and implements required interfaces - rw := &responseWriter{ResponseWriter: w, statusCode: 200} - - // The transcoder handles protocol translation and routing - s.transcoder.ServeHTTP(rw, r) - }) + wrappedTranscoder := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Create a response writer that implements required interfaces for gRPC streaming + rw := &responseWriter{ResponseWriter: w} + s.transcoder.ServeHTTP(rw, r) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (17)
protographic/src/operation-to-proto.ts(2 hunks)router-tests/connectrpc/connectrpc_server_lifecycle_test.go(1 hunks)router-tests/testdata/connectrpc/README.md(1 hunks)router-tests/testdata/connectrpc/buf.gen.yaml(1 hunks)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go(1 hunks)router/core/router.go(5 hunks)router/core/router_config.go(4 hunks)router/pkg/config/config.go(2 hunks)router/pkg/config/config.schema.json(1 hunks)router/pkg/connectrpc/constructor_validation_test.go(1 hunks)router/pkg/connectrpc/error_handling_test.go(1 hunks)router/pkg/connectrpc/handler.go(1 hunks)router/pkg/connectrpc/handler_test.go(1 hunks)router/pkg/connectrpc/proto_loader.go(1 hunks)router/pkg/connectrpc/server.go(1 hunks)router/pkg/connectrpc/server_test.go(1 hunks)router/pkg/connectrpc/validator.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- router/core/router_config.go
- router-tests/testdata/connectrpc/buf.gen.yaml
- router/pkg/connectrpc/server_test.go
- router/pkg/config/config.go
- router/pkg/connectrpc/handler.go
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/error_handling_test.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/handler_test.go
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/handler_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter/pkg/connectrpc/constructor_validation_test.gorouter/pkg/connectrpc/error_handling_test.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/core/router.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/validator.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/validator.go
📚 Learning: 2025-07-30T09:29:46.660Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-30T09:29:04.257Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/core/operation_processor.go:0-0
Timestamp: 2025-07-30T09:29:04.257Z
Learning: GraphQL operation names don't allow characters with more than 1 code point, so string length operations and slicing work correctly for both byte and character counting in GraphQL operation name processing.
Applied to files:
protographic/src/operation-to-proto.ts
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
🧬 Code graph analysis (4)
router-tests/connectrpc/connectrpc_server_lifecycle_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)router/pkg/connectrpc/server.go (3)
NewServer(50-166)ServerConfig(21-33)Server(36-47)
router/core/router.go (3)
router/pkg/connectrpc/server.go (2)
Server(36-47)ServerConfig(21-33)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)router/pkg/config/config.go (1)
ConnectRPCConfiguration(1012-1017)
router/pkg/connectrpc/validator.go (1)
router/pkg/connectrpc/proto_loader.go (1)
ProtoLoader(53-59)
router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.go (33)
FindEmployeesByPetsRequest(321-327)FindEmployeesByPetsRequest(342-342)FindEmployeesByPetsRequest(357-359)FindEmployeesByPetsResponse(368-375)FindEmployeesByPetsResponse(390-390)FindEmployeesByPetsResponse(405-407)FindEmployeesByPetsInlineFragmentRequest(586-592)FindEmployeesByPetsInlineFragmentRequest(607-607)FindEmployeesByPetsInlineFragmentRequest(622-624)FindEmployeesByPetsNamedFragmentRequest(681-687)FindEmployeesByPetsNamedFragmentRequest(702-702)FindEmployeesByPetsNamedFragmentRequest(717-719)GetEmployeeByIdRequest(227-233)GetEmployeeByIdRequest(248-248)GetEmployeeByIdRequest(263-265)GetEmployeeByIdResponse(274-280)GetEmployeeByIdResponse(295-295)GetEmployeeByIdResponse(310-312)GetEmployeesRequest(501-505)GetEmployeesRequest(520-520)GetEmployeesRequest(535-537)GetEmployeesResponse(539-545)GetEmployeesResponse(560-560)GetEmployeesResponse(575-577)GetEmployeesWithMoodRequest(416-420)GetEmployeesWithMoodRequest(435-435)GetEmployeesWithMoodRequest(450-452)UpdateEmployeeMoodRequest(124-131)UpdateEmployeeMoodRequest(146-146)UpdateEmployeeMoodRequest(161-163)UpdateEmployeeMoodResponse(179-186)UpdateEmployeeMoodResponse(201-201)UpdateEmployeeMoodResponse(216-218)
🪛 markdownlint-cli2 (0.18.1)
router-tests/testdata/connectrpc/README.md
16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build-router
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
🔇 Additional comments (21)
protographic/src/operation-to-proto.ts (1)
267-279: PascalCase validation looks correct.The regex correctly enforces PascalCase naming (starts with uppercase, contains at least one lowercase, alphanumeric only). The suggested name using
upperFirst(camelCase(operationName))provides helpful guidance when validation fails.router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
1-307: Generated code follows standard Connect patterns.This is generated code by
protoc-gen-connect-gowith correct:
- Version compatibility assertion (line 21)
- Idempotency annotations (queries marked
IdempotencyNoSideEffects, mutation has none)- Client/handler interface symmetry
- Unimplemented handler stubs
router-tests/connectrpc/connectrpc_server_lifecycle_test.go (1)
85-93: Error scenario test correctly fixed.The test now correctly asserts the error at
NewServerconstruction time rather than atStart(), which aligns with the implementation whereDiscoverServicesis called during construction.router/pkg/connectrpc/error_handling_test.go (2)
28-62: Test coverage for HTTP-to-Connect code mapping is valuable.While this tests a simple switch case, it serves as documentation and regression protection for the mapping behavior. A previous reviewer suggested removal, but keeping this provides clarity on expected behavior.
64-303: Comprehensive error handling test suite.The data-driven approach effectively covers:
- HTTP transport errors (401, 500, 503)
- GraphQL CRITICAL errors (null data with errors)
- GraphQL NON-CRITICAL errors (partial data with errors)
- Metadata assertions (HTTP status, error classification, GraphQL errors, partial data)
The test structure is clean and maintainable.
router/pkg/connectrpc/handler_test.go (4)
16-23: LGTM!Clean implementation of a test double for simulating transport errors.
184-219: Well-structured header forwarding tests.Good use of nested subtests with descriptive names following the suggested pattern. The test properly verifies that
Content-Lengthis handled by the HTTP client andContent-Typeis set correctly.
468-474: Empty JSON input returns empty object without error.The test expects
convertProtoJSONToGraphQLVariablesto return{}for empty input (""). Verify this is the intended behavior—an empty string is technically invalid JSON. If strict validation is desired, this should return an error.
553-577: Thorough edge case coverage for snakeToCamel.Good coverage of edge cases including multiple underscores, leading/trailing underscores, and empty strings. The test table is well-organized.
router/core/router.go (2)
1536-1540: LGTM! Proper use of wg.Go for concurrent shutdown.The shutdown flow correctly uses
wg.Go(Go 1.25+) for concurrent cleanup of multiple servers and providers, which improves shutdown time while maintaining proper error aggregation.Also applies to: 1544-1548, 1551-1557, 1559-1564
2208-2212: LGTM!Clean implementation of the
WithConnectRPCoption following the existing pattern.router/pkg/connectrpc/validator.go (2)
1-23: LGTM!Clean type definitions and constructor for the message validator.
54-62: Debug logging now uses structured logging.Good improvement—the previous
fmt.Printfstatements have been replaced with proper structured logging via zap.router/pkg/connectrpc/proto_loader.go (4)
52-72: LGTM!Clean struct definition and constructor. The use of a local
protoregistry.Filesinstead of the global registry addresses the previously identified race condition concern.
138-174: Package uniqueness validation is now correctly implemented.The logic properly tracks existing services before loading each file (line 139-142) and only validates newly added services (line 157-158). This addresses the previously identified issue where duplicate packages weren't detected after the first directory.
244-260: Local registry usage eliminates race condition.The code now correctly uses the local
pl.filesregistry instead ofprotoregistry.GlobalFiles, and fails fast on registration errors instead of silently ignoring them. This addresses the previously identified TOCTOU vulnerability.
336-350: LGTM!
GetMethodcorrectly returns a pointer to the method in the slice, avoiding unnecessary copies while providing clear error messages for missing services/methods.router/pkg/connectrpc/server.go (4)
49-166: LGTM - well-structured initialization with good error handling.The constructor validates configuration, sets sensible defaults, and properly sequences component initialization. Error handling is thorough with wrapped errors providing context.
301-321: LGTM!Clean component initialization with proper dependency injection.
358-391: LGTM - proper implementation of optional HTTP interfaces.The
responseWritercorrectly implementshttp.Flusher,http.Pusher, andhttp.Hijackervia type assertions, which is essential for gRPC streaming compatibility.
393-407: LGTM!Proper nil checks and clean delegation for introspection methods.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
92ba712 to
7895b2a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router/pkg/connectrpc/server.go (2)
190-196: Consider making HTTP server timeouts configurable.The server timeouts are hardcoded (
ReadTimeout: 30s,WriteTimeout: 30s,IdleTimeout: 60s) whileconfig.RequestTimeoutis used only for the HTTP client. For consistency and flexibility, consider exposing these as configuration options or deriving them from existing config values.
350-357: Unused status code capture.The
responseWritercapturesstatusCode(line 353) but this value is never read or used. The comment mentions "capture response status" but the captured status serves no purpose currently.Either remove the unused capture or add the intended functionality (e.g., logging, metrics):
🔎 Option 1: Remove unused wrapper if only pass-through is needed
// Wrap transcoder to capture response status - wrappedTranscoder := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Create a response writer that captures the status code and implements required interfaces - rw := &responseWriter{ResponseWriter: w, statusCode: 200} - - // The transcoder handles protocol translation and routing - s.transcoder.ServeHTTP(rw, r) - }) + wrappedTranscoder := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Create a response writer that implements required interfaces for gRPC streaming + rw := &responseWriter{ResponseWriter: w} + s.transcoder.ServeHTTP(rw, r) + })
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/pkg/config/config.schema.json
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: image_scan
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
router/pkg/connectrpc/server.go (8)
61-63: Verify protocol validation for GraphQL endpoint.The commit message mentions "enforce protocol in graphql_endpoint validation", but this code only checks if the endpoint is empty. There's no validation that the URL includes a scheme (e.g.,
http://orhttps://).If protocol validation is expected here, consider adding:
if !strings.HasPrefix(config.GraphQLEndpoint, "http://") && !strings.HasPrefix(config.GraphQLEndpoint, "https://") { return nil, fmt.Errorf("graphql endpoint must include http:// or https:// scheme") }If this validation is performed elsewhere (e.g., at the config layer), please confirm.
89-171: LGTM!The service initialization flow is well-structured: discover services → load protos → build operations → create registry → initialize components → create vanguard service → create transcoder. Error handling provides good context, and the warning for services without operations is helpful for debugging.
220-237: LGTM!The graceful shutdown implementation is correct. Using the parent context with an additional 5-second limit ensures shutdown doesn't hang indefinitely while still respecting tighter caller-imposed deadlines.
239-306: LGTM!The reload implementation correctly creates entirely new instances of all components (proto loader, registry, handler, vanguard service, transcoder) for atomic hot-reload. The h2c wrapper is properly applied to maintain HTTP/2 compatibility after reload. Past review issues have been addressed.
308-326: LGTM!The method correctly uses the caller-populated
operationRegistryandprotoLoader. The comment clearly documents the precondition that these must be set before calling.
328-344: LGTM!Clean implementation that correctly builds the operations map only for services with operation files.
372-437: LGTM!All getter methods have proper nil guards and cleanly delegate to underlying components.
388-421: LGTM on interface implementations.The
responseWritercorrectly implementshttp.Flusher,http.Pusher, andhttp.Hijackerwith proper type assertions and fallbacks. These are required for gRPC streaming compatibility.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
3e6dc86 to
88d8335
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
router/pkg/connectrpc/server.go (1)
78-79: Document why retry client logging is disabled.The internal retry logger is explicitly disabled. Consider adding a brief comment explaining this choice (e.g., to avoid duplicate/verbose logs).
🔎 Suggested documentation
// Create HTTP client with retry retryClient := retryablehttp.NewClient() + // Disable internal retry logs to avoid verbose output (errors are logged by our handler) retryClient.Logger = nil httpClient := retryClient.StandardClient()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-07-30T09:29:46.660Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
🔇 Additional comments (3)
router/pkg/connectrpc/server.go (3)
239-306: LGTM: Reload implementation is correct and thread-safe.The atomic hot-reload implementation properly:
- Guards against being called before Start (line 243)
- Recreates all components with fresh state before swapping the handler
- Applies the h2c wrapper for HTTP/2 compatibility (line 302)
- Maintains thread safety by not mutating live components
All critical issues from previous reviews have been successfully addressed.
308-326: LGTM: Component initialization follows correct dependency order.The method correctly uses the caller-populated operation registry and proto loader without creating its own, resolving the initialization ordering issue from previous reviews.
388-421: LGTM: responseWriter correctly implements required interfaces.The implementation properly:
- Captures status codes for observability
- Implements Flusher for gRPC streaming support
- Safely delegates to underlying writer with capability checks
- Returns appropriate errors when features are unsupported
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
2 times, most recently
from
51962c7 to
0be24bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
router/pkg/config/config.schema.json (1)
2149-2151: Addgraphql_endpointto the required fields when enabled.The
then.requiredarray should includegraphql_endpointalongsideservices_provider_idto align with the Go code's runtime validation, which returns an error ifgraphql_endpointis empty when ConnectRPC is enabled.🔎 Proposed fix
"then": { - "required": ["services_provider_id"] + "required": ["services_provider_id", "graphql_endpoint"] },
🧹 Nitpick comments (3)
router/core/header_rule_engine_test.go (1)
20-27: Helper function correctly extracts skipped header names.The implementation is correct and straightforward. Note that map iteration order is non-deterministic in Go, but this doesn't affect test correctness since the tests verify each header independently.
Optional: Reduce redundant allocations
The helper is called multiple times within the same test function (e.g., lines 228 and 248). Consider caching the result or calling once per test to avoid redundant map iterations:
func TestPropagateHeaderRule(t *testing.T) { + skippedHeaders := getSkippedHeaderNames() t.Run("Should not propagate as disallowed headers / named", func(t *testing.T) { rules := []*config.RequestHeaderRule{ { Operation: "propagate", Named: "X-Test-1", }, } - for _, name := range getSkippedHeaderNames() { + for _, name := range skippedHeaders { rules = append(rules, &config.RequestHeaderRule{ Operation: "propagate", Named: name, }) }Apply similar changes to other test functions that call the helper multiple times.
router/pkg/connectrpc/server.go (2)
169-213: Solid Start implementation with correct h2c wrapper.The
Startmethod properly:
- Validates server readiness (line 177)
- Wraps the handler with
h2c.NewHandlerfor HTTP/2 over cleartext (line 183)- Creates and binds the listener before starting
- Starts serving asynchronously with proper error handling
The hardcoded server timeouts (lines 188-190) are reasonable defaults, though making them configurable via
ServerConfigcould improve flexibility for different deployment scenarios.
234-301: All critical reload issues have been addressed!The
Reloadmethod now correctly:
- Guards against being called before
Start(line 238) ✅- Creates fresh
ProtoLoaderbeforeinitializeComponents()(line 254) ✅- Avoids the redundant
Clear()that corrupted state for in-flight requests ✅- Atomically swaps the handler with the h2c wrapper (line 297) ✅
The design of creating entirely new component instances before the atomic handler swap (line 297) elegantly preserves the old handler chain for in-flight requests while serving new requests with updated components.
Optional improvement: While the atomic pointer write at line 297 is safe, concurrent
Reload()calls could still race during the component recreation phase (lines 244-293). Consider documenting thatReloadshould be serialized by the caller, or adding async.Mutexto guard the reload process.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
router/core/header_rule_engine.gorouter/core/header_rule_engine_test.gorouter/pkg/config/config.schema.jsonrouter/pkg/connectrpc/constructor_validation_test.gorouter/pkg/connectrpc/server.gorouter/pkg/connectrpc/server_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- router/core/header_rule_engine.go
- router/pkg/connectrpc/constructor_validation_test.go
- router/pkg/connectrpc/server_test.go
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-07-30T15:23:03.295Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/config/config.schema.jsonrouter/pkg/connectrpc/server.go
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: Based on direct user experience from endigma (PR author), pyroscope-go's Config.ServerAddress field accepts simple host:port format (e.g., "localhost:4040") in practice, even though the official documentation primarily shows full URL examples with schemes (e.g., "http://localhost:4040"). The library likely handles URL normalization internally.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
🧬 Code graph analysis (1)
router/core/header_rule_engine_test.go (1)
router/internal/headers/headers.go (1)
SkippedHeaders(6-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
router/pkg/config/config.schema.json (2)
1379-1380: LGTM!Adding
format: "uri"andpattern: "^https?://"tocontrolplane_urlprovides proper validation to ensure the URL is well-formed and uses an HTTP scheme.
2152-2186: LGTM!The
connect_rpcproperties are well-structured with clear descriptions, appropriate format validations, and proper use ofadditionalProperties: false. Thegraphql_endpointformat constraint using both"format": "uri"and"pattern": "^https?://"is consistent with thecontrolplane_urlvalidation pattern.router/core/header_rule_engine_test.go (6)
13-13: LGTM!The import correctly references the centralized headers package.
228-265: LGTM!The test correctly verifies that skipped headers are not propagated even when explicitly configured in rules. The centralized approach ensures tests stay in sync with production header filtering logic.
396-434: LGTM!The test correctly verifies that headers cannot be renamed to skipped header names, maintaining header filtering integrity.
457-477: LGTM!The test properly verifies that a wildcard propagate rule still filters out all skipped headers. The individual assertion for each header ensures comprehensive coverage.
600-652: LGTM!The test correctly verifies that subgraph-specific rules also respect skipped headers, ensuring consistent header filtering across all rule scopes.
663-716: LGTM!The test properly verifies that even wildcard matching rules in subgraph configurations cannot propagate or rename to skipped headers, maintaining security boundaries.
router/pkg/connectrpc/server.go (6)
1-51: LGTM! Clean structure and imports.The package structure, imports, and configuration types are well-organized. The
ServerConfigprovides all necessary configuration for the ConnectRPC lifecycle, and theServerstruct appropriately holds the required components (transcoder, proto loader, operation registry, handler, vanguard service).
53-167: Excellent initialization flow with proper ordering.The
NewServerfunction correctly implements the initialization sequence:
- Configuration validation with sensible defaults
- Service discovery
- ProtoLoader creation before component initialization (addresses past ordering issue)
- Operations map building
- Immutable operation registry creation
- Component initialization with pre-populated registry
- Vanguard wiring
The consolidated debug logging at lines 160-164 provides good observability without being verbose.
215-232: Clean graceful shutdown implementation.The
Stopmethod correctly guards against calling on a non-started server and implements graceful shutdown with a reasonable 5-second timeout. The context handling withdefer cancel()ensures proper resource cleanup.
303-321: Correct component initialization without registry corruption.The
initializeComponentsmethod now correctly uses the caller-populateds.operationRegistry(line 313) without overwriting it. The comment at lines 303-304 clearly documents the precondition, and this addresses the critical issue from past reviews where an empty registry was being created here.
323-339: LGTM! Clear operations map builder.The
buildOperationsMaphelper correctly iterates through discovered services and loads operations for each. Error handling is appropriate with contextual wrapping.
367-432: LGTM! Well-implemented getters and interface delegations.The getter methods (
GetServiceCount,GetServiceNames,GetOperationCount,Addr) correctly guard against nil components. TheresponseWriterproperly delegatesFlush,Push, andHijackto the underlyinghttp.ResponseWriterwhen supported, which is essential for gRPC streaming and HTTP/2 functionality.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
0459db2 to
8d9e2d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI Agents
In @router-tests/connectrpc/connectrpc_server_lifecycle_test.go:
- Around line 54-85: The test currently matches the platform-specific string
"address already in use" when inspecting errors from ts.Server.Start(); replace
that fragile string check with a platform-agnostic helper (e.g., isAddrInUse(err
error)) that returns true if the error unwraps to syscall.EADDRINUSE (use
errors.Is(err, syscall.EADDRINUSE)), or if the unwrapped error is a *net.OpError
/ *os.SyscallError whose Err equals syscall.EADDRINUSE; as a fallback you can
also check the Windows message "Only one usage of each socket address" via
strings.Contains. Update the loop that builds portConflictCount to call
isAddrInUse(err) instead of matching the hard-coded string.
In @router-tests/testdata/connectrpc/services/employee.v1/service.proto:
- Line 50: Update the grammatical error in the service comment that currently
reads "This mutation update the mood of an employee." — change it to "This
mutation updates the mood of an employee." by editing the comment immediately
above the mutation definition in the employee.v1 service proto (the line
containing that sentence).
In @router/core/router.go:
- Around line 1008-1014: The code builds routerGraphQLEndpoint using path.Join
which is for filesystem paths; instead parse r.connectRPC.GraphQLEndpoint when
present or construct the URL from r.listenAddr and r.graphqlPath using net/url
(e.g., url.Parse on r.listenAddr and then ResolveReference / JoinPath with
r.graphqlPath) so URL semantics (scheme, host, slashes) are correct; update the
same pattern used for the MCP server (the r.listenAddr + r.graphqlPath branch)
to use the same net/url-based construction and assign the resulting string to
routerGraphQLEndpoint.
In @router/pkg/config/config.schema.json:
- Around line 2138-2187: The schema's conditional for connect_rpc (property
"connect_rpc" with "if" checking enabled=true) currently only requires
"services_provider_id" in its "then.required" but the runtime also requires
"graphql_endpoint"; update the "then.required" array to include
"graphql_endpoint" alongside "services_provider_id" so that when
connect_rpc.enabled is true the schema validation enforces both properties
(refer to the connect_rpc object, its then.required, and the graphql_endpoint
property).
In @router/pkg/config/connectrpc_test.go:
- Around line 47-60: The test case sets base_url in the YAML but never asserts
it; add a wantBaseURL string field to the test-case struct (next to
wantListenAddr/wantGraphQL/wantProviderID), set wantBaseURL:
"http://example.com" in the "full config with overrides" case, and add an
assertion comparing the parsed configuration's Server.BaseURL
(cfg.Server.BaseURL) to tc.wantBaseURL in the existing assertion block so
BaseURL is verified.
In @router/pkg/connectrpc/operation_registry_test.go:
- Around line 381-399: The test uses a nonexistent wg.Go method; replace the
loop with the correct WaitGroup pattern: iterate (e.g. for i := 0; i < 10; i++)
{ wg.Add(1); go func() { defer wg.Done(); for j := 0; j < 100; j++ { _ =
registry.GetOperationForService(serviceName, "Test"); _ =
registry.HasOperationForService(serviceName, "Test"); _ =
registry.GetAllOperationsForService(serviceName); _ = registry.Count(); _ =
registry.CountForService(serviceName) } }() } and then call wg.Wait(); this
fixes the concurrency spawn and ensures goroutines call wg.Done().
In @router/pkg/connectrpc/vanguard_service_test.go:
- Around line 249-261: The test mutates the private operations map on the
created opRegistry (violating immutability); instead build a
map[string]map[string]*schemaloader.Operation with the service key (serviceName)
and the "GetEmployeeById" schemaloader.Operation entry first, then call
NewOperationRegistry(operations) to construct opRegistry; update references to
the operations field removal and ensure the Operation values (Name,
OperationType, OperationString) are set prior to passing the map into
NewOperationRegistry.
🧹 Nitpick comments (16)
router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql (1)
1-12: Consider renaming the file to match the plural operation semantics.The filename
QueryGetEmployeeByPets.graphqluses singular "Employee," but the operationFindEmployeesByPetsqueries multiple employees. For consistency and clearer navigation, consider renaming toQueryGetEmployeesByPets.graphqlorQueryFindEmployeesByPets.graphql.router/pkg/connectrpc/operation_loader.go (1)
107-134: Optionally tighten operation document expectations (one operation per file)
extractOperationInforeturns the first operation definition it encounters and ignores the rest. Given your convention of one GraphQL operation per file, you might instead:
- Detect multiple
OperationDefinitionnodes and treat that as an error, or- At least log a warning when more than one is present, to avoid silently dropping additional operations.
This would make mis‑authored operation files much more obvious without impacting the happy path.
router/pkg/connectrpc/handler_test.go (1)
29-179: Consider hierarchical test organization.The test structure could be improved by following the feedback from a previous review about organizing subtests hierarchically to form readable "sentences". For example:
t.Run("field name handling", func(t *testing.T) { t.Run("preserves camelCase from protobuf JSON", func(t *testing.T) { ... }) }) t.Run("enum value handling", func(t *testing.T) { t.Run("with loaded schema", func(t *testing.T) { t.Run("strips proto enum prefixes", func(t *testing.T) { ... }) t.Run("omits _UNSPECIFIED values", func(t *testing.T) { ... }) }) t.Run("without schema", func(t *testing.T) { t.Run("passes through enum values unchanged", func(t *testing.T) { ... }) }) })This makes the test intent clearer and groups related test cases.
Based on past reviews.
router-tests/connectrpc/connectrpc_test.go (2)
40-54: Consider enhancing reload verification.This test only verifies that
Reload()doesn't return an error. Consider adding assertions that state remains consistent after reload (e.g., service count, service names) similar toTestConnectRPC_ServerLifecycle_StateTransitionsin the lifecycle tests.
56-73: Test doesn't exercise ConnectRPC functionality.The test is named
TestConnectRPC_Integrationbut only verifies the GraphQL endpoint works. It doesn't enable or test any ConnectRPC functionality. Consider either:
- Renaming to reflect what it actually tests, or
- Enabling ConnectRPC in the config and making actual RPC calls
router-tests/connectrpc/connectrpc_client_test.go (1)
28-28: Redundant cleanup call.Per past review discussion,
NewTestConnectRPCServeralready registers cleanup viat.Cleanup(). This explicitdefer ts.Close()is redundant but harmless - it will be called twice (once explicitly and once via cleanup).🔎 Remove redundant defer
ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{ GraphQLHandler: EmployeeGraphQLHandler(), }) - defer ts.Close()router/pkg/connectrpc/helpers_test.go (1)
84-95: Consider setting Response.Request for completeness.The
mockRoundTripperdoesn't setResponse.Request, which some HTTP client code may expect to be non-nil. While the current tests don't appear to access it, setting it would make the mock more robust.🔎 Set Response.Request
func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { return &http.Response{ StatusCode: m.statusCode, Body: io.NopCloser(strings.NewReader(m.responseBody)), Header: make(http.Header), + Request: req, }, nil }router/core/router.go (1)
999-1006: Service discovery is performed twice.
DiscoverServicesis called here for logging purposes, butconnectrpc.NewServerinternally calls it again (per theserver.gorelevant snippet). Consider either:
- Passing the discovered services to
NewServerto avoid re-discovery, or- Removing this call and relying on the server's internal discovery
This is a minor efficiency concern, not a correctness issue.
router/pkg/connectrpc/vanguard_service.go (3)
151-178: Consider using a map for method lookup instead of linear search.The method validation iterates through all methods linearly on every request. For services with many methods, this could be optimized by building a method lookup map during service registration.
🔎 Suggested optimization
// createServiceHandler creates an HTTP handler for a specific proto service -func (vs *VanguardService) createServiceHandler(serviceName string, serviceDef *ServiceDefinition) http.Handler { +func (vs *VanguardService) createServiceHandler(serviceName string, serviceDef *ServiceDefinition, methodSet map[string]bool) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Extract and validate method name from path methodName := vs.extractMethodName(r.URL.Path, serviceName) if methodName == "" { connectErr := connect.NewError(connect.CodeNotFound, fmt.Errorf("invalid path format")) vs.writeConnectError(w, connectErr, serviceName, methodName) return } - // Validate method exists in service - methodExists := false - for _, method := range serviceDef.Methods { - if method.Name == methodName { - methodExists = true - break - } - } - - if !methodExists { + if !methodSet[methodName] { connectErr := connect.NewError(connect.CodeNotFound, fmt.Errorf("method not found: %s", methodName)) vs.writeConnectError(w, connectErr, serviceName, methodName) return }Build the method set in
registerServices:methodSet := make(map[string]bool) for _, method := range serviceDef.Methods { methodSet[method.Name] = true } serviceHandler := vs.createServiceHandler(serviceName, serviceDef, methodSet)
262-274: Log JSON unmarshal errors for GraphQL error metadata.When parsing GraphQL errors from metadata fails, the error is silently ignored. Consider logging this for debugging purposes.
🔎 Suggested fix
if graphqlErrorsJSON := connectErr.Meta().Values(MetaKeyGraphQLErrors); len(graphqlErrorsJSON) > 0 { // Parse the GraphQL errors JSON from metadata var graphqlErrors []GraphQLError - if err := json.Unmarshal([]byte(graphqlErrorsJSON[0]), &graphqlErrors); err == nil && len(graphqlErrors) > 0 { + if err := json.Unmarshal([]byte(graphqlErrorsJSON[0]), &graphqlErrors); err != nil { + vs.logger.Debug("failed to parse GraphQL errors from metadata", zap.Error(err)) + } else if len(graphqlErrors) > 0 { // Include GraphQL errors in the response for better debugging errorResponse["graphql_errors"] = graphqlErrors
318-325: GetServiceNames returns names in non-deterministic order.The function iterates over a map, which yields non-deterministic ordering. This may cause issues in tests or when comparing results. Consider sorting the names.
🔎 Suggested fix
+import "sort" // GetServiceNames returns the names of all registered services func (vs *VanguardService) GetServiceNames() []string { names := make([]string, 0, len(vs.services)) for serviceName := range vs.protoLoader.GetServices() { names = append(names, serviceName) } + sort.Strings(names) return names }router-tests/connectrpc/connectrpc_test_helpers.go (1)
230-245: Consider escaping the message parameter in ErrorGraphQLHandler.The
messageparameter is directly interpolated into JSON without escaping. If the message contains special characters like quotes, the JSON will be malformed.🔎 Suggested fix using json.Marshal for proper escaping
// ErrorGraphQLHandler returns a handler that responds with a GraphQL error func ErrorGraphQLHandler(message string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - w.Write([]byte(fmt.Sprintf(`{"errors": [{"message": "%s"}]}`, message))) + // Use json.Marshal to properly escape the message + response := map[string]any{ + "errors": []map[string]string{{"message": message}}, + } + json.NewEncoder(w).Encode(response) } }router/pkg/connectrpc/server.go (1)
72-76: Silencing retry client logs may hide transient failures.Setting
retryClient.Logger = nilsilences all retry attempt logs. Consider using a leveled logger that logs at Debug level instead, so operators can enable it when troubleshooting connectivity issues.router/pkg/connectrpc/service_discovery.go (1)
177-217: Proto parsing is fragile for edge cases.The simple string-based parsing for extracting package and service names doesn't handle:
- Comments containing
packageorservicekeywords- Multi-line declarations
- Strings containing these keywords
For a convention-based discovery system this is acceptable, but consider documenting this limitation.
router/pkg/connectrpc/handler.go (1)
446-448: Same json.Marshal error handling issue in makePartialGraphQLError.The partial error helper has the same silent error discarding pattern.
🔎 Suggested fix
func (h *RPCHandler) makePartialGraphQLError(errors []GraphQLError, data json.RawMessage, httpStatus int) error { // Serialize errors to JSON for metadata - errorsJSON, _ := json.Marshal(errors) + errorsJSON, err := json.Marshal(errors) + if err != nil { + h.logger.Warn("failed to marshal GraphQL errors for metadata", zap.Error(err)) + errorsJSON = []byte("[]") + }router/pkg/connectrpc/proto_loader.go (1)
368-372: Consider returning a defensive copy of the services map.The comment says the map "should be treated as read-only" but callers could still mutate it. For true immutability, consider returning a copy or using a different pattern.
🔎 Option 1: Return a copy
// GetServices returns all loaded service definitions. -// The returned map should be treated as read-only to prevent accidental mutation. func (pl *ProtoLoader) GetServices() map[string]*ServiceDefinition { - return pl.services + copy := make(map[string]*ServiceDefinition, len(pl.services)) + for k, v := range pl.services { + copy[k] = v + } + return copy }🔎 Option 2: Provide an iterator method instead
// ForEachService calls fn for each loaded service definition. func (pl *ProtoLoader) ForEachService(fn func(fullName string, def *ServiceDefinition)) { for name, def := range pl.services { fn(name, def) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
router-tests/go.sumis excluded by!**/*.sumrouter-tests/testdata/connectrpc/client/employee.v1/service.pb.gois excluded by!**/*.pb.gorouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (60)
router-tests/connectrpc/connectrpc_client_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter-tests/connectrpc/connectrpc_test.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter-tests/go.modrouter-tests/testdata/connectrpc/README.mdrouter-tests/testdata/connectrpc/buf.gen.yamlrouter-tests/testdata/connectrpc/buf.yamlrouter-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.gorouter-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphqlrouter-tests/testdata/connectrpc/services/employee.v1/service.protorouter-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.jsonrouter/connect.config.yamlrouter/core/header_rule_engine.gorouter/core/header_rule_engine_test.gorouter/core/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/go.modrouter/internal/headers/headers.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/connectrpc_test.gorouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/connectrpc/connect_util.gorouter/pkg/connectrpc/constructor_validation_test.gorouter/pkg/connectrpc/error_handling_test.gorouter/pkg/connectrpc/handler.gorouter/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/helpers_test.gorouter/pkg/connectrpc/operation_loader.gorouter/pkg/connectrpc/operation_registry.gorouter/pkg/connectrpc/operation_registry_test.gorouter/pkg/connectrpc/proto_field_options.gorouter/pkg/connectrpc/proto_loader.gorouter/pkg/connectrpc/proto_loader_test.gorouter/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphqlrouter/pkg/connectrpc/samples/services/employee.v1/service.protorouter/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.jsonrouter/pkg/connectrpc/server.gorouter/pkg/connectrpc/server_test.gorouter/pkg/connectrpc/service_discovery.gorouter/pkg/connectrpc/service_discovery_test.gorouter/pkg/connectrpc/testdata/test.protorouter/pkg/connectrpc/vanguard_service.gorouter/pkg/connectrpc/vanguard_service_test.gorouter/pkg/mcpserver/server.go
✅ Files skipped from review due to trivial changes (1)
- router/core/router_config.go
🚧 Files skipped from review as they are similar to previous changes (22)
- router-tests/testdata/connectrpc/buf.gen.yaml
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router/pkg/mcpserver/server.go
- router/pkg/connectrpc/constructor_validation_test.go
- router-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.json
- router/core/header_rule_engine_test.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql
- router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql
- router/pkg/connectrpc/testdata/test.proto
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql
- router/connect.config.yaml
- router/pkg/config/testdata/config_full.json
- router/pkg/connectrpc/server_test.go
- router/pkg/connectrpc/operation_registry.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql
- router-tests/go.mod
- router/internal/headers/headers.go
- router-tests/testdata/connectrpc/README.md
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql
- router/go.mod
🧰 Additional context used
🧠 Learnings (25)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/operation_registry_test.gorouter/pkg/connectrpc/error_handling_test.gorouter-tests/connectrpc/connectrpc_client_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter-tests/connectrpc/connectrpc_test.gorouter/pkg/config/connectrpc_test.gorouter/pkg/connectrpc/helpers_test.gorouter/pkg/connectrpc/service_discovery_test.gorouter/pkg/connectrpc/handler_test.gorouter-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go
📚 Learning: 2025-07-29T08:19:55.720Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2088
File: demo/pkg/subgraphs/projects/src/main_test.go:0-0
Timestamp: 2025-07-29T08:19:55.720Z
Learning: In Go testing, t.Fatal, t.FailNow, t.Skip* and similar methods cannot be called from goroutines other than the main test goroutine, as they will cause a runtime panic. Use channels, t.Error*, or other synchronization mechanisms to communicate errors from goroutines back to the main test thread.
Applied to files:
router/pkg/connectrpc/operation_registry_test.go
📚 Learning: 2025-08-20T22:13:25.222Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2157
File: router-tests/go.mod:16-16
Timestamp: 2025-08-20T22:13:25.222Z
Learning: github.com/mark3labs/mcp-go v0.38.0 has regressions and should not be used in the wundergraph/cosmo project. v0.36.0 is the stable version that should be used across router-tests and other modules.
Applied to files:
router/pkg/connectrpc/operation_registry_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter-tests/connectrpc/connectrpc_test.gorouter/pkg/config/config.gorouter/core/router.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/error_handling_test.gorouter/pkg/connectrpc/server.gorouter/pkg/connectrpc/vanguard_service_test.gorouter/pkg/connectrpc/vanguard_service.gorouter-tests/connectrpc/connectrpc_test.gorouter/pkg/config/config.gorouter/pkg/connectrpc/helpers_test.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter/pkg/connectrpc/proto_loader.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function at lines 571-578, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the computed query hash before any APQ operations occur. There's also a test case that verifies this behavior.
Applied to files:
router/pkg/connectrpc/error_handling_test.gorouter/pkg/connectrpc/server.gorouter/pkg/connectrpc/handler_test.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router/pkg/connectrpc/error_handling_test.gorouter/pkg/connectrpc/server.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-08-28T09:18:10.121Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:100-108
Timestamp: 2025-08-28T09:18:10.121Z
Learning: In router-tests/http_subscriptions_test.go heartbeat tests, the message ordering should remain strict with data messages followed by heartbeat messages, as the timing is deterministic and known by design in the Cosmo router implementation.
Applied to files:
router-tests/connectrpc/connectrpc_server_lifecycle_test.go
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: Based on direct user experience from endigma (PR author), pyroscope-go's Config.ServerAddress field accepts simple host:port format (e.g., "localhost:4040") in practice, even though the official documentation primarily shows full URL examples with schemes (e.g., "http://localhost:4040"). The library likely handles URL normalization internally.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: In the Cosmo router codebase, JSON schema validation prevents null values in TrafficShapingRules subgraph configurations, making nil checks unnecessary when dereferencing subgraph rule pointers in NewSubgraphTransportOptions.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.jsonrouter/pkg/connectrpc/handler.go
📚 Learning: 2025-06-30T20:39:02.387Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 1929
File: router/internal/circuit/manager.go:16-25
Timestamp: 2025-06-30T20:39:02.387Z
Learning: In the Cosmo router project, parameter validation for circuit breaker configuration is handled at the JSON schema level rather than through runtime validation methods on structs. The config.schema.json file contains comprehensive validation constraints for circuit breaker parameters.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-07-21T14:46:34.879Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/authentication/jwks_token_decoder.go:80-106
Timestamp: 2025-07-21T14:46:34.879Z
Learning: In the Cosmo router project, required field validation for JWKS configuration (Secret, Algorithm, KeyId) is handled at the JSON schema level in config.schema.json rather than through runtime validation in the Go code at router/pkg/authentication/jwks_token_decoder.go.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-07-21T15:06:36.664Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2067
File: router/pkg/config/config.schema.json:1637-1644
Timestamp: 2025-07-21T15:06:36.664Z
Learning: In the Cosmo router project, when extending JSON schema validation for security-sensitive fields like JWKS secrets, backwards compatibility is maintained by implementing warnings in the Go code rather than hard validation constraints in the schema. This allows existing configurations to continue working while alerting users to potential security issues.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-08-20T10:08:17.857Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2155
File: router/core/router.go:1857-1866
Timestamp: 2025-08-20T10:08:17.857Z
Learning: router/pkg/config/config.schema.json forbids null values for traffic_shaping.subgraphs: additionalProperties references $defs.traffic_shaping_subgraph_request_rule with type "object". Therefore, in core.NewSubgraphTransportOptions, dereferencing each subgraph rule pointer is safe under schema-validated configs, and a nil-check is unnecessary.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.jsonrouter/pkg/connectrpc/handler.go
📚 Learning: 2025-07-30T15:23:03.295Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2079
File: router/pkg/config/config.schema.json:2942-2954
Timestamp: 2025-07-30T15:23:03.295Z
Learning: OCI (Open Container Initiative) registry URLs in the Cosmo router project should not include HTTP/HTTPS schemas. They are specified as hostnames only (e.g., "registry.example.com" or "registry.example.com/namespace"). The JSON schema validation should use plain "string" type without "http-url" format for plugin registry URLs.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/config/config.schema.json
📚 Learning: 2025-09-02T12:52:27.677Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2181
File: router/core/operation_processor.go:0-0
Timestamp: 2025-09-02T12:52:27.677Z
Learning: Hash validation for persisted queries with query bodies is performed in router/core/graphql_prehandler.go in the handleOperation function, not in the APQ processing logic in operation_processor.go. This validates that extensions.persistedQuery.sha256Hash matches the query body before any APQ operations occur.
Applied to files:
router/pkg/connectrpc/server.gorouter/pkg/connectrpc/handler.go
📚 Learning: 2025-09-19T15:08:03.085Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2223
File: router/cmd/main.go:32-33
Timestamp: 2025-09-19T15:08:03.085Z
Learning: pyroscope-go's Config.ServerAddress field accepts both full URLs (e.g., "http://localhost:4040") and simple host:port format (e.g., "localhost:4040"). The library handles URL normalization internally, so both formats work correctly.
Applied to files:
router/pkg/connectrpc/server.go
📚 Learning: 2025-09-17T20:55:39.456Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2172
File: router/core/graph_server.go:0-0
Timestamp: 2025-09-17T20:55:39.456Z
Learning: The Initialize method in router/internal/retrytransport/manager.go has been updated to properly handle feature-flag-only subgraphs by collecting subgraphs from both routerConfig.GetSubgraphs() and routerConfig.FeatureFlagConfigs.ConfigByFeatureFlagName, ensuring all subgraphs receive retry configuration.
Applied to files:
router/pkg/config/testdata/config_defaults.jsonrouter/core/router.go
📚 Learning: 2026-01-05T11:53:49.921Z
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2438
File: router-tests/go.mod:15-15
Timestamp: 2026-01-05T11:53:49.921Z
Learning: For mcp-go dependency in wundergraph/cosmo, patch releases may not have formal release notes published. Tag diffs should be reviewed to verify patch-level changes contain no API surface changes or breaking behavior before adopting new patch versions.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.25 minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-09-24T12:54:00.765Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2222
File: router-tests/websocket_test.go:2238-2302
Timestamp: 2025-09-24T12:54:00.765Z
Learning: The wundergraph/cosmo project uses Go 1.25 (Go 1.23+ minimum), so fmt.Appendf and other newer Go standard library functions are available and can be used without compatibility concerns.
Applied to files:
router/core/router.go
📚 Learning: 2025-11-13T10:10:47.680Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2329
File: router/pkg/pubsub/datasource/subscription_event_updater.go:86-88
Timestamp: 2025-11-13T10:10:47.680Z
Learning: In router/pkg/pubsub/datasource/subscription_event_updater.go, the SetHooks method is intentionally designed to only replace hook handlers, not reconfigure timeout or semaphore settings. The timeout and semaphore fields are meant to be set once during construction via NewSubscriptionEventUpdater and remain immutable. If different timeout or concurrency settings are needed, a new updater instance should be created rather than modifying the existing one.
Applied to files:
router/core/router.go
📚 Learning: 2025-11-16T11:52:34.064Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/pkg/pubsub/redis/engine_datasource.go:37-42
Timestamp: 2025-11-16T11:52:34.064Z
Learning: In router/pkg/pubsub/redis/engine_datasource.go (and similar files for kafka/nats), the MutableEvent.GetData() method intentionally returns the internal Data slice without cloning, while Event.GetData() clones the slice. This is by design: MutableEvent is designed to be mutable (so callers can modify the data), while Event is immutable (cloning prevents modification). This difference is part of the Cosmo Streams v1 hook interface design.
Applied to files:
router/pkg/connectrpc/proto_loader.go
📚 Learning: 2025-07-30T09:29:46.660Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2090
File: router/pkg/config/config.schema.json:0-0
Timestamp: 2025-07-30T09:29:46.660Z
Learning: The "operation_name_trim_limit" configuration property in router/pkg/config/config.schema.json should be placed at the security level as a sibling to complexity_limits, not inside the complexity_limits object.
Applied to files:
router/pkg/config/config.schema.json
📚 Learning: 2025-08-07T12:05:06.775Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2079
File: proto/wg/cosmo/platform/v1/platform.proto:39-45
Timestamp: 2025-08-07T12:05:06.775Z
Learning: In the Cosmo project, the proto fields for schema, mappings, and lock in ProtoInput are intentionally kept as string types rather than bytes because the team works with text data and wants it to be UTF-8 encoded for readability and text processing purposes.
Applied to files:
router/pkg/connectrpc/handler.go
🧬 Code graph analysis (15)
router/pkg/connectrpc/operation_registry_test.go (2)
router/pkg/connectrpc/operation_registry.go (1)
NewOperationRegistry(22-30)router/pkg/connectrpc/operation_loader.go (1)
LoadOperationsForService(20-105)
router/pkg/connectrpc/error_handling_test.go (2)
router/pkg/connectrpc/handler.go (6)
MetaKeyHTTPStatus(47-47)MetaKeyErrorClassification(48-48)ErrorClassificationCritical(56-56)ErrorClassificationPartial(57-57)NewRPCHandler(105-138)HandlerConfig(96-102)router/pkg/connectrpc/helpers_test.go (1)
MockHTTPClient(65-72)
router-tests/connectrpc/connectrpc_client_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (5)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)EmployeeGraphQLHandler(222-228)ErrorGraphQLHandler(231-237)HTTPErrorHandler(240-245)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
NewEmployeeServiceClient(89-141)
router/core/supervisor_instance.go (1)
router/core/router.go (1)
WithConnectRPC(2222-2226)
router-tests/connectrpc/connectrpc_server_lifecycle_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)router/pkg/connectrpc/server.go (3)
NewServer(54-167)ServerConfig(23-37)Server(40-51)
router/pkg/connectrpc/service_discovery.go (2)
router/pkg/mcpserver/util.go (1)
Logger(6-9)router/pkg/pubsub/datasource/error.go (1)
Error(3-6)
router/pkg/connectrpc/vanguard_service.go (3)
router/pkg/connectrpc/proto_loader.go (3)
ProtoLoader(55-64)ServiceDefinition(19-32)MethodDefinition(35-52)router/pkg/connectrpc/connect_util.go (1)
ConnectCodeToHTTPStatus(50-80)router/pkg/mcpserver/server.go (1)
GraphQLError(157-159)
router-tests/connectrpc/connectrpc_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
NewTestConnectRPCServer(94-144)ConnectRPCServerOptions(77-83)router-tests/testenv/testenv.go (1)
Environment(1735-1771)
router/pkg/config/config.go (2)
router/pkg/connectrpc/server.go (1)
Server(40-51)router/core/graph_server.go (1)
Server(69-72)
router/pkg/connectrpc/proto_loader_test.go (2)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)demo/pkg/subgraphs/test1/subgraph/model/models_gen.go (1)
InputType(346-348)
router-tests/connectrpc/connectrpc_test_helpers.go (1)
router/pkg/connectrpc/server.go (1)
ServerConfig(23-37)
router/core/router.go (2)
router/pkg/connectrpc/server.go (3)
Server(40-51)ServerConfig(23-37)NewServer(54-167)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)
router/pkg/connectrpc/service_discovery_test.go (1)
router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)
router/pkg/connectrpc/handler_test.go (2)
router/pkg/connectrpc/handler.go (1)
RPCHandler(87-93)router/pkg/connectrpc/proto_loader.go (1)
NewProtoLoader(67-78)
router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
router-tests/testdata/connectrpc/client/employee.v1/service.pb.go (42)
FindEmployeesByPetsRequest(321-327)FindEmployeesByPetsRequest(342-342)FindEmployeesByPetsRequest(357-359)FindEmployeesByPetsResponse(368-375)FindEmployeesByPetsResponse(390-390)FindEmployeesByPetsResponse(405-407)FindEmployeesByPetsInlineFragmentRequest(586-592)FindEmployeesByPetsInlineFragmentRequest(607-607)FindEmployeesByPetsInlineFragmentRequest(622-624)FindEmployeesByPetsInlineFragmentResponse(633-640)FindEmployeesByPetsInlineFragmentResponse(655-655)FindEmployeesByPetsInlineFragmentResponse(670-672)FindEmployeesByPetsNamedFragmentRequest(681-687)FindEmployeesByPetsNamedFragmentRequest(702-702)FindEmployeesByPetsNamedFragmentRequest(717-719)FindEmployeesByPetsNamedFragmentResponse(728-735)FindEmployeesByPetsNamedFragmentResponse(750-750)FindEmployeesByPetsNamedFragmentResponse(765-767)GetEmployeeByIdRequest(227-233)GetEmployeeByIdRequest(248-248)GetEmployeeByIdRequest(263-265)GetEmployeeByIdResponse(274-280)GetEmployeeByIdResponse(295-295)GetEmployeeByIdResponse(310-312)GetEmployeesRequest(501-505)GetEmployeesRequest(520-520)GetEmployeesRequest(535-537)GetEmployeesResponse(539-545)GetEmployeesResponse(560-560)GetEmployeesResponse(575-577)GetEmployeesWithMoodRequest(416-420)GetEmployeesWithMoodRequest(435-435)GetEmployeesWithMoodRequest(450-452)GetEmployeesWithMoodResponse(454-460)GetEmployeesWithMoodResponse(475-475)GetEmployeesWithMoodResponse(490-492)UpdateEmployeeMoodRequest(124-131)UpdateEmployeeMoodRequest(146-146)UpdateEmployeeMoodRequest(161-163)UpdateEmployeeMoodResponse(179-186)UpdateEmployeeMoodResponse(201-201)UpdateEmployeeMoodResponse(216-218)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: build_test
| t.Run("concurrent start attempts: exactly one succeeds, others fail with port conflict", func(t *testing.T) { | ||
| ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{}) | ||
|
|
||
| var wg sync.WaitGroup | ||
| errors := make([]error, 3) | ||
|
|
||
| // Try to start server concurrently | ||
| for i := 0; i < 3; i++ { | ||
| wg.Add(1) | ||
| go func(idx int) { | ||
| defer wg.Done() | ||
| errors[idx] = ts.Server.Start() | ||
| }(i) | ||
| } | ||
|
|
||
| wg.Wait() | ||
|
|
||
| // Count successes and port conflict failures | ||
| successCount := 0 | ||
| portConflictCount := 0 | ||
| for _, err := range errors { | ||
| if err == nil { | ||
| successCount++ | ||
| } else if strings.Contains(err.Error(), "address already in use") { | ||
| portConflictCount++ | ||
| } | ||
| } | ||
|
|
||
| // Exactly one should succeed, the other two should fail with port conflicts | ||
| assert.Equal(t, 1, successCount, "exactly one start should succeed") | ||
| assert.Equal(t, 2, portConflictCount, "two starts should fail with port conflict") | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the repository structure
git ls-files | head -20Repository: wundergraph/cosmo
Length of output: 753
🏁 Script executed:
# Look for CI configuration files
fd -t f \( -name "*.yml" -o -name "*.yaml" \) .github | head -10Repository: wundergraph/cosmo
Length of output: 291
🏁 Script executed:
# Check if there's any Windows-specific CI configuration
fd -t f \( -name "*.yml" -o -name "*.yaml" \) | xargs grep -l -i "windows" 2>/dev/null | head -10Repository: wundergraph/cosmo
Length of output: 291
🏁 Script executed:
# Look for the test file mentioned in the review
fd -t f "connectrpc_server_lifecycle_test.go"Repository: wundergraph/cosmo
Length of output: 120
🏁 Script executed:
# Search for other error message checks in test files, particularly for port conflicts
rg "address already in use|socket address|port.*conflict" --type go -iRepository: wundergraph/cosmo
Length of output: 1057
🏁 Script executed:
# Check the server implementation to see how it handles errors
fd -t f "server.go" | grep -i connectrpc | head -5Repository: wundergraph/cosmo
Length of output: 92
🏁 Script executed:
# Check CI workflow files
ls -la .github/workflows/*.yml 2>/dev/null || ls -la .github/workflows/*.yaml 2>/dev/null | head -20Repository: wundergraph/cosmo
Length of output: 477
🏁 Script executed:
# Check what CI platforms are configured
find .github/workflows -type f \( -name "*.yml" -o -name "*.yaml" \) -exec basename {} \;Repository: wundergraph/cosmo
Length of output: 696
🏁 Script executed:
# Look at the test file to understand context better
cat -n router-tests/connectrpc/connectrpc_server_lifecycle_test.go | head -100Repository: wundergraph/cosmo
Length of output: 3531
🏁 Script executed:
# Check the server implementation for how it handles listen/bind errors
cat router/pkg/connectrpc/server.go | head -150Repository: wundergraph/cosmo
Length of output: 4453
🏁 Script executed:
# Search for how errors are handled in other parts of the codebase for "address already in use"
rg "address already in use" --type go -B 3 -A 3Repository: wundergraph/cosmo
Length of output: 653
🏁 Script executed:
# Search for platform-specific error handling patterns
rg "runtime.GOOS|runtime.GOARCH" --type go -B 2 -A 2 | head -50Repository: wundergraph/cosmo
Length of output: 2332
🏁 Script executed:
# Check the router-ci.yaml file to see if it runs on multiple platforms
cat .github/workflows/router-ci.yaml | head -100Repository: wundergraph/cosmo
Length of output: 2897
🏁 Script executed:
# Search for "windows" or "win" in CI files
grep -r "windows\|WIN" .github/workflows/*.yaml 2>/dev/null | head -20Repository: wundergraph/cosmo
Length of output: 523
🏁 Script executed:
# Look at the complete Start() method in server.go to see how it handles the listen error
grep -n "func.*Start" router/pkg/connectrpc/server.goRepository: wundergraph/cosmo
Length of output: 97
🏁 Script executed:
# Get the Start() method implementation
sed -n '200,400p' router/pkg/connectrpc/server.goRepository: wundergraph/cosmo
Length of output: 6327
🏁 Script executed:
# Get the full Start() method implementation
sed -n '170,230p' router/pkg/connectrpc/server.goRepository: wundergraph/cosmo
Length of output: 1814
🏁 Script executed:
# Search for where the listener is created
grep -n "net.Listen\|listener" router/pkg/connectrpc/server.go | head -20Repository: wundergraph/cosmo
Length of output: 512
🏁 Script executed:
# Check if there's any error handling that mentions "address already in use"
grep -r "address already in use" . --type go 2>/dev/nullRepository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Look for how other tests handle platform-specific error checking
rg "GOOS|runtime\." router-tests --type go -B 2 -A 2Repository: wundergraph/cosmo
Length of output: 10596
🏁 Script executed:
# Let me check for any Windows-specific error handling patterns in the net package usage
rg "net.Listen.*err|failed to create listener" router --type go -B 2 -A 5Repository: wundergraph/cosmo
Length of output: 518
🏁 Script executed:
# Check if there are any other tests that validate error messages from system calls
rg "strings.Contains.*err.Error" router-tests --type go -B 3 -A 3Repository: wundergraph/cosmo
Length of output: 653
Make error message check platform-compatible.
The test checks for "address already in use" which is specific to Linux/macOS. On Windows, net.Listen() returns "Only one usage of each socket address". While Windows CI doesn't currently run these tests, the check should handle both error messages for portability and to prevent failures when developers run tests locally on Windows.
Consider using a helper function or checking for the underlying error type (e.g., *net.OpError) instead of string matching.
🤖 Prompt for AI Agents
In @router-tests/connectrpc/connectrpc_server_lifecycle_test.go around lines 54
- 85, The test currently matches the platform-specific string "address already
in use" when inspecting errors from ts.Server.Start(); replace that fragile
string check with a platform-agnostic helper (e.g., isAddrInUse(err error)) that
returns true if the error unwraps to syscall.EADDRINUSE (use errors.Is(err,
syscall.EADDRINUSE)), or if the unwrapped error is a *net.OpError /
*os.SyscallError whose Err equals syscall.EADDRINUSE; as a fallback you can also
check the Windows message "Only one usage of each socket address" via
strings.Contains. Update the loop that builds portConflictCount to call
isAddrInUse(err) instead of matching the hard-coded string.
router-tests/testdata/connectrpc/services/employee.v1/service.proto
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
32-41: Remove dead code: body is read but never used.The request body is read at line 34 for debugging purposes per the comment, but it's never actually logged or used. This adds unnecessary overhead.
🔎 Proposed fix
mux.HandleFunc("/graphql", func(w http.ResponseWriter, r *http.Request) { - // Log the incoming request for debugging - body, _ := io.ReadAll(r.Body) - _ = r.Body.Close() - r.Body = io.NopCloser(bytes.NewBuffer(body)) - if m.handler != nil { m.handler(w, r) } })
229-236: Potential invalid JSON if message contains special characters.Using
fmt.Fprintfwith%sfor the error message could produce malformed JSON if the message contains quotes, backslashes, or other characters requiring JSON escaping. While test messages are typically controlled, this could cause confusing test failures.🔎 Proposed fix using json.Marshal for safety
+import "encoding/json" + // ErrorGraphQLHandler returns a handler that responds with a GraphQL error func ErrorGraphQLHandler(message string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - _, _ = fmt.Fprintf(w, `{"errors": [{"message": "%s"}]}`, message) + msgBytes, _ := json.Marshal(message) + _, _ = fmt.Fprintf(w, `{"errors": [{"message": %s}]}`, msgBytes) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
router-tests/connectrpc/connectrpc_client_test.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter-tests/connectrpc/testdata/connectrpc/concurrent_response.jsonrouter-tests/connectrpc/testdata/connectrpc/employee_response.jsonrouter-tests/connectrpc/testdata/connectrpc/error_http_404.jsonrouter-tests/connectrpc/testdata/connectrpc/error_http_500.jsonrouter-tests/connectrpc/testdata/connectrpc/error_no_data.jsonrouter-tests/connectrpc/testdata/connectrpc/error_partial_data.jsonrouter-tests/testdata/connectrpc/employee_response.json
✅ Files skipped from review due to trivial changes (5)
- router-tests/connectrpc/testdata/connectrpc/concurrent_response.json
- router-tests/connectrpc/testdata/connectrpc/error_http_404.json
- router-tests/testdata/connectrpc/employee_response.json
- router-tests/connectrpc/testdata/connectrpc/error_no_data.json
- router-tests/connectrpc/testdata/connectrpc/error_http_500.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router-tests/connectrpc/connectrpc_client_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router-tests/connectrpc/connectrpc_test_helpers.go
📚 Learning: 2025-08-28T09:17:49.477Z
Learnt from: endigma
Repo: wundergraph/cosmo PR: 2141
File: router-tests/http_subscriptions_test.go:17-55
Timestamp: 2025-08-28T09:17:49.477Z
Learning: The Cosmo router uses a custom, intentionally rigid multipart implementation for GraphQL subscriptions. The multipart parsing in test files should remain strict and not be made more tolerant, as this rigidity is by design.
Applied to files:
router-tests/connectrpc/testdata/connectrpc/error_partial_data.json
🧬 Code graph analysis (2)
router-tests/connectrpc/connectrpc_client_test.go (2)
router-tests/connectrpc/connectrpc_test_helpers.go (4)
NewTestConnectRPCServer(93-143)ConnectRPCServerOptions(76-82)EmployeeGraphQLHandler(221-227)HTTPErrorHandler(239-244)router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
NewEmployeeServiceClient(89-141)
router-tests/connectrpc/connectrpc_test_helpers.go (1)
router/pkg/connectrpc/server.go (1)
ServerConfig(23-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
router-tests/connectrpc/testdata/connectrpc/employee_response.json (1)
1-18: LGTM!Well-structured test fixture that matches the expected employee response format used by the GraphQL handlers in the test helpers.
router-tests/connectrpc/connectrpc_client_test.go (4)
1-19: LGTM!Imports are well-organized and appropriate for the multi-protocol ConnectRPC testing needs.
21-116: LGTM!Comprehensive protocol coverage testing Connect, gRPC (with proper h2c client setup), and gRPC-Web. The explicit
defer ts.Close()at line 30 ensures proper cleanup.
118-284: LGTM!Thorough error handling test coverage with proper isolation per subtest. The error code mappings (GraphQL errors →
CodeUnknown, HTTP 404 →CodeNotFound, HTTP 500 →CodeInternal) align with Connect protocol conventions.
286-369: LGTM!Solid concurrency test that validates parallel request handling. The atomic counter and buffered channels ensure safe concurrent access.
router-tests/connectrpc/connectrpc_test_helpers.go (3)
1-16: LGTM!Clean imports appropriate for the test helper utilities.
75-143: LGTM!Well-structured test helper with sensible defaults, automatic cleanup via
t.Cleanup, and double-close protection via thecleanupDoneflag.
145-191: LGTM!Clean delegation pattern with proper timeout handling on
Stopand idempotentCloseimplementation.router-tests/connectrpc/testdata/connectrpc/error_partial_data.json (1)
1-4: No changes needed. The test data file is correct.The use of
"unknown"as the Connect error code for GraphQL partial success is intentional and follows the handler's error classification strategy. As documented inrouter/pkg/connectrpc/handler.go(lines 443–476), themakePartialGraphQLErrorfunction explicitly usesconnect.CodeUnknownwith an inline comment stating "Use Unknown for partial failures." The distinction between critical and partial GraphQL errors is conveyed through metadata (error-classification: PARTIAL), not through different Connect error codes. This design follows Relay's field-level error pattern, where both critical and partial errors use the same error code but are differentiated by metadata. The test data file correctly reflects this behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
router/pkg/connectrpc/vanguard_service_test.go (2)
276-280: Consider extracting repeated service lookup into a helper.This pattern for getting the first service from the map is repeated 5 times in this test (lines 276-280, 322-326, 343-347, 363-367, 384-388). Consider extracting to a test helper:
func getFirstService(t *testing.T, loader *ProtoLoader) *ServiceDefinition { t.Helper() services := loader.GetServices() require.NotEmpty(t, services, "Should have at least one service") for _, svc := range services { return svc } t.Fatal("unreachable") return nil }
313-316: Response content verification could be strengthened.The test verifies the response is valid JSON but doesn't validate the actual content matches expectations. Consider adding assertions for the expected employee data:
assert.Equal(t, float64(1), response["id"]) assert.Equal(t, "Test Employee", response["name"])This would ensure the GraphQL response is properly transformed in the handler output.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router/pkg/connectrpc/vanguard_service_test.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
📚 Learning: 2025-10-01T20:39:16.113Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2252
File: router-tests/telemetry/telemetry_test.go:9684-9693
Timestamp: 2025-10-01T20:39:16.113Z
Learning: Repo preference: In router-tests/telemetry/telemetry_test.go, keep strict > 0 assertions for request.operation.*Time (parsingTime, normalizationTime, validationTime, planningTime) in telemetry-related tests; do not relax to >= 0 unless CI flakiness is observed.
Applied to files:
router/pkg/connectrpc/vanguard_service_test.go
📚 Learning: 2025-11-19T15:13:57.821Z
Learnt from: dkorittki
Repo: wundergraph/cosmo PR: 2273
File: router/core/graphql_handler.go:0-0
Timestamp: 2025-11-19T15:13:57.821Z
Learning: In the Cosmo router (wundergraph/cosmo), error handling follows a two-phase pattern: (1) Prehandler phase handles request parsing, validation, and setup errors using `httpGraphqlError` and `writeOperationError` (in files like graphql_prehandler.go, operation_processor.go, parse_multipart.go, batch.go); (2) Execution phase handles resolver execution errors using `WriteError` in GraphQLHandler.ServeHTTP. Because all `httpGraphqlError` instances are caught in the prehandler before ServeHTTP is invoked, any error type checks for `httpGraphqlError` in the execution-phase WriteError method are unreachable code.
Applied to files:
router/pkg/connectrpc/vanguard_service_test.go
📚 Learning: 2026-01-06T12:37:21.521Z
Learnt from: asoorm
Repo: wundergraph/cosmo PR: 2379
File: router/pkg/connectrpc/operation_registry_test.go:381-399
Timestamp: 2026-01-06T12:37:21.521Z
Learning: In Go code (Go 1.25+), prefer using sync.WaitGroup.Go(func()) to run a function in a new goroutine, letting the WaitGroup manage Add/Done automatically. Avoid manual wg.Add(1) followed by go func() { defer wg.Done(); ... }() patterns. Apply this guidance across all Go files in the wundergraph/cosmo repository where concurrency is used.
Applied to files:
router/pkg/connectrpc/vanguard_service_test.go
🧬 Code graph analysis (1)
router/pkg/connectrpc/vanguard_service_test.go (4)
router/pkg/connectrpc/vanguard_service.go (2)
NewVanguardService(47-72)VanguardServiceConfig(19-23)router/pkg/connectrpc/proto_loader.go (3)
ProtoLoader(55-64)NewProtoLoader(67-78)ServiceDefinition(19-32)router/pkg/connectrpc/handler.go (3)
RPCHandler(87-93)NewRPCHandler(105-138)HandlerConfig(96-102)router/pkg/connectrpc/operation_registry.go (2)
NewOperationRegistry(22-30)OperationRegistry(14-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
router/pkg/connectrpc/vanguard_service_test.go (6)
18-90: LGTM!Good coverage of constructor validation including nil handler, nil proto loader, logger fallback, and empty services scenarios.
92-226: LGTM!Getter and validator tests provide solid coverage for both success and failure paths. Good use of
require.NoErrorfor setup steps andassertfor actual assertions.
239-317: LGTM on the test setup pattern.The operations map is now correctly built before passing to
NewOperationRegistry(), which follows the immutability contract properly.
403-458: LGTM!Table-driven test with comprehensive coverage of path parsing edge cases including leading slash handling and invalid path formats.
460-473: LGTM!Basic validation that file descriptors are loaded from the proto files.
475-480: LGTM!Clean test helper for simulating body read errors.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| @@ -0,0 +1,122 @@ | |||
| package connectrpc | |||
|
|
|||
| import ( | |||
There was a problem hiding this comment.
Is there a good reason to put this package into the router rather than the router-tests pkg?
There was a problem hiding this comment.
This file is used for unit tests which test internal package behaviour. Examples RPCHandler or VanguardService.
my thoughts were that router-tests is for integration tests - which actually tests the full router e2e.
So for me - I wasn't so keen on creating dependency or mixing concerns between unit tests and the integration test package.
Please advise if you think I should change approach.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@router/pkg/config/config.schema.json`:
- Around line 2138-2185: Update the JSON Schema for the connect_rpc object to
make storage.provider_id conditionally required only when connect_rpc.enabled is
true: add an if/then block on the connect_rpc object that checks {"properties":
{"enabled": {"const": true}}} and in then require ["storage"] with storage
having required ["provider_id"], so storage.provider_id is enforced only when
enabled is true (leave graphql_endpoint optional). This aligns schema validation
with the runtime checks that only validate provider_id when connect_rpc.enabled
is true.
♻️ Duplicate comments (2)
router/pkg/config/connectrpc_test.go (1)
25-116: Add coverage forserver.base_url.The YAML in the “full config with overrides” case sets
base_url, but the test never asserts it, so regressions would slip by.✅ Suggested test tweak
tests := []struct { name string yaml string envVars map[string]string wantEnabled bool wantListenAddr string wantGraphQL string wantProviderID string + wantBaseURL string }{ { name: "minimal config with defaults", @@ { name: "full config with overrides", yaml: `connect_rpc: enabled: true server: listen_addr: "0.0.0.0:8080" base_url: "http://example.com" storage: provider_id: "fs-protos" graphql_endpoint: "http://localhost:4000/graphql" `, wantEnabled: true, wantListenAddr: "0.0.0.0:8080", wantGraphQL: "http://localhost:4000/graphql", wantProviderID: "fs-protos", + wantBaseURL: "http://example.com", }, @@ assert.Equal(t, tt.wantEnabled, cfg.Enabled) assert.Equal(t, tt.wantListenAddr, cfg.Server.ListenAddr) + if tt.wantBaseURL != "" { + assert.Equal(t, tt.wantBaseURL, cfg.Server.BaseURL) + } assert.Equal(t, tt.wantGraphQL, cfg.GraphQLEndpoint) assert.Equal(t, tt.wantProviderID, cfg.Storage.ProviderID)router/core/router.go (1)
1008-1014: Use URL joining for ConnectRPC GraphQL endpoint (path.Join drops scheme/host).At Line 1013,
path.Join(r.listenAddr, r.graphqlPath)yields a string without a scheme (e.g.,localhost:8080/graphql), which most HTTP clients treat as an invalid URL. Prefer building fromr.baseURLwithurl.JoinPath(orurl.Parse+ResolveReference) and handle the error. This was already flagged previously.🔧 Suggested fix
// Determine the router GraphQL endpoint var routerGraphQLEndpoint string if r.connectRPC.GraphQLEndpoint != "" { routerGraphQLEndpoint = r.connectRPC.GraphQLEndpoint } else { - routerGraphQLEndpoint = path.Join(r.listenAddr, r.graphqlPath) + routerGraphQLEndpoint, err = url.JoinPath(r.baseURL, r.graphqlPath) + if err != nil { + return fmt.Errorf("failed to join graphql endpoint url: %w", err) + } }Go net/url.JoinPath vs path.Join for URL construction
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
6d4ee5f to
e18b97e
Compare
…ding Implements ConnectRPC support allowing gRPC, Connect, and gRPC-Web clients to interact with GraphQL APIs through protocol transcoding using Vanguard. Key features: - Proto loader with dynamic parsing using protocompile - Convention-based service discovery from proto files - Service-scoped, immutable operation registry - RPC handler with predefined GraphQL operations - GraphQL error handling with Relay-style classification - Support for GraphQL variable name field options in proto definitions - CORS support for cross-origin requests Configuration: - New `connect_rpc` config section with enabled, server, storage options - Storage provider references file_system providers for proto/operation files - Optional graphql_endpoint override for proxy scenarios Testing: - Comprehensive unit tests for all components - Integration tests with generated Connect clients - Golden file tests for error responses
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
2593058 to
2635b33
Compare
|
@coderabbitai please review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@router-tests/connectrpc/connectrpc_client_test.go`:
- Around line 25-30: The test currently calls NewTestConnectRPCServer which
already registers cleanup via t.Cleanup(func() { ts.Close() }), but the test
also has an explicit defer ts.Close() causing redundant cleanup; remove the
explicit defer ts.Close() line from the test so only the t.Cleanup-registered
closure runs (leave NewTestConnectRPCServer and its t.Cleanup usage intact and
only delete the defer ts.Close() reference).
🧹 Nitpick comments (9)
router/internal/headers/headers.go (1)
1-37: Consider a helper for canonicalized lookups (single access point).Right now lookups depend on callers providing canonicalized names. A small helper makes usage safer and more consistent (and can be adopted incrementally).
♻️ Proposed refactor
package headers +import "net/http" + // SkippedHeaders are headers that should not be forwarded to downstream services. // These headers are connection-specific or should be set by the client/server // rather than being forwarded from the original request. var SkippedHeaders = map[string]struct{}{ "Alt-Svc": {}, "Connection": {}, "Proxy-Connection": {}, // non-standard but still sent by libcurl and rejected by e.g. google @@ "Host": {}, "Content-Length": {}, } + +// IsSkippedHeader performs a canonicalized lookup against SkippedHeaders. +func IsSkippedHeader(name string) bool { + _, ok := SkippedHeaders[http.CanonicalHeaderKey(name)] + return ok +}Then callers can use
headers.IsSkippedHeader(...)to avoid case-sensitivity pitfalls.router/pkg/connectrpc/vanguard_service_test.go (1)
309-312: Avoid brittle Content-Type equality checks.Some handlers append
charsettoContent-Type; parsing the media type keeps the test resilient.♻️ Suggested tweak
import ( "bytes" "encoding/json" "io" + "mime" "net/http" "net/http/httptest" "testing" @@ - assert.Equal(t, "application/json", w.Header().Get("Content-Type")) + mediaType, _, err := mime.ParseMediaType(w.Header().Get("Content-Type")) + require.NoError(t, err) + assert.Equal(t, "application/json", mediaType)router-tests/connectrpc/connectrpc_server_lifecycle_test.go (1)
57-67: Prefersync.WaitGroup.Gofor goroutine launches.This repo prefers the newer helper to avoid manual
Add/Donebookkeeping.Based on learnings, prefer `sync.WaitGroup.Go` across the repo.♻️ Suggested refactor
- for i := 0; i < 3; i++ { - wg.Add(1) - go func(idx int) { - defer wg.Done() - errors[idx] = ts.Server.Start() - }(i) - } + for i := 0; i < 3; i++ { + idx := i + wg.Go(func() { + errors[idx] = ts.Server.Start() + }) + }router/core/router.go (1)
1003-1052: Avoid double service discovery during ConnectRPC bootstrap.
NewServeralready discovers services; this pre-scan duplicates filesystem work. Consider relying onNewServerand usingGetServiceCount()for logging.♻️ Suggested refactor
- // Discover services using convention-based approach - discoveredServices, err := connectrpc.DiscoverServices(connectrpc.ServiceDiscoveryConfig{ - ServicesDir: servicesDir, - Logger: r.logger, - }) - if err != nil { - return fmt.Errorf("failed to discover ConnectRPC services: %w", err) - } - ... r.logger.Info("ConnectRPC server ready", zap.String("listen_addr", r.connectRPC.Server.ListenAddr), - zap.Int("services", len(discoveredServices)), + zap.Int("services", crpcServer.GetServiceCount()), zap.Int("operations", crpcServer.GetOperationCount()))router/pkg/connectrpc/server.go (1)
202-210: Server error from goroutine is only logged, not propagated.If
Servefails immediately after starting (e.g., port already in use after listener creation), the error is logged butStart()has already returnednil. The caller has no way to know the server failed to start serving.Consider using a channel or error callback to propagate startup errors, or alternatively document that callers should monitor logs for startup failures.
router/pkg/connectrpc/operation_registry.go (2)
20-30: Document defensive copying expectation or perform shallow copy.The comment at line 21 states "callers should not modify it after passing", but this is a documentation-only contract. If a caller inadvertently modifies the map, it could cause data races or unexpected behavior.
Consider either:
- Performing a shallow copy of the map in the constructor (safer)
- Accepting this as a performance trade-off and ensuring all call sites are audited (current approach)
The current approach is acceptable for internal use within the same package where callers are controlled.
35-42: Returning pointer allows mutation of Operation internals.
GetOperationForServicereturns*schemaloader.Operationdirectly from the internal map. While the map itself is immutable, callers could mutate theOperationstruct's fields (e.g.,Name,Document), breaking the immutability guarantee for concurrent readers.If
Operationfields are ever modified after construction, this could cause data races. Consider returning a copy or documenting thatOperationinstances must not be modified.router/pkg/connectrpc/handler.go (1)
554-562: Consider wrapping internal error to preserve context.When JSON parsing fails, the code returns
ErrInternalServerwhich loses the original parsing error. While this is good for security (not exposing internals), it may complicate debugging.Consider logging the parse error with more context (currently only logs response body length).
router/pkg/connectrpc/service_discovery.go (1)
219-253: Proto parsing using simple line matching is adequate for this convention-based discovery pattern.The
extractPackageFromProtoandextractServiceNameFromProtofunctions handle normal proto files correctly. Comments are safely skipped because lines starting with//or/*don't match the "package " or "service " prefix after whitespace trimming. String literals in typical proto syntax don't appear at the start of trimmed lines, so they don't trigger false matches.Since this system enforces one proto file per directory and expects developer-managed proto files following standard conventions, the current implementation is sufficient. If robustness against extremely malformed or adversarial proto syntax becomes important, consider using a proper proto parsing library in the future—but this is not a blocker for the current design.
| // Use shared helper for employee GraphQL handler | ||
| ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{ | ||
| GraphQLHandler: EmployeeGraphQLHandler(), | ||
| }) | ||
| defer ts.Close() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test helpers file
find . -name "connectrpc_test_helpers.go" -o -name "*connectrpc*helpers*"
Repository: wundergraph/cosmo
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Inspect NewTestConnectRPCServer implementation and Close method
rg -n "func NewTestConnectRPCServer|func \(.*TestConnectRPCServer\) Close|t\.Cleanup" router-tests/connectrpc/ -A 10
Repository: wundergraph/cosmo
Length of output: 2586
🏁 Script executed:
#!/bin/bash
# Get the full structure of connectrpc_test_helpers.go to understand lifecycle
wc -l router-tests/connectrpc/connectrpc_test_helpers.go 2>/dev/null || echo "File not found, searching..."
fd connectrpc_test_helpers router-tests/
Repository: wundergraph/cosmo
Length of output: 166
Remove the explicit defer ts.Close() as cleanup is already registered.
NewTestConnectRPCServer already registers cleanup via t.Cleanup(func() { ts.Close() }), making the explicit defer ts.Close() redundant. While Close() is idempotent and handles multiple calls safely, the defer duplicates the cleanup unnecessarily.
Suggested change
- defer ts.Close()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Use shared helper for employee GraphQL handler | |
| ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{ | |
| GraphQLHandler: EmployeeGraphQLHandler(), | |
| }) | |
| defer ts.Close() | |
| // Use shared helper for employee GraphQL handler | |
| ts := NewTestConnectRPCServer(t, ConnectRPCServerOptions{ | |
| GraphQLHandler: EmployeeGraphQLHandler(), | |
| }) | |
🤖 Prompt for AI Agents
In `@router-tests/connectrpc/connectrpc_client_test.go` around lines 25 - 30, The
test currently calls NewTestConnectRPCServer which already registers cleanup via
t.Cleanup(func() { ts.Close() }), but the test also has an explicit defer
ts.Close() causing redundant cleanup; remove the explicit defer ts.Close() line
from the test so only the t.Cleanup-registered closure runs (leave
NewTestConnectRPCServer and its t.Cleanup usage intact and only delete the defer
ts.Close() reference).
…n connectrpc tests - Remove goldie dependency and testdata golden files - Use require.JSONEq with inline expected JSON for clearer, self-documenting tests - Replace deprecated DialTLS with DialTLSContext
Use canonical source from router/pkg/connectrpc/samples/services instead of maintaining a duplicate copy in router-tests/testdata.
- Remove TestConnectRPC_Integration (unrelated to ConnectRPC) - Remove concurrent start attempts subtest (flaky, tests OS behavior) - Remove TestConnectRPC_Server_GetServiceInfo (redundant with ServiceDiscovery)
Remove tests that add little value: - TestNewOperationRegistry (trivial constructor) - TestFindProtoFilesInDir, TestFindOperationFiles (file globbing impl details) - TestExtractPackageFromProto, TestExtractServiceNameFromProto (regex impl details) - TestVanguardService_GetServices, GetServiceNames, GetFileDescriptors (trivial getters) - TestVanguardService_ExtractMethodName (string parsing impl detail) - TestStripEnumPrefixWithType, TestToUpperSnakeCase (utility function tests) - TestConvertKeysRecursiveWithTracking (internal implementation test)
jensneuse
left a comment
jensneuse
left a comment
There was a problem hiding this comment.
please also ask @StarpTech for a review
Overview
This PR introduces a ConnectRPC handler that enables gRPC/Connect/gRPC-Web clients to interact with GraphQL APIs through protocol translation. The implementation provides a bridge between protobuf-based RPC protocols and GraphQL operations.
Key Features
Architecture
File Responsibilities
handler.go: Core RPC request handler, GraphQL execution, error classificationoperation_registry.go: Thread-safe registry for service-scoped GraphQL operationsproto_loader.go: Protobuf file parsing and service definition extractionserver.go: HTTP/2 server with lifecycle management (start/stop/reload)service_discovery.go: Convention-based service discovery from filesystemvanguard_service.go: Vanguard integration for protocol transcodingSupporting Files
handler_error_test.go: Error handling and classification testshandler_test.go: Core handler functionality testsoperation_registry_test.go: Registry thread-safety and service-scoping testsproto_loader_test.go: Proto parsing testsserver_test.go: Server configuration testsserver_lifecycle_test.go: Server lifecycle and reload testsservice_discovery_test.go: Service discovery convention testsvanguard_service_test.go: Protocol transcoding testsManual Testing
Prerequisites
Start the router with appropriate config
The services directory should contain
protoand named GraphQL operations. See./pkg/connectrpc/samples/servicesas an example:tree ./pkg/connectrpc/samples/services ./pkg/connectrpc/samples/services └── employee.v1 ├── MutationUpdateEmployeeMood.graphql ├── QueryGetEmployeeById.graphql ├── QueryGetEmployeeByPets.graphql ├── QueryGetEmployeeWithMood.graphql ├── QueryGetEmployees.graphql ├── QueryGetEmployeesByPetsInlineFragment.graphql ├── QueryGetEmployeesByPetsNamedFragment.graphql ├── service.proto └── service.proto.lock.jsonTesting with curl (Connect Protocol)
Client Validation Error
OK
Testing with grpcurl (gRPC Protocol)
Note: Requires proto files as reflection protocol is not supported.
Testing with buf CLI (Connect Protocol)
grpc-web
buf curl --protocol grpcweb \ --schema ./pkg/connectrpc/samples/services/employee.v1/service.proto \ --data '{"employee_id": 1}' \ http://localhost:5026/employee.v1.EmployeeService/GetEmployeeByIdService Discovery Convention
Services are discovered using this directory structure:
Related Issues
Closes #ENG-8277
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.