fix(systemtests): to avoid invalid order_by used in TestGetTxEvents_GRPCGateway #23290
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe pull request introduces modifications to 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
|
![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
🧹 Nitpick comments (1)
tests/systemtests/tx_test.go (1)
378-379: Consider using a named constant for the invalid order value.While the test correctly validates error handling for invalid order values, using a magic number (777) reduces code readability. Consider defining a constant for this test value.
+const invalidOrderBy = 777 // Value outside valid tx.OrderBy range + { "invalid request: invalid order by", - fmt.Sprintf("%s/cosmos/tx/v1beta1/txs?query=%s&query=%s&order_by=%d", baseURL, bankMsgSendEventAction, "message.module='bank'", 777), + fmt.Sprintf("%s/cosmos/tx/v1beta1/txs?query=%s&query=%s&order_by=%d", baseURL, bankMsgSendEventAction, "message.module='bank'", invalidOrderBy), true, "is not a valid tx.OrderBy", 0, },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/systemtests/tx_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/systemtests/tx_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"
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (3)
tests/systemtests/tx_test.go (3)
366-367: LGTM! Good use of type-safe constants.The change from string literals to integer constants (
tx.OrderBy_ORDER_BY_ASC) improves type safety and maintainability.
372-373: LGTM! Consistent use of type-safe constants.The change maintains consistency with the ascending order case by using the integer constant
tx.OrderBy_ORDER_BY_DESC.
404-404: LGTM! Clear assertion of expected results.The assertion correctly validates the number of transactions returned matches the expected length.
v0.52 also use enum for order_by right? |
Yeah, it uses the enum name, not its value. In release/v0.52.x: simapp $ go test -mod=readonly -failfast -timeout=15m -tags='system_test' --run TestGetTxEvents_GRPCGateway ./... --wait-time=45s --verbose
PASSsimapp v2 $ go test -mod=readonly -failfast -timeout=15m -tags='system_test' --run TestGetTxEvents_GRPCGateway ./... --wait-time=45s --verbose --binary=simdv2
FAILEDSo it is a regression in v2. |
|
It looks like however it is the same handling: cosmos-sdk/server/v2/cometbft/grpc.go Line 461 in 517839b cosmos-sdk/x/auth/tx/service.go Line 337 in 517839b |
|
Looks like both enum values and enum string works in 0.52 but only enum values work in v2. |
not sure, v2 also don't have swagger yet |
github-actions
bot
added
C:server/v2
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/api/grpcgateway/uri.go(1 hunks)tests/systemtests/tx_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/systemtests/tx_test.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/grpcgateway/uri.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: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
| err = decoder.Decode(nestedParams) | ||
| if err != nil && !strings.Contains(err.Error(), "order_by") { | ||
| return nil, status.Error(codes.InvalidArgument, err.Error()) |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Error masking prevents proper enum validation
The verification confirms this is a problematic implementation:
- It's the only instance in the codebase that silently ignores errors based on error message content
- The
OrderByenum is properly defined in protobuf, suggesting it should handle both numeric and string values correctly - This error masking approach hides potential validation issues instead of properly supporting enum string values
Consider implementing proper enum string support by:
- Using protobuf's built-in enum string handling
- Adding explicit validation for enum values
- Properly documenting supported formats in the API
🔗 Analysis chain
Critical: Avoid masking decoding errors based on error message content
The current change ignores decoding errors that contain "order_by" in their message. This is problematic for several reasons:
- It could mask legitimate errors that happen to contain "order_by" in their message
- It makes the behavior inconsistent with the documentation
- It doesn't address the root cause of why enum strings stopped working in v2 while they work in v0.52
Consider implementing a proper fix that addresses the root cause:
- Implement proper enum string support in v2 to maintain compatibility with v0.52
- If enum strings must be deprecated, provide clear documentation and migration guide
- Add proper validation for enum values instead of relying on error message content
Let's verify if this change affects other enum parameters:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other enum parameters that might be affected
echo "Searching for other enum parameters in proto definitions..."
rg -l "enum.*{" | while read -r file; do
echo "=== $file ==="
ast-grep --pattern 'enum $enumName {
$$$
}' "$file"
done
echo "Searching for similar error handling patterns..."
rg 'if.*err.*Contains.*\(.*Error.*\).*{'
Length of output: 523
Script:
#!/bin/bash
# Find proto files and enum definitions
echo "=== Proto files with enums ==="
fd '\.proto$' | xargs rg -l 'enum\s+\w+\s*{'
echo -e "\n=== Enum definitions ==="
fd '\.proto$' | xargs rg -A 5 'enum\s+\w+\s*{'
echo -e "\n=== Similar error handling patterns ==="
# Search for error string checks in Go files
rg -t go 'if.*err.*Contains.*\(' -A 2
echo -e "\n=== Specific file context ==="
# Get more context around the modified code
rg -t go 'decoder\.Decode.*nestedParams' -A 5 server/v2/api/grpcgateway/uri.go
Length of output: 3805
Script:
#!/bin/bash
# Find proto files with enum definitions
echo "=== Proto files with enums ==="
find . -name "*.proto" -type f -exec grep -l "enum" {} \;
echo -e "\n=== Enum definitions ==="
find . -name "*.proto" -type f -exec grep -B1 -A5 "enum.*{" {} \;
echo -e "\n=== Transaction related protos ==="
find . -name "*.proto" -type f -exec grep -l "Transaction\|Order" {} \;
Length of output: 8192
There was a problem hiding this comment.
@julienrbrt seems fixed by PopulateQueryParameters instead of decode now
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
yes, swagger is being added here: #23092 |
|
i think this may be fixed in #23262. haven't tested yet though. |
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