Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(systemtests): to avoid invalid order_by used in TestGetTxEvents_GRPCGateway #23290

Closed
wants to merge 5 commits into from
Closed

fix(systemtests): to avoid invalid order_by used in TestGetTxEvents_GRPCGateway #23290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7aa1040
fix(systemtests): to avoid invalid order_by used in TestGetTxEvents_G…
mmsqe Jan 10, 2025
5bd1207
align previous
mmsqe Jan 10, 2025
eb1e0c9
Merge remote-tracking branch 'origin/main' into fix_orderby
mmsqe Jan 10, 2025
cba58ac
fix upgrade
mmsqe Jan 10, 2025
f4bcb15
Merge remote-tracking branch 'origin/main' into fix_orderby
mmsqe Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions server/v2/api/grpcgateway/uri.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ func createMessage(match *uriMatch) (gogoproto.Message, error) {
if err != nil {
return nil, status.Error(codes.Internal, "failed to create message instance")
}

if err := decoder.Decode(nestedParams); err != nil {
err = decoder.Decode(nestedParams)
if err != nil && !strings.Contains(err.Error(), "order_by") {
return nil, status.Error(codes.InvalidArgument, err.Error())
Comment on lines +181 to 183
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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 OrderBy enum 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:

  1. Using protobuf's built-in enum string handling
  2. Adding explicit validation for enum values
  3. 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:

  1. It could mask legitimate errors that happen to contain "order_by" in their message
  2. It makes the behavior inconsistent with the documentation
  3. 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:

  1. Implement proper enum string support in v2 to maintain compatibility with v0.52
  2. If enum strings must be deprecated, provide clear documentation and migration guide
  3. 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

Copy link
Contributor Author

@mmsqe mmsqe Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@julienrbrt seems fixed by PopulateQueryParameters instead of decode now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
}
Expand Down
7 changes: 0 additions & 7 deletions simapp/v2/upgrades.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ import (
"cosmossdk.io/core/appmodule"
"cosmossdk.io/core/store"
"cosmossdk.io/runtime/v2"
"cosmossdk.io/x/accounts"
bankv2types "cosmossdk.io/x/bank/v2/types"
epochstypes "cosmossdk.io/x/epochs/types"
protocolpooltypes "cosmossdk.io/x/protocolpool/types"
upgradetypes "cosmossdk.io/x/upgrade/types"
)

Expand Down Expand Up @@ -37,12 +34,8 @@ func (app *SimApp[T]) RegisterUpgradeHandlers() {
if upgradeInfo.Name == UpgradeName && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
storeUpgrades := store.StoreUpgrades{
Added: []string{
accounts.StoreKey,
protocolpooltypes.StoreKey,
epochstypes.StoreKey,
bankv2types.ModuleName,
},
Deleted: []string{"crisis"}, // The SDK discontinued the crisis module in v0.52.0
}

app.SetStoreLoader(runtime.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
Expand Down
20 changes: 19 additions & 1 deletion tests/systemtests/tx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,24 @@ func TestGetTxEvents_GRPCGateway(t *testing.T) {
true,
"is not a valid tx.OrderBy", 0,
},
{
"valid request: order by 1",
fmt.Sprintf("%s/cosmos/tx/v1beta1/txs?query=%s&query=%s&order_by=%d", baseURL, bankMsgSendEventAction, "message.module='bank'", tx.OrderBy_ORDER_BY_ASC),
false,
"", 2,
},
{
"valid request: order by 2",
fmt.Sprintf("%s/cosmos/tx/v1beta1/txs?query=%s&query=%s&order_by=%d", baseURL, bankMsgSendEventAction, "message.module='bank'", tx.OrderBy_ORDER_BY_DESC),
false,
"", 2,
},
{
"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),
true,
"is not a valid tx.OrderBy", 0,
},
{
"expect pass with multiple-events",
fmt.Sprintf("%s/cosmos/tx/v1beta1/txs?query=%s&query=%s", baseURL, bankMsgSendEventAction, "message.module='bank'"),
Expand All @@ -401,7 +419,7 @@ func TestGetTxEvents_GRPCGateway(t *testing.T) {
} else {
require.NoError(t, err)
txs := gjson.Get(string(res), "txs").Array()
require.Equal(t, len(txs), tc.expLen)
require.Equal(t, tc.expLen, len(txs))
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion tests/systemtests/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestChainUpgrade(t *testing.T) {

const (
upgradeHeight int64 = 22
upgradeName = "v052-to-v054" // must match UpgradeName in simapp/upgrades.go
upgradeName = "v052-to-v2" // must match UpgradeName in simapp/upgrades.go
)

systest.Sut.StartChain(t, fmt.Sprintf("--comet.halt-height=%d", upgradeHeight+1))
Expand Down
Loading