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
|
|
@julienrbrt any suggestion on how to test this? i was hoping there were tests i could pattern match from the |
| "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.
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,
+ },|
we should create an issue with the follow ups so they dont get lost |
Created #23188 |
|
Calling mergify to do a partial backport. |
|
@Mergifyio backport release/v0.52.x |
✅ Backports have been created
|
(cherry picked from commit 884a7a5) # Conflicts: # server/v2/CHANGELOG.md # server/v2/api/grpcgateway/server.go # server/v2/go.mod
… (#23189) Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr>
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