fix(server/v2): return ErrHelp #22399
Conversation
kocubinski
requested review from
julienrbrt,
a team and
hieuvubk
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request focus on improving error handling in command parsing within the Changes
Possibly related PRs
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
|
|
@kocubinski your pull request is missing a changelog! |
julienrbrt
added
the
backport/v0.52.x
|
Added flag for simapp v2 partial backport (we should use latest main commit for server v2 there) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
simapp/v2/simdv2/cmd/root_test.go (1)
47-67: Approve with suggestions for enhanced test coverage
The test implementation is solid and aligns well with the PR objective. Good use of table-driven tests and proper verification of help output.
Consider these enhancements:
- Add test case descriptions for better clarity
- Include deeper subcommand tests (>2 levels)
- Add negative test cases
Here's a suggested enhancement:
func TestHelpRequested(t *testing.T) {
- argz := [][]string{
- {"query", "--help"},
- {"query", "tx", "-h"},
- {"--help"},
- {"start", "-h"},
+ testCases := []struct {
+ name string
+ args []string
+ }{
+ {
+ name: "root query help",
+ args: []string{"query", "--help"},
+ },
+ {
+ name: "nested command help",
+ args: []string{"query", "tx", "-h"},
+ },
+ {
+ name: "root help",
+ args: []string{"--help"},
+ },
+ {
+ name: "start command help",
+ args: []string{"start", "-h"},
+ },
+ {
+ name: "deep nested command help",
+ args: []string{"query", "tx", "sign", "--help"},
+ },
}
- for _, args := range argz {
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
- rootCmd, err := cmd.NewRootCmd[transaction.Tx](args...)
+ rootCmd, err := cmd.NewRootCmd[transaction.Tx](tc.args...)
require.NoError(t, err)
var out bytes.Buffer
- rootCmd.SetArgs(args)
+ rootCmd.SetArgs(tc.args)
rootCmd.SetOut(&out)
require.NoError(t, rootCmd.Execute())
- require.Contains(t, out.String(), args[0])
+ require.Contains(t, out.String(), tc.args[0])
require.Contains(t, out.String(), "--help")
require.Contains(t, out.String(), "Usage:")
+ })
}
}server/v2/command_factory.go (1)
Line range hint 156-158: Consider wrapping errors with additional context
The error handling could be improved by wrapping errors with additional context using fmt.Errorf("failed to %v: %w", action, err). This would make debugging easier by providing more context about where and why errors occur.
Example locations:
- Flag parsing errors
- Home directory access errors
- Config reading errors
Example improvement:
home, err := cmd.Flags().GetString(FlagHome)
if err != nil {
- return nil, nil, nil, err
+ return nil, nil, nil, fmt.Errorf("failed to get home flag: %w", err)
}Also applies to: 161-163, 166-168, 175-177, 179-181, 184-186
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- server/v2/command_factory.go (1 hunks)
- simapp/v2/simdv2/cmd/root_di.go (2 hunks)
- simapp/v2/simdv2/cmd/root_test.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
server/v2/command_factory.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/root_di.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
simapp/v2/simdv2/cmd/root_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 (5)
simapp/v2/simdv2/cmd/root_test.go (1)
4-4: LGTM: Import follows Go conventions
The "bytes" import is correctly grouped with standard library imports and is properly utilized in the test.
simapp/v2/simdv2/cmd/root_di.go (2)
4-8: LGTM: Import changes are appropriate
The added imports for errors and pflag packages are necessary for the enhanced error handling implementation.
44-46: LGTM: Proper error handling for help requests
The implementation correctly handles the --help flag by checking for pflag.ErrHelp and returning the root command without error, which should prevent the panic mentioned in the PR objectives.
Let's verify that this handles all help scenarios correctly:
✅ Verification successful
Let me check the help handling implementation in the other files to ensure consistency.
Help flag handling is consistent and properly implemented
The implementation is verified to be correct and consistent with other help handling patterns in the codebase:
- Test coverage in
root_test.goconfirms handling of various help flag scenarios (--help,-h) - Similar error handling pattern exists in
command_factory.gousingerrors.Is(err, pflag.ErrHelp) - Direct help flag checks in
validate.goandclient/cmd.goproperly callcmd.Help() - No help-related panic points were found in the codebase
The new implementation aligns well with existing patterns and provides a robust solution to prevent panics.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify help flag handling across different command variations
# Expected: All help-related flags should be handled by this error check
# Test 1: Search for other help flag handling patterns
echo "Checking for other help flag handling patterns..."
rg -l "ErrHelp|--help" --type go
# Test 2: Look for potential help-related panics
echo "Checking for potential help-related panic points..."
ast-grep --pattern 'if $_ == "--help" { panic($$$) }'
Length of output: 1617
Script:
#!/bin/bash
# Check the actual help handling implementations
echo "Checking help handling in root_test.go..."
rg "ErrHelp|--help" simapp/v2/simdv2/cmd/root_test.go -C 3
echo "\nChecking help handling in command_factory.go..."
rg "ErrHelp|--help" server/v2/command_factory.go -C 3
echo "\nChecking help handling in validate.go..."
rg "ErrHelp|--help" client/v2/autocli/validate.go -C 3
echo "\nChecking help handling in client/cmd.go..."
rg "ErrHelp|--help" client/cmd.go -C 3
Length of output: 1394
server/v2/command_factory.go (2)
152-152: LGTM: Proper error propagation for help requests
The change correctly propagates pflag.ErrHelp instead of returning nil. This fix:
- Prevents the panic when
--helpflag is used - Follows Go's error handling best practices
- Allows proper help message display in upstream command handlers
Line range hint 24-30: Consider creating a tracking issue for viper removal
The TODO comment indicates that the vipr field should be removed after merging #22267. To ensure this technical debt is properly tracked:
- Create a dedicated issue if Refactor: remove viper in server context #22388 doesn't already track this
- Add a deprecation notice in the code documentation
Let's verify if this is already being tracked:
✅ Verification successful
Technical debt is already tracked in issue #22388
The TODO comment is properly tracked in issue #22388 which specifically addresses the removal of viper from server context. The issue aligns perfectly with the TODO comment's reference to #22267 and confirms the plan to replace viper.Viper with server.ConfigMap.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if issue #22388 exists and contains viper removal details
gh issue view 22388 2>/dev/null || echo "Issue #22388 not found"
Length of output: 808
(cherry picked from commit 470e085) # Conflicts: # server/v2/command_factory.go
* main: (24 commits) build(deps): upgrade to iavl@v1.3.1 (#22436) docs: Update tendermint validators query pagination documentation (#22412) refactor(client/v2): offchain uses client/v2/factory (#22344) feat: wire new handlers to grpc (#22333) fix(x/group): proper address rendering in error (#22425) refactor(serevr/v2/cometbft): update `RegisterQueryHandlers` and GRPC queries (#22403) docs: update ADR 59 (#22423) build(deps): Bump github.com/fsnotify/fsnotify from 1.7.0 to 1.8.0 in /tools/cosmovisor (#22407) docs: Module account address documentation (#22289) feat(baseapp): add per message telemetry (#22175) docs: Update Twitter Links to X in Documentation (#22408) docs: redirect the remote generation page (#22404) refactor(serverv2): remove unused interface methods, honuor context (#22394) fix(server/v2): return ErrHelp (#22399) feat(indexer): implement `schema.HasModuleCodec` interface in the `bank` module (#22349) refactor(math): refactor ApproxRoot for readality (#22263) docs: fix KWallet Handbook (#22395) feat(client/v2): broadcast logic (#22282) refactor(server/v2): eager config loading (#22267) test(system): check feePayers signature (#22389) ...
Description
Fixes a panic when passing
--helpto any command.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