refactor(server/v2): use net/http for matching logic in auto-gateway #23390
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant changes to the Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Handler as ProtoHandler
participant AppManager as Application Manager
participant Gateway as gRPC Gateway
Client->>Handler: Send HTTP Request
Handler->>Handler: Extract Path Parameters
Handler->>Handler: Populate gRPC Message
Handler->>AppManager: Process Request
alt Request Successful
AppManager-->>Handler: Return Response
Handler-->>Client: Send HTTP Response
else Request Failed
Handler->>Gateway: Fallback to Gateway
Gateway-->>Client: Send Error Response
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
No backport label needed for this, all server/v2 components except cometbft server are tagged from main. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
server/v2/api/grpcgateway/handler.go (3)
123-131: Validate and sanitize thex-cosmos-block-heightheader properly.Trimming quotes from
heightStrmay not handle all invalid inputs. Consider usingstrings.TrimSpaceand validating the header value to ensure it's correctly formatted.Apply this diff to enhance header parsing:
heightStr := request.Header.Get(GRPCBlockHeightHeader) -heightStr = strings.Trim(heightStr, `\"`) +heightStr = strings.TrimSpace(heightStr) if heightStr != "" && heightStr != "latest" { height, err = strconv.ParseUint(heightStr, 10, 64) if err != nil { runtime.HTTPError(request.Context(), p.gateway, out, writer, request, status.Errorf(codes.InvalidArgument, "invalid height in header: %s", heightStr)) return } }
29-29: ExportMaxBodySizeor make it configurable for flexibility.Consider exporting
MaxBodySizeor allowing it to be configured, so that it can be adjusted according to application requirements and limitations.Suggestion:
-const MaxBodySize = 1 << 20 // 1 MB +const MaxBodySize = 1 << 20 // 1 MB // Exported for configurationAlternatively, add it to a configuration struct to allow runtime adjustment.
90-100: Add GoDoc comments to exported types for clarity.The exported type
protoHandlerlacks a GoDoc comment. Adding a comment that starts with the type name enhances readability and aligns with Go conventions.Apply this diff to add the comment:
// protoHandler handles turning data in http.Request to the gogoproto.Message +// protoHandler is an HTTP handler that processes incoming HTTP requests and converts them to gRPC messages. type protoHandler[T transaction.Tx] struct {server/v2/api/grpcgateway/doc.go (1)
4-4: Remove extra newline for consistent formatting.There's an unnecessary extra newline after the comment block. Removing it improves the consistency of the code formatting.
// Header `x-cosmos-block-height` allows you to specify a height for the query. // -// // Requests that do not have a dynamic handler will be routed to the canonical gRPC gateway mux.server/v2/api/grpcgateway/server.go (1)
79-81: Update error message to reflect the new functionality.The error message indicates a failure to create the gRPC-Gateway interceptor, but the interceptor has been replaced with
registerGatewayToMux. Update the error message to accurately describe the operation for clarity.Apply this diff to correct the error message:
err := registerGatewayToMux[T](mux, s.GRPCGatewayRouter, appManager) if err != nil { - return nil, fmt.Errorf("failed to create grpc-gateway interceptor: %w", err) + return nil, fmt.Errorf("failed to register gRPC-Gateway handlers: %w", err) }server/v2/api/grpcgateway/handler_test.go (3)
18-45: LGTM! Consider enhancing test case names.The test function is well-structured and covers the essential scenarios for URI pattern conversion. The table-driven test pattern is appropriately used.
Consider making the test case names more specific to better document the behavior:
- name: "replaces catch all", + name: "converts gRPC-gateway catch-all syntax to net/http syntax", - name: "returns original", + name: "returns original URI when no catch-all present", - name: "doesn't tamper with normal wildcard", + name: "preserves normal wildcard patterns unchanged",
47-79: LGTM! Consider adding edge cases.The test cases comprehensively cover the main scenarios for wildcard key extraction.
Consider adding these edge cases to strengthen the test coverage:
{ name: "duplicate wildcard names", uri: "/foo/{bar}/baz/{bar}", want: []string{"bar", "bar"}, }, { name: "wildcard with special characters", uri: "/foo/{bar-baz_123}", want: []string{"bar-baz_123"}, },
Line range hint
81-203: Consider adding more test cases for better coverage.The current test cases are good, but adding these scenarios would improve coverage:
{ name: "malformed JSON body", request: func() *http.Request { return httptest.NewRequest( http.MethodPost, "/dummy", bytes.NewReader([]byte(`{"invalid json`)), ) }, wantErr: true, errCode: codes.InvalidArgument, }, { name: "content-type handling", request: func() *http.Request { req := httptest.NewRequest( http.MethodPost, "/dummy", bytes.NewReader([]byte(`{"foo": "bar"}`)), ) req.Header.Set("Content-Type", "application/json") return req }, expected: &DummyProto{Foo: "bar"}, },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/v2/api/grpcgateway/doc.go(1 hunks)server/v2/api/grpcgateway/handler.go(1 hunks)server/v2/api/grpcgateway/handler_test.go(3 hunks)server/v2/api/grpcgateway/interceptor.go(0 hunks)server/v2/api/grpcgateway/server.go(1 hunks)server/v2/api/grpcgateway/uri.go(0 hunks)server/v2/api/grpcgateway/uri_test.go(0 hunks)
💤 Files with no reviewable changes (3)
- server/v2/api/grpcgateway/uri.go
- server/v2/api/grpcgateway/uri_test.go
- server/v2/api/grpcgateway/interceptor.go
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/api/grpcgateway/doc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/handler_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (2)
server/v2/api/grpcgateway/doc.go (1)
1-2: Complete the package documentation sentence.The package comment appears incomplete, ending abruptly after "match." Please complete the sentence to clearly convey the purpose of the package.
[typographical_issue]
Suggestion:
-// Package grpcgateway provides a custom http mux that utilizes the global gogoproto registry to match -// to create dynamic query handlers. +// Package grpcgateway provides a custom HTTP mux that utilizes the global gogoproto registry +// to match requests and create dynamic query handlers.server/v2/api/grpcgateway/handler_test.go (1)
Line range hint
204-256: LGTM! Test types are well-structured.The helper types properly implement the proto.Message interface and demonstrate good practices for protobuf struct tags.
| // we need to wrap this in a panic handler because cosmos SDK proto stubs contains a duplicate annotation | ||
| // that causes the registration to panic. | ||
| func(u string, qMD queryMetadata) { | ||
| defer func() { | ||
| _ = recover() | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using recover to handle panics during handler registration.
Using recover to handle panics caused by duplicate annotations can mask underlying issues and make debugging difficult. Instead, consider checking for duplicate annotations before registering handlers to prevent panics proactively.
Apply this diff to detect and skip duplicate annotations:
func registerMethods[T transaction.Tx](mux *http.ServeMux, am appmanager.AppManager[T], gateway *runtime.ServeMux, annotationToMetadata map[string]queryMetadata) {
// existing code
for _, uri := range uris {
queryMD := annotationToMetadata[uri]
+ // Check if the URI is already registered
+ if _, patternExists := mux.Handler(&http.Request{URL: &url.URL{Path: uri}}); patternExists {
+ // Log or handle the duplicate annotation as needed
+ return
+ }
mux.Handle(uri, &protoHandler[T]{
msg: queryMD.msg,
gateway: gateway,
appManager: am,
wildcardKeyNames: queryMD.wildcardKeyNames,
})
}
}Committable suggestion skipped: line range outside the PR's diff.
| if req.ContentLength > MaxBodySize { | ||
| return nil, status.Errorf(codes.InvalidArgument, "request body too large: %d bytes, max=%d", req.ContentLength, MaxBodySize) | ||
| } | ||
|
|
||
| // this block of code ensures that the body can be re-read. this is needed as if the query fails in the | ||
| // app's query handler, we need to pass the request back to the canonical gateway, which needs to be able to | ||
| // read the body again. | ||
| bodyBytes, err := io.ReadAll(req.Body) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "%v", err) | ||
| } | ||
| req.Body = io.NopCloser(bytes.NewReader(bodyBytes)) | ||
|
|
There was a problem hiding this comment.
Limit the request body size to prevent potential DoS attacks.
Relying solely on req.ContentLength may not prevent reading excessively large bodies if the Content-Length header is incorrect or missing. Use an io.LimitedReader to enforce MaxBodySize when reading the request body, ensuring that the server doesn't process more data than intended.
Apply this diff to limit the body read size:
if req.ContentLength > 0 {
if req.ContentLength > MaxBodySize {
return nil, status.Errorf(codes.InvalidArgument, "request body too large: %d bytes, max=%d", req.ContentLength, MaxBodySize)
}
- bodyBytes, err := io.ReadAll(req.Body)
+ limitedReader := io.LimitReader(req.Body, MaxBodySize)
+ bodyBytes, err := io.ReadAll(limitedReader)
if err != nil {
return nil, status.Errorf(codes.InvalidArgument, "%v", err)
}
req.Body = io.NopCloser(bytes.NewReader(bodyBytes))
// existing code
}Committable suggestion skipped: line range outside the PR's diff.
| name: "simple query params and body", | ||
| request: func() *http.Request { | ||
| body := `{"denoms": ["hello", "there"]}` | ||
|
|
||
| req := httptest.NewRequest( | ||
| http.MethodGet, | ||
| "/foo", // this doesn't really matter | ||
| bytes.NewReader([]byte(body)), | ||
| ) | ||
| return req | ||
| }, | ||
| wildcardValues: map[string]string{ | ||
| "foo": "wildFooValue", // from path wildcard e.g. /dummy/{foo} | ||
| }, | ||
| expected: &DummyProto{ | ||
| Foo: "wildFooValue", | ||
| Denoms: []string{"hello", "there"}, | ||
| }, | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix HTTP method in body test case.
The test case uses GET method with a request body, which is not a common practice and might be rejected by some HTTP clients. Consider using POST, PUT, or PATCH instead.
- http.MethodGet,
+ http.MethodPost,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "simple query params and body", | |
| request: func() *http.Request { | |
| body := `{"denoms": ["hello", "there"]}` | |
| req := httptest.NewRequest( | |
| http.MethodGet, | |
| "/foo", // this doesn't really matter | |
| bytes.NewReader([]byte(body)), | |
| ) | |
| return req | |
| }, | |
| wildcardValues: map[string]string{ | |
| "foo": "wildFooValue", // from path wildcard e.g. /dummy/{foo} | |
| }, | |
| expected: &DummyProto{ | |
| Foo: "wildFooValue", | |
| Denoms: []string{"hello", "there"}, | |
| }, | |
| wantErr: false, | |
| }, | |
| name: "simple query params and body", | |
| request: func() *http.Request { | |
| body := `{"denoms": ["hello", "there"]}` | |
| req := httptest.NewRequest( | |
| http.MethodPost, | |
| "/foo", // this doesn't really matter | |
| bytes.NewReader([]byte(body)), | |
| ) | |
| return req | |
| }, | |
| wildcardValues: map[string]string{ | |
| "foo": "wildFooValue", // from path wildcard e.g. /dummy/{foo} | |
| }, | |
| expected: &DummyProto{ | |
| Foo: "wildFooValue", | |
| Denoms: []string{"hello", "there"}, | |
| }, | |
| wantErr: false, | |
| }, |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
server/v2/api/grpcgateway/server.go (1)
Line range hint
74-86: Avoid Silently Recovering from Panics During Handler RegistrationIn the
registerMethodsfunction (lines 76-79), you are deferring a function that recovers from panics without handling them. Silently suppressing panics can obscure underlying issues and make debugging difficult. It's better to log the panic or handle it appropriately.Modify the deferred function to log the panic:
func(u string, qMD queryMetadata) { defer func() { - _ = recover() + if r := recover(); r != nil { + s.logger.Error("recovered from panic during handler registration", "uri", u, "error", r) + } }() mux.Handle(u, &protoHandler[T]{ msg: qMD.msg, gateway: gateway, appManager: am, wildcardKeyNames: qMD.wildcardKeyNames, }) }(uri, queryMD)Alternatively, investigate the root cause of the panics (e.g., duplicate annotations) and address them to prevent panics from occurring.
🧹 Nitpick comments (4)
server/v2/api/grpcgateway/doc.go (1)
1-7: Documentation is clear but could be more comprehensive.The package documentation clearly explains the core functionality and important features. However, consider adding more details about:
- The net/http based matching logic and its benefits
- Usage of PopulateFieldFromPath for message field population
- Example usage patterns for common scenarios
Consider expanding the documentation with this additional content:
// Package grpcgateway provides a custom http mux that utilizes the global gogoproto registry to match // to create dynamic query handlers. // // Header `x-cosmos-block-height` allows you to specify a height for the query. // // Requests that do not have a dynamic handler will be routed to the canonical gRPC gateway mux. +// +// The package uses net/http router for efficient and safe URL pattern matching, improving upon the +// previous regex-based approach. Message fields are populated using PopulateFieldFromPath, ensuring +// correct handling of URL escaped parameters. +// +// Example usage: +// mux := grpcgateway.NewServeMux(client) +// http.ListenAndServe(":8080", mux) package grpcgatewayserver/v2/api/grpcgateway/handler_test.go (1)
Line range hint
81-203: Solid test coverage for message population!The test cases effectively cover query parameters, body content, error handling, and wildcard values.
Consider adding these test cases to improve coverage:
- POST request with different content types
- Additional error scenarios:
- Invalid JSON body
- Conflicting query parameters and body fields
- Missing required fields
server/v2/api/grpcgateway/handler.go (2)
122-131: Refactor Block Height Parsing into a Utility FunctionThe logic for parsing the
GRPCBlockHeightHeader(lines 123-131) could be reused elsewhere. Extracting this logic into a utility function enhances readability and maintainability.Example of extracting the parsing logic:
func parseBlockHeight(headerValue string) (uint64, error) { heightStr := strings.Trim(headerValue, `\"`) if heightStr == "" || heightStr == "latest" { return 0, nil } height, err := strconv.ParseUint(heightStr, 10, 64) if err != nil { return 0, fmt.Errorf("invalid height in header: %s", headerValue) } return height, nil }And update the code:
var height uint64 -heightStr := request.Header.Get(GRPCBlockHeightHeader) -heightStr = strings.Trim(heightStr, `\"`) -if heightStr != "" && heightStr != "latest" { - height, err = strconv.ParseUint(heightStr, 10, 64) - if err != nil { - runtime.HTTPError(request.Context(), p.gateway, out, writer, request, status.Errorf(codes.InvalidArgument, "invalid height in header: %s", heightStr)) - return - } -} +height, err := parseBlockHeight(request.Header.Get(GRPCBlockHeightHeader)) +if err != nil { + runtime.HTTPError(request.Context(), p.gateway, out, writer, request, status.Errorf(codes.InvalidArgument, "%v", err)) + return +}
170-183: Limit Request Body Size Usinghttp.MaxBytesReaderChecking
req.ContentLengthto enforceMaxBodySize(lines 171-172) may not be reliable, as clients might not setContent-Lengthcorrectly. Usehttp.MaxBytesReaderto enforce maximum body size and prevent resource exhaustion.Modify the code to wrap
req.Body:if req.ContentLength > 0 { - if req.ContentLength > MaxBodySize { - return nil, status.Errorf(codes.InvalidArgument, "request body too large: %d bytes, max=%d", req.ContentLength, MaxBodySize) - } - - // this block of code ensures that the body can be re-read. - bodyBytes, err := io.ReadAll(req.Body) + req.Body = http.MaxBytesReader(nil, req.Body, MaxBodySize) + bodyBytes, err := io.ReadAll(req.Body) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "%v", err) } + req.Body = io.NopCloser(bytes.NewReader(bodyBytes))This ensures that the body cannot exceed
MaxBodySize, regardless ofContent-Length.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/v2/api/grpcgateway/doc.go(1 hunks)server/v2/api/grpcgateway/handler.go(1 hunks)server/v2/api/grpcgateway/handler_test.go(3 hunks)server/v2/api/grpcgateway/interceptor.go(0 hunks)server/v2/api/grpcgateway/server.go(1 hunks)server/v2/api/grpcgateway/uri.go(0 hunks)server/v2/api/grpcgateway/uri_test.go(0 hunks)
💤 Files with no reviewable changes (3)
- server/v2/api/grpcgateway/uri.go
- server/v2/api/grpcgateway/uri_test.go
- server/v2/api/grpcgateway/interceptor.go
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/api/grpcgateway/doc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/handler_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (5)
server/v2/api/grpcgateway/handler_test.go (3)
18-45: Well-structured test cases for catch-all pattern handling!The test cases comprehensively cover the transformation of catch-all patterns while maintaining other URI patterns unchanged.
47-79: Comprehensive test coverage for wildcard key extraction!The test cases effectively cover all wildcard scenarios including single, multiple, catch-all, and no wildcards.
Line range hint
204-256: Clean implementation of test types!The dummy proto types provide a good foundation for testing various message scenarios.
server/v2/api/grpcgateway/handler.go (2)
296-297: Ensure Correct Handling of Wildcard Key NamesIn the
extractWildcardKeyNamesfunction (lines 296-297), trimming dots from the parameter names may not handle all edge cases, such as parameter names that legitimately end with dots. Verify that this approach correctly extracts wildcard key names without affecting valid parameter names.Consider adding unit tests to cover scenarios with complex URI patterns and ensure that parameter names are extracted accurately.
184-186: Handle Empty Request Body Errors AppropriatelyWhen decoding the request body (line 184), if the body is empty,
Decodemay returnio.EOF. Currently, you return an error unless the error is exactlyio.EOF. Consider allowing empty bodies if acceptable for the message type.Ensure that an empty request body is valid for the expected
inputmessage. If so, adjust the error handling:if err = marshaler.NewDecoder(bytes.NewReader(bodyBytes)).Decode(input); err != nil { - if !errors.Is(err, io.EOF) { - return nil, status.Errorf(codes.InvalidArgument, "%v", err) - } + return nil, status.Errorf(codes.InvalidArgument, "%v", err) }Alternatively, if
io.EOFis acceptable, you can ignore it:if err != nil && !errors.Is(err, io.EOF) { return nil, status.Errorf(codes.InvalidArgument, "%v", err) }✅ Verification successful
Current Implementation is Correct
The current error handling approach matches the standard pattern used throughout the gRPC-gateway codebase. The code correctly allows empty request bodies while still catching other decoding errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find the complete handler function containing this code ast-grep --pattern 'func $_($_, $_) ($_, error) { $$$ if err = marshaler.NewDecoder(bytes.NewReader(bodyBytes)).Decode(input); err != nil && !errors.Is(err, io.EOF) { return nil, status.Errorf(codes.InvalidArgument, "%v", err) } $$$ }' # Look for similar error handling patterns rg 'marshaler.*Decode.*err.*io\.EOF' -A 2 -B 2 # Find the type declaration of input ast-grep --pattern 'func $_($_, $_ *$_) ($_, error)'Length of output: 5311
| err := registerGatewayToMux[T](mux, s.GRPCGatewayRouter, appManager) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Error Message to Reflect Functionality Change
The error message on line 80 still references "failed to create grpc-gateway interceptor," but the code now uses registerGatewayToMux instead. Update the error message to accurately describe the failure.
Apply this diff to correct the error message:
if err != nil {
- return nil, fmt.Errorf("failed to create grpc-gateway interceptor: %w", err)
+ return nil, fmt.Errorf("failed to register gRPC gateway to mux: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := registerGatewayToMux[T](mux, s.GRPCGatewayRouter, appManager) | |
| if err != nil { | |
| err := registerGatewayToMux[T](mux, s.GRPCGatewayRouter, appManager) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to register gRPC gateway to mux: %w", err) | |
| } |
| // we need to wrap this in a panic handler because cosmos SDK proto stubs contains a duplicate annotation | ||
| // that causes the registration to panic. | ||
| func(u string, qMD queryMetadata) { | ||
| defer func() { | ||
| _ = recover() | ||
| }() | ||
| mux.Handle(u, &protoHandler[T]{ | ||
| msg: qMD.msg, | ||
| gateway: gateway, | ||
| appManager: am, | ||
| wildcardKeyNames: qMD.wildcardKeyNames, | ||
| }) | ||
| }(uri, queryMD) |
There was a problem hiding this comment.
Avoid Silently Recovering from Panics in Registration Loop
In the registerMethods function (lines 76-86), the deferred function recovers from panics without any handling (line 78). Silently recovering from panics hides errors and makes troubleshooting difficult. It's advisable to log the panic or handle it to ensure issues are not ignored.
Consider logging the panic for better error visibility:
func(u string, qMD queryMetadata) {
defer func() {
- _ = recover()
+ if r := recover(); r != nil {
+ fmt.Printf("Recovered from panic while registering handler for URI %s: %v\n", u, r)
+ }
}()
mux.Handle(u, &protoHandler[T]{
msg: qMD.msg,
gateway: gateway,
appManager: am,
wildcardKeyNames: qMD.wildcardKeyNames,
})
}(uri, queryMD)Or, prevent the panic by checking for duplicates before registration.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
server/v2/api/grpcgateway/doc.go (1)
1-6: Enhance package documentation for clarityThe current package documentation provides minimal information about the
grpcgatewaypackage's functionality. To improve understandability for other developers, consider expanding the documentation to include:
- An overview of how the package creates dynamic query handlers.
- Details on how it utilizes the global gogoproto registry.
- Explanation of request routing and handling logic.
This will help maintain clarity, especially after significant changes to the package's implementation.
server/v2/api/grpcgateway/server.go (1)
79-82: Update error message to reflect new functionThe error message references 'failed to create grpc-gateway interceptor', but the code now uses
registerGatewayToMux. Updating the error message will provide clarity and accurate context in case of errors.Apply this diff to correct the error message:
err := registerGatewayToMux[T](mux, s.GRPCGatewayRouter, appManager) if err != nil { - return nil, fmt.Errorf("failed to create grpc-gateway interceptor: %w", err) + return nil, fmt.Errorf("failed to register gateway to mux: %w", err) }server/v2/api/grpcgateway/handler_test.go (4)
18-45: Add test cases for edge scenarios.While the current test cases cover basic scenarios, consider adding cases for:
- Multiple catch-all patterns in the same URI
- Empty URI
- URI with special characters
tests := []struct { name string uri string want string }{ + { + name: "multiple catch-alls", + uri: "/foo/{bar=**}/baz/{qux=**}", + want: "/foo/{bar...}/baz/{qux...}", + }, + { + name: "empty uri", + uri: "", + want: "", + }, + { + name: "special characters", + uri: "/foo/@{bar=**}/%20", + want: "/foo/@{bar...}/%20", + },
47-79: Add test case for invalid wildcard pattern.Consider adding a test case for malformed wildcard patterns to ensure robust error handling:
tests := []struct { name string uri string want []string }{ + { + name: "malformed wildcard", + uri: "/foo/{bar/baz}", + want: nil, + },
128-129: Reconsider the comment about URL not mattering.The comment "this doesn't really matter" might be misleading. The URL could affect request parsing in some cases. Consider either:
- Using a more realistic URL that matches the expected pattern
- Adding a comment explaining why the URL truly doesn't matter in this context
180-180: Consider expanding test coverage with a more complete handler setup.The current minimal setup might miss edge cases. Consider adding test cases that:
- Test concurrent request handling
- Verify behavior with different handler configurations
- Test edge cases that might depend on handler state
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/v2/api/grpcgateway/doc.go(1 hunks)server/v2/api/grpcgateway/handler.go(1 hunks)server/v2/api/grpcgateway/handler_test.go(3 hunks)server/v2/api/grpcgateway/interceptor.go(0 hunks)server/v2/api/grpcgateway/server.go(1 hunks)server/v2/api/grpcgateway/uri.go(0 hunks)server/v2/api/grpcgateway/uri_test.go(0 hunks)
💤 Files with no reviewable changes (3)
- server/v2/api/grpcgateway/uri_test.go
- server/v2/api/grpcgateway/uri.go
- server/v2/api/grpcgateway/interceptor.go
🧰 Additional context used
📓 Path-based instructions (4)
server/v2/api/grpcgateway/doc.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/handler_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpcgateway/handler.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: main
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
server/v2/api/grpcgateway/handler.go (1)
68-70: Verify compatibility with Go version for 'maps' and 'slices' packagesThe code utilizes
maps.Keysandslices.Sorted, which are part of the standard library starting from Go 1.21. Please verify that the project is set to build with Go 1.21 or higher to ensure compatibility and avoid build errors.server/v2/api/grpcgateway/handler_test.go (1)
186-191: Add test cases for concurrent message population.The
populateMessagefunction might be called concurrently in production. Consider adding test cases that verify thread safety:
| defer func() { | ||
| _ = recover() | ||
| }() | ||
| mux.Handle(u, &protoHandler[T]{ |
There was a problem hiding this comment.
Avoid swallowing panics without logging
The deferred recover in the anonymous function silently swallows any panic without logging or handling it. This can obscure critical issues and make debugging difficult. It's important to log the panic to aid in diagnosis and ensure that unexpected errors are not missed.
You can modify the defer function to log the panic:
func(u string, qMD queryMetadata) {
defer func() {
- _ = recover()
+ if r := recover(); r != nil {
+ s.logger.Error("Recovered from panic in handler registration", "error", r)
+ }
}()
mux.Handle(u, &protoHandler[T]{
msg: qMD.msg,
gateway: gateway,
appManager: am,
wildcardKeyNames: qMD.wildcardKeyNames,
})
}(uri, queryMD)Ensure that you have access to the logger s.logger in this context or pass a logger to the function.
Committable suggestion skipped: line range outside the PR's diff.
| "io" | ||
| "maps" | ||
| "net/http" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
| for pathKey, pathValue := range pathParams { | ||
| if err := runtime.PopulateFieldFromPath(input, pathKey, pathValue); err != nil { | ||
| return nil, status.Error(codes.InvalidArgument, fmt.Errorf("failed to populate field %s with value %s: %w", pathKey, pathValue, err).Error()) | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for k := range pathParams { | ||
| prefixPaths = append(prefixPaths, []string{k}) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for uri, queryInputName := range annotations { | ||
| // extract the proto message type. | ||
| msgType := gogoproto.MessageType(queryInputName) | ||
| if msgType == nil { | ||
| continue | ||
| } | ||
| msg, ok := reflect.New(msgType.Elem()).Interface().(gogoproto.Message) | ||
| if !ok { | ||
| return nil, fmt.Errorf("query input type %q does not implement gogoproto.Message", queryInputName) | ||
| } | ||
| annotationToMetadata[uri] = queryMetadata{ | ||
| msg: msg, | ||
| wildcardKeyNames: extractWildcardKeyNames(uri), | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
Description
Closes: N/A
found a bug with the regex matching logic related to URL escaped parameters, and decided to just think of a different way to handle this. switched everything to use net/http's router. should be more performant and safe.
PopulateFieldFromPathto populate message fields instead of mapstructure decoding from a mapAuthor Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Documentation
Refactor
Removed
interceptor.goanduri.gofilesNew Features