🐛 bug: Respect DisablePathNormalizing during client requests #3773
Conversation
WalkthroughAdds a client- and request-level disablePathNormalizing flag, propagates it via Config into Request construction and hooks, applies the effective setting to RawRequest.URI (optionally forcing PathBytes = PathOriginal), resets flags on Reset, and adds tests ensuring pre-encoded paths are preserved when normalization is disabled. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant R as Request
participant H as Hooks
participant U as RawRequest.URI
Note over C,R: Request creation and config application
C->>R: apply Config (may call r.SetDisablePathNormalizing(true))
C->>H: Build request
H->>R: read r.DisablePathNormalizing()
H->>C: read c.DisablePathNormalizing()
H->>H: effective = c || r
H->>U: set U.DisablePathNormalizing = effective
alt effective == true
H->>U: set U.PathBytes = U.PathOriginal (preserve encoding)
else
H->>U: allow default path normalization
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gaby
changed the title
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3773 +/- ##
==========================================
- Coverage 91.70% 91.62% -0.08%
==========================================
Files 113 113
Lines 11940 11959 +19
==========================================
+ Hits 10949 10958 +9
- Misses 728 736 +8
- Partials 263 265 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for disabling path normalization during client requests to preserve encoded paths in URLs. This addresses issue #3096 where encoded characters in URL paths were being unintentionally normalized.
- Adds
disablePathNormalizingfields to both Client and Request structs with corresponding getter/setter methods - Updates URL parsing logic to respect the disable path normalizing setting and preserve original encoded paths
- Includes comprehensive test coverage for both client-level and request-level path normalization control
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| client/client.go | Adds disablePathNormalizing field to Client struct with getter/setter methods and Config support |
| client/request.go | Adds disablePathNormalizing field to Request struct with getter/setter methods and Reset handling |
| client/hooks.go | Updates URL parsing logic to check and respect path normalization settings |
| client/hooks_test.go | Adds regression tests for both client and request level path normalization control |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/client.go(5 hunks)client/hooks.go(1 hunks)client/hooks_test.go(1 hunks)client/request.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/hooks.goclient/request.goclient/hooks_test.goclient/client.go
🧬 Code graph analysis (1)
client/hooks_test.go (2)
client/client.go (1)
New(651-656)client/request.go (2)
AcquireRequest(980-986)ReleaseRequest(990-993)
🪛 GitHub Actions: golangci-lint
client/client.go
[error] 32-32: golangci-lint: govet: fieldalignment: struct of size 312 could be 304
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (8)
client/hooks_test.go (1)
211-240: LGTM! Comprehensive test coverage for path normalization control.The two test cases effectively verify that DisablePathNormalizing works at both the request and client levels, ensuring percent-encoded characters (like
%2Fand%23) are preserved when normalization is disabled.client/client.go (4)
358-367: LGTM! API follows established patterns.The getter/setter methods are consistent with other client options like
Debug()/DisableDebug()and follow the builder pattern correctly.
545-545: LGTM! Reset behavior is correct.The flag is properly reset to
falsein theReset()method, ensuring clean state when the client is reused.
574-574: LGTM! Config field follows naming conventions.The field name and placement in the
Configstruct are consistent with other configuration options.
620-622: LGTM! Config propagation is correct.The logic correctly propagates the
DisablePathNormalizingflag fromConfigto the request when set totrue, consistent with how other boolean config options are handled.client/hooks.go (1)
96-101: LGTM! Combined flag logic and path preservation are correct.The implementation correctly:
- Combines client and request-level flags using OR logic (either disables normalization)
- Applies the flag to the underlying
fasthttp.Request.URI()- Preserves the original encoded path by setting
PathBytestoPathOriginal()when normalization is disabledThe order of operations is correct: first parse the URI with
SetRequestURI, then override the path to preserve encoding when needed.client/request.go (2)
610-620: LGTM! Request-level API is consistent and correct.The getter/setter methods follow the same pattern as the client-level API. Setting
RawRequest.URI().DisablePathNormalizingin the setter ensures the flag is propagated early, which is then reinforced by the hooks logic. This redundancy is safe and ensures consistency.
688-688: LGTM! Reset behavior is correct.The flag is properly cleared when the request is recycled, ensuring clean state for pooled objects.
|
@gaby can you check liniting |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/client.go(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/client.go
🧬 Code graph analysis (1)
client/client.go (1)
client/request.go (3)
Cookie(775-775)PathParam(823-823)File(929-934)
🪛 GitHub Actions: golangci-lint
client/client.go
[error] 58-58: golangci-lint: File is not properly formatted (gofmt).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (5)
client/client.go (5)
357-360: LGTM!The getter method correctly returns the path normalizing flag and follows the established pattern for client configuration accessors.
560-574: LGTM!The
DisablePathNormalizingfield addition to theConfigstruct is correctly typed and positioned, enabling request-level control over path normalization.
619-621: Verify one-way override behavior is intentional.The logic only propagates
DisablePathNormalizingwhentrue, meaning a request cannot re-enable path normalization if the client has it disabled. Confirm this one-way override aligns with the intended API semantics.
544-544: LGTM!Correctly resets the path normalizing flag to its default state during client reset.
362-366: Thread-safety consistent with existing simple setters. SetDisablePathNormalizing follows the established pattern of one-time boolean configuration during client initialization and isn’t modified concurrently. No mutex required.
|
@ReneWerner87 Should be good now |
Summary
Fixes #3096