🧹 chore: Respect immutable config in Params(), Protocol(), and Body() #3676
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated AutoFormat documentation to list additional content types. DefaultReq now returns copied param strings when App.Config.Immutable is true and reads headers via r.App().getString(...). Two tests asserting param immutability were added (duplicated). No exported signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant Ctx
participant Req
Client->>App: HTTP request
App->>Ctx: Acquire context
Ctx->>Req: Params("user")
alt App.Config.Immutable == true
Req-->>Ctx: return copy of value
else
Req-->>Ctx: return original reference
end
Ctx-->>Client: Handler uses param
sequenceDiagram
participant Ctx
participant Req
participant App
Ctx->>Req: read header (Protocol/Content-Encoding)
Req->>App: getString(Header.*)
App-->>Req: normalized safe string
Req-->>Ctx: header-derived value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented changes to improve the handling of immutable configurations within the application, focusing on how route parameters and certain HTTP headers are accessed. The primary goal is to ensure that when an Immutable setting is active, data retrieved from requests, such as route parameters, are provided as safe, independent copies. This prevents unintended side effects from subsequent modifications to the underlying request data. Additionally, I've updated the documentation to reflect broader content type support for automatic formatting.
Highlights
- Enhanced Immutability for Route Parameters: I've ensured that when the
Immutableconfiguration is enabled, route parameters returned by theParamsmethod are now independent copies. This prevents external modifications to the underlying request path from inadvertently altering the parameter values, enhancing data integrity and predictability. - Config-Aware Header Retrieval: I've updated the retrieval of HTTP
Content-EncodingandProtocolheaders to use a new config-aware string getter. This change ensures that these header values are handled consistently with the application's immutability settings, moving away from potentially unsafe string conversions. - New Test Case for Params Immutability: I've added a new test case specifically to validate the immutability behavior of the
Paramsmethod. This test confirms that even if the underlying request path is modified after a parameter is retrieved, the original parameter value remains unchanged, verifying the effectiveness of the immutability fix. - Documentation Update for AutoFormat: I've updated the documentation comments for the
AutoFormatmethod in bothctx_interface_gen.goandres_interface_gen.goto explicitly listapplication/vnd.msgpackandapplication/cboras supported content types. This provides clearer information for developers on the full range of content negotiation capabilities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
gaby
changed the title
There was a problem hiding this comment.
Code Review
This pull request correctly implements the logic to respect the Immutable configuration for route parameters and some headers, ensuring that values are copied when the setting is enabled. A new test is added to verify this behavior. The changes are consistent with the project's design and improve the framework's robustness when dealing with immutable data.
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the Fiber framework properly respects the Immutable configuration setting when accessing route parameters and HTTP headers. The changes prevent memory sharing issues that could occur when the Immutable config is enabled.
- Updates
Params()method to copy parameter values when Immutable config is enabled - Replaces direct header access with config-aware string getters for Protocol and Content-Encoding
- Updates documentation to reflect additional supported content types for AutoFormat
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| req.go | Modified Params() to copy strings when Immutable config is enabled, and updated Protocol() and Body() methods to use config-aware string getters |
| ctx_test.go | Added test case to verify Params() creates independent memory copies under Immutable config |
| res_interface_gen.go | Updated AutoFormat documentation to include additional supported content types |
| ctx_interface_gen.go | Updated AutoFormat documentation to include additional supported content types |
![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
♻️ Duplicate comments (1)
ctx_test.go (1)
3115-3131: Test is correct; consider table-driven to cover mutable as wellGood coverage for Immutable=true. Suggest converting to a table-driven test to also assert the mutable behavior (Immutable=false yields “paul” after mutation).
-func Test_Ctx_Params_Immutable(t *testing.T) { - t.Parallel() - app := New(Config{Immutable: true}) - c := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) //nolint:errcheck,forcetypeassert // not needed - - c.route = &Route{Params: []string{"user"}} - c.path = []byte("/test/john") - c.values[0] = utils.UnsafeString(c.path[6:]) - - param := c.Params("user") - c.path[6] = 'p' - c.path[7] = 'a' - c.path[8] = 'u' - c.path[9] = 'l' - - require.Equal(t, "john", param) -} +func Test_Ctx_Params_Immutable(t *testing.T) { + t.Parallel() + tests := []struct { + name string + immutable bool + expected string + }{ + {name: "immutable", immutable: true, expected: "john"}, + {name: "mutable", immutable: false, expected: "paul"}, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + app := New(Config{Immutable: tt.immutable}) + c := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) //nolint:errcheck,forcetypeassert + defer app.ReleaseCtx(c) + + c.route = &Route{Params: []string{"user"}} + c.path = []byte("/test/john") + c.values[0] = utils.UnsafeString(c.path[6:]) + + param := c.Params("user") + c.path[6] = 'p' + c.path[7] = 'a' + c.path[8] = 'u' + c.path[9] = 'l' + + require.Equal(t, tt.expected, param) + }) + } +}
🧹 Nitpick comments (2)
req.go (2)
150-152: Redundant lowercasing pass on encodingsheaderEncoding is already lowercased before splitting. Lowercasing each token again is redundant and adds overhead.
Apply this diff to drop the extra pass:
- for i := range encodingOrder { - encodingOrder[i] = utils.ToLower(encodingOrder[i]) - }
583-587: Params respects Immutable — good; minor nit to reuse local appReturning a copied param when Immutable is enabled is the right fix. You can reuse the app local var instead of calling r.App() again.
- val := values[i] - if r.App().config.Immutable { + val := values[i] + if app.config.Immutable { return utils.CopyString(val) } return val
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx_interface_gen.go(1 hunks)ctx_test.go(1 hunks)req.go(3 hunks)res_interface_gen.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ctx_test.go (3)
ctx.go (5)
DefaultCtx(47-67)DefaultCtx(116-118)DefaultCtx(128-130)DefaultCtx(140-142)DefaultCtx(304-306)router.go (1)
Route(43-62)req.go (1)
Params(608-614)
⏰ 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 (5)
res_interface_gen.go (1)
47-47: Doc update LGTMExpanding the supported content types list is accurate and consistent with the codebase.
ctx_interface_gen.go (1)
323-323: Doc update LGTMAutoFormat’s supported content types list looks correct and matches Res interface docs.
req.go (2)
140-142: Use of config-aware getter + normalization is correctSwitching to App.getString(...) and lowercasing the Content-Encoding header improves consistency with Immutable semantics and avoids unsafe conversions.
658-659: Protocol uses App.getString — goodSwitching to the config-aware getter aligns with the Immutable strategy and avoids unsafe conversions.
ctx_test.go (1)
3115-3131: No duplicate Test_Ctx_Params_Immutable found — resolvedRipgrep across the repository returned a single match at ctx_test.go:3115, so there is no duplicate test definition to remove.
- Location: ctx_test.go:3115 — func Test_Ctx_Params_Immutable(t *testing.T) { }
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3676 +/- ##
=======================================
Coverage 91.85% 91.85%
=======================================
Files 113 113
Lines 11453 11457 +4
=======================================
+ Hits 10520 10524 +4
Misses 669 669
Partials 264 264
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:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: f21b54c | Previous: 77667e5 | Ratio |
|---|---|---|---|
Benchmark_Ctx_Protocol |
4.362 ns/op 0 B/op 0 allocs/op |
2.49 ns/op 0 B/op 0 allocs/op |
1.75 |
Benchmark_Ctx_Protocol - ns/op |
4.362 ns/op |
2.49 ns/op |
1.75 |
This comment was automatically generated by workflow using github-action-benchmark.
Summary