feat(server/v2)!: grpcgateway autoregistration #22941
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to the gRPC gateway functionality in the Cosmos SDK server package. The changes focus on creating a more flexible and automated approach to routing gRPC gateway queries. A new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
github-actions
bot
added
C:x/authz
C:server/v2
technicallyty
changed the title
|
@julienrbrt any suggestion on how to test this? i was hoping there were tests i could pattern match from the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
server/v2/api/grpcgateway/uri.go
Outdated
| for getPattern, queryInputName := range getPatternToQueryInputName { | ||
| getPattern = strings.TrimRight(getPattern, "/") | ||
|
|
||
| regexPattern, wildcardNames := patternToRegex(getPattern) | ||
|
|
||
| regex := regexp.MustCompile(regexPattern) | ||
| matches := regex.FindStringSubmatch(uri) | ||
|
|
||
| if matches != nil && len(matches) > 1 { | ||
| // first match is the full string, subsequent matches are capture groups | ||
| params := make(map[string]string) | ||
| for i, name := range wildcardNames { | ||
| params[name] = matches[i+1] | ||
| } | ||
|
|
||
| return &uriMatch{ | ||
| QueryInputName: queryInputName, | ||
| Params: params, | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
As you disabled the manual registration in simapp/v2, this is already tested. We have gRPC gateway system tests, and if they are passing, this means it is working 👌🏾 |
|
|
||
| for _, mod := range deps.ModuleManager.Modules() { | ||
| if gmod, ok := mod.(module.HasGRPCGateway); ok { | ||
| gmod.RegisterGRPCGatewayRoutes(deps.ClientContext, server.GRPCGatewayRouter) |
There was a problem hiding this comment.
A nice follow-up of this is to start de-wire grpc gateway from module on main, by removing
version: v1
plugins:
- name: gocosmos
out: ..
opt: plugins=grpc,Mgoogle/protobuf/any.proto=github.com/cosmos/gogoproto/types/any
- - name: grpc-gateway
- out: ..
- opt: logtostderr=true,allow_colon_final_segments=trueAnd remove all generated *.gw.go from modules and RegisterGRPCGatewayRoutes.
It will need to wait 1. of #22904 but that's very soon.
| for key, value := range match.Params { | ||
| parts := strings.Split(key, ".") | ||
| current := nestedParams | ||
|
|
||
| // step through nested levels | ||
| for i, part := range parts { | ||
| if i == len(parts)-1 { | ||
| // Last part - set the value | ||
| current[part] = value | ||
| } else { | ||
| // continue nestedness | ||
| if _, exists := current[part]; !exists { | ||
| current[part] = make(map[string]any) | ||
| } | ||
| current = current[part].(map[string]any) | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| query, err := g.appManager.Query(request.Context(), height, msg) | ||
| if err != nil { | ||
| // if we couldn't find a handler for this request, just fall back to the gateway mux. | ||
| if errors.Is(err, stf.ErrNoHandler) { |
There was a problem hiding this comment.
Not sure if we want to have stf package imported. I can just do something like strings.Contains(err.Error(), "no handler") instead if this is not good.
technicallyty
requested review from
a team,
kocubinski and
hieuvubk
as code owners
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
server/v2/api/grpcgateway/uri_test.go (3)
15-92: Consider adding more edge cases to TestMatchURIWhile the test cases are comprehensive for valid scenarios, consider adding:
- Test case for invalid URL parsing
- Test case for malformed wildcard patterns (e.g.,
{foo=**/*})- Test case for overlapping wildcard patterns
testCases := []struct { name string uri string mapping map[string]string expected *uriMatch }{ + { + name: "invalid URL", + uri: "http://[::1]:8080:invalid", + mapping: map[string]string{"/foo/bar": "query.Bank"}, + expected: nil, + }, + { + name: "malformed wildcard pattern", + uri: "https://localhost:8080/foo/bar", + mapping: map[string]string{"/foo/{bar=**/*}": "query.Bank"}, + expected: nil, + },
94-100: Add nil map test case to TestURIMatch_HasParamsAdd a test case to verify behavior when Params is nil.
func TestURIMatch_HasParams(t *testing.T) { u := uriMatch{Params: map[string]string{"foo": "bar"}} require.True(t, u.HasParams()) u = uriMatch{} require.False(t, u.HasParams()) + + u = uriMatch{Params: nil} + require.False(t, u.HasParams()) }
102-125: Enhance DummyProto implementationThe DummyProto implementation could be more robust:
- String() method should return a meaningful string representation
- Reset() method should actually reset the fields
-func (d DummyProto) Reset() {} +func (d *DummyProto) Reset() { + *d = DummyProto{} +} -func (d DummyProto) String() string { return dummyProtoName } +func (d DummyProto) String() string { + return fmt.Sprintf("DummyProto{Foo: %s, Bar: %v, Baz: %d}", d.Foo, d.Bar, d.Baz) +}server/v2/api/grpcgateway/server.go (2)
41-41: Enhance documentation for the new parameter.
The added parameter “appManager” significantly alters the function signature. Consider elaborating on its usage in the function docstring so that new contributors quickly understand its role.
142-143: Use standard naming conventions for gRPC metadata constants.
Consider prefixing the constant with a recognized prefix (e.g., “Grpc” or “GRPC”), or confirm it's consistent with any existing naming patterns from the rest of the codebase.server/v2/api/grpcgateway/interceptor.go (4)
24-37: Ensure that gatewayInterceptor’s fields are documented.
Although there are short comments, we might consider more elaboration about concurrency and usage patterns, especially for public fields or ones that can be accessed outside the package.
39-51: Handle partial failures while building customEndpointMapping.
The current logic returns an error if getHTTPGetAnnotationMapping() fails entirely. If partial failures or missing proto definitions are possible, consider how best to proceed to avoid shutting down the entire server.
83-85: Check large header inputs.
When parsing block height in the header, consider if large or malicious values could impact the system. You may want to bound-check the height.
95-105: Log fallback for clarity.
When no handler is found in the app manager, you fall back to the gateway. Consider logging this scenario for troubleshooting.server/v2/api/grpcgateway/uri.go (3)
20-31: Improve docstring for Params usage.
The field “Params” is central to the dynamic message creation. It might be helpful to clarify that the keys come from both path wildcards and query parameters.
38-85: Consider partial or incremental matching logic.
matchURL uses a simple loop with direct regex compilation. If the mapping grows large, this might become a bottleneck. Investigate indexing strategies or precomputed structures for better performance.
114-134: Validate JSON input size or structure.
Use caution under large or malformed JSON bodies, since you only limit the size to 1MB. This might still be large enough for certain denial-of-service conditions. If feasible, consider streaming or chunked processing if extremely large payloads are expected.server/v2/go.mod (1)
37-37: Pin dependency version carefully.
The module google.golang.org/genproto/googleapis/api is pinned to a pseudo-version. If an officially tagged version is available, consider using that to avoid potential build reproducibility issues.CHANGELOG.md (3)
Line range hint
1-1: Add version table of contents for better navigationConsider adding a table of contents at the start of the changelog to help users quickly navigate to specific versions.
# Changelog ## Table of Contents - [v0.47.0](#v0470) - [v0.46.x](#v046x) - [v0.45.x](#v045x) ...
47-47: Fix typo in changelog entryChange "feat(server/v2)!: grpcgateway autoregistration" to "feat(server/v2)!: gRPC gateway auto-registration" for better readability and consistent capitalization.
Line range hint
1-2000: Improve version header formatting consistencyThe version headers have inconsistent formatting - some use H2 (##) while others use H3 (###). Consider standardizing all version headers to H2 level for better visual hierarchy.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)server/v2/api/grpcgateway/interceptor.go(1 hunks)server/v2/api/grpcgateway/server.go(4 hunks)server/v2/api/grpcgateway/uri.go(1 hunks)server/v2/api/grpcgateway/uri_test.go(1 hunks)server/v2/go.mod(2 hunks)simapp/v2/simdv2/cmd/commands.go(1 hunks)tests/systemtests/distribution_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
tests/systemtests/distribution_test.go (3)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
server/v2/api/grpcgateway/uri_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"
simapp/v2/simdv2/cmd/commands.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/uri.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
server/v2/api/grpcgateway/interceptor.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (10)
server/v2/api/grpcgateway/uri_test.go (1)
1-13: LGTM: Clean and well-organized imports
The imports are properly organized and all are being used within the test file.
server/v2/api/grpcgateway/server.go (2)
17-17: Check import usage.
The imported package "cosmossdk.io/server/v2/appmanager" is used for the newly introduced parameter. Ensure the module “appmanager” is needed only in this file to avoid extraneous imports.
✅ Verification successful
Let me analyze the results. The rg search shows that the appmanager package is used in multiple components of the codebase:
- In server/v2/api/grpcgateway/server.go (the file under review) for gateway server implementation
- In server/v2/api/rest/server.go for REST server implementation
- In server/v2/cometbft for CometBFT server implementation
- In runtime/v2 for app runtime implementation
Let me verify the actual usage pattern in the file under review to ensure it's necessary.
The appmanager import is correctly used and necessary in this file
The appmanager package is legitimately used in the gRPC gateway server implementation as it's required for the server's core functionality. The import is used to define the AppManager field in the Server struct and is passed as a parameter in the server's constructor. This is consistent with the architecture where other server components (REST, CometBFT) also use the same package for app management functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm usage of appmanager in server/v2/api/grpcgateway/server.go only
rg 'appmanager' -A 5
Length of output: 15189
Script:
#!/bin/bash
# Check how appmanager is used in the file under review
rg -A 10 'appmanager' server/v2/api/grpcgateway/server.go
Length of output: 778
81-85: Confirm concurrency safety.
The interceptor is created once and is shared across requests. Verify that no shared state in the interceptor is modified unsafely by multiple concurrent ServeHTTP calls.
✅ Verification successful
Let me check the implementation of the interceptor's ServeHTTP method to verify concurrency safety.
Let me try a different approach to check the implementation.
The interceptor implementation is concurrency safe
After reviewing the implementation, I can confirm that the interceptor is concurrency safe because:
- The
customEndpointMappingis initialized once during creation and is never modified during request handling - All other fields (
logger,gateway,appManager) are read-only during request handling - The
ServeHTTPmethod only uses local variables for request processing and doesn't modify any shared state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Grep for shared variables in gatewayInterceptor that might be mutated
rg -A 8 'type gatewayInterceptor'
Length of output: 815
Script:
#!/bin/bash
# Check the ServeHTTP implementation and any methods that might modify shared state
ast-grep --pattern 'func (i *gatewayInterceptor[$_]) ServeHTTP($_) {
$$$
}'
# Also check if there are any methods that modify the customEndpointMapping
rg -A 5 'customEndpointMapping\[.*\]'
Length of output: 119
Script:
#!/bin/bash
# Check the entire interceptor.go file to see the implementation
cat server/v2/api/grpcgateway/interceptor.go
# Also check for any modifications to the map in the codebase
rg -g '*.go' 'customEndpointMapping.*='
Length of output: 5043
server/v2/api/grpcgateway/interceptor.go (2)
69-82: 🛠️ Refactor suggestion
Optimize regex usage.
Compiling new regex objects in the request path may affect performance if done very frequently (e.g., high-traffic scenarios). Caching compiled regex can boost throughput.
-for getPattern, queryInputName := range getPatternToQueryInputName {
- regexPattern, wildcardNames := patternToRegex(getPattern)
- regex := regexp.MustCompile(regexPattern)
- ...
+var compiledRegexes = map[string]*regexp.Regexp{}
+for getPattern, queryInputName := range getPatternToQueryInputName {
+ regexPattern, wildcardNames := patternToRegex(getPattern)
+ if _, exists := compiledRegexes[regexPattern]; !exists {
+ compiledRegexes[regexPattern] = regexp.MustCompile(regexPattern)
+ }
+ regex := compiledRegexes[regexPattern]
+ ...Likely invalid or redundant comment.
53-63: Confirm fall-back behavior is acceptable.
When the request URL doesn’t match customEndpointMapping, the interceptor falls back to the standard gRPC gateway. Verify that this transition covers all edge cases, including malformed paths or queries.
✅ Verification successful
Let me gather more information about the URL matching and error handling logic.
Fall-back behavior is properly implemented and secure
The implementation shows a robust handling of unmatched URLs with proper error management:
- The
matchURLfunction thoroughly processes URLs, handling both exact matches and wildcard patterns - Unmatched requests cleanly fall back to the standard gateway with proper error handling via
DefaultHTTPProtoErrorHandler - The system handles malformed paths through regex validation and proper URL sanitization (trimming trailing slashes)
- Error cases are consistently handled across the gateway implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Spot-check fallback usage in the codebase to confirm there's no mismatch
rg 'ServeHTTP' -A 5
Length of output: 2220
Script:
#!/bin/bash
# Check matchURL implementation and customEndpointMapping usage
ast-grep --pattern 'func matchURL($_, $_) $_'
# Check error handling patterns
rg 'DefaultHTTPProtoErrorHandler' -A 3
Length of output: 4832
server/v2/api/grpcgateway/uri.go (1)
136-187: Watch for collision in multi-level params.
The code merges all param keys into a nested structure. If user input can introduce repeated param keys, it might cause collisions or unexpected overwrites.
server/v2/go.mod (1)
18-18: Confirm usage of server/v2/stf.
You’ve declared a pseudo-version for stf. Ensure that it is indeed being used in this module. If stable tags are available, referencing them may simplify dependency management.
✅ Verification successful
STF package is actively used throughout the codebase
The server/v2/stf package is extensively used across multiple components:
- Core runtime components in
runtime/v2/for router services, store services, and app initialization - Integration tests in
tests/integration/v2/ - App management in
server/v2/appmanager - CometBFT ABCI implementation
- gRPC gateway interceptors
The pseudo-version is being actively used and is a critical dependency. However, for better dependency management, you may want to use a stable tag if available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm references to stf in the entire server/v2 subtree
fd --extension go --exec rg 'server/v2/stf'
Length of output: 1105
Script:
#!/bin/bash
# Let's find the actual Go files that import and use the stf package
rg -l "cosmossdk.io/server/v2/stf" --type go
# Also check for any actual usage patterns of stf
rg "stf\." --type go -A 2 -B 2
Length of output: 11840
simapp/v2/simdv2/cmd/commands.go (1)
159-159: Breaking change: AppManager parameter added to grpcgateway.New
The addition of AppManager parameter aligns with the PR objectives of implementing HTTP handler routing for grpc-gateway requests to the application manager's query router.
Let's verify the impact of this breaking change:
✅ Verification successful
Breaking change verified: AppManager parameter added to grpcgateway.New
The verification confirms that grpcgateway.New has been updated to include the AppManager parameter, and all its usages in the codebase have been properly updated:
- In
simapp/v2/simdv2/cmd/commands.go: The call includessimApp.App.AppManager - In
simapp/v2/simdv2/cmd/testnet.go: Uses the factory methodNewWithConfigOptionswhich internally handles the new parameter
The implementation in server/v2/api/grpcgateway/server.go shows that the AppManager parameter is used to create a gateway interceptor, aligning with the PR's objective of implementing HTTP handler routing for grpc-gateway requests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to grpcgateway.New to ensure they're updated
rg -A 2 "grpcgateway\.New\[.*\]"
Length of output: 211
Script:
#!/bin/bash
# Let's check the implementation of grpcgateway.New to understand the breaking change
ast-grep --pattern 'func New[$_]($$$) ($$$)'
Length of output: 22404
Script:
#!/bin/bash
# Let's check for any other instances of grpcgateway.New to ensure all calls are updated
rg -B 5 -A 5 "grpcgateway\.New"
Length of output: 1609
tests/systemtests/distribution_test.go (1)
182-183: Improved error messages for invalid height parameters
The error messages now clearly indicate whether the parsing failed for starting_height or ending_height, making it easier to identify the source of validation failures.
Let's verify the consistency of error messages across similar endpoints:
Also applies to: 190-190, 196-196
✅ Verification successful
Error message format is consistent across height validations
Based on the search results, the error message format for height validation is consistent across the codebase:
- The test cases in
distribution_test.gouse the standard format for bothstarting_heightandending_heightvalidation errors - Other height validations in the codebase (in
server/v2/api/grpcgateway/interceptor.goandserver/v2/api/grpc/server.go) follow a similar pattern of usingstrconv.ParseUintfor validation and providing clear error messages that identify the specific height parameter that failed validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar height parameter validations
ast-grep --pattern 'cannot parse $_height as uint: strconv.ParseUint'
Length of output: 71
Script:
#!/bin/bash
# Let's search for error messages related to height validation using ripgrep
rg -i "height.*strconv\.ParseUint|ParseUint.*height" -A 2 -B 2
Length of output: 2763
Script:
#!/bin/bash
# Search for test files containing height validation tests
fd ".*test.*\.go$" --exec rg -l "height.*invalid"
Length of output: 127
Script:
#!/bin/bash
# Search for error message strings containing height validation
rg "cannot parse.*height" -A 2 -B 2
Length of output: 804
CHANGELOG.md (1)
Line range hint 1-2000: LGTM on overall changelog structure and content
The changelog follows good practices by:
- Clearly separating breaking vs non-breaking changes
- Including links to relevant PRs/issues
- Providing migration guides where needed
- Organizing changes by module/component
| // patternToRegex converts a URI pattern with wildcards to a regex pattern. | ||
| // Returns the regex pattern and a slice of wildcard names in order | ||
| func patternToRegex(pattern string) (string, []string) { | ||
| escaped := regexp.QuoteMeta(pattern) | ||
| var wildcardNames []string | ||
|
|
||
| // extract and replace {param=**} patterns | ||
| r1 := regexp.MustCompile(`\\\{([^}]+?)=\\\*\\\*\\}`) | ||
| escaped = r1.ReplaceAllStringFunc(escaped, func(match string) string { | ||
| // extract wildcard name without the =** suffix | ||
| name := regexp.MustCompile(`\\\{(.+?)=`).FindStringSubmatch(match)[1] | ||
| wildcardNames = append(wildcardNames, name) | ||
| return "(.+)" | ||
| }) | ||
|
|
||
| // extract and replace {param} patterns | ||
| r2 := regexp.MustCompile(`\\\{([^}]+)\\}`) | ||
| escaped = r2.ReplaceAllStringFunc(escaped, func(match string) string { | ||
| // extract wildcard name from the curl braces {}. | ||
| name := regexp.MustCompile(`\\\{(.*?)\\}`).FindStringSubmatch(match)[1] | ||
| wildcardNames = append(wildcardNames, name) | ||
| return "([^/]+)" | ||
| }) | ||
|
|
||
| return "^" + escaped + "$", wildcardNames | ||
| } |
There was a problem hiding this comment.
Validate wildcard usage.
Ensure that special characters in wildcard expansions do not cause misrouting or potential security concerns in parameter injection, particularly if these param values are used downstream without sanitization.
server/v2/api/grpcgateway/server.go
Outdated
There was a problem hiding this comment.
nit, can you remove the // TODO: register the gRPC-Gateway routes from this file? As this does that.
There was a problem hiding this comment.
done in remove todo
| query, err := g.appManager.Query(request.Context(), height, msg) | ||
| if err != nil { | ||
| // if we couldn't find a handler for this request, just fall back to the gateway mux. | ||
| if errors.Is(err, stf.ErrNoHandler) { |
server/v2/go.mod
Outdated
| @@ -15,6 +15,7 @@ require ( | |||
| cosmossdk.io/core/testing v0.0.1 | |||
| cosmossdk.io/log v1.5.0 | |||
| cosmossdk.io/server/v2/appmanager v0.0.0-00010101000000-000000000000 | |||
| cosmossdk.io/server/v2/stf v0.0.0-00010101000000-000000000000 | |||
There was a problem hiding this comment.
CHANGELOG.md
Outdated
| @@ -44,6 +44,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
|
|
|||
| * (baseapp) [#20291](https://github.com/cosmos/cosmos-sdk/pull/20291) Simulate nested messages. | |||
| * (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input. | |||
| * (server/v2) [#22715](https://github.com/cosmos/cosmos-sdk/pull/22941) Add custom HTTP handler for grpc-gateway that removes the need to manually register grpc-gateway services. | |||
There was a problem hiding this comment.
Could oyu move this to server/v2/CHANGELOG.md? (We don't need the server/v2 prefix then)
There was a problem hiding this comment.
done in add changelog to server/v2!
also question: given this broke the grpcgateway.New function, should i add a line to the API Breaking section?
There was a problem hiding this comment.
it's beta so it is fine as is 👍🏾
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/v2/api/grpcgateway/interceptor.go (3)
39-50: Validate error handling for getHTTPGetAnnotationMapping.
The function gracefully returns an error if the annotation mapping retrieval fails. This approach is good, but ensure calling code logs relevant context for debugging.
83-92: Check user-supplied block height.
Great validation of user-supplied block height. Make sure to handle negative or extremely large values gracefully.
94-104: Brace for potential error types.
Falling back to the gateway mux when "no handler" is found is appropriate. For more robust error handling, consider typed errors rather than matching strings.server/v2/api/grpcgateway/server.go (1)
79-83: Interceptor creation is well-placed.
This intercepts all requests via mux.Handle and ensures a single source of truth for custom routing.Consider adding more explanatory logs so operators can distinguish fallback vs. custom interception in logs.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/v2/CHANGELOG.md(1 hunks)server/v2/api/grpcgateway/interceptor.go(1 hunks)server/v2/api/grpcgateway/server.go(4 hunks)server/v2/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
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/interceptor.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (8)
server/v2/api/grpcgateway/interceptor.go (4)
23-36: Clearly structured gatewayInterceptor.
This struct is well-organized, showing clear responsibilities for logging, gateway fallback, and routing. The doc comment also clarifies how queries are routed via the app manager.
52-62: Fallback to gateway mux is logical.
Falling back to the gateway mux for unrecognized endpoints keeps default gRPC-gateway behavior. Good design.
68-76: Consider support for other HTTP methods if required.
Currently, only POST and GET are handled. It might be worth confirming if other methods (PUT, PATCH, DELETE) are needed now or in the future.
109-145: Efficient scanning for custom GET annotations.
The approach to gather HTTP GET annotations from the merged proto registry is efficient. Remember to handle potential duplicates in the final map (should not happen in valid proto, but still a possibility).
server/v2/CHANGELOG.md (1)
25-28: Changelog entry is concise and follows guidelines.
It references GitHub issue #22715 and clearly states the new feature. This is consistent with the recommended changelog structure.
server/v2/api/grpcgateway/server.go (3)
17-17: AppManager import suits new functionality.
Importing "cosmossdk.io/server/v2/appmanager" aligns with the new gateway logic.
Line range hint 41-50: Public New function is well-documented.
The new signature adding appManager is a logical extension, but ensure downstream calls handle any potential nil appManager references.
140-141: Constant definition clarifies usage.
Defining GRPCBlockHeightHeader in one place is a good improvement for maintainability and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/v2/api/grpcgateway/uri_test.go (2)
15-92: Consider adding edge cases to TestMatchURIWhile the test cases are comprehensive, consider adding these scenarios:
- Empty URI path
- URI with special characters in parameters (e.g., URL-encoded values)
- URI with multiple catch-all wildcards (to verify proper handling of invalid patterns)
func TestMatchURI(t *testing.T) { testCases := []struct { name string uri string mapping map[string]string expected *uriMatch }{ + { + name: "empty path", + uri: "https://localhost:8080", + mapping: map[string]string{"/": "query.Root"}, + expected: &uriMatch{QueryInputName: "query.Root", Params: map[string]string{}}, + }, + { + name: "special characters in parameters", + uri: "https://localhost:8080/foo/bar?q=hello%20world&type=%2Fbank%2Fsend", + mapping: map[string]string{"/foo/bar": "query.Bank"}, + expected: &uriMatch{QueryInputName: "query.Bank", Params: map[string]string{"q": "hello world", "type": "/bank/send"}}, + }, + { + name: "multiple catch-all wildcards should not match", + uri: "https://localhost:8080/foo/bar/baz", + mapping: map[string]string{"/foo/{bar=**}/{baz=**}": "query.Invalid"}, + expected: nil, + },
94-100: Add nil map test case to TestURIMatch_HasParamsAdd a test case for nil params map to ensure robust handling.
func TestURIMatch_HasParams(t *testing.T) { u := uriMatch{Params: map[string]string{"foo": "bar"}} require.True(t, u.HasParams()) u = uriMatch{} require.False(t, u.HasParams()) + + u = uriMatch{Params: nil} + require.False(t, u.HasParams()) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/v2/api/grpcgateway/uri_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpcgateway/uri_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"
🔇 Additional comments (4)
server/v2/api/grpcgateway/uri_test.go (4)
1-13: LGTM! Clean imports organization
The imports are well-organized with standard library imports separated from third-party imports.
102-125: Add cleanup for registered proto type
The test registers a type with gogoproto but doesn't clean it up, which could affect other tests.
126-206: Add cleanup and enhance error test cases
- Add cleanup for registered proto type
- Consider adding these error cases:
- Invalid nested field path
- Non-existent field
- Invalid field type conversion for nested fields
func TestCreateMessage(t *testing.T) {
+ t.Cleanup(func() {
+ gogoproto.RegisterType(nil, dummyProtoName)
+ })
gogoproto.RegisterType(&DummyProto{}, dummyProtoName)
testCases := []struct {
// ... existing test cases ...
+ {
+ name: "invalid nested field path",
+ uri: uriMatch{
+ QueryInputName: dummyProtoName,
+ Params: map[string]string{"page..nest.foo": "5"},
+ },
+ expErr: true,
+ },
+ {
+ name: "non-existent field",
+ uri: uriMatch{
+ QueryInputName: dummyProtoName,
+ Params: map[string]string{"nonexistent": "value"},
+ },
+ expErr: true,
+ },
+ {
+ name: "invalid nested field type",
+ uri: uriMatch{
+ QueryInputName: dummyProtoName,
+ Params: map[string]string{"page.nest.foo": "not_a_number"},
+ },
+ expErr: true,
+ },208-264: Enhance TestCreateMessageFromJson with additional cases
- Add cleanup for registered proto type
- Add test cases for:
- Request body read error
- Empty request body
- Invalid field types in JSON
func TestCreateMessageFromJson(t *testing.T) {
+ t.Cleanup(func() {
+ gogoproto.RegisterType(nil, dummyProtoName)
+ })
gogoproto.RegisterType(&DummyProto{}, dummyProtoName)
testCases := []struct {
// ... existing test cases ...
+ {
+ name: "empty request body",
+ uri: uriMatch{QueryInputName: dummyProtoName},
+ request: func() *http.Request {
+ return &http.Request{Body: io.NopCloser(bytes.NewReader(nil))}
+ },
+ expErr: true,
+ },
+ {
+ name: "invalid field types",
+ uri: uriMatch{QueryInputName: dummyProtoName},
+ request: func() *http.Request {
+ return &http.Request{Body: io.NopCloser(bytes.NewReader([]byte(`{"baz":"not_a_number"}`)))}
+ },
+ expErr: true,
+ },
Description
Closes: #22715
This PR is marked breaking as the grpcgateway.New function signature has changed.
This PR implements a http.Handler that routes queries from grpc-gateway requests to the application manager's query router.
It does so by using an internal mapping of http get annotations to query input type names, built from the global gogoproto registry.
The interceptor handles GET requests with wildcard queries (foo/bar/{baz}), catch-all wildcards (foo/bar/{baz=**}, query parameters (foo/bar?baz=hi), and query parameters with nested struct setting (foo/bar?pagination.limit=5), as well as POST requests with JSON bodies. Please let me know if there are other cases i am missing.
Author 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
New Features
Bug Fixes
Tests
Chores
Documentation