refactor(baseapp)!: remove hybrid handlers #22831
Conversation
testinginprod
requested review from
julienrbrt,
hieuvubk,
a team,
facundomedica,
aaronc and
kocubinski
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request involve renaming methods and variables across several files to simplify the handling of gRPC requests and message routing. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
|
@testinginprod your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
baseapp/msg_service_router_test.go (1)
Line range hint
103-115: Rename the test function to match the updated method nameThe test function
TestHybridHandlerByMsgNameshould be renamed toTestHandlerByMsgNameto reflect the updated method name being tested.baseapp/msg_service_router.go (1)
91-92: Consider renaming registerHybridHandler methodWhile the HandlerByMsgName method has been updated, the registerHybridHandler method name still contains "Hybrid". Consider renaming it to registerHandler for consistency with the rest of the changes.
-func (msr *MsgServiceRouter) registerHybridHandler(sd *grpc.ServiceDesc, method grpc.MethodDesc, handler interface{}) error { +func (msr *MsgServiceRouter) registerHandler(sd *grpc.ServiceDesc, method grpc.MethodDesc, handler interface{}) error {Also applies to: 108-108, 116-116, 131-131
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
baseapp/grpcrouter.go(6 hunks)baseapp/grpcrouter_test.go(1 hunks)baseapp/internal/protocompat/protocompat.go(2 hunks)baseapp/msg_service_router.go(5 hunks)baseapp/msg_service_router_test.go(1 hunks)runtime/environment.go(2 hunks)runtime/router.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
baseapp/msg_service_router_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"
runtime/router.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/grpcrouter_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"
baseapp/msg_service_router.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
runtime/environment.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/internal/protocompat/protocompat.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
baseapp/grpcrouter.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (13)
baseapp/grpcrouter.go (4)
21-21: LGTM! Interface method renamed for clarity
The removal of "Hybrid" prefix simplifies the interface while maintaining the same functionality.
32-35: LGTM! Improved handler map documentation
The updated comments better explain the purpose of the handlers map, clarifying its generic nature versus ABCI-specific functionality.
61-61: LGTM! Constructor updated to match struct changes
The initialization is correctly updated to use the renamed handlers map.
147-148: LGTM! Methods renamed consistently with interface changes
The implementation correctly reflects the interface changes while maintaining proper error handling and registration logic.
Also applies to: 155-171
baseapp/grpcrouter_test.go (1)
59-59: LGTM! Tests updated to match API changes
The test correctly uses the renamed method while maintaining comprehensive coverage for both protov2 and gogoproto handlers.
baseapp/msg_service_router.go (3)
29-29: LGTM! Interface aligned with QueryRouter changes
The method renaming maintains consistency with the QueryRouter interface changes.
36-36: LGTM! Struct field aligned with GRPCQueryRouter changes
The handlers map renaming maintains consistency with similar changes in GRPCQueryRouter.
47-47: LGTM! Constructor updated to match struct changes
The initialization correctly uses the renamed handlers map.
runtime/environment.go (1)
94-96: LGTM! Verify consistent renaming across the codebase
The method rename from HybridHandlerByRequestName to HandlerByRequestName aligns with the PR objective of removing hybrid handlers.
✅ Verification successful
Method rename is consistently applied across the codebase
The verification shows that:
- No references to the old
HybridHandlerByRequestNamemethod exist in the codebase - The new
HandlerByRequestNamemethod is properly defined in:QueryRouterinterface inbaseapp/grpcrouter.go- Implementation in
baseapp/grpcrouter.go - Failing implementation in
runtime/environment.go - Used consistently in router implementations and tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent renaming of HybridHandlerByRequestName across codebase
rg -l "HybridHandlerByRequestName"
Length of output: 34
Script:
#!/bin/bash
# Let's verify both the old and new method names in the interfaces and implementations
# First, check for any remaining references to the old name
rg "HybridHandlerByRequestName" -A 2 -B 2
# Then check the current usage of the new name to ensure consistency
rg "HandlerByRequestName" -A 2 -B 2
# Also check for any interface definitions that might be related
ast-grep --pattern 'interface { $$$ HandlerByRequestName($$$) $$$ }'
Length of output: 1947
runtime/router.go (4)
Line range hint 40-44: LGTM! Method call updated consistently
The handler retrieval call has been updated to use HandlerByMsgName, maintaining consistency with the interface changes.
Line range hint 66-73: LGTM! Method call updated consistently
The handler retrieval and execution logic has been updated to use HandlerByMsgName, maintaining consistency with the interface changes.
Line range hint 99-104: LGTM! Method call updated consistently
The handler retrieval and validation logic has been updated to use HandlerByRequestName, maintaining consistency with the interface changes.
Line range hint 127-134: LGTM! Method call updated consistently
The handler retrieval, validation, and execution logic has been updated to use HandlerByRequestName, maintaining consistency with the interface changes.
| // MakeHandler returns a handler that can handle both gogo and protov2 messages, no matter | ||
| // if the handler is a gogo or protov2 handler. | ||
| func MakeHybridHandler(cdc codec.BinaryCodec, sd *grpc.ServiceDesc, method grpc.MethodDesc, handler interface{}) (Handler, error) { | ||
| func MakeHandler(sd *grpc.ServiceDesc, method grpc.MethodDesc, handler interface{}) (Handler, error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update the function comment to accurately describe the function's behavior
The comment for MakeHandler suggests that it returns a handler capable of handling both gogo and protov2 messages. However, the implementation returns an error for protov2 handlers. Please update the comment to reflect that only gogo handlers are supported.
| func (failingMsgRouter) HandlerByMsgName(msgName string) func(ctx context.Context, req, resp protoiface.MessageV1) error { | ||
| panic("message router not set") | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Update test file to match the renamed method
The method HybridHandlerByMsgName has been renamed to HandlerByMsgName in runtime/environment.go, but there's still a test using the old name in baseapp/msg_service_router_test.go. Please update the test name to maintain consistency.
baseapp/msg_service_router_test.go: RenameTestHybridHandlerByMsgNametoTestHandlerByMsgName
🔗 Analysis chain
LGTM! Verify consistent renaming across the codebase
The method rename from HybridHandlerByMsgName to HandlerByMsgName aligns with the PR objective of removing hybrid handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent renaming of HybridHandlerByMsgName across codebase
rg -l "HybridHandlerByMsgName"
Length of output: 65
Script:
#!/bin/bash
# Check the context of HybridHandlerByMsgName usage in the test file
rg -A 5 "HybridHandlerByMsgName" baseapp/msg_service_router_test.go
Length of output: 230
julienrbrt
added
the
backport/v0.52.x
| @@ -18,7 +18,7 @@ import ( | |||
| ) | |||
|
|
|||
| type QueryRouter interface { | |||
| HybridHandlerByRequestName(name string) []func(ctx context.Context, req, resp protoiface.MessageV1) error | |||
| HandlerByRequestName(name string) []func(ctx context.Context, req, resp protoiface.MessageV1) error | |||
There was a problem hiding this comment.
Do we need to break this interface name?
This is a public interface and could break consumers
There was a problem hiding this comment.
I think it is fine, sourcegraph tells me there are no users.
We ask user to migrate to use core services anyway via environment, so those API should be hidden from them.
There was a problem hiding this comment.
Sure, but couldn't we lower the impact of this change by just maintaining the name of this function for the interface?
There was a problem hiding this comment.
Do we gain anything by breaking this name?
There was a problem hiding this comment.
The impact is low as there are no consumers. Hybrid handlers never got fully used as we decided after 0.50 to not support proto v1 & proto v2. Keeping such API name is more confusing than simplifying the API imho.
There was a problem hiding this comment.
since there are no consumers and the code is no longer working hybrid i feel like its fine. Currently users dont make it this far and the ones that do cant actually swap the router other than in 52+ because there was no interface before and the baseapp struct took the concrete implementation.
julienrbrt
changed the title
|
Nuking this for now:
Will come with a smarter and quick fix to the issue. |
Description
Closes: #XXXX
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
Documentation
Tests