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
|
Warning Rate limit exceeded@asoorm has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a ConnectRPC subsystem (discovery, proto loading, operation registry, RPC handler, Vanguard integration, server lifecycle), config/schema, protographic changes for proto generation, centralized header-skipping, CLI flag removal, many tests, samples, and generated clients. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
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)
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.
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
Call Stop() on ConnectRPC and MCP servers if Start() fails during bootstrap to ensure proper cleanup of allocated resources.
Build operations map before creating registry, making it immutable and lock-free for concurrent reads. On reload, swap entire registry instead of mutating.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router/pkg/connectrpc/operation_loader.go (1)
39-98: Consider accumulating errors to detect complete loading failures.The function logs warnings and continues for individual file failures (lines 42-45, 53-56, 62-65), which is reasonable for resilience. However, if all files fail to load, the function returns success with an empty operations map. This could lead to silent runtime failures when operations are expected but missing.
🔎 Optional enhancement to track and report loading failures
operations := make(map[string]*schemaloader.Operation) + var failedFiles int // Track operation names to detect duplicates within this service seenOperations := make(map[string]string) // operation name -> file path // Load each operation file for _, filePath := range operationFiles { content, err := os.ReadFile(filePath) if err != nil { logger.Warn("failed to read operation file", zap.String("file", filePath), zap.Error(err)) + failedFiles++ continue } // ... rest of parsing logic ... } logger.Info("loaded operations for service", zap.String("service", serviceName), - zap.Int("operation_count", len(operations))) + zap.Int("operation_count", len(operations)), + zap.Int("failed_files", failedFiles)) + if len(operations) == 0 && len(operationFiles) > 0 { + return nil, fmt.Errorf("failed to load any operations for service %s (%d files attempted)", serviceName, len(operationFiles)) + } + return operations, nil
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
router/pkg/connectrpc/constructor_validation_test.gorouter/pkg/connectrpc/error_handling_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/server.gorouter/pkg/connectrpc/vanguard_service_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- router/pkg/connectrpc/constructor_validation_test.go
- router/pkg/connectrpc/operation_registry_test.go
- 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.
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/error_handling_test.gorouter/pkg/connectrpc/helpers_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/error_handling_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: 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/pkg/connectrpc/operation_loader.go (1)
router/core/context.go (3)
OperationTypeQuery(514-514)OperationTypeMutation(515-515)OperationTypeSubscription(516-516)
router/pkg/connectrpc/error_handling_test.go (3)
router/pkg/connectrpc/handler.go (7)
MetaKeyHTTPStatus(47-47)MetaKeyErrorClassification(48-48)ErrorClassificationCritical(56-56)ErrorClassificationPartial(57-57)MetaKeyGraphQLErrors(49-49)MetaKeyGraphQLPartialData(50-50)MetaKeyHTTPResponseBody(51-51)router/pkg/connectrpc/helpers_test.go (1)
MockHTTPClient(65-72)router/pkg/connectrpc/operation_registry.go (2)
OperationRegistry(14-18)NewOperationRegistry(22-30)
router/pkg/connectrpc/server.go (6)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)router/pkg/connectrpc/operation_registry.go (2)
OperationRegistry(14-18)NewOperationRegistry(22-30)router/pkg/connectrpc/handler.go (3)
RPCHandler(87-93)NewRPCHandler(105-138)HandlerConfig(96-102)router/pkg/connectrpc/vanguard_service.go (3)
VanguardService(26-31)NewVanguardService(34-59)VanguardServiceConfig(19-23)router/pkg/connectrpc/service_discovery.go (3)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)DiscoveredService(13-26)router/pkg/connectrpc/operation_loader.go (1)
LoadOperationsForService(20-105)
router/pkg/connectrpc/helpers_test.go (6)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)router/pkg/connectrpc/server.go (2)
Server(38-49)NewServer(52-174)router/internal/expr/expr.go (2)
Request(66-75)Body(99-101)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)router/pkg/mcpserver/util.go (1)
Logger(6-9)
⏰ 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 (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
router/pkg/connectrpc/error_handling_test.go (3)
29-267: LGTM! Comprehensive error handling test coverage.The table-driven test structure effectively covers HTTP transport errors, GraphQL critical errors (null data), and partial success scenarios. The metadata assertions validate proper error classification and ensure GraphQL error details are correctly propagated to Connect clients.
270-340: LGTM! Success path testing is thorough.The test validates both simple and nested GraphQL response structures with proper JSON comparison using
JSONEq.
343-376: LGTM! Important security validation.This test ensures response bodies are not leaked into client-facing metadata (line 371), preventing potential information disclosure while confirming other metadata remains intact.
router/pkg/connectrpc/operation_loader.go (1)
107-134: LGTM! Operation extraction logic is correct.The helper properly extracts operation name and type from the AST, handles all GraphQL operation types, and returns clear errors for invalid documents.
router/pkg/connectrpc/helpers_test.go (1)
1-122: LGTM! Well-designed test utilities.The test helpers provide clean abstractions for ConnectRPC testing:
- Thread-safe proto loader caching (lines 25-41) prevents registration conflicts across parallel tests
- Mock HTTP client and server enable isolated unit testing
- Sensible defaults in
NewTestRPCHandlerreduce test boilerplaterouter/pkg/connectrpc/operation_registry.go (1)
14-30: LGTM! Immutable design enables lock-free concurrent reads.The immutable registry design (lines 11-13 documentation, line 17 immutable map) properly addresses concurrency concerns. By building the operations map before construction and never modifying it afterward, the code achieves thread-safety without locking overhead—exactly as recommended in past reviews.
router/pkg/connectrpc/server.go (4)
52-174: LGTM! NewServer initialization is well-structured.The initialization flow properly sequences component creation:
- Config validation
- Service discovery and proto loading
- Operations map building
- Immutable registry creation
- Handler and Vanguard service initialization
This addresses past concerns about initialization order.
177-220: LGTM! Server startup properly configured for gRPC.The h2c wrapper (line 190) enables HTTP/2 cleartext for gRPC compatibility, and the server is started asynchronously with proper error handling.
243-308: LGTM! Reload properly implements atomic hot-reload.The reload flow correctly addresses past review concerns:
- Guards against calling before Start (lines 245-247)
- Creates fresh proto loader before initializing components (line 261)
- Wraps handler with h2c for gRPC compatibility (line 304)
- Atomic replacement of all components ensures in-flight requests continue using old instances until completion
333-435: LGTM! Supporting methods are well-implemented.The helper methods properly delegate to underlying components with appropriate nil-checks, and the
responseWritercorrectly implements required interfaces (Flusher, Pusher, Hijacker) for gRPC streaming support.
…tors - Remove assignment overwriting populated operation registry in initializeComponents - Deduplicate file descriptors in GetFileDescriptors when multiple services share proto files
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
749aea8 to
e0c1c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
router/pkg/connectrpc/vanguard_service.go (1)
197-206: Consider limiting request body size to prevent memory exhaustion.
io.ReadAll(r.Body)reads the entire request body into memory without any size limit. A malicious client could send an extremely large body, causing memory exhaustion (DoS).🔎 Proposed fix using io.LimitReader
+const maxRequestBodySize = 10 * 1024 * 1024 // 10MB limit + // Read request body (JSON for POST requests) - requestBody, err = io.ReadAll(r.Body) + requestBody, err = io.ReadAll(io.LimitReader(r.Body, maxRequestBodySize)) if err != nil { vs.logger.Error("failed to read request body", zap.Error(err)) connectErr := connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("failed to read request")) vs.writeConnectError(w, connectErr, serviceName, methodName) return } + if len(requestBody) >= maxRequestBodySize { + connectErr := connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("request body too large")) + vs.writeConnectError(w, connectErr, serviceName, methodName) + return + }Please verify if there's upstream middleware (e.g., in Vanguard or the HTTP server) that already limits request body size. If so, this concern may be mitigated.
router/pkg/connectrpc/server.go (3)
82-86: Consider configuring retry client parameters.The
retryablehttp.NewClient()uses default retry settings (4 retries with exponential backoff). For a production system, you may want to explicitly configure retry count, backoff strategy, and retry conditions to match your operational requirements.// Example: Explicit retry configuration retryClient := retryablehttp.NewClient() retryClient.Logger = nil retryClient.RetryMax = 3 retryClient.RetryWaitMin = 100 * time.Millisecond retryClient.RetryWaitMax = 1 * time.Second
212-220: Consider propagating server startup errors.The server runs in a goroutine, and startup errors (after listener creation) are only logged but not propagated. If
Servefails immediately (e.g., TLS misconfiguration), the caller ofStart()won't know.Consider using a channel or error callback to propagate startup errors:
// Option 1: Use a startup confirmation channel startupErr := make(chan error, 1) go func() { if err := s.httpServer.Serve(s.listener); err != nil && !errors.Is(err, http.ErrServerClosed) { startupErr <- err s.logger.Error("server error", zap.Error(err)) } close(startupErr) }() // Wait briefly to catch immediate startup failures select { case err := <-startupErr: return fmt.Errorf("server failed to start: %w", err) case <-time.After(100 * time.Millisecond): return nil }However, the current pattern is common and may be acceptable if the listener creation (which can fail) is the main validation point.
356-362: Status code capture appears unused.The
responseWritercapturesstatusCodebut it's never read after being set. If this is for future use (e.g., metrics, logging), consider adding a TODO comment. Otherwise, consider removing the unused capture.// Option 1: Remove unused capture if not needed s.transcoder.ServeHTTP(w, r) // Option 2: Add comment if planned for future use // TODO: Use rw.statusCode for metrics/observability
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
router/pkg/connectrpc/server.gorouter/pkg/connectrpc/vanguard_service.go
🧰 Additional context used
🧠 Learnings (3)
📓 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
📚 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.go
🧬 Code graph analysis (2)
router/pkg/connectrpc/server.go (6)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)router/pkg/connectrpc/operation_registry.go (2)
OperationRegistry(14-18)NewOperationRegistry(22-30)router/pkg/connectrpc/handler.go (3)
RPCHandler(87-93)NewRPCHandler(105-138)HandlerConfig(96-102)router/pkg/connectrpc/vanguard_service.go (3)
VanguardService(39-44)NewVanguardService(47-72)VanguardServiceConfig(19-23)router/pkg/connectrpc/service_discovery.go (3)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)DiscoveredService(13-26)router/pkg/connectrpc/operation_loader.go (1)
LoadOperationsForService(20-105)
router/pkg/connectrpc/vanguard_service.go (2)
router/pkg/connectrpc/proto_loader.go (2)
ServiceDefinition(19-32)MethodDefinition(35-52)router/pkg/connectrpc/connect_util.go (1)
ConnectCodeToHTTPStatus(50-80)
⏰ 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 (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- 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: build_test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
router/pkg/connectrpc/vanguard_service.go (7)
1-44: LGTM! Clean imports and well-documented type definitions.The package structure is clean with appropriate imports. The
VanguardServicestruct is well-documented with a comprehensive docstring explaining the protocol adapter pattern and transcoding flow.
46-72: LGTM! Constructor follows defensive coding with appropriate validation.The constructor properly validates required dependencies and provides a sensible default for the logger.
74-149: LGTM! Service registration with custom type resolver is well implemented.The implementation correctly:
- Validates that services exist before registration
- Uses a custom type resolver (
dynamicpb.NewTypes) to avoid global registry pollution- Logs aggregate statistics at appropriate levels (Info for summary, Debug for details)
- Hardcodes Connect+JSON as target protocol/codec (per prior discussion, this simplifies the handler)
241-285: LGTM! Error response formatting follows Connect protocol conventions.The error handling:
- Correctly maps Connect error codes to HTTP status codes
- Includes GraphQL errors from metadata when present
- Uses appropriate JSON structure for Connect protocol errors
- Logs errors with sufficient context for debugging
287-306: LGTM! Path parsing is simple and correct.The method correctly extracts the method name from the expected path format and validates that the service name matches.
308-371: LGTM! Accessor methods properly delegate to the proto loader.The accessor methods provide a clean API for service introspection and correctly delegate to the underlying proto loader.
373-393: LGTM! File descriptor deduplication correctly implemented.The
GetFileDescriptorsmethod properly deduplicates file descriptors by path, addressing the concern about multiple services from the same proto file.router/pkg/connectrpc/server.go (7)
1-52: LGTM! Clean structure with appropriate dependencies.The imports and type definitions are well-organized. The
Serverstruct appropriately encapsulates all components needed for lifecycle management.
225-242: LGTM! Graceful shutdown with appropriate timeout.The stop method correctly guards against being called on an unstarted server and uses a context-based graceful shutdown.
244-311: LGTM! Atomic hot-reload correctly implemented.The reload method:
- Guards against being called before
Start()- Creates entirely new component instances for atomic swap
- Preserves old handler chain for in-flight requests (since handler swap is a single pointer assignment)
- Correctly applies the h2c wrapper (addressing the past review comment)
313-331: LGTM! Component initialization correctly delegates to caller for registry setup.The method now correctly uses the caller-populated
operationRegistryinstead of creating an empty one, addressing the past review comment about registry overwriting.
333-349: LGTM! Operations map building is straightforward and correct.The method correctly builds the nested operations map and handles services without operation files gracefully.
393-426: LGTM! Response writer correctly implements required HTTP interfaces.The
responseWriterwrapper properly implementshttp.Flusher,http.Pusher, andhttp.Hijackerinterfaces, which are required for gRPC streaming and HTTP/2 features.
377-442: LGTM! Accessors are safely implemented with nil guards.All accessor methods properly guard against nil components and delegate to the appropriate underlying service.
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
e0c1c5f to
e53ed93
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
router-tests/testdata/connectrpc/services/employee.v1/service.proto (1)
50-50: Grammatical error in comment (upstream issue).The comment "This mutation update the mood" should be "This mutation updates the mood". As noted in previous review comments, this originates from the upstream GraphQL schema (
demo/pkg/subgraphs/mood/subgraph/schema.graphqls).router/pkg/connectrpc/constructor_validation_test.go (1)
102-102: Path issue persists: use "pkg/connectrpc/samples/services" instead of "samples/services".The relative path
"samples/services"does not exist when tests run from the router directory. This should be"pkg/connectrpc/samples/services"to correctly resolve from the router module root.Similar issues exist at:
- Line 113:
ServicesDir: "samples/services"- Line 125:
LoadFromDirectory("samples/services/employee.v1")- Line 191:
ServicesDir: "samples/services"- Line 202:
ServicesDir: "samples/services"🔎 Proposed fix for all occurrences
- ServicesDir: "samples/services", + ServicesDir: "pkg/connectrpc/samples/services",For line 125:
- err := protoLoader.LoadFromDirectory("samples/services/employee.v1") + err := protoLoader.LoadFromDirectory("pkg/connectrpc/samples/services/employee.v1")router/pkg/config/connectrpc_test.go (1)
47-60: Missing assertion forbase_urlfield.The test case sets
base_url: "http://example.com"in the YAML (line 52) but never asserts its value. This leaves the BaseURL field untested in this scenario.Add a
wantBaseURLfield to the test struct (around line 32) and assertcfg.Server.BaseURLin the assertion block (around lines 109-112).🔎 Suggested fix
Add the field to the test struct:
wantEnabled bool wantListenAddr string wantGraphQL string wantProviderID string + wantBaseURL stringSet it in the test case:
wantEnabled: true, wantListenAddr: "0.0.0.0:8080", wantGraphQL: "http://localhost:4000/graphql", wantProviderID: "fs-protos", + wantBaseURL: "http://example.com",And add the assertion:
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)router/core/router.go (2)
1016-1023: Add RequestTimeout configuration to ServerConfig.The ServerConfig is missing the
RequestTimeoutfield, which should be configured from the router's configuration. According to the ServerConfig definition in router/pkg/connectrpc/server.go (lines 23-37), RequestTimeout is a required field with a default of 30 seconds if not provided.🔎 Proposed fix to add timeout configuration
// Initialize the ConnectRPC server with the services directory serverConfig := connectrpc.ServerConfig{ ServicesDir: servicesDir, ListenAddr: r.connectRPC.Server.ListenAddr, GraphQLEndpoint: routerGraphQLEndpoint, Logger: r.logger, + RequestTimeout: r.connectRPC.Server.RequestTimeout, CorsConfig: r.corsOptions, }Note: Ensure that
r.connectRPC.Server.RequestTimeoutis properly configured in the config structure. If it's not present, you may need to add it to the ConnectRPCServer configuration type.
1008-1014: Use URL construction instead of filesystem path joining.
path.Joinis designed for filesystem paths and doesn't handle URL components correctly. For a listen address likelocalhost:8080, this produceslocalhost:8080/graphqlwithout a scheme.🔎 Proposed fix using URL construction
- // Determine the router GraphQL endpoint - var routerGraphQLEndpoint string - if r.connectRPC.GraphQLEndpoint != "" { - routerGraphQLEndpoint = r.connectRPC.GraphQLEndpoint - } else { - routerGraphQLEndpoint = path.Join(r.listenAddr, r.graphqlPath) - } + // Determine the router GraphQL endpoint + var routerGraphQLEndpoint string + if r.connectRPC.GraphQLEndpoint != "" { + routerGraphQLEndpoint = r.connectRPC.GraphQLEndpoint + } else { + // Construct endpoint with proper scheme + if r.tlsConfig != nil && r.tlsConfig.Enabled { + routerGraphQLEndpoint = "https://" + r.listenAddr + r.graphqlPath + } else { + routerGraphQLEndpoint = "http://" + r.listenAddr + r.graphqlPath + } + }router/pkg/connectrpc/handler.go (1)
126-129: Add a warning log when automatically defaulting to HTTP protocol.Per past review feedback, automatically prepending
http://to endpoints without a scheme should log a warning. This helps operators identify misconfigurations in production where HTTPS might be expected.🔎 Suggested change
// Ensure the endpoint has a protocol if !strings.Contains(config.GraphQLEndpoint, "://") { + config.Logger.Warn("GraphQL endpoint has no protocol scheme, defaulting to http://", + zap.String("original_endpoint", config.GraphQLEndpoint), + zap.String("modified_endpoint", "http://"+config.GraphQLEndpoint)) config.GraphQLEndpoint = "http://" + config.GraphQLEndpoint }router/pkg/connectrpc/samples/services/employee.v1/service.proto (1)
50-51: Fix grammatical error in comment.The comment has a subject-verb agreement error.
🔎 Suggested fix
- // This mutation update the mood of an employee. + // This mutation updates the mood of an employee.
🧹 Nitpick comments (15)
protographic/src/operations/proto-text-generator.ts (2)
123-130: Use the constant for field number to avoid duplication.The field number
50001is duplicated here and inGRAPHQL_VARIABLE_NAME.fieldNumber. Consider using the constant to keep them in sync.🔎 Proposed fix
// Add field option extension if used if (usesGraphQLVariableName) { lines.push('// Field option extension for GraphQL variable name mapping'); lines.push('extend google.protobuf.FieldOptions {'); - lines.push(' string graphql_variable_name = 50001;'); + lines.push(` string graphql_variable_name = ${GRAPHQL_VARIABLE_NAME.fieldNumber};`); lines.push('}'); lines.push(''); }
305-322: Consider escaping special characters in string option values.If a string value contains double quotes or backslashes, the generated proto would be malformed. While GraphQL variable names are typically simple identifiers, adding defensive escaping would be safer.
🔎 Proposed fix
.map(([key, value]) => { // The key already includes parentheses if it's an extension option // e.g., "(graphql_variable_name)" // Handle string values with quotes - const formattedValue = typeof value === 'string' ? `"${value}"` : value; + const formattedValue = typeof value === 'string' + ? `"${value.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"` + : value; return `${key} = ${formattedValue}`; })protographic/src/operations/message-builder.ts (1)
303-311: Consider computingcurrentPathonce at function entry.The
currentPathis computed twice with identical logic (lines 361 and 399) when processing fields with selection sets. Computing it once at the start ofprocessFieldSelectionwould reduce duplication and improve clarity.🔎 Proposed refactor
function processFieldSelection( field: FieldNode, message: protobuf.Type, parentType: GraphQLObjectType | GraphQLInterfaceType | GraphQLUnionType, typeInfo: TypeInfo, options?: MessageBuilderOptions, fieldNumberManager?: FieldNumberManager, messagePath?: string, ): void { const fieldName = field.name.value; + + // Compute current path once for both nested message creation and field numbering + const currentPath = messagePath || message.name; // Skip __typename - it's a GraphQL introspection field that doesn't need to be in proto if (fieldName === '__typename') { return; }Then remove the duplicate computations at lines 361 and 399.
Also applies to: 360-361, 398-400
router/pkg/config/config.schema.json (1)
2137-2185: LGTM! Well-structured ConnectRPC configuration schema.The
connect_rpcconfiguration block follows established patterns and includes appropriate validation:
- Conditional requirement:
services_provider_idis required whenenabled: true- Format constraints properly applied (hostname-port, http-url, url)
- Comprehensive field descriptions
- Reasonable defaults (listen_addr: localhost:5026)
The description for
services_provider_id(line 2177) is quite lengthy. Consider breaking it into a shorter description with additional documentation linked separately, though this is a minor style preference and not a functional issue.protographic/src/operation-to-proto.ts (2)
295-303: Unnecessaryas anycast.Line 302 casts the string result to
any, which is unnecessary sincemethodNameis already typed asstring. This appears to be leftover from a previous refactor.🔎 Proposed fix
if (this.prefixOperationType) { const operationTypePrefix = upperFirst(node.operation.toLowerCase()); - methodName = `${operationTypePrefix}${methodName}` as any; + methodName = `${operationTypePrefix}${methodName}`; }
295-328: Step numbering in comments is inconsistent.The step comments are out of order: 4 (line 295), 4 (line 305), 3.5 (line 321), 6 (line 328). Consider renumbering for clarity.
router/pkg/connectrpc/connect_util.go (1)
48-79: Add mappings for additional Connect error codes to improve HTTP status semantics.
ConnectCodeToHTTPStatusis missing explicit mappings for three Connect error codes that have standard HTTP equivalents per the Connect protocol specification:
CodeCanceled→ 499 (Client Closed Request)CodeAlreadyExists→ 409 (Conflict)CodeDataLoss→ 500 (Internal Server Error)While the default fallback to 500 is safe, explicit mappings would provide more accurate HTTP status semantics and prevent information loss, especially for
CodeAlreadyExistswhich maps to the same 409 used forCodeAborted.Proposed additions
func ConnectCodeToHTTPStatus(code connect.Code) int { switch code { + case connect.CodeCanceled: + return 499 // Client Closed Request case connect.CodeInvalidArgument: return http.StatusBadRequest // 400 case connect.CodeUnauthenticated: return http.StatusUnauthorized // 401 case connect.CodePermissionDenied: return http.StatusForbidden // 403 case connect.CodeNotFound: return http.StatusNotFound // 404 + case connect.CodeAlreadyExists: + return http.StatusConflict // 409 case connect.CodeAborted: return http.StatusConflict // 409 case connect.CodeFailedPrecondition: return http.StatusPreconditionFailed // 412 case connect.CodeResourceExhausted: return http.StatusTooManyRequests // 429 case connect.CodeOutOfRange: return http.StatusRequestedRangeNotSatisfiable // 416 case connect.CodeDeadlineExceeded: return http.StatusGatewayTimeout // 504 case connect.CodeUnimplemented: return http.StatusNotImplemented // 501 case connect.CodeUnavailable: return http.StatusServiceUnavailable // 503 case connect.CodeInternal: return http.StatusInternalServerError // 500 + case connect.CodeDataLoss: + return http.StatusInternalServerError // 500 default: return http.StatusInternalServerError // 500 } }router/pkg/connectrpc/operation_loader.go (1)
73-80: Verify "last one wins" behavior for duplicate operations.The current implementation logs a warning but allows duplicate operation names within the same service, with the last loaded operation overwriting earlier ones. This could silently hide configuration errors if duplicate operation names are unintentional.
Consider whether stricter validation (returning an error for duplicates) would be more appropriate, or if this "last one wins" behavior is intentional to support configuration overrides.
router-tests/connectrpc/connectrpc_client_test.go (1)
28-28: Redundant cleanup: defer statement unnecessary.The
defer ts.Close()on line 28 is redundant becauseNewTestConnectRPCServeralready registerst.Cleanup(func() { ts.Close() })(as shown in the helper code). This meansClose()will be called twice.While harmless, you can remove the defer statement for clarity since
t.Cleanuphandles cleanup properly.🔎 Suggested change
}) - defer ts.Close() err := ts.Start()router/pkg/connectrpc/operation_registry.go (1)
98-102: Simplify by removing redundant exists check.In Go,
len()on a nil map returns 0, so the exists check is unnecessary.🔎 Proposed simplification
-// CountForService returns the number of operations for a specific service. -// This method is safe for concurrent use (no locking needed due to immutability). -func (r *OperationRegistry) CountForService(serviceName string) int { - return len(r.operations[serviceName]) -} +// CountForService returns the number of operations for a specific service. +// Returns 0 if the service doesn't exist (Go returns 0 for len of nil map). +// This method is safe for concurrent use (no locking needed due to immutability). +func (r *OperationRegistry) CountForService(serviceName string) int { + return len(r.operations[serviceName]) +}router-tests/connectrpc/connectrpc_test_helpers.go (2)
55-62: Consider returning an error instead of panicking.Using
panicin test helper constructors makes debugging harder when tests fail due to port conflicts or resource exhaustion. Consider returning an error fromNewMockGraphQLServerto allow tests to handle failures gracefully.🔎 Suggested change
-func NewMockGraphQLServer(handler http.HandlerFunc) *MockGraphQLServer { +func NewMockGraphQLServer(handler http.HandlerFunc) (*MockGraphQLServer, error) { m := &MockGraphQLServer{ handler: handler, } // ... mux setup ... listener, err := net.Listen("tcp", m.server.Addr) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to create listener: %w", err) } m.URL = "http://" + listener.Addr().String() go m.server.Serve(listener) - return m + return m, nil }
230-237: Consider escaping the message parameter to prevent JSON injection.The
ErrorGraphQLHandlerusesfmt.Sprintfto build JSON, but ifmessagecontains quotes or special characters, it will produce malformed JSON or enable injection in test scenarios.🔎 Suggested fix
func ErrorGraphQLHandler(message string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + response := map[string]any{ + "errors": []map[string]string{{"message": message}}, + } + jsonBytes, _ := json.Marshal(response) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - w.Write([]byte(fmt.Sprintf(`{"errors": [{"message": "%s"}]}`, message))) + w.Write(jsonBytes) } }router/pkg/connectrpc/service_discovery.go (1)
235-253: Proto parsing may miss edge cases with comments or multi-line declarations.The
extractServiceNameFromProtofunction uses simple line-by-line prefix matching. This could fail for:
- Service declarations spanning multiple lines
- Commented-out service declarations
- Services declared with additional syntax
For a convention-based tool, this is acceptable, but consider documenting the expected proto format requirements.
router/pkg/connectrpc/vanguard_service.go (1)
165-178: Linear method lookup could be optimized for services with many methods.The method validation iterates through all methods on each request. For services with many RPC methods, consider using a pre-built map for O(1) lookups.
🔎 Suggested optimization
Build a method lookup map during service registration:
type VanguardService struct { // ... methodsByService map[string]map[string]bool // serviceName -> methodName -> exists } // In registerServices: vs.methodsByService[serviceName] = make(map[string]bool) for _, method := range serviceDef.Methods { vs.methodsByService[serviceName][method.Name] = true } // In createServiceHandler: if !vs.methodsByService[serviceName][methodName] { // Return error }router/pkg/connectrpc/proto_loader.go (1)
368-372: Consider returning a copy to prevent accidental mutation.The
GetServicesmethod returns the internal map directly. While the comment warns about treating it as read-only, callers could accidentally mutate it. Consider returning a shallow copy for safety.🔎 Suggested change
// 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 + result := make(map[string]*ServiceDefinition, len(pl.services)) + for k, v := range pl.services { + result[k] = v + } + return result }
📜 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 (69)
cli/src/commands/grpc-service/commands/generate.tsprotographic/OPERATIONS_TO_PROTO.mdprotographic/src/naming-conventions.tsprotographic/src/operation-to-proto.tsprotographic/src/operations/message-builder.tsprotographic/src/operations/proto-field-options.tsprotographic/src/operations/proto-text-generator.tsprotographic/src/operations/request-builder.tsprotographic/tests/operations/fragments.test.tsprotographic/tests/operations/nested-message-field-numbering.test.tsprotographic/tests/operations/operation-validation.test.tsrouter-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/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/go.modrouter/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/httputil/headers.gorouter/pkg/mcpserver/server.go
✅ Files skipped from review due to trivial changes (3)
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.json
- router/pkg/connectrpc/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (28)
- router/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.json
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router/pkg/connectrpc/vanguard_service_test.go
- router/pkg/config/testdata/config_full.json
- router-tests/testdata/connectrpc/README.md
- router/core/router_config.go
- router-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphql
- router/pkg/httputil/headers.go
- router/pkg/connectrpc/proto_field_options.go
- router/pkg/connectrpc/testdata/test.proto
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql
- router/pkg/connectrpc/error_handling_test.go
- router/pkg/connectrpc/server_test.go
- router/pkg/connectrpc/operation_registry_test.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphql
- router/core/supervisor_instance.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql
- protographic/src/naming-conventions.ts
- router/pkg/mcpserver/server.go
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql
- protographic/src/operations/proto-field-options.ts
- router/pkg/config/config.go
- router/pkg/connectrpc/server.go
- router/go.mod
- router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql
- protographic/tests/operations/fragments.test.ts
- protographic/src/operations/request-builder.ts
🧰 Additional context used
🧠 Learnings (21)
📓 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-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/OPERATIONS_TO_PROTO.mdprotographic/tests/operations/operation-validation.test.tsprotographic/src/operation-to-proto.ts
📚 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/go.modrouter-tests/connectrpc/connectrpc_client_test.gorouter/core/router.gorouter-tests/testdata/connectrpc/buf.gen.yamlrouter-tests/connectrpc/connectrpc_server_lifecycle_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-tests/go.modrouter-tests/connectrpc/connectrpc_client_test.gorouter/pkg/connectrpc/constructor_validation_test.gorouter-tests/connectrpc/connectrpc_test.gorouter/pkg/connectrpc/service_discovery_test.gorouter/pkg/config/connectrpc_test.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go
📚 Learning: 2025-10-27T09:45:41.622Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.
Applied to files:
protographic/tests/operations/nested-message-field-numbering.test.ts
📚 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.jsonrouter/pkg/connectrpc/handler.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/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/core/router.gorouter/pkg/config/testdata/config_defaults.json
📚 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-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/vanguard_service.gorouter/pkg/connectrpc/helpers_test.gorouter/pkg/connectrpc/proto_loader.gorouter/pkg/connectrpc/handler.gorouter-tests/connectrpc/connectrpc_test_helpers.go
📚 Learning: 2025-08-20T10:38:49.191Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2151
File: protographic/src/sdl-to-proto-visitor.ts:0-0
Timestamp: 2025-08-20T10:38:49.191Z
Learning: Inline `extend google.protobuf.FieldOptions` declarations in proto3 files work correctly with protoc, despite theoretical concerns about proto3 compatibility with extension declarations.
Applied to files:
protographic/src/operations/proto-text-generator.ts
📚 Learning: 2025-09-10T09:53:42.914Z
Learnt from: JivusAyrus
Repo: wundergraph/cosmo PR: 2156
File: studio/src/pages/[organizationSlug]/[namespace]/graph/[slug]/checks/index.tsx:189-197
Timestamp: 2025-09-10T09:53:42.914Z
Learning: In protobuf-generated TypeScript code, repeated fields (arrays) are always initialized with default empty arrays and cannot be undefined, making defensive programming checks like `|| []` or optional chaining unnecessary for these fields.
Applied to files:
protographic/src/operations/proto-text-generator.ts
📚 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-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
📚 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-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
🧬 Code graph analysis (11)
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)
protographic/tests/operations/nested-message-field-numbering.test.ts (1)
protographic/tests/util.ts (1)
expectValidProto(29-31)
protographic/tests/operations/operation-validation.test.ts (1)
protographic/src/operation-to-proto.ts (1)
compileOperationsToProto(87-141)
router/pkg/connectrpc/proto_loader_test.go (1)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)
router/core/router.go (3)
router/pkg/connectrpc/server.go (3)
Server(41-52)ServerConfig(24-38)NewServer(55-177)router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)router/pkg/config/config.go (1)
ConnectRPCConfiguration(1016-1021)
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/connectrpc/vanguard_service.go (4)
router/pkg/connectrpc/handler.go (2)
MetaKeyGraphQLErrors(49-49)GraphQLError(73-78)router/pkg/connectrpc/proto_loader.go (2)
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)
protographic/src/operations/proto-text-generator.ts (2)
protographic/src/sdl-to-proto-visitor.ts (1)
formatIndent(2217-2219)protographic/src/operations/proto-field-options.ts (1)
GRAPHQL_VARIABLE_NAME(16-19)
router/pkg/config/connectrpc_test.go (1)
router/pkg/config/config.go (3)
ConnectRPCConfiguration(1016-1021)LoadConfig(1150-1262)Config(1043-1119)
router/pkg/connectrpc/helpers_test.go (3)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)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)
router-tests/connectrpc/connectrpc_test_helpers.go (2)
router/pkg/mcpserver/util.go (1)
Logger(6-9)router/pkg/connectrpc/server.go (1)
ServerConfig(24-38)
🪛 GitHub Actions: Protographic CI
protographic/tests/operations/operation-validation.test.ts
[error] 244-359: Test failures in Operation Name PascalCase Validation: expected error message to match /Operation name "getUser" must start with an uppercase letter/ (and similar for other test cases), but the actual messages now state /Operation name "" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded.../. This indicates a change in error text from the validation logic: tests are asserting the older wording. Update tests to reflect the new PascalCase guidance or adjust validation messages accordingly.
🪛 GitHub Check: build_test
protographic/tests/operations/operation-validation.test.ts
[failure] 359-359: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should validate subscription operation names
AssertionError: expected [Function] to throw error matching /Operation name "onMessageAdded" must …/ but got 'Operation name "onMessageAdded" must …'
- Expected:
/Operation name "onMessageAdded" must start with an uppercase letter/
- Received:
"Operation name "onMessageAdded" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "OnMessageAdded". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:359:10
[failure] 329-329: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should validate mutation operation names
AssertionError: expected [Function] to throw error matching /Operation name "createUser" must star…/ but got 'Operation name "createUser" must be i…'
- Expected:
/Operation name "createUser" must start with an uppercase letter/
- Received:
"Operation name "createUser" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "CreateUser". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:329:10
[failure] 303-303: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should provide helpful error message for camelCase
AssertionError: expected [Function] to throw error matching /must start with an uppercase letter.*…/ but got 'Operation name "getUserById" must be …'
- Expected:
/must start with an uppercase letter.*Examples: GetUser, CreatePost, HRService, GETUSER/
- Received:
"Operation name "getUserById" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "GetUserById". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:303:10
[failure] 289-289: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should accept operation names with only uppercase and numbers
AssertionError: expected [Function] to not throw an error but 'Error: Operation name "GET123USER" mu…' was thrown
- Expected:
undefined
- Received:
"Error: Operation name "GET123USER" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "Get123User". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:289:14
[failure] 274-274: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should accept all-UPPERCASE operation names
AssertionError: expected [Function] to not throw an error but 'Error: Operation name "GETUSER" must …' was thrown
- Expected:
undefined
- Received:
"Error: Operation name "GETUSER" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "Getuser". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:274:14
[failure] 259-259: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should reject snake_case operation names
AssertionError: expected [Function] to throw error matching /Operation name "get_user" must start …/ but got 'Operation name "get_user" must be in …'
- Expected:
/Operation name "get_user" must start with an uppercase letter/
- Received:
"Operation name "get_user" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "GetUser". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:259:10
[failure] 244-244: tests/operations/operation-validation.test.ts > Operation Validation > Operation Name PascalCase Validation > should reject camelCase operation names
AssertionError: expected [Function] to throw error matching /Operation name "getUser" must start w…/ but got 'Operation name "getUser" must be in P…'
- Expected:
/Operation name "getUser" must start with an uppercase letter/
- Received:
"Operation name "getUser" must be in PascalCase (start with uppercase letter, followed by mixed-case letters/numbers). Examples: GetUser, CreatePost, OnMessageAdded. Suggested name: "GetUser". This ensures the RPC method name exactly matches the GraphQL operation name."
❯ tests/operations/operation-validation.test.ts:244:10
⏰ 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 (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
protographic/src/operation-to-proto.ts (1)
295-365: Consider renumbering the step comments for consistency.The step numbering in comments is inconsistent:
- Line 295: "4. Create method name"
- Line 305: "4. Create request message" (duplicate 4)
- Line 321: "3.5. Process any input object types" (out of sequence)
- Line 328: "6. Create response message" (skips 5)
This appears to be a side effect of adding the new step 2 validation (lines 267-279) without updating all subsequent step numbers. Consider renumbering for clarity:
- Line 305: Change "4." → "5."
- Line 321: Change "3.5." → "6."
- Line 328: Change "6." → "7."
- Line 351: Change "7." → "8."
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
protographic/src/operation-to-proto.ts
🧰 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-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
⏰ 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: image_scan (nonroot)
- GitHub Check: image_scan
- GitHub Check: build_push_image (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_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
protographic/src/operation-to-proto.ts (2)
267-279: LGTM! Operation name validation now enforces protobuf RPC conventions.The new validation correctly requires operation names to start with an uppercase letter and contain only alphanumeric characters. The error message provides clear guidance with examples and includes the suggested name using
upperFirst(camelCase(operationName)), which addresses the previous review feedback.Based on past review comments, this implementation matches the requested approach.
103-110: LGTM! Error messages now use clearer terminology.The updated error messages appropriately reference "deterministic proto schema generation" and "proto schema generation consistency" instead of the previous terminology. This better communicates the constraints and aligns with protobuf/gRPC domain language.
Also applies to: 285-290
…ographic-improvements
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
from
77c8fc4 to
6f4c88c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
protographic/tests/operations/operation-validation.test.ts (1)
232-361: Test failures already documented in previous review.All test failures in this test suite have been comprehensively documented in the previous review comments (lines 232-245 and 247-361), which provide:
- Behavioral change: UPPERCASE-only operation names are now rejected (lines 262-290)
- Error message updates: All error assertions need updating to match the new detailed format with examples and suggestions (lines 232-245, 247-260, 292-304, 306-330, 332-360)
Please refer to the existing review comments for detailed proposed fixes.
router/pkg/connectrpc/operation_registry_test.go (1)
381-399: Fix the compilation error: sync.WaitGroup has no Go() method.Line 386 calls
wg.Go(func() { ... }), butsync.WaitGroupdoes not have aGo()method. This code will not compile.🔎 Proposed fix
t.Run("concurrent reads are safe", func(t *testing.T) { var wg sync.WaitGroup // Start multiple goroutines reading concurrently - for range 10 { - wg.Go(func() { + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() - for range 100 { + for j := 0; j < 100; j++ { _ = registry.GetOperationForService(serviceName, "Test") _ = registry.HasOperationForService(serviceName, "Test") _ = registry.GetAllOperationsForService(serviceName) _ = registry.Count() _ = registry.CountForService(serviceName) } - }) + }() } // Wait for all goroutines to complete wg.Wait() })router/pkg/config/connectrpc_test.go (1)
47-60: Add assertion for the base_url field.The test sets
base_url: "http://example.com"in the YAML (line 52) but never asserts its value in the expectations or assertion block. This leaves the BaseURL field untested.🔎 Proposed fix
Add a wantBaseURL field to the test case:
{ name: "full config with overrides", yaml: `connect_rpc: enabled: true server: listen_addr: "0.0.0.0:8080" base_url: "http://example.com" services_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", },Then add the assertion after line 112:
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.ServicesProviderID)router/core/router.go (1)
1008-1014: Use URL construction instead of filesystem path joining.
path.Joinis designed for filesystem paths, not URLs. For constructing a GraphQL endpoint URL, use proper URL construction to ensure the scheme is included.This was flagged in a previous review. The suggested fix was:
if r.tlsConfig != nil && r.tlsConfig.Enabled { routerGraphQLEndpoint = "https://" + r.listenAddr + r.graphqlPath } else { routerGraphQLEndpoint = "http://" + r.listenAddr + r.graphqlPath }router/pkg/connectrpc/handler.go (1)
126-129: Add warning log when auto-prepending HTTP protocol.Per past review, production deployments expecting HTTPS could silently use HTTP. Add a log warning:
// Ensure the endpoint has a protocol if !strings.Contains(config.GraphQLEndpoint, "://") { + config.Logger.Warn("GraphQL endpoint missing protocol, defaulting to http://", + zap.String("original", config.GraphQLEndpoint), + zap.String("resolved", "http://"+config.GraphQLEndpoint)) config.GraphQLEndpoint = "http://" + config.GraphQLEndpoint }router/pkg/connectrpc/samples/services/employee.v1/service.proto (1)
50-51: Fix grammatical error in comment.- // This mutation update the mood of an employee. + // This mutation updates the mood of an employee.
🧹 Nitpick comments (13)
router/pkg/mcpserver/server.go (1)
698-698: Consider wrapping the header skip check in a helper function for clarity.The centralization of
SkippedHeadersis good for consistency. However, a past review comment suggested exposing a more meaningful function rather than the raw map lookup to clarify that these headers are skipped because they're hop-by-hop or would cause issues when forwarded to origin servers.Based on learnings, this aligns with past feedback suggesting a function wrapper would better document the intent.
Example wrapper function
In
router/pkg/httputil/headers.go:// ShouldSkipHeader returns true if the header should not be forwarded // to origin servers (e.g., hop-by-hop headers or headers that would cause issues). func ShouldSkipHeader(headerName string) bool { _, skip := SkippedHeaders[headerName] return skip }Then use:
-if _, ok := httputil.SkippedHeaders[key]; ok { +if httputil.ShouldSkipHeader(key) { continue }router/pkg/connectrpc/vanguard_service_test.go (1)
250-261: Direct mutation of immutableOperationRegistryinternals.The
OperationRegistryis designed to be immutable after construction (per its docstring: "This map is immutable after construction - no locks needed for reads"). Directly accessing and mutatingopRegistry.operationsbypasses this contract.Consider using the
buildTestOperationshelper fromhelpers_test.goto construct the operations map upfront:🔎 Suggested refactor
- opRegistry := NewOperationRegistry(nil) - - // Manually add a test operation 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{ + serviceName := "employee.v1.EmployeeService" + operations := map[string]map[string]*schemaloader.Operation{ + serviceName: { + "GetEmployeeById": &schemaloader.Operation{ + Name: "GetEmployeeById", + OperationType: "query", + OperationString: "query GetEmployeeById($id: Int!) { employee(id: $id) { id name } }", + }, + }, + } + opRegistry := NewOperationRegistry(operations) - Name: "GetEmployeeById", - OperationType: "query", - OperationString: "query GetEmployeeById($id: Int!) { employee(id: $id) { id name } }", - }router-tests/connectrpc/connectrpc_client_test.go (1)
25-31: Explicitdefer ts.Close()is redundant but harmless.The
NewTestConnectRPCServerhelper already registers a cleanup function viat.Cleanup(). The explicitdefer ts.Close()is harmless (double-close is likely safe) but unnecessary.router/pkg/connectrpc/helpers_test.go (1)
89-95: Mock response missingRequestfield.The
http.Responsereturned bymockRoundTripperdoesn't set theRequestfield. Some HTTP client code may expect this to be non-nil. Consider adding it:🔎 Suggested fix
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-tests/connectrpc/connectrpc_server_lifecycle_test.go (1)
74-84: Platform-specific error message check.The check for
"address already in use"may not work on all platforms. On Windows, the error message might be different (e.g., "Only one usage of each socket address").Consider a more resilient check or document that this test may be platform-specific:
🔎 More portable approach
// Check if it's a network/bind error rather than matching exact message } else if err != nil { // Any non-nil error from concurrent starts is expected // (could be "address already in use", "bind: address already in use", etc.) portConflictCount++ }Alternatively, keep the current approach if tests only run on Linux/macOS.
router/core/router.go (1)
999-1006:discoveredServicesis only used for logging.The
discoveredServicesslice is computed but only its length is used at line 1044. Consider whether this call is necessary, or if the server'sGetServiceCount()method (used at line 1045) provides sufficient information.🔎 Simplify if discovery is redundant
If
NewServeralready performs discovery internally and you only need the count for logging, you could remove the explicitDiscoverServicescall:- // 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) - } - // Determine the router GraphQL endpoint ... // Single consolidated INFO log for ConnectRPC startup 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()))However, if you want early validation before server construction, keeping it is fine.
router-tests/connectrpc/connectrpc_test_helpers.go (2)
34-36: Minor: Silently ignoring read error.The error from
io.ReadAllis silently discarded. While this is test code, it could hide issues during debugging.- body, _ := io.ReadAll(r.Body) + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "failed to read body", http.StatusBadRequest) + return + }
230-237: Potential JSON injection in error message.If
messagecontains unescaped characters like"or\, the JSON will be malformed. Usejson.Marshalfor safety.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))) + response := map[string]any{"errors": []map[string]string{{"message": message}}} + json.NewEncoder(w).Encode(response) } }router/pkg/connectrpc/proto_loader.go (1)
368-372: Consider returning a defensive copy for GetServices.The comment warns callers to treat the map as read-only, but returning the internal map directly allows accidental mutation. 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 }Alternatively, if performance is a concern and callers are trusted, the current approach with the comment is acceptable.
router/pkg/connectrpc/server.go (2)
195-201: Consider making HTTP server timeouts configurable.The
ReadTimeout,WriteTimeout, andIdleTimeoutare hard-coded. For production use cases with varying request sizes or slow clients, these might need tuning.+ readTimeout := 30 * time.Second + if s.config.ReadTimeout > 0 { + readTimeout = s.config.ReadTimeout + } s.httpServer = &http.Server{ Addr: s.config.ListenAddr, Handler: h2cHandler, - ReadTimeout: 30 * time.Second, - WriteTimeout: 30 * time.Second, + ReadTimeout: readTimeout, + WriteTimeout: readTimeout, IdleTimeout: 60 * time.Second, }
233-234: Shutdown timeout ignores caller's context deadline.The 5-second timeout is created independently of the passed
ctx. If the caller's context has a shorter deadline, it will be ignored.- shutdownCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + // Use the shorter of the caller's deadline or 5 seconds + shutdownCtx, cancel := context.WithTimeout(ctx, 5*time.Second)This is likely acceptable for most use cases, but consider documenting this behavior.
router/pkg/connectrpc/vanguard_service.go (1)
56-58: Inconsistent nil logger handling.
VanguardServiceConfigdefaultsLoggertozap.NewNop()if nil, but other components likeNewRPCHandlerreturn an error for nil logger. Consider making this consistent.- if config.Logger == nil { - config.Logger = zap.NewNop() - } + if config.Logger == nil { + return nil, fmt.Errorf("logger is required") + }router/pkg/connectrpc/handler.go (1)
417-441: Consider handling json.Marshal errors.The
json.Marshalcall at line 419 ignores the error. While unlikely to fail for valid GraphQL errors, defensive error handling is better:- errorsJSON, _ := json.Marshal(errors) + errorsJSON, err := json.Marshal(errors) + if err != nil { + h.logger.Error("failed to marshal GraphQL errors", zap.Error(err)) + errorsJSON = []byte("[]") + }
📜 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 (69)
cli/src/commands/grpc-service/commands/generate.tsprotographic/OPERATIONS_TO_PROTO.mdprotographic/src/naming-conventions.tsprotographic/src/operation-to-proto.tsprotographic/src/operations/message-builder.tsprotographic/src/operations/proto-field-options.tsprotographic/src/operations/proto-text-generator.tsprotographic/src/operations/request-builder.tsprotographic/tests/operations/fragments.test.tsprotographic/tests/operations/nested-message-field-numbering.test.tsprotographic/tests/operations/operation-validation.test.tsrouter-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/router.gorouter/core/router_config.gorouter/core/supervisor_instance.gorouter/go.modrouter/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/httputil/headers.gorouter/pkg/mcpserver/server.go
✅ Files skipped from review due to trivial changes (1)
- router/pkg/connectrpc/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (40)
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeById.graphql
- router/pkg/connectrpc/proto_field_options.go
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeByPets.graphql
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql
- router/pkg/connectrpc/error_handling_test.go
- router-tests/testdata/connectrpc/services/employee.v1/service.proto.lock.json
- router/pkg/connectrpc/samples/services/employee.v1/MutationUpdateEmployeeMood.graphql
- protographic/src/operations/request-builder.ts
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeByPets.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router/core/router_config.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql
- router/pkg/config/testdata/config_defaults.json
- router/connect.config.yaml
- router/pkg/config/testdata/config_full.json
- router/pkg/httputil/headers.go
- protographic/tests/operations/fragments.test.ts
- router/pkg/connectrpc/constructor_validation_test.go
- router-tests/testdata/connectrpc/buf.gen.yaml
- router/pkg/connectrpc/testdata/test.proto
- protographic/src/operations/proto-field-options.ts
- router-tests/go.mod
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployeeWithMood.graphql
- router/pkg/connectrpc/operation_loader.go
- router/pkg/connectrpc/service_discovery.go
- router/pkg/connectrpc/server_test.go
- router/go.mod
- router/pkg/connectrpc/samples/services/employee.v1/service.proto.lock.json
- router/core/supervisor_instance.go
- router/pkg/connectrpc/operation_registry.go
- router-tests/testdata/connectrpc/services/employee.v1/QueryGetEmployees.graphql
- router-tests/connectrpc/connectrpc_test.go
- protographic/src/naming-conventions.ts
- router-tests/testdata/connectrpc/README.md
- router-tests/testdata/connectrpc/buf.yaml
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeeById.graphql
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployees.graphql
- router/pkg/config/config.schema.json
- router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsNamedFragment.graphql
- protographic/src/operations/proto-text-generator.ts
🧰 Additional context used
🧠 Learnings (21)
📓 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-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/mcpserver/server.gorouter-tests/connectrpc/connectrpc_server_lifecycle_test.gorouter/core/router.gorouter/pkg/connectrpc/operation_registry_test.gorouter/pkg/config/config.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/pkg/mcpserver/server.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/mcpserver/server.gorouter/pkg/connectrpc/vanguard_service_test.gorouter-tests/connectrpc/connectrpc_test_helpers.gorouter/pkg/connectrpc/proto_loader.gorouter/pkg/connectrpc/vanguard_service.gorouter/pkg/config/config.gorouter/pkg/connectrpc/handler.gorouter/pkg/connectrpc/helpers_test.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/pkg/mcpserver/server.gorouter/core/router.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/OPERATIONS_TO_PROTO.mdprotographic/src/operation-to-proto.tsprotographic/tests/operations/operation-validation.test.ts
📚 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_server_lifecycle_test.gorouter/pkg/connectrpc/service_discovery_test.gorouter/pkg/config/connectrpc_test.goprotographic/tests/operations/operation-validation.test.tsrouter-tests/connectrpc/connectrpc_client_test.gorouter/pkg/connectrpc/operation_registry_test.gorouter-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go
📚 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-10-27T09:45:41.622Z
Learnt from: Noroth
Repo: wundergraph/cosmo PR: 2290
File: protographic/src/sdl-to-proto-visitor.ts:1350-1390
Timestamp: 2025-10-27T09:45:41.622Z
Learning: In protographic/src/sdl-to-proto-visitor.ts, resolver-related messages (created by `createResolverRequestMessage` and `createResolverResponseMessage`) are special messages that should not be tracked in the proto lock file, unlike other message types.
Applied to files:
protographic/tests/operations/nested-message-field-numbering.test.ts
📚 Learning: 2025-09-22T11:13:45.617Z
Learnt from: SkArchon
Repo: wundergraph/cosmo PR: 2224
File: router/pkg/authentication/keyfunc/keyfunc_test.go:142-154
Timestamp: 2025-09-22T11:13:45.617Z
Learning: When reviewing forked code, especially test files with test fixtures like JWT tokens, avoid suggesting modifications to maintain alignment with upstream and preserve the original author's structure. Test fixtures that are clearly marked as such (not real secrets) should generally be left unchanged in forked implementations.
Applied to files:
protographic/tests/operations/operation-validation.test.ts
📚 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:
protographic/tests/operations/operation-validation.test.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-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-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-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-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-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
📚 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.go
📚 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-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/handler.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/connectrpc/handler.go
🧬 Code graph analysis (14)
router/pkg/mcpserver/server.go (1)
router/pkg/httputil/headers.go (1)
SkippedHeaders(6-27)
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)
router/pkg/connectrpc/service_discovery_test.go (1)
router/pkg/connectrpc/service_discovery.go (2)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)
protographic/tests/operations/nested-message-field-numbering.test.ts (1)
protographic/tests/util.ts (1)
expectValidProto(29-31)
router/pkg/config/connectrpc_test.go (3)
router/pkg/config/config.go (3)
ConnectRPCConfiguration(1016-1021)LoadConfig(1150-1262)Config(1043-1119)router/core/router_config.go (1)
Config(50-147)router/pkg/connectrpc/server.go (1)
Server(41-52)
router/pkg/connectrpc/server.go (6)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)router/pkg/connectrpc/operation_registry.go (2)
OperationRegistry(14-18)NewOperationRegistry(22-30)router/pkg/connectrpc/handler.go (2)
RPCHandler(87-93)HandlerConfig(96-102)router/pkg/connectrpc/vanguard_service.go (3)
VanguardService(39-44)NewVanguardService(47-72)VanguardServiceConfig(19-23)router/pkg/connectrpc/service_discovery.go (3)
DiscoverServices(46-153)ServiceDiscoveryConfig(29-34)DiscoveredService(13-26)router/pkg/connectrpc/operation_loader.go (1)
LoadOperationsForService(20-105)
router-tests/connectrpc/connectrpc_test_helpers.go (1)
router/pkg/connectrpc/server.go (1)
ServerConfig(24-38)
router/pkg/connectrpc/proto_loader_test.go (1)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)
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)
protographic/src/operations/message-builder.ts (1)
protographic/src/operations/field-numbering.ts (2)
assignFieldNumbersFromLockData(212-229)FieldNumberManager(14-68)
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-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (2)
router-plugin/httpclient/client.go (1)
ClientOption(34-34)router/pkg/pubsub/datasource/error.go (1)
NewError(12-17)
router/pkg/connectrpc/handler.go (3)
router/pkg/httputil/headers.go (1)
SkippedHeaders(6-27)router/pkg/connectrpc/connect_util.go (1)
HTTPStatusToConnectCode(11-46)router/pkg/connectrpc/proto_field_options.go (1)
GraphQLVariableNameFieldNumber(36-36)
router/pkg/connectrpc/helpers_test.go (3)
router/pkg/connectrpc/proto_loader.go (2)
ProtoLoader(55-64)NewProtoLoader(67-78)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). (11)
- GitHub Check: build_test
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (42)
protographic/tests/operations/operation-validation.test.ts (2)
181-199: Test suite rename improves clarity.The rename from "Reversibility Considerations" to "Proto Schema Consistency" better describes the test's purpose—validating deterministic proto schema generation for the ConnectRPC bridge.
202-230: LGTM: Happy path tests are correct.The tests properly validate that PascalCase operation names (with and without numbers) are accepted by the validator.
router-tests/testdata/connectrpc/services/employee.v1/MutationUpdateEmployeeMood.graphql (1)
1-2: No changes needed. The variable-to-argument naming ($employeeIdtoemployeeID) is intentional and matches the corresponding sample file exactly. This is valid GraphQL syntax.cli/src/commands/grpc-service/commands/generate.ts (1)
347-348: LGTM! Clear documentation of the processing strategy.The updated comments effectively explain the sequential processing approach for field number stability and the AST-based merging strategy, improving code maintainability.
router-tests/testdata/connectrpc/services/employee.v1/service.proto (3)
1-32: LGTM! Service definition is well-structured.The proto3 syntax and service definition are correct. The
UpdateEmployeeMoodRPC appropriately omits theidempotency_level = NO_SIDE_EFFECTSoption since it's a mutation operation, while all query operations correctly declareNO_SIDE_EFFECTS.
34-178: Message definitions are valid with unconventional field numbering.The message structures are syntactically correct. The field numbering has non-sequential gaps (e.g.,
forename=5, surname=6, pets=9skipping 1-4 and 7-8, or starting at field 11 in some messages), which is valid proto3 but unconventional. Since this is test data for the GraphQL-to-gRPC bridge and likely mirrors the source GraphQL schema structure (as indicated by the intentional grammatical error in the comment on line 50), the field numbering is probably intentional for testing specific scenarios.
180-191: LGTM! Enum definitions follow proto3 best practices.Both
MoodandGenderenums correctly define_UNSPECIFIEDas the zero value, following proto3 conventions for safe default handling.protographic/src/operation-to-proto.ts (2)
267-279: LGTM: Clear validation with helpful error guidance.The operation name validation enforces protobuf RPC naming conventions (uppercase start, alphanumeric) and provides a suggested name using
upperFirst(camelCase(operationName)). This addresses the past review feedback about printing an approximation of the correct name.
295-303: LGTM: Consistent with validation.Using
operationNameas-is is correct since the validation at lines 267-279 ensures the name already follows protobuf RPC naming conventions. The optional prefixing logic is preserved for whenprefixOperationTypeis enabled.protographic/src/operations/message-builder.ts (3)
81-88: LGTM: Clear hierarchical path tracking for field numbering.The addition of
parentPathand construction offullPathusing dot notation enables correct field number management for nested messages. This prevents collisions between identically-named nested messages in different hierarchies.
211-229: LGTM: Path-based field numbering prevents collisions.The signature change from
message: protobuf.TypetomessagePath: stringwith optionalmessageenables path-based lookup in the lock manager. This correctly tracks field numbers using the full message hierarchy (e.g.,"GetEmployeeResponse.GetEmployee.Details"), preventing collisions when sibling branches have identically-named nested messages.
360-369: LGTM: Consistent path propagation for nested messages.Passing
currentPathtobuildMessageFromSelectionSetcorrectly propagates the parent path for nested message tracking. The comment clearly explains the intent for lock file operations.protographic/OPERATIONS_TO_PROTO.md (1)
719-720: LGTM: Documentation aligns with implementation.The updated limitation phrasing ("deterministic proto schema generation" and "proto schema generation consistency") accurately reflects the validation semantics implemented in
operation-to-proto.tsand is clearer than the previous "reversibility" terminology.protographic/tests/operations/nested-message-field-numbering.test.ts (1)
1-502: LGTM: Comprehensive test coverage for nested message field numbering.The test suite thoroughly validates the path-based field numbering implementation with excellent coverage:
- Basic nested message field numbering
- Independent numbering for sibling nested messages with identical names
- Field number stability when adding fields (using lockData)
- Reserved numbers for removed fields (backward compatibility)
- Deep nesting (5+ levels) with full dot-notation path tracking
Each test verifies both the generated proto structure and the lockData entries using inline snapshots, ensuring the full message hierarchy is correctly tracked (e.g.,
'GetEmployeeResponse.GetEmployee.Details').router/pkg/connectrpc/samples/services/employee.v1/QueryGetEmployeesByPetsInlineFragment.graphql (1)
1-14: LGTM!The GraphQL query operation is well-formed and correctly uses inline fragments. The structure follows GraphQL conventions and aligns with the ConnectRPC integration's sample operations.
router/pkg/config/config.go (1)
1016-1027: LGTM!The ConnectRPC configuration types are well-structured and follow the existing configuration patterns in the codebase. The use of environment variable tags (
env,envDefault,envPrefix) is consistent and correct.router/pkg/connectrpc/proto_loader_test.go (1)
1-109: LGTM!The test suite for ProtoLoader is well-structured and comprehensive. The tests properly verify service loading, method details, and metadata inspection. The use of a helper function and testify assertions follows Go testing best practices.
router/pkg/connectrpc/connect_util.go (1)
1-80: LGTM! Clean utility functions with well-documented mappings.The HTTP-to-Connect and Connect-to-HTTP status code mappings align with the Connect RPC specification. The asymmetric mappings (e.g., both 413 and 429 →
CodeResourceExhausted, but reverse maps to 429) are intentional and reasonable design choices.router/pkg/connectrpc/vanguard_service_test.go (2)
18-90: Well-structured tests for VanguardService construction.Good coverage of success and error scenarios including nil checks and edge cases.
402-457: Clean table-driven tests for path extraction.Good coverage of edge cases including leading slashes, wrong service names, and malformed paths.
router-tests/connectrpc/connectrpc_client_test.go (2)
59-89: Good h2c setup for gRPC plaintext testing.The HTTP/2 cleartext (h2c) client configuration correctly mimics
grpcurl -plaintextbehavior.
240-296: Solid concurrency test with proper synchronization.Good use of atomic counter, goroutines, and channel for collecting results.
router/pkg/connectrpc/helpers_test.go (1)
17-41: Shared proto loader caching is well-implemented.Good pattern to avoid proto registration conflicts across parallel tests. The mutex properly guards the shared map.
router/pkg/connectrpc/service_discovery_test.go (2)
13-353: Comprehensive service discovery test coverage.Excellent coverage of discovery scenarios including:
- Single/multiple services
- Nested directories with ADR-compliant stop behavior
- One-proto-per-directory enforcement
- Duplicate service detection
- Error cases (missing package, missing service, nonexistent directory)
355-502: Well-structured helper function tests.Good separation of concerns with unit tests for individual helper functions (
findProtoFilesInDir,findOperationFiles,extractPackageFromProto,extractServiceNameFromProto).router-tests/connectrpc/connectrpc_server_lifecycle_test.go (2)
16-52: Good lifecycle test coverage.Tests cover the complete start → reload → stop flow and verify service counts remain consistent.
147-171: Service info consistency test is well-designed.Good verification that service metadata remains stable through the lifecycle.
router/core/router.go (2)
1550-1571: Proper use of Go 1.25'ssync.WaitGroup.Gofor concurrent shutdown.Clean shutdown orchestration with proper nil checks before stopping each component.
2222-2226: NewWithConnectRPCoption follows existing patterns.Consistent with other
With*configuration options in the router.router-tests/connectrpc/connectrpc_test_helpers.go (1)
85-91: LGTM: Clean test server wrapper with idempotent cleanup.The
cleanupDoneflag properly prevents double-cleanup issues, and the struct cleanly encapsulates the server and mock GraphQL server references.router/pkg/connectrpc/proto_loader.go (2)
228-284: Well-designed batch parsing with proper import resolution.The
parseProtoFilesfunction correctly uses the root directory as import path and wraps the resolver with standard imports. This addresses the earlier review concern about per-file parsing breaking common proto import layouts.
118-203: LGTM: Package uniqueness validation is correctly implemented.The logic properly tracks existing services before loading each directory and validates package uniqueness only for newly added services. This addresses the earlier review concern about broken validation logic.
router/pkg/connectrpc/server.go (2)
244-311: LGTM: Reload correctly orders component initialization.The
Reloadmethod properly:
- Checks server is started before proceeding
- Creates fresh
ProtoLoaderbeforeinitializeComponents- Creates new immutable
OperationRegistry- Wraps handler with h2c for HTTP/2 compatibility
This addresses all previous review concerns about initialization order and h2c wrapping.
393-426: LGTM: responseWriter properly implements streaming interfaces.The wrapper correctly delegates to underlying implementations for
Flush,Push, andHijack, returning appropriate errors when not supported. This is essential for gRPC streaming compatibility.router/pkg/connectrpc/vanguard_service.go (3)
260-274: Verify intentional exposure of GraphQL errors to clients.The
writeConnectErrorfunction parses GraphQL errors from metadata and includes them in the response body (errorResponse["graphql_errors"]). This is useful for debugging but exposes internal GraphQL error details to RPC clients.If this is intentional for the bridge use case, consider documenting it. If not, these should be logged only, not returned to clients.
213-230: LGTM: Error sanitization properly implemented.Internal errors are logged with full details (service, method, original error) while clients receive only a generic "internal server error" message. This addresses the previous security concern about leaking internal details.
373-386: LGTM: FileDescriptor deduplication implemented correctly.The
seenmap keyed by file path ensures unique descriptors are returned, addressing the earlier review concern about duplicates when multiple services come from the same proto file.router/pkg/connectrpc/handler.go (3)
277-322: LGTM: Enum handling correctly scoped to _UNSPECIFIED values.The
convertKeysRecursiveWithTrackingfunction properly:
- Tracks fields that became empty from
_UNSPECIFIEDenums viaunspecifiedFieldsmap- Only omits empty strings that are tracked as coming from enum conversion
- Preserves legitimate empty string values
This addresses the previous review concern about broadly omitting all empty strings.
544-551: LGTM: Response body not exposed in client-facing metadata.The HTTP response body is only logged server-side (line 542) and NOT included in the Connect error metadata returned to clients. This addresses the previous security concern about leaking sensitive information.
564-577: LGTM: Data presence check correctly handles empty objects.The check at line 568 now treats
{}as valid data (only checking for"null"string), correctly classifying responses with empty object data as PARTIAL when errors are present. This follows the GraphQL spec where an empty object is valid data.router/pkg/connectrpc/samples/services/employee.v1/service.proto (1)
1-32: LGTM: Well-structured proto service definition.The service correctly uses
idempotency_level = NO_SIDE_EFFECTSfor read operations (queries) and omits it for the mutation (UpdateEmployeeMood). Enum definitions follow proto3 conventions withUNSPECIFIEDas value 0.router-tests/testdata/connectrpc/client/employee.v1/employeev1connect/service.connect.go (1)
1-14: LGTM: Auto-generated Connect client/server bindings.This is generated code from
protoc-gen-connect-go. The file correctly implements the client interface, server handler, and routing for all 7 RPCs defined in the proto. The past concern about CI regeneration was addressed.
Move SkippedHeaders to internal/headers package and remove httputil package
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
router/pkg/connectrpc/handler.go (1)
126-129: Add warning log when defaulting to HTTP protocol.The code silently prepends
http://when no protocol is specified. This could mask configuration issues in production where HTTPS is intended.🔎 Suggested fix
// Ensure the endpoint has a protocol if !strings.Contains(config.GraphQLEndpoint, "://") { + config.Logger.Warn("GraphQL endpoint missing protocol, defaulting to HTTP", + zap.String("original_endpoint", config.GraphQLEndpoint), + zap.String("defaulted_endpoint", "http://"+config.GraphQLEndpoint)) config.GraphQLEndpoint = "http://" + config.GraphQLEndpoint }
🧹 Nitpick comments (2)
router/pkg/connectrpc/handler.go (2)
66-84: Consider consolidating GraphQL response types withmcpserverpackage.The
GraphQLErrorandGraphQLResponsetypes overlap with similar types inrouter/mcpserver/server.go. The types here are more complete (withPath,Locations,Extensionsfields), while the mcpserver versions are simpler.Consider moving these types to a shared package (e.g.,
router/internal/graphql) and having both packages import from there, or defining a more complete canonical type that both can use.
537-542: Consider truncating response body in error logs.Logging the full response body at ERROR level could expose sensitive information (stack traces, internal URLs, auth tokens) in server logs. While server-side logging is safer than client exposure, consider truncating large responses or redacting sensitive patterns.
🔎 Suggested fix
+ // Truncate response body for logging to avoid exposing sensitive info + logBody := string(responseBody) + const maxLogBodyLen = 500 + if len(logBody) > maxLogBodyLen { + logBody = logBody[:maxLogBodyLen] + "...[truncated]" + } + // Log full response body server-side only h.logger.Error("HTTP error from GraphQL endpoint", zap.Int("status_code", resp.StatusCode), zap.String("connect_code", code.String()), zap.Int("response_body_length", len(responseBody)), - zap.String("response_body", string(responseBody))) + zap.String("response_body", logBody))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router/core/header_rule_engine.gorouter/internal/headers/headers.gorouter/pkg/connectrpc/handler.gorouter/pkg/mcpserver/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- router/pkg/mcpserver/server.go
🧰 Additional context used
🧠 Learnings (7)
📓 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/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
📚 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.go
📚 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-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/handler.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/connectrpc/handler.go
🧬 Code graph analysis (2)
router/pkg/connectrpc/handler.go (4)
router/pkg/connectrpc/operation_registry.go (1)
OperationRegistry(14-18)router/internal/headers/headers.go (1)
SkippedHeaders(6-37)router/pkg/connectrpc/connect_util.go (1)
HTTPStatusToConnectCode(11-46)router/pkg/connectrpc/proto_field_options.go (1)
GraphQLVariableNameFieldNumber(36-36)
router/core/header_rule_engine.go (1)
router/core/modules.go (1)
EnginePreOriginHandler(122-126)
⏰ 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 (./events)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
router/internal/headers/headers.go (1)
1-37: LGTM! Well-structured centralized header filter.The new centralized
SkippedHeadersmap is well-documented and uses an efficient data structure for membership checking. The header selection appropriately covers connection-specific, hop-by-hop, content negotiation, and WebSocket headers that should not be forwarded.router/pkg/connectrpc/handler.go (6)
140-192: LGTM!Good error handling with detailed diagnostics. The operation lookup failure logging includes available operations, which aids debugging.
216-271: LGTM!Clean implementation with good graceful degradation for edge cases (nil protoLoader, method not found) and an optimization to skip unnecessary transformations.
392-413: LGTM!The implementation correctly uses
strings.Builderfor efficiency. Note: the byte indexing on line 405 assumes ASCII input, which is valid for proto enum type names (proto identifiers are ASCII-only).
649-685: LGTM!Clean implementation using proto reflection to extract custom field options. The iteration stops early when the target field is found, and nil checks are properly handled.
600-647: LGTM!Good optimization to avoid unnecessary work. The recursive check properly handles nested structures and returns early when transformation is needed.
273-367: LGTM!Well-structured recursive conversion with proper tracking of
_UNSPECIFIEDenum fields. The fix to only omit empty strings that came from enum conversions (not legitimate empty string values) correctly addresses the earlier concern.
| func (h *RPCHandler) makeCriticalGraphQLError(errors []GraphQLError, httpStatus int) error { | ||
| // Serialize GraphQL errors to JSON for metadata | ||
| errorsJSON, _ := json.Marshal(errors) |
There was a problem hiding this comment.
Consider handling json.Marshal error for error metadata.
The json.Marshal error is silently discarded. While marshaling []GraphQLError is unlikely to fail, if it does, the metadata will contain an empty string, which could complicate debugging.
🔎 Suggested fix
func (h *RPCHandler) makeCriticalGraphQLError(errors []GraphQLError, httpStatus int) error {
// Serialize GraphQL 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("[]")
+ }🤖 Prompt for AI Agents
In router/pkg/connectrpc/handler.go around lines 417 to 419,
json.Marshal(errors) is currently ignoring its error return; change it to
capture the error, handle it, and ensure the metadata still contains useful
information: call json.Marshal and check err, if successful use the JSON string,
otherwise log or attach a concise fallback string (e.g., an error message or a
minimal serialized placeholder) and include the marshal error details in the
error metadata so callers/ops can diagnose serialization failures.
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
from
88d8335 to
51962c7
Compare
asoorm
force-pushed
the
ahmet/eng-8277-connect-rpc-handler-final
branch
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.
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
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.