fix(server/v2): support for url encoded path parameters #23386
Conversation
📝 WalkthroughWalkthroughThe pull request introduces improvements to URL path parameter handling in the Changes
Sequence DiagramsequenceDiagram
participant URI as Incoming URI
participant Matcher as URI Matcher
participant Decoder as URL Decoder
URI ->> Matcher: Receive URI
Matcher ->> Matcher: Check for URL encoding
alt URL is encoded
Matcher ->> Decoder: Attempt to unescape
Decoder -->> Matcher: Return decoded value
else URL is not encoded
Matcher ->> Matcher: Use original value
end
Matcher ->> Matcher: Store parameter
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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
|
|
@technicallyty your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/v2/api/grpcgateway/uri.go (1)
31-40: LGTM! Consider adding a code example in the comment.The logic for detecting URL encoded paths is correct. The comment could be enhanced with an example to better illustrate when
url.RawPathis non-empty.- // the url.RawPath is non-empty when URL encoded values are detected in the path params. - // this requires different handling when getting path parameter values. + // url.RawPath is non-empty when URL encoded values are detected in the path params. + // Example: for URL "/foo/bar/%2Fbaz", RawPath="/foo/bar/%2Fbaz" and Path="/foo/bar/baz" + // This requires different handling when getting path parameter values.server/v2/api/grpcgateway/uri_test.go (1)
81-86: Add test case for URL decoding failure.The current test case verifies successful URL decoding, but we should also test the fallback behavior when decoding fails. This ensures the robustness of the error handling logic.
Add this test case:
{ name: "match with invalid URL-encoded values", uri: "https://localhost:8080/foo/bar/invalid%", mapping: map[string]string{"/foo/bar/{bytes}": "queryAddrBytes"}, expected: &uriMatch{QueryInputName: "queryAddrBytes", Params: map[string]string{"bytes": "invalid%"}}, },
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/v2/api/grpcgateway/uri.go(2 hunks)server/v2/api/grpcgateway/uri_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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"
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 (9)
- GitHub Check: tests (03)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: build (amd64)
- GitHub Check: Summary
| var matchValue string | ||
| if isURLEncoded { | ||
| // we'll try to unescape the URL encoded values, | ||
| // but if that doesn't work, we can just try the raw value. | ||
| if decodedMatch, err := url.QueryUnescape(matches[i+1]); err == nil { | ||
| matchValue = matches[i+1] | ||
| } else { | ||
| matchValue = decodedMatch | ||
| } | ||
| } else { | ||
| matchValue = matches[i+1] | ||
| } | ||
| params[name] = matchValue |
There was a problem hiding this comment.
Fix inverted logic in URL unescaping condition.
There's a critical logic error in the URL unescaping condition. When url.QueryUnescape succeeds, the code uses the encoded value (matches[i+1]) instead of the decoded value (decodedMatch), and vice versa.
Apply this fix:
if isURLEncoded {
// we'll try to unescape the URL encoded values,
// but if that doesn't work, we can just try the raw value.
if decodedMatch, err := url.QueryUnescape(matches[i+1]); err == nil {
- matchValue = matches[i+1]
- } else {
matchValue = decodedMatch
+ } else {
+ matchValue = matches[i+1]
}
} else {
matchValue = matches[i+1]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var matchValue string | |
| if isURLEncoded { | |
| // we'll try to unescape the URL encoded values, | |
| // but if that doesn't work, we can just try the raw value. | |
| if decodedMatch, err := url.QueryUnescape(matches[i+1]); err == nil { | |
| matchValue = matches[i+1] | |
| } else { | |
| matchValue = decodedMatch | |
| } | |
| } else { | |
| matchValue = matches[i+1] | |
| } | |
| params[name] = matchValue | |
| var matchValue string | |
| if isURLEncoded { | |
| // we'll try to unescape the URL encoded values, | |
| // but if that doesn't work, we can just try the raw value. | |
| if decodedMatch, err := url.QueryUnescape(matches[i+1]); err == nil { | |
| matchValue = decodedMatch | |
| } else { | |
| matchValue = matches[i+1] | |
| } | |
| } else { | |
| matchValue = matches[i+1] | |
| } | |
| params[name] = matchValue |
Description
Closes: n/a
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
Bug Fixes
Tests
The changes ensure more accurate and reliable processing of URL-encoded parameters across the application's routing mechanism.