feat(ctx): add conditional copy helpers #3703
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. WalkthroughAdds App-level immutability helpers GetString/GetBytes, renames internal conversion hooks to toString/toBytes, introduces runtime rodata checks, updates many call sites/tests/docs to use the new APIs, and standardizes numerous imports to the utils alias; no exported signature removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant App
participant Ctx
participant Req
Client->>App: incoming HTTP request
App->>Ctx: create context
Ctx->>Req: read raw headers/params/body ([]byte / string)
Req->>App: App.GetString(...) / App.GetBytes(...)
alt App.Immutable == true
App->>App: isReadOnly(ptr) check (runtime rodata)
alt aliasing detected
App->>App: clone/copy (strings.Clone / utils.CopyBytes)
App-->>Req: detached copy
else
App-->>Req: return original (no copy)
end
else
App-->>Req: return original (no copy)
end
Req-->>Ctx: safe value (string/[]byte)
Ctx-->>Client: produce response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds conditional copy helpers CopyString and CopyBytes to the Fiber context API to improve performance when the Immutable configuration is enabled. These helpers conditionally copy data based on the app's immutable setting, eliminating unnecessary allocations when data is already safe to use.
Key changes:
- Add
CopyStringandCopyBytesmethods to the Fiber context interface and implementation - Replace direct
utils.CopyStringandutils.CopyBytescalls with the new context methods across middleware - Update documentation to reflect the new API
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ctx.go | Implements the new CopyString and CopyBytes methods on DefaultCtx |
| ctx_interface_gen.go | Adds method signatures to the Ctx interface |
| req.go | Simplifies parameter copying logic using the new context method |
| res.go | Updates SendFile method to use context-aware copying |
| middleware/cache/cache.go | Replaces utils copy calls with context methods |
| middleware/cache/config.go | Updates default KeyGenerator to use context method |
| middleware/idempotency/idempotency.go | Updates key and body copying to use context methods |
| middleware/proxy/proxy.go | Updates URL copying to use context methods |
| docs/* | Updates documentation and examples to reflect the new API |
Comments suppressed due to low confidence (2)
middleware/cache/config.go:7
- The import of "github.com/gofiber/utils/v2" should be removed since it's no longer used after replacing utils.CopyString calls with c.CopyString.
)
middleware/cache/cache_test.go:19
- The import of "github.com/gofiber/utils/v2" should be removed since it's no longer used after replacing utils.CopyString calls with c.CopyString.
"github.com/stretchr/testify/require"
![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 @ReneWerner87, 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!
This pull request introduces new CopyString and CopyBytes helper methods to the fiber.Ctx context. These methods intelligently handle string and byte slice copying based on the application's Immutable configuration, aiming to optimize performance by avoiding unnecessary allocations when immutability is guaranteed. The changes also involve integrating these new helpers across various Fiber middleware and core components, along with comprehensive documentation updates.
Highlights
- New Context Helpers: Added CopyString and CopyBytes methods to fiber.Ctx to provide conditional string and byte slice copying.
- Interface Updates: Updated fiber.Ctx interface (ctx_interface_gen.go) to include the new CopyString and CopyBytes methods.
- Middleware Integration: Refactored cache, idempotency, and proxy middleware to utilize the new fiber.Ctx.CopyString and fiber.Ctx.CopyBytes helpers, replacing direct calls to utils.CopyString and utils.CopyBytes.
- Core Component Refactoring: Streamlined req.go and res.go to leverage the new context-aware copying methods for parameter and URL handling.
- Documentation Updates: Enhanced documentation in docs/api/ctx.md, docs/intro.md, and docs/whats_new.md to introduce and explain the new conditional copy helpers.
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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
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. ↩
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3703 +/- ##
==========================================
- Coverage 91.79% 91.77% -0.03%
==========================================
Files 114 115 +1
Lines 11536 11551 +15
==========================================
+ Hits 10590 10601 +11
- Misses 684 687 +3
- Partials 262 263 +1
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.
Code Review
This pull request introduces convenient CopyString and CopyBytes helpers on fiber.Ctx to conditionally copy data, improving performance when the Immutable setting is used. The changes are well-integrated across various middlewares and documentation.
My review focuses on two main points. First, I've suggested clarifying the comments for the new helper functions in ctx.go to better explain their behavior. More importantly, I've identified a potential issue in req.go where the Params method's behavior is altered to always return a safe string. This contradicts the existing documentation and the intended use of the new helpers, so I've proposed a fix to restore the expected behavior while still addressing an underlying bug from the previous implementation.
![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 (9)
docs/whats_new.md (1)
479-481: Add a brief migration note for custom Ctx implementersThese bullets are great; consider adding a note that the Ctx interface now includes these methods so custom contexts may need to implement/forward them.
Apply this patch to add a short callout under the two bullets:
- **CopyBytes**: Returns the byte slice directly when `Immutable` is enabled, or allocates a copy otherwise. - **CopyString**: Returns the string directly when `Immutable` is enabled, or allocates a copy otherwise. + + :::note Interface change for custom contexts + If you created a custom context via `NewWithCustomCtx` that implements `fiber.Ctx` manually (without embedding `fiber.Ctx`), + you must add the following methods to satisfy the interface: + + ```go + CopyBytes(b []byte) []byte + CopyString(s string) string + ``` + + Wrappers that embed `fiber.Ctx` typically continue to compile without changes. + :::docs/middleware/cache.md (1)
112-112: Minor wording fix in the config table (“KeyGenerator” row)Consistency/grammar nit: the description starts with “Key allows…”, but the property is “KeyGenerator”.
Apply this one-line tweak:
-| KeyGenerator | `func(fiber.Ctx) string` | Key allows you to generate custom keys. The HTTP method is appended automatically. | `func(c fiber.Ctx) string { return c.CopyString(c.Path()) }` | +| KeyGenerator | `func(fiber.Ctx) string` | KeyGenerator allows you to generate custom keys. The HTTP method is appended automatically. | `func(c fiber.Ctx) string { return c.CopyString(c.Path()) }` |middleware/proxy/proxy.go (1)
185-194: Use of c.CopyString to snapshot and restore URIs is correct; minor naming nitUsing c.CopyString for both the original and target URLs ensures stable snapshots in mutable mode and avoids double copies in immutable mode. One small readability tweak: the variable name copiedURL can mislead in immutable mode where no copy occurs. Consider targetURL for intent clarity.
Apply this diff locally:
- copiedURL := c.CopyString(addr) - req.SetRequestURI(copiedURL) + targetURL := c.CopyString(addr) + req.SetRequestURI(targetURL) // NOTE: if req.isTLS is true, SetRequestURI keeps the scheme as https. // Reference: https://github.com/gofiber/fiber/issues/1762 - if scheme := getScheme(utils.UnsafeBytes(copiedURL)); len(scheme) > 0 { + if scheme := getScheme(utils.UnsafeBytes(targetURL)); len(scheme) > 0 { req.URI().SetSchemeBytes(scheme) }ctx_test.go (1)
3154-3169: Test uses app.getString to decouple from backing bytes — goodUpdating c.values[0] to use c.app.getString(c.path[6:]) makes the immutability assertion robust even when the underlying path buffer is mutated. Consider adding a sibling test for Immutable=false that relies on CopyString at the call site to verify both code paths, if not already present elsewhere.
I can draft a companion test for the mutable case if you want.
docs/intro.md (2)
55-56: Clarify semantics and mention both helpers (CopyString and CopyBytes).Spell out that copies are conditional on Config.Immutable and surface CopyBytes alongside CopyString to keep the intro aligned with the new API.
-Fiber provides a `CopyString` method on the context that performs the above when needed. +Fiber provides `CopyString` and `CopyBytes` helpers on the context to make safe copies only when needed: +they return the input unchanged when `Immutable` is enabled, and otherwise allocate an independent copy.
58-64: Tweak example wording; optionally add a bytes example.“Variable is now immutable” is a bit imprecise—what matters is that it’s safe to persist beyond the handler. Also consider adding a short CopyBytes example right below to mirror the new API surface.
app.Get("/:foo", func(c fiber.Ctx) error { - // Variable is now immutable + // Safe to persist outside the handler result := c.CopyString(c.Params("foo")) // ... })Example for bytes (optional to include after this block):
app.Post("/echo", func(c fiber.Ctx) error { // Make a stable copy of the request body if needed safe := c.CopyBytes(c.Body()) // persist/use 'safe' beyond the handler if required return c.Send(safe) })middleware/cache/cache.go (1)
239-243: Cache persistence relies on CopyBytes’ Immutable semantics—verify safety.Switching to
c.CopyBytes(...)is consistent with the new API. One nuance: cache entries may outlive the request; inImmutablemodeCopyBytesreturns the original slice. This assumes request/response accessors already return stable (copied) memory underImmutable. If any accessor still exposes a reused buffer, cached content could alias mutable memory.Action:
- Verify (or add tests) that, with
Immutable: true, the following return stable memory:c.Response().Body(),Header.ContentType(),Header.Peek(Content-Encoding), andHeader.All()values.- If any accessor isn’t stable under
Immutable, consider always copying in the cache path for that value, or fix the accessor to honorImmutable.Happy to provide a targeted test that mutates the underlying buffers after handler completion to prove stability.
Also applies to: 262-262
docs/api/ctx.md (2)
629-631: Keep the lifetime warning crisp and consistent across sections.The info block reads well; consider ensuring this exact phrasing is reused in other sections to avoid drift. No change required if already deduped by docs tooling.
633-669: Polish wording and emphasize the no-op behavior under Immutable.Minor grammar and clarity improvement; also highlight that the helpers are a no-op (no allocation) when
Immutableis enabled.-Conditionally returns a safe copy of `b`. When the application is configured -with [`Immutable`](./ctx.md), the original slice is already safe to use and is -returned unchanged. Otherwise, a new copy is allocated. +Conditionally returns a safe copy of `b`. When the application is configured +with [`Immutable`](./ctx.md), the original slice is already safe to use and is +returned unchanged. Otherwise, a new copy is allocated.-Conditionally returns a safe copy of `s`. When [`Immutable`](./ctx.md) is -enabled, the string is returned as-is; otherwise a new copy is made. +Conditionally returns a safe copy of `s`. When [`Immutable`](./ctx.md) is +enabled, the string is returned as-is; otherwise, a new copy is made.Optional (structure): these helpers are generally applicable (not request-only). If you’re reorganizing the page, consider placing them in a short “Utilities” subsection near the top of Ctx to improve discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
ctx.go(1 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(1 hunks)docs/api/ctx.md(1 hunks)docs/intro.md(1 hunks)docs/middleware/cache.md(3 hunks)docs/whats_new.md(1 hunks)middleware/cache/cache.go(2 hunks)middleware/cache/cache_test.go(1 hunks)middleware/cache/config.go(2 hunks)middleware/idempotency/idempotency.go(2 hunks)middleware/proxy/proxy.go(1 hunks)req.go(1 hunks)res.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/whats_new.mddocs/middleware/cache.mddocs/api/ctx.mddocs/intro.md
🧠 Learnings (5)
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
docs/intro.md
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
docs/intro.md
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/intro.md
🪛 LanguageTool
docs/middleware/cache.md
[grammar] ~112-~112: There might be a mistake here.
Context: ...ing { return c.CopyString(c.Path()) }| | ExpirationGenerator |func(fiber.Ctx...
(QB_NEW_EN)
docs/api/ctx.md
[grammar] ~630-~630: There might be a mistake here.
Context: ...ead. Read more... ::: ### CopyBytes Conditionally return...
(QB_NEW_EN)
[grammar] ~654-~654: There might be a mistake here.
Context: ... of s. When Immutable is enabled, the string is returned as-is; o...
(QB_NEW_EN)
⏰ 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 (11)
ctx_interface_gen.go (1)
101-104: Action Required: Document Ctx interface expansion and migrate utils.Copy usage
- Confirmed that there are custom-Ctx entry points via
NewWithCustomCtxinapp.go,router_test.goandctx_test.go. EmbeddingDefaultCtxwill pull in the newCopyString/CopyBytesmethods, but any hand-rolledCustomCtximplementations not embeddingDefaultCtxmust add these two methods to satisfy the expandedCtxinterface.- Please add a brief migration note in the v3 “What’s New” or “Custom Ctx” documentation explaining that the
Ctxinterface has grown withCopyString(s string) stringandCopyBytes(b []byte) []byte, and include a snippet showing how existing custom‐Ctx authors can adapt their implementations.- Since
ctx_interface_gen.gois generated, update the generator/template so that runningmake generatein future doesn’t regress this change.- We found numerous references to
utils.CopyStringandutils.CopyBytesacross the codebase (e.g. inreq.go,res.go, various middleware/packages undermiddleware/…,binder/…,client/…, and tests). Audit these usages:
- For code paths that should honor the app’s
Immutableconfiguration, replace calls toutils.CopyString/utils.CopyByteswith the correspondingctx.CopyString/ctx.CopyBytes.- Retain direct
utils.Copy*calls only where an unconditional deep copy is still required regardless ofImmutable.- After making these updates, run a final search for
utils.CopyStringandutils.CopyBytesto verify no remaining calls in contexts that ought to respect the new Ctx semantics.middleware/idempotency/idempotency.go (2)
79-79: Switch to c.CopyString prevents ephemeral header aliasing — goodUsing c.CopyString(c.Get(...)) ensures the idempotency key remains valid outside the handler regardless of Immutable. Nice, no functional regressions.
122-122: c.CopyBytes for response body is the right durability boundaryStoring c.CopyBytes(c.Response().Body()) correctly avoids aliasing in non-Immutable mode and avoids double allocation in Immutable mode. Change is sound.
middleware/cache/cache_test.go (1)
566-567: Tests aligned to new APIUsing c.CopyString(c.Path()) in KeyGenerator keeps tests aligned with the new Ctx helpers. Import set already excludes utils/v2.
docs/middleware/cache.md (2)
81-81: Docs update to c.CopyString is correctUsing c.CopyString(c.Path()) in the example matches the runtime changes and avoids double copying under Immutable.
129-129: Default config snippet updated correctlyDefault KeyGenerator now uses c.CopyString(c.Path()), matching the middleware implementation and tests.
req.go (1)
593-594: Route param copying now goes through Ctx.CopyString — good centralizationSwitching to r.c.CopyString(val) delegates the immutable vs. copy semantics to the context helper, reducing duplication and preventing accidental unsafe-string aliasing when Immutable is false. Looks solid.
middleware/cache/config.go (1)
29-31: Key generator and docs aligned on c.CopyString — good move
- Comment default updated to reflect CopyString usage.
- ConfigDefault.KeyGenerator now returns c.CopyString(c.Path()), correctly delegating copy behavior to the context.
This keeps API consumers on the new helpers and avoids direct utils dependency.
Also applies to: 84-85
res.go (1)
777-779: SendFile: snapshot mutable fields with CopyString before URI mutations — LGTM
- r.c.pathOriginal = r.c.CopyString(r.c.pathOriginal) preserves the original path when the handler rewrites RequestURI.
- originalURL := r.c.CopyString(r.c.OriginalURL()) ensures the deferred restore uses a stable value across mutable/immutable modes.
These changes align with the new helpers and prevent aliasing bugs.
Also applies to: 809-810
docs/intro.md (1)
11-11: Replace placeholder release date before shipping.“Month xx, 202x” is still present; please set the actual Fiber v3 release date to avoid confusion in the published docs.
ctx.go (1)
265-279: Centralized conditional-copy helpers look good.The implementation correctly defers to app-level immutability and unifies copy semantics through the context. API shape is simple and discoverable.
Consider adding focused tests that verify identity vs. copy behavior:
- CopyString/CopyBytes return the same reference when
Immutable: true(no extra alloc).- They return independent copies when
Immutable: false.- Round-trip through common producers (Params, Query, Body, Response header peek) to ensure the helpers produce stable data suitable for persistence.
If you want, I can draft the tests.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/api/ctx.md (1)
628-631: Update Cookies guidance and add example using the new helpers.Good call-out on using App.ImmutableString/ImmutableBytes. Consider adding a short example to make it copy-pasteable for readers.
:::info Returned value is only valid within the handler. Do not store any references. Use [`App.ImmutableString`](./app.md#immutablestring) or [`App.ImmutableBytes`](./app.md#immutablebytes) to create copies when needed, or enable the [**`Immutable`**](./fiber.md#immutable) setting. [Read more...](../#zero-allocation) ::: +```go title="Example (safe persistence)" +app.Get("/", func(c fiber.Ctx) error { + safeName := c.App().ImmutableString(c.Cookies("name")) + safeBody := c.App().ImmutableBytes(c.Body()) + // use safeName/safeBody outside the handler scope if needed + return c.SendStatus(fiber.StatusOK) +}) +``` </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>app_test.go (3)</summary><blockquote> `29-29`: **Silence gosec G103 in tests or avoid unsafe entirely.** The test imports unsafe only to read string data addresses. Either: - Keep assertions and add a focused #nosec at the call sites, or - Avoid unsafe by asserting semantic equality and (optionally) allocations. Below adds #nosec per use (preferred minimal change). Apply this diff near the unsafe calls (see next comments for exact lines): ```diff - if unsafe.StringData(same) != unsafe.StringData(s) { + if unsafe.StringData(same) != unsafe.StringData(s) { // #nosec G103 - pointer read for test-only identity checkAnd similarly for the other unsafe.StringData comparison.
985-1001: Augment assertions and silence gosec at pointer reads.
- Add a content equality assertion to complement the pointer check.
- Add a zero-length edge case to exercise the early return.
- Add #nosec G103 at the unsafe.StringData call.
If you adopt the refactor that removes unsafe from app.go, these tests can remain as-is (they don’t validate conditional vs unconditional semantics). Here’s a small test polish:
func Test_App_ImmutableString(t *testing.T) { t.Parallel() s := "fiber" app := New() same := app.ImmutableString(s) - if unsafe.StringData(same) != unsafe.StringData(s) { + if unsafe.StringData(same) != unsafe.StringData(s) { // #nosec G103 - test-only identity check t.Errorf("expected original string when immutable is disabled") } + if same != s { + t.Errorf("content changed unexpectedly") + } appImmutable := New(Config{Immutable: true}) copied := appImmutable.ImmutableString(s) - if unsafe.StringData(copied) == unsafe.StringData(s) { + if unsafe.StringData(copied) == unsafe.StringData(s) { // #nosec G103 - test-only identity check t.Errorf("expected a copy when immutable is enabled") } + if copied != s { + t.Errorf("content differs after copy") + } + + // zero-length edge case + empty := "" + if got := appImmutable.ImmutableString(empty); got != "" { + t.Errorf("expected empty string passthrough, got %q", got) + } }
1002-1017: Add mutation-safety checks and zero-length coverage.
- Verify the returned slice content equals the input and is independent (modifying the source doesn’t affect the copy).
- Add a zero-length case.
func Test_App_ImmutableBytes(t *testing.T) { t.Parallel() b := []byte("fiber") app := New() same := app.ImmutableBytes(b) if &same[0] != &b[0] { t.Errorf("expected original slice when immutable is disabled") } + if string(same) != string(b) { + t.Errorf("content changed unexpectedly") + } appImmutable := New(Config{Immutable: true}) copied := appImmutable.ImmutableBytes(b) if &copied[0] == &b[0] { t.Errorf("expected a copy when immutable is enabled") } + if string(copied) != string(b) { + t.Errorf("content differs after copy") + } + // Mutation should not affect the copy + b[0] = 'F' + if copied[0] == 'F' { + t.Errorf("copied slice shares backing array with original") + } + + // zero-length edge case + var empty []byte + if got := appImmutable.ImmutableBytes(empty); got != nil && len(got) != 0 { + t.Errorf("expected empty slice passthrough, got %#v", got) + } }docs/intro.md (1)
55-61: Clarify semantics: these helpers copy when Immutable is enabled.Current wording “perform the above when Immutable is enabled” is vague and suggests conditional behavior. The implementation and tests copy unconditionally under Immutable. Make this explicit to avoid confusion.
-Fiber provides `ImmutableString` and `ImmutableBytes` methods on the app that perform the above when `Immutable` is enabled. +Fiber provides `ImmutableString` and `ImmutableBytes` methods on the app that copy the input when `Immutable` is enabled.Optionally add a sentence explaining that when Immutable is disabled, they return the input unchanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
app.go(2 hunks)app_test.go(2 hunks)docs/api/app.md(1 hunks)docs/api/ctx.md(13 hunks)docs/intro.md(1 hunks)docs/middleware/cache.md(1 hunks)docs/whats_new.md(1 hunks)req.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- req.go
- docs/whats_new.md
- docs/middleware/cache.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/app.mddocs/api/ctx.mddocs/intro.md
🧠 Learnings (3)
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
app_test.go
📚 Learning: 2025-07-17T11:39:38.265Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the `Cookie.CopyTo(src *Cookie)` method copies the source cookie (parameter) to the receiver cookie, so `nc.CopyTo(c)` copies cookie `c` into cookie `nc`.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.md
🧬 Code graph analysis (1)
app_test.go (1)
app.go (2)
New(515-628)Config(113-418)
🪛 GitHub Check: lint
app_test.go
[failure] 997-997:
G103: Use of unsafe calls should be audited (gosec)
[failure] 991-991:
G103: Use of unsafe calls should be audited (gosec)
app.go
[failure] 661-661:
G103: Use of unsafe calls should be audited (gosec)
[failure] 647-647:
G103: Use of unsafe calls should be audited (gosec)
🪛 GitHub Actions: golangci-lint
app.go
[error] 647-647: golangci-lint (gosec) G103: Use of unsafe calls should be audited.
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~108-~108: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeader Returns the HTTP...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeaders Returns the HTT...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRouteURL Generates URLs to ...
(QB_NEW_EN)
[grammar] ~569-~569: There might be a mistake here.
Context: ...ead. Read more... ::: ### BodyRaw Returns the raw reques...
(QB_NEW_EN)
[grammar] ~591-~591: There might be a mistake here.
Context: ...ead. Read more... ::: ### ClientHelloInfo `ClientHelloIn...
(QB_NEW_EN)
[grammar] ~630-~630: There might be a mistake here.
Context: ...ing. Read more... ::: ### FormFile MultipartForm files ca...
(QB_NEW_EN)
[grammar] ~710-~710: There might be a mistake here.
Context: ...ead. Read more... ::: ### Host Returns the host derived ...
(QB_NEW_EN)
[grammar] ~736-~736: There might be a mistake here.
Context: ...ead. Read more... ::: ### Hostname Returns the hostname ...
(QB_NEW_EN)
[grammar] ~759-~759: There might be a mistake here.
Context: ...ead. Read more... ::: ### IP Returns the remote IP addre...
(QB_NEW_EN)
[grammar] ~957-~957: There might be a mistake here.
Context: ...ead. Read more... ::: ### Params This method can be used...
(QB_NEW_EN)
[grammar] ~1008-~1008: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
[grammar] ~1185-~1185: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
⏰ 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). (3)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/api/ctx.md (1)
629-631: Clarify helper naming here (Ctx.Copy vs App.Immutable), and include a short example**This block currently recommends App.ImmutableString/ImmutableBytes. The PR objective mentions new Ctx-level CopyString/CopyBytes helpers, and there are prior review comments suggesting examples using them. Please align the guidance to the final API name and add a one-liner example for quick copy usage.
If the final API is Ctx.CopyString/CopyBytes, consider this doc tweak:
-Use [`App.ImmutableString`](./app.md#immutablestring) or [`App.ImmutableBytes`](./app.md#immutablebytes) to create copies when needed, or enable the [**`Immutable`**](./fiber.md#immutable) setting. [Read more...](../#zero-allocation) +Prefer [`Ctx.CopyString`](./ctx.md#copystring) or [`Ctx.CopyBytes`](./ctx.md#copybytes) to create copies when needed: + +```go +safeName := c.CopyString(c.Cookies("name")) +safeBody := c.CopyBytes(c.Body()) +``` + +Alternatively, use [`App.ImmutableString`](./app.md#immutablestring) or [`App.ImmutableBytes`](./app.md#immutablebytes), or enable the [**`Immutable`**](./fiber.md#immutable) setting. [Read more...](../#zero-allocation)To confirm which API landed across the codebase, run:
#!/bin/bash # Verify presence of Ctx-level copy helpers vs App-level immutable helpers rg -nP --type=go -C2 '\bCopyString\s*\(|\bCopyBytes\s*\(' rg -nP --type=go -C2 '\bImmutableString\s*\(|\bImmutableBytes\s*\('
🧹 Nitpick comments (2)
req.go (1)
181-182: Avoid an extra allocation for decoded bodies in Immutable mode (optional)
Body()decodes into a freshly allocatedbodyslice when Content-Encoding is present. Wrapping that again withapp.ImmutableBytescan cause a redundant copy whenImmutableis enabled. It’s safe to returnbodyas-is after decoding because it no longer aliases fasthttp’s internal buffer. Consider skipping the extra copy post-decode and rely ongetBody()to apply immutability when no decoding occurs.- return r.c.app.ImmutableBytes(body) + // body here is from decoding (fresh allocation). No need to copy again. + return bodyIf you prefer absolute consistency (always “immutable” return in Immutable mode), keep as-is; the above is purely an allocation win.
app_test.go (1)
1002-1017: Test_App_ImmutableBytes: complements string test; consider guarding against empty slicesGreat coverage mirroring the string case. Minor nit: if this ever regresses to testing an empty slice,
&b[0]will panic. Not an issue today since"fiber"is non-empty.Optionally add a quick guard to future-proof:
require.NotZero(t, len(b))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app.go(2 hunks)app_test.go(2 hunks)docs/api/ctx.md(13 hunks)req.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app.go
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.md
🧠 Learnings (3)
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
app_test.go
📚 Learning: 2025-07-17T11:39:38.265Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the `Cookie.CopyTo(src *Cookie)` method copies the source cookie (parameter) to the receiver cookie, so `nc.CopyTo(c)` copies cookie `c` into cookie `nc`.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.md
🧬 Code graph analysis (1)
app_test.go (1)
app.go (2)
New(515-628)Config(113-418)
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~108-~108: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeader Returns the HTTP...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeaders Returns the HTT...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRouteURL Generates URLs to ...
(QB_NEW_EN)
[grammar] ~569-~569: There might be a mistake here.
Context: ...ead. Read more... ::: ### BodyRaw Returns the raw reques...
(QB_NEW_EN)
[grammar] ~591-~591: There might be a mistake here.
Context: ...ead. Read more... ::: ### ClientHelloInfo `ClientHelloIn...
(QB_NEW_EN)
[grammar] ~630-~630: There might be a mistake here.
Context: ...ing. Read more... ::: ### FormFile MultipartForm files c...
(QB_NEW_EN)
[grammar] ~711-~711: There might be a mistake here.
Context: ...ead. Read more... ::: ### Host Returns the host derived ...
(QB_NEW_EN)
[grammar] ~737-~737: There might be a mistake here.
Context: ...ead. Read more... ::: ### Hostname Returns the hostname ...
(QB_NEW_EN)
[grammar] ~760-~760: There might be a mistake here.
Context: ...ead. Read more... ::: ### IP Returns the remote IP addre...
(QB_NEW_EN)
[grammar] ~958-~958: There might be a mistake here.
Context: ...ead. Read more... ::: ### Params This method can be used...
(QB_NEW_EN)
[grammar] ~1009-~1009: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
[grammar] ~1186-~1186: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
⏰ 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 (4)
docs/api/ctx.md (1)
108-109: Anchor updates to fiber.md#immutable look correctThe repeated link changes to point at fiber.md#immutable are consistent and keep the zero-allocation “Read more…” note. No issues spotted.
Also applies to: 134-135, 147-148, 569-570, 591-592, 711-712, 737-738, 760-761, 958-959, 1009-1010, 1186-1187
req.go (2)
913-914: Centralizing immutability in getBody() is a good cleanupWrapping
Request.Body()once ingetBody()keepsBody()/BodyRaw()semantics consistent and reduces duplication. Looks good.
590-591: Preserve safety in Immutable mode while avoiding double‐copyThe current implementation of Params relies on
r.c.app.ImmutableString(val)to produce a safe, standalone copy whenImmutableis enabled—but it does so via two successive allocations ([]byte(string) → string), and simply returningvalwould bypass that safety guarantee. Instead, we should perform one copy whenImmutableis on and no copy otherwise.• Location:
req.go, inDefaultReq.Params
• Replace the return site in the matching branch:- val := values[i] - return r.c.app.ImmutableString(val) + val := values[i] + // In Immutable mode, make a single copy of the param string + if app.config.Immutable { + return utils.CopyString(val) + } + // Otherwise, return the zero‐copy view + return valAlternatively, you can refactor
App.ImmutableStringitself to useutils.CopyString(one allocation) instead of chaininggetBytesImmutable→getStringImmutable, so the existing call site remains unchanged:func (app *App) ImmutableString(s string) string { - if !app.config.Immutable { - return s - } - return app.getString(app.getBytes(s)) + if !app.config.Immutable { + return s + } + // Single allocation for safe, standalone string + return utils.CopyString(s) }Likely an incorrect or invalid review comment.
app_test.go (1)
985-1000: Test_App_ImmutableString: targeted and effective pointer-identity checkThe
unsafe.StringDatachecks are appropriate for validating the conditional copy behavior. Nice test.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/api/app.md (1)
12-18: Docs still claim conditional copying; verify implementation and align wording.The descriptions for ImmutableString/ImmutableBytes say “returns a detached copy … if it still references request or response memory.” In past iterations, the implementation copied unconditionally when Immutable was enabled. If that remains true, please update the wording to reflect unconditional copying. If the code now truly detects aliasing and copies conditionally, keep the current phrasing but confirm via a quick grep (see script below). Until verified, this mismatch risks confusing users.
Doc-only fix (if copy is unconditional under Immutable):
-When [`Immutable`](./fiber.md#immutable) is enabled, returns a detached copy of `s` if it still references request or response memory. If immutability is disabled, `s` is returned unchanged. +When [`Immutable`](./fiber.md#immutable) is enabled, returns a detached copy of `s`. If immutability is disabled, `s` is returned unchanged. @@ -When [`Immutable`](./fiber.md#immutable) is enabled, returns a detached copy of `b` if it still references request or response memory. If immutability is disabled, `b` is returned unchanged. +When [`Immutable`](./fiber.md#immutable) is enabled, returns a detached copy of `b`. If immutability is disabled, `b` is returned unchanged.To verify behavior in code:
#!/bin/bash # Verify whether App.ImmutableString/ImmutableBytes copy unconditionally or conditionally. rg -nP 'func\s+\(app\s+\*App\)\s+Immutable(String|Bytes)\b' -n -C3 rg -nP 'Immutable(String|Bytes)\b' app.go -C5 || true # Heuristic check: look for alias/provenance checks vs straight copy rg -nP 'unsafe\.StringData|Unsafe(String|Bytes)|reflect\.SliceHeader|copy\(' app.go -C3 || trueAlso applies to: 20-26
docs/api/ctx.md (1)
628-631: Recommend c.CopyString here (not App.Immutable) to match ctx-level helpers.*The Cookies section advises using App.ImmutableString/ImmutableBytes. Since Cookies returns a string and this PR adds ctx-level conditional copy helpers, docs should show
c.CopyString(c.Cookies("name"))as the primary pattern. This also mirrors prior reviewer feedback.-Returned value is only valid within the handler. Do not store any references. -Use [`App.ImmutableString`](./app.md#immutablestring) or [`App.ImmutableBytes`](./app.md#immutablebytes) when immutability is enabled, or manually copy values (for example with [`utils.CopyString`](https://github.com/gofiber/utils) / `utils.CopyBytes`) when it's disabled. [Read more...](../#zero-allocation) +Returned value is only valid within the handler. Do not store any references. +Use `c.CopyString(c.Cookies("name"))` (preferred), or enable [`Immutable`](./fiber.md#immutable). +If `Immutable` is disabled, manually copy values (e.g., with [`utils.CopyString`](https://github.com/gofiber/utils)). [Read more...](../#zero-allocation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
docs/api/app.md(1 hunks)docs/api/ctx.md(13 hunks)docs/intro.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/intro.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.mddocs/api/app.md
🧠 Learnings (2)
📚 Learning: 2025-07-17T11:39:38.265Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the `Cookie.CopyTo(src *Cookie)` method copies the source cookie (parameter) to the receiver cookie, so `nc.CopyTo(c)` copies cookie `c` into cookie `nc`.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.md
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~108-~108: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeader Returns the HTTP...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeaders Returns the HTT...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRouteURL Generates URLs to ...
(QB_NEW_EN)
[grammar] ~569-~569: There might be a mistake here.
Context: ...ead. Read more... ::: ### BodyRaw Returns the raw reques...
(QB_NEW_EN)
[grammar] ~591-~591: There might be a mistake here.
Context: ...ead. Read more... ::: ### ClientHelloInfo `ClientHelloIn...
(QB_NEW_EN)
[grammar] ~630-~630: There might be a mistake here.
Context: ...led. Read more... ::: ### FormFile MultipartForm files c...
(QB_NEW_EN)
[grammar] ~711-~711: There might be a mistake here.
Context: ...ead. Read more... ::: ### Host Returns the host derived ...
(QB_NEW_EN)
[grammar] ~737-~737: There might be a mistake here.
Context: ...ead. Read more... ::: ### Hostname Returns the hostname ...
(QB_NEW_EN)
[grammar] ~760-~760: There might be a mistake here.
Context: ...ead. Read more... ::: ### IP Returns the remote IP addre...
(QB_NEW_EN)
[grammar] ~958-~958: There might be a mistake here.
Context: ...ead. Read more... ::: ### Params This method can be used...
(QB_NEW_EN)
[grammar] ~1009-~1009: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
[grammar] ~1186-~1186: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
🪛 GitHub Check: markdownlint
docs/api/app.md
[failure] 28-28: Multiple consecutive blank lines
docs/api/app.md:28 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md012.md
🪛 GitHub Actions: markdownlint
docs/api/app.md
[error] 28-28: markdownlint-cli2-action: MD012/no-multiple-blanks: Multiple consecutive blank lines (Expected: 1; Actual: 2).
⏰ 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). (3)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: Compare
|
@coderabbitai should we rename the new functions? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the implementation of these new functions to provide a well-informed recommendation about their naming. 🏁 Script executed: Length of output: 138 🏁 Script executed: Length of output: 1731 🏁 Script executed: Length of output: 117 🏁 Script executed: Length of output: 3455 🏁 Script executed: Length of output: 275 🏁 Script executed: Length of output: 1447 @gaby Looking at the implementation and usage of these functions, I can see the reasoning behind the current names, but there are some considerations for potential renaming. Current Implementation AnalysisThe
Naming ConsiderationsPotential Issues with Current Names:
Alternative Naming Options:
RecommendationI'd suggest considering
The documentation would then read: "SafeString ensures the returned string doesn't reference request or response memory when Immutable is enabled..." What do you think about this naming suggestion? |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/api/ctx.md (1)
106-109: Unify immutability guidance: reference the new conditional copy helpers consistently.Many info boxes still say “Make copies or use Immutable,” which misses the new helpers. Recommend consistent wording that prioritizes the helpers and clarifies options when Immutable is disabled.
Apply the pattern below to each affected info box:
-Returned value is only valid within the handler. Do not store any references. -Make copies or use the [**`Immutable`**](./fiber.md#immutable) setting instead. [Read more...](../#zero-allocation) +Returned value is only valid within the handler. Do not store any references. +Prefer `c.App().SafeString` / `c.App().SafeBytes` for conditional safe copying, or enable [**`Immutable`**](./fiber.md#immutable). +If `Immutable` is disabled and you need to retain data beyond the handler, manually copy (e.g., with `utils.CopyString` / `utils.CopyBytes`). [Read more...](../#zero-allocation)Affected sections in this file include the info boxes under GetReqHeaders, GetRespHeader, GetRespHeaders, Body, BodyRaw, FormValue, Get, Host, Hostname, Params, and Query.
Also applies to: 132-135, 145-148, 567-570, 589-592, 671-674, 709-712, 735-738, 758-761, 956-959, 1007-1010, 1184-1187
🧹 Nitpick comments (3)
app_test.go (2)
1002-1017: Mirror the aliasing scenario for bytes and use unsafe.SliceData to avoid panics on empty slices.
- Construct b as a subslice of a larger buffer to guarantee aliasing semantics are exercised.
- Replace &b[0] address checks with unsafe.SliceData to be robust if future edits use empty slices.
Proposed patch:
func Test_App_SafeBytes(t *testing.T) { t.Parallel() - b := []byte("fiber") - app := New() - same := app.SafeBytes(b) - if &same[0] != &b[0] { - t.Errorf("expected original slice when immutable is disabled") - } + // Create an aliased slice backed by a larger buffer. + parent := []byte("xxfiber") + b := parent[2:] // "fiber" + + app := New() + same := app.SafeBytes(b) + // With Immutable disabled, SafeBytes should not force a copy. + if unsafe.SliceData(same) != unsafe.SliceData(b) { + t.Errorf("expected original slice when immutable is disabled") + } appImmutable := New(Config{Immutable: true}) copied := appImmutable.SafeBytes(b) - if &copied[0] == &b[0] { - t.Errorf("expected a copy when immutable is enabled") - } + // With Immutable enabled and aliased input, SafeBytes must copy. + if unsafe.SliceData(copied) == unsafe.SliceData(b) { + t.Errorf("expected a copy when immutable is enabled for aliased input") + } }Additional coverage (outside this hunk) worth adding:
- SafeBytes(nil) returns nil and remains safe to retain.
- SafeBytes([]byte{}) returns an empty slice (len=0) and remains safe to retain.
- SafeString("") returns an empty string and remains safe to retain.
I can provide those subtests if helpful.
985-1000: Refactor Test_App_SafeString to exercise true aliasing behaviorThe current test in app_test.go uses a string literal for both branches, which over-constrains the helper’s intended semantics and doesn’t actually validate the “copy only if aliased” behavior documented on SafeString. Instead, construct a string that shares its backing memory with a byte slice to simulate a request-buffer alias and:
- Preserve the existing non-Immutable (disabled) check against the original literal.
- For the Immutable case, build an aliased string from a byte slice and assert that it’s copied.
- (Optional) Add a sub-test using a literal to document that copying isn’t guaranteed for non-aliased inputs.
Suggested changes in app_test.go (around lines 985–1000):
func Test_App_SafeString(t *testing.T) { t.Parallel() - s := "fiber" - app := New() - same := app.SafeString(s) - if unsafe.StringData(same) != unsafe.StringData(s) { //nolint:gosec - t.Errorf("expected original string when immutable is disabled") - } + // A plain literal should never be forced to copy when Immutable is disabled. + literal := "fiber" + app := New() + same := app.SafeString(literal) + if unsafe.StringData(same) != unsafe.StringData(literal) { //nolint:gosec + t.Errorf("expected original literal to be returned when Immutable=false") + } + // Build a string that truly aliases slice data (e.g. request buffer). + src := []byte("fiber") + alias := unsafe.String(unsafe.SliceData(src), len(src)) + appImmutable := New(Config{Immutable: true}) + copied := appImmutable.SafeString(alias) + // Immutable=true must force a copy when the input is aliased. + if unsafe.StringData(copied) == unsafe.StringData(alias) { //nolint:gosec + t.Errorf("expected aliased input to be copied when Immutable=true") + } + // Optional: demonstrate that literals are not guaranteed to copy. + t.Run("literal under Immutable", func(t *testing.T) { + c := New(Config{Immutable: true}).SafeString(literal) + // Only aliasing forces copy; literals may or may not be copied. + _ = c + }) }
- File: app_test.go (lines ~985–1000)
- Key changes: replace the single literal s with two scenarios—one for a true alias and one for the literal under Immutable=false.
- Optional sub-test added to document that literal inputs aren’t guaranteed to copy under Immutable=true.
docs/api/ctx.md (1)
611-626: Double-check naming and call site consistency (Ctx vs App helpers).This page is about Ctx, but the guidance points to App.SafeString/SafeBytes. If the final API is App-level only, include a short example showing the call site:
// example safe := c.App().SafeString(c.Cookies("name"))If the finalized API is instead
c.CopyString/c.CopyBytes, update references accordingly across ctx.md, app.md, and what’s new.This avoids readers concluding the helpers are on Ctx due to the surrounding context.
Also applies to: 969-1011, 1160-1187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
app.go(2 hunks)app_test.go(2 hunks)docs/api/app.md(5 hunks)docs/api/ctx.md(13 hunks)docs/intro.md(1 hunks)docs/whats_new.md(1 hunks)req.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/intro.md
- app.go
- req.go
- docs/whats_new.md
- docs/api/app.md
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.md
🧠 Learnings (3)
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
app_test.go
📚 Learning: 2025-07-17T11:39:38.265Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the `Cookie.CopyTo(src *Cookie)` method copies the source cookie (parameter) to the receiver cookie, so `nc.CopyTo(c)` copies cookie `c` into cookie `nc`.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.md
🧬 Code graph analysis (1)
app_test.go (1)
app.go (2)
New(515-628)Config(113-418)
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~108-~108: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeader Returns the HTTP...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeaders Returns the HTT...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRouteURL Generates URLs to ...
(QB_NEW_EN)
[grammar] ~569-~569: There might be a mistake here.
Context: ...ead. Read more... ::: ### BodyRaw Returns the raw reques...
(QB_NEW_EN)
[grammar] ~591-~591: There might be a mistake here.
Context: ...ead. Read more... ::: ### ClientHelloInfo `ClientHelloIn...
(QB_NEW_EN)
[grammar] ~630-~630: There might be a mistake here.
Context: ...led. Read more... ::: ### FormFile MultipartForm files c...
(QB_NEW_EN)
[grammar] ~711-~711: There might be a mistake here.
Context: ...ead. Read more... ::: ### Host Returns the host derived ...
(QB_NEW_EN)
[grammar] ~737-~737: There might be a mistake here.
Context: ...ead. Read more... ::: ### Hostname Returns the hostname ...
(QB_NEW_EN)
[grammar] ~760-~760: There might be a mistake here.
Context: ...ead. Read more... ::: ### IP Returns the remote IP addre...
(QB_NEW_EN)
[grammar] ~958-~958: There might be a mistake here.
Context: ...ead. Read more... ::: ### Params This method can be used...
(QB_NEW_EN)
[grammar] ~1009-~1009: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
[grammar] ~1186-~1186: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
⏰ 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). (3)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
app_test.go (1)
29-29: Unsafe import in tests is fine; no action needed.Using unsafe here is justified to assert backing memory behavior. The module already targets newer Go versions, so
unsafe.StringData/unsafe.SliceDatausage is appropriate.docs/api/ctx.md (1)
106-109: No broken anchor: “Zero Allocation” exists in docs/intro.md
The heading### Zero Allocationin docs/intro.md generates the
#zero-allocationanchor at the site root. All occurrences of[Read more...](../#zero-allocation)in docs/api/ctx.md and docs/api/bind.md correctly resolve to that section. No changes are needed.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces SafeString and SafeBytes helpers to conditionally copy data when the Immutable setting is enabled. This is a good initiative to centralize and clarify the logic for handling data from request/response buffers. The changes also include updating documentation and refactoring existing code to use these new helpers.
However, the implementation of SafeString and SafeBytes appears to be flawed. They unconditionally copy data when Immutable is true, rather than checking if the data actually comes from a request buffer. This can lead to unnecessary allocations and performance issues. The tests for these helpers also seem to confirm this incorrect behavior. My review includes detailed comments on these issues in app.go and app_test.go.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
helpers.go (2)
112-139: Use-after-free risk: returning string backed by pooled buffer (bytebufferpool) in quoteRawStringSame issue: app.toString(bb.B) can alias pooled memory when Immutable=false. Copy before returning.
Apply:
func (app *App) quoteRawString(raw string) string { @@ - return app.toString(bb.B) + // Copy because bb will be returned to the pool via defer. + return string(bb.B) }
99-106: Review & Fix Unsafe Pooled-Buffer toString/toBytes UsagesWe found multiple sites where a pooled
bytebufferpoolbuffer is converted viatoStringortoBytesand then returned or stored after the buffer has been put back into the pool, which can lead to dangling-memory bugs if your conversions alias the original slice:• helpers.go (quoteString) – lines 99–106
bb := bytebufferpool.Get() quoted := app.toString(fasthttp.AppendQuotedArg(bb.B, app.toBytes(raw))) bytebufferpool.Put(bb) return quoted(originally flagged)
• helpers.go (percent-encoding helper) – lines 112–139
bb := bytebufferpool.Get() // … append to bb.B … bytebufferpool.Put(bb) // confirm there’s a Put here return app.toString(bb.B) // toString on a pooled‐buffer slice(ensure that you don’t alias bb.B into the returned string post-Put)
• res.go – DefaultRes.Links (HeaderLink) – lines 540–543
bb := bytebufferpool.Get() // … build Link header in bb … r.setCanonical(HeaderLink, utils.TrimRight(r.c.app.toString(bb.Bytes()), ',')) bytebufferpool.Put(bb)(string result may share bb.Bytes() after Put)
• redirect.go – Redirect with query strings – lines 261–275
queryText := bytebufferpool.Get() defer bytebufferpool.Put(queryText) // … build queryText … return r.To(location + "?" + r.c.app.toString(queryText.Bytes()))(toString called on pooled buffer and returned after Put)
Please update these sites to ensure that your
toString/toBytesimplementations allocate or copy the data before the buffer is returned to the pool—e.g., by using a safestring(bytes)copy or by postponing the pool.Put until after you’ve made an owned copy of the data.
♻️ Duplicate comments (3)
docs/api/app.md (1)
20-26: GetBytes docs rely on conditions that aren’t actually reliable for []byte
- “read-only memory” doesn’t apply to Go byte slices; the rodata pointer-range check will never be true for
[]byte.- Treating
cap(b) == len(b)as “already detached” is unsound; a slice can fully span a buffer that will be reused after the handler returns (e.g., fasthttp’s request/response buffers).This can lead to values being returned unchanged under
Immutable, violating the immutability contract.Two options:
A) Fix the code to always copy non-empty
[]bytewhenImmutableis enabled (recommended), then keep docs simple:-Returns `b` unchanged when [`Immutable`](./fiber.md#immutable) is disabled, `b` resides in read-only memory, or `cap(b) == len(b)`. Otherwise it returns a detached copy. +When [`Immutable`](./fiber.md#immutable) is disabled, returns `b` unchanged. When it is enabled, returns a detached copy of `b`.B) If keeping conditional behavior, remove the “read-only memory” clause for bytes and drop the
cap==lenclaim, or clearly mark them as best-effort heuristics with the warning that they may still require a copy depending on provenance.docs/api/ctx.md (1)
106-109: Unify the immutability guidance across info boxes and reference the new helpersSeveral “Returned value is only valid…” notes still say “Make copies or use Immutable” but don’t mention the new
App.GetString/App.GetByteshelpers. Please update all repeated boxes for consistency.Example patch (apply consistently to each occurrence):
-Returned value is only valid within the handler. Do not store any references. -Make copies or use the [**`Immutable`**](./fiber.md#immutable) setting instead. [Read more...](../#zero-allocation) +Returned value is only valid within the handler. Do not store any references. +Prefer `c.App().GetString` / `c.App().GetBytes` for conditional safe copying when [`Immutable`](./fiber.md#immutable) is enabled. +If `Immutable` is disabled and you need to persist values, manually copy (e.g., with `utils.CopyString` / `utils.CopyBytes`). [Read more...](../#zero-allocation)Verification script to ensure no stragglers remain:
#!/bin/bash # Find info boxes that still lack App.GetString/GetBytes guidance rg -nP --type=md -C2 'Returned value is only valid within the handler\..*Immutable' docs/api/ctx.md \ | sed -n '1,200p'Also applies to: 132-135, 145-148, 567-570, 589-592, 709-712, 735-738, 758-761, 956-959, 1007-1010, 1186-1187
req.go (1)
584-592: Params(): confirm GetString is conditionally copying (behavioral impact risk)
return r.c.app.GetString(val)is correct if and only ifGetString:
- Copies when Immutable is enabled and
valaliases request/response memory.- Returns
valunchanged when Immutable is disabled orvalis already detached.If
GetStringalways copies, this would changec.Params()semantics to “always safe-to-retain,” contradicting the established contract and the docs/examples where users callc.CopyString(c.Params("foo"))when they need to retain values beyond the handler.Action:
- Verify
App.GetStringbehavior and its alias-detection/Immutable gate.- If it’s not conditionally copying, revert to
return valhere to preserve established behavior.Run this to confirm the implementation and call sites:
#!/bin/bash # 1) Inspect App.GetString to ensure conditional copy semantics rg -nP 'func\s+\(.*\*\s*App\)\s+GetString\s*\(' -C3 # 2) Check whether GetString consults Immutable or alias range checks rg -nP 'GetString\s*\(.*\)\s*{' -A50 | rg -nP 'Immutable|alias|request|response|copy|unsafe' -n -C2 # 3) Ensure ctx-level CopyString exists and is used in docs/tests rg -nP '\bCopyString\s*\(' -n -C2 rg -nP '\bCopyBytes\s*\(' -n -C2 # 4) Find all Params() returns to ensure no accidental global behavior change rg -nP '(?s)func\s+\(r\s+\*DefaultReq\)\s+Params\(' -n -C3If
GetStringis conditionally copying as intended, this change is good and fixes the previous double-copy bug while keeping runtime behavior consistent.
🧹 Nitpick comments (5)
docs/intro.md (1)
55-55: Wording tweak: make the “when Immutable is disabled” caveat explicit about safetyGood guidance overall. One nit: explicitly call out that
App.GetString/App.GetByteswill not allocate whenImmutableis disabled, so the returned values are unsafe to persist. This helps prevent accidental misuse.Apply:
-Fiber provides `GetString` and `GetBytes` methods on the app that detach values when `Immutable` is enabled and the data isn't already read-only. When it's disabled, use `utils.CopyString` and `utils.CopyBytes` to allocate only when needed. +Fiber provides `GetString` and `GetBytes` methods on the app that detach values when `Immutable` is enabled (they may return the original value if it’s already read-only). When `Immutable` is disabled, these helpers return the original value (no copy), so use `utils.CopyString` / `utils.CopyBytes` if you need to persist it beyond the handler.app.go (1)
639-649: String path: OK, but keep an eye on portability of isReadOnly
GetString’s behavior matches the docs. TheisReadOnly(unsafe.StringData(s))relies on thereadonly.golinkname hack—ensure that file is guarded or has a fallback so this doesn’t break builds on unsupported toolchains/platforms.If portability concerns arise, a safe alternative is to always copy when
Immutableis on, and drop the rodata probe. Otherwise, please keep the inline justification for the unsafe call as-is to satisfy gosec.redirect.go (1)
274-275: Encode query parameters to avoid malformed URLsThe manual concatenation of queries does not URL-encode keys/values. For safety and interoperability, encode both. Deterministic ordering (sorted keys) can also improve test stability.
Apply something like:
@@ - i := 1 - for k, v := range cfg.Queries { - queryText.WriteString(k + "=" + v) - - if i != len(cfg.Queries) { - queryText.WriteString("&") - } - i++ - } + keys := make([]string, 0, len(cfg.Queries)) + for k := range cfg.Queries { + keys = append(keys, k) + } + sort.Strings(keys) + for i, k := range keys { + v := cfg.Queries[k] + queryText.WriteString(url.QueryEscape(k)) + queryText.WriteByte('=') + queryText.WriteString(url.QueryEscape(v)) + if i < len(keys)-1 { + queryText.WriteByte('&') + } + }And add imports:
+ "net/url" + "sort"res.go (1)
496-505: Harden JSONP callback to prevent injectionJSONP remains susceptible if callback contains unsafe characters. Consider validating the callback against a safe identifier pattern (e.g., JS identifier with dot-notation) before writing the response.
Example:
@@ - if len(callback) > 0 { - cb = callback[0] - } else { - cb = "callback" - } + if len(callback) > 0 { + cb = callback[0] + } else { + cb = "callback" + } + // allow identifiers/dotted paths only + if !isSafeCallback(cb) { + return NewError(StatusBadRequest, "invalid callback") + } @@ - result = cb + "(" + r.c.app.toString(raw) + ");" + result = cb + "(" + r.c.app.toString(raw) + ");"Add helper (once per package):
var cbRx = regexp.MustCompile(`^[A-Za-z_$][0-9A-Za-z_$]*(\.[A-Za-z_$][0-9A-Za-z_$]*)*$`) func isSafeCallback(s string) bool { return cbRx.MatchString(s) }req.go (1)
285-295: GetReqHeader(): correct conversion; fix minor doc typo
- Using
c.App().toString(c.Request().Header.Peek(key))is the right choice for ephemeral header bytes before generic parsing.Also, minor typo in the comment above: “filed” → “field”.
Apply this diff to fix the typo:
-// GetReqHeader returns the HTTP request header specified by filed. +// GetReqHeader returns the HTTP request header specified by field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
app.go(5 hunks)app_test.go(4 hunks)ctx.go(6 hunks)ctx_test.go(3 hunks)docs/api/app.md(5 hunks)docs/api/ctx.md(13 hunks)docs/intro.md(1 hunks)docs/whats_new.md(1 hunks)helpers.go(6 hunks)readonly.go(1 hunks)redirect.go(2 hunks)req.go(15 hunks)res.go(7 hunks)router_test.go(15 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/whats_new.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ctx_test.go
- app_test.go
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.mddocs/intro.mddocs/api/app.md
🧠 Learnings (5)
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx.go
📚 Learning: 2025-07-17T11:39:38.265Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the `Cookie.CopyTo(src *Cookie)` method copies the source cookie (parameter) to the receiver cookie, so `nc.CopyTo(c)` copies cookie `c` into cookie `nc`.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
docs/api/ctx.md
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
router_test.go
🧬 Code graph analysis (2)
res.go (1)
app.go (1)
New(515-628)
req.go (1)
app.go (1)
App(68-110)
🪛 GitHub Check: lint
readonly.go
[failure] 14-14:
directive //nolint:gosec // pointer arithmetic for rodata check is unused for linter "gosec" (nolintlint)
[failure] 9-9:
var-naming: don't use underscores in Go names; var runtime_erodata should be runtimeErodata (revive)
[failure] 6-6:
var-naming: don't use underscores in Go names; var runtime_rodata should be runtimeRodata (revive)
[failure] 3-3:
should only use grouped 'import' declarations (grouper)
🪛 GitHub Actions: golangci-lint
readonly.go
[error] 3-3: Step 'golangci-lint run' failed: readonly.go:3:1 - should only use grouped 'import' declarations (grouper)
🪛 LanguageTool
docs/api/ctx.md
[grammar] ~108-~108: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeader Returns the HTTP...
(QB_NEW_EN)
[grammar] ~134-~134: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRespHeaders Returns the HTT...
(QB_NEW_EN)
[grammar] ~147-~147: There might be a mistake here.
Context: ...ead. Read more... ::: ### GetRouteURL Generates URLs to ...
(QB_NEW_EN)
[grammar] ~569-~569: There might be a mistake here.
Context: ...ead. Read more... ::: ### BodyRaw Returns the raw reques...
(QB_NEW_EN)
[grammar] ~591-~591: There might be a mistake here.
Context: ...ead. Read more... ::: ### ClientHelloInfo `ClientHelloIn...
(QB_NEW_EN)
[grammar] ~630-~630: There might be a mistake here.
Context: ...led. Read more... ::: ### FormFile MultipartForm files c...
(QB_NEW_EN)
[grammar] ~711-~711: There might be a mistake here.
Context: ...ead. Read more... ::: ### Host Returns the host derived ...
(QB_NEW_EN)
[grammar] ~737-~737: There might be a mistake here.
Context: ...ead. Read more... ::: ### Hostname Returns the hostname ...
(QB_NEW_EN)
[grammar] ~760-~760: There might be a mistake here.
Context: ...ead. Read more... ::: ### IP Returns the remote IP addre...
(QB_NEW_EN)
[grammar] ~958-~958: There might be a mistake here.
Context: ...ead. Read more... ::: ### Params This method can be used...
(QB_NEW_EN)
[grammar] ~1009-~1009: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
[grammar] ~1186-~1186: There might be a mistake here.
Context: ...ead. Read more... ::: In certain scenarios, it can be use...
(QB_NEW_EN)
⏰ 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). (6)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: markdownlint
- GitHub Check: Compare
🔇 Additional comments (22)
docs/intro.md (1)
59-61: Example looks goodThe example correctly demonstrates using
c.App().GetString(...)to conditionally detach whenImmutableis enabled.readonly.go (1)
11-16: Please verify go:linkname usage and provide a guarded fallbackThe current implementation in
readonly.golinks directly toruntime.rodataandruntime.erodata, which are unsupported, version- and architecture-specific symbols. This may lead to unexpected build failures or runtime panics on some Go versions or platforms. To mitigate risk, gate thego:linkname-based implementation behind a dedicated build tag and supply a safe default fallback that always returns false.• Confirm cross-platform builds
- Test with Go 1.25 on linux/darwin/windows for both amd64 and arm64, with and without the
link_runtime_rodatatag.- Add a CI script that runs:
#!/usr/bin/env bash set -euo pipefail for TAG in "" "link_runtime_rodata"; do echo "Building with tags: '${TAG}'" GOOS=linux GOARCH=amd64 go build -tags "$TAG" ./... GOOS=linux GOARCH=arm64 go build -tags "$TAG" ./... GOOS=darwin GOARCH=amd64 go build -tags "$TAG" ./... GOOS=darwin GOARCH=arm64 go build -tags "$TAG" ./... GOOS=windows GOARCH=amd64 go build -tags "$TAG" ./... GOOS=windows GOARCH=arm64 go build -tags "$TAG" ./... done• Introduce tagged implementation and fallback
- Create
readonly_linked.go(requires//go:build link_runtime_rodata)://go:build link_runtime_rodata package fiber import "unsafe" //go:linkname runtimeRodata runtime.rodata var runtimeRodata byte //go:linkname runtimeErodata runtime.erodata var runtimeErodata byte func isReadOnly(p unsafe.Pointer) bool { start := uintptr(unsafe.Pointer(&runtimeRodata)) //nolint:gosec end := uintptr(unsafe.Pointer(&runtimeErodata)) //nolint:gosec addr := uintptr(p) return addr >= start && addr < end }- Create
readonly_fallback.go(default build)://go:build !link_runtime_rodata package fiber import "unsafe" func isReadOnly(_ unsafe.Pointer) bool { return false }This approach isolates the high-risk
go:linknameusage and ensures a stable default on all platforms and Go releases.docs/api/app.md (1)
10-18: Docs for GetString align with current codeBehavior and signature are clear and accurate: conditional copy only when Immutable is enabled and the string is not in rodata.
router_test.go (1)
86-86: Consistent migration to app.toString in tests — looks goodAll assertions now use app.toString on []byte bodies, aligning tests with the new immutability-aware conversion helpers. No functional issues spotted.
Also applies to: 95-95, 113-113, 122-122, 161-161, 181-181, 190-190, 208-208, 217-217, 235-235, 273-273, 282-282, 291-291, 309-309, 327-327
redirect.go (1)
328-331: Cookie value conversion via app.toString is safe in this call pathdst is a fresh byte slice, and Cookie() immediately copies it into a fasthttp cookie. Using app.toString here keeps allocations minimal when Immutable=false and still copies when Immutable=true. Good tradeoff.
res.go (1)
137-137: Alignment with immutability wrappers is correct; ensure no string alias escapes mutable storageSwitching to app.toString across header/body conversions matches the new internal contract and avoids unnecessary copies under Immutable=false. In all changed sites, the derived strings are used immediately (Append, AutoFormat, Get/GetHeaders, JSONP assembly, Link header composition, template parsing), so aliasing doesn’t escape. No functional concerns.
Also applies to: 378-378, 408-408, 418-420, 504-505, 540-541, 651-651
helpers.go (2)
657-685: isEtagStale: conversion changes are fine; no behavioral regressionsUsing app.toString on header slices is safe here; inputs originate from request buffers that outlive this function. Logic (wildcard short-circuit and token scanning) remains unchanged.
784-790: Immutable conversion helpers — OKtoStringImmutable/toBytesImmutable now perform definite copies, matching the intended semantics when Config.Immutable=true.
ctx.go (1)
234-234: Ctx conversions migrated to toString/toBytes — good
- OriginalURL/Path now use app.toString.
- XHR uses app.toBytes for case-insensitive match.
- Reset and renderExtensions conversions align with immutability behavior.
- getDetectionPath returning app.toString is consistent with the rest.
No functional issues found.
Also applies to: 251-251, 389-389, 431-431, 481-484, 519-519
req.go (13)
181-182: Centralizing immutability handling in Body() looks goodReturning
r.c.app.GetBytes(body)ensures conditional detachment is handled in one place and keeps the method’s contract intact. No functional concerns here.
196-197: Cookies(): correct use of toString for ephemeral dataUsing
app.toStringaligns with the “valid only within the handler” guarantee and avoids unnecessary copies whenImmutableis off.
218-219: FormValue(): consistent ephemeral semanticsSame rationale as Cookies(); good consistency and no hidden allocations in the hot path.
250-256: Fresh(): safe/cheap ETag extraction
app.toString(response.Header.Peek(HeaderETag))preserves the ephemeral semantics of header reads and avoids allocations in the common case.
304-306: GetHeaders(): efficient build with ephemeral stringsMapping keys/values via
toStringkeeps this method allocation-lean unlessImmutableis enabled at higher layers. Contract already states the map is only valid within the handler, so this is correct.
327-328: Host(): correct detachment boundaryUsing
toStringonURI().Host()matches other header/URI conversions and respects TrustProxy logic above.
491-497: Is(): Content-Type parsing with toString is appropriateLightweight conversion, then normalization/compare. Looks correct and performant.
564-565: OriginalURL(): consistent ephemeral returnReturning
toString(RequestURI())matches method docs (valid only within handler). Good.
638-654: Scheme(): correct use of toString for header-derived valuesBoth the X-Forwarded-Proto/Protocol extraction and X-Url-Scheme path now avoid unnecessary copies while honoring proxy trust. Behavior unchanged.
660-663: Protocol(): appropriate ephemeral conversion
toString(Request.Header.Protocol())is consistent with the rest of the API surface.
698-701: Queries(): ephemeral key/value conversion is consistentThe map remains handler-scoped as documented; using
toStringavoids extra allocations whenImmutableis off.
726-733: QueryV: efficient parse pathReading via
toStringand deferring allocation togenericParseTypeonly when needed is the right trade-off. LGTM.
912-914: getBody(): aligning BodyRaw with conditional copy policy
app.GetBytes(Request.Body())ensures BodyRaw respectsImmutablewithout changing the “valid within handler” guidance. Good centralization.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app_test.go (2)
985-1006: Good fix: literals are no longer forced-copied under Immutable=trueThis addresses prior feedback that copying a string literal is unnecessary. The heap-backed vs. literal split is sensible. Minor cleanups below improve consistency and signal intent.
Suggested tightening with testify for consistency and clearer failures:
func Test_App_GetString(t *testing.T) { t.Parallel() heap := string([]byte("fiber")) appMutable := New() same := appMutable.GetString(heap) - if unsafe.StringData(same) != unsafe.StringData(heap) { //nolint:gosec // compare pointer addresses - t.Errorf("expected original string when immutable is disabled") - } + require.Equal(t, unsafe.StringData(heap), unsafe.StringData(same), "expected original string when immutable is disabled") //nolint:gosec appImmutable := New(Config{Immutable: true}) copied := appImmutable.GetString(heap) - if unsafe.StringData(copied) == unsafe.StringData(heap) { //nolint:gosec // compare pointer addresses - t.Errorf("expected a copy for heap-backed string when immutable is enabled") - } + require.NotEqual(t, unsafe.StringData(heap), unsafe.StringData(copied), "expected a copy for heap-backed string when immutable is enabled") //nolint:gosec literal := "fiber" sameLit := appImmutable.GetString(literal) - if unsafe.StringData(sameLit) != unsafe.StringData(literal) { //nolint:gosec // compare pointer addresses - t.Errorf("expected original literal when immutable is enabled") - } + require.Equal(t, unsafe.StringData(literal), unsafe.StringData(sameLit), "expected original literal when immutable is enabled") //nolint:gosec }Optional: add a mutation check to assert that copied strings are detached (e.g., produce a heap string from a mutable byte slice, mutate the slice after GetString, and confirm the returned string remains unchanged). I can draft that if desired.
1008-1033: Clarify expected semantics for GetBytes under Immutable=true and strengthen the test with mutation checksThis test currently asserts an unconditional copy under Immutable=true (including when cap==len). That’s fine if it matches the intended contract. However, the PR objective mentions “conditional copying,” which could imply more nuanced behavior. Please confirm if GetBytes should:
- always copy when Immutable=true, or
- conditionally copy only when aliasing/ephemeral buffers are suspected.
Either way, adding mutation checks will make the test purpose crystal-clear and reduce reliance on pointer comparisons alone.
Proposed improvements:
func Test_App_GetBytes(t *testing.T) { t.Parallel() b := []byte("fiber") appMutable := New() same := appMutable.GetBytes(b) - if &same[0] != &b[0] { - t.Errorf("expected original slice when immutable is disabled") - } + if len(b) > 0 { // guard even if future edits change b + require.Equal(t, &b[0], &same[0], "expected original slice when immutable is disabled") + } alias := make([]byte, 10) copy(alias, b) sub := alias[:5] appImmutable := New(Config{Immutable: true}) copied := appImmutable.GetBytes(sub) - if &copied[0] == &sub[0] { - t.Errorf("expected a copy for aliased slice when immutable is enabled") - } + require.NotEqual(t, &sub[0], &copied[0], "expected a copy for aliased slice when immutable is enabled") + // mutation check: ensure detachment + sub[0] = 'X' + require.Equal(t, byte('f'), copied[0], "copied slice must not be affected by source mutation") full := make([]byte, 5) copy(full, b) detached := appImmutable.GetBytes(full) - if &detached[0] == &full[0] { - t.Errorf("expected a copy even when cap==len") - } + require.NotEqual(t, &full[0], &detached[0], "expected a copy even when cap==len") + // mutation check: ensure detachment + full[0] = 'Y' + require.Equal(t, byte('f'), detached[0], "copied slice must not be affected by source mutation") + + // Optional: If intended semantics are unconditional copy under Immutable, + // verify that even a fresh slice is copied: + fresh := []byte("fiber") + copiedFresh := appImmutable.GetBytes(fresh) + require.NotEqual(t, &fresh[0], &copiedFresh[0], "immutable app should copy any byte slice") }If the intended contract is “conditional copy,” consider adjusting the final assertion accordingly and documenting the conditions (e.g., “copies when input may alias request/response buffers or is not known to be read-only”).
🧹 Nitpick comments (2)
app_test.go (2)
593-593: Decouple test from internal implementation detail (prefer string(body) over app.toString)This test verifies functional behavior (response body), not immutability plumbing. Using app.toString couples the test to an internal function pointer and obscures the intent. Prefer a plain conversion for clarity and resilience.
Apply:
- require.Equal(t, "اختبار", app.toString(body)) + require.Equal(t, "اختبار", string(body))
629-629: Same as above: avoid relying on app.toString in testsFor readability and to avoid coupling to private wiring, use a plain conversion.
Apply:
- require.Equal(t, "/AbC", app.toString(body)) + require.Equal(t, "/AbC", string(body))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
.golangci.ymlis excluded by!**/*.yml
📒 Files selected for processing (3)
app.go(5 hunks)app_test.go(4 hunks)docs/api/app.md(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api/app.md
- app.go
🧰 Additional context used
🧬 Code graph analysis (1)
app_test.go (1)
app.go (2)
New(515-628)Config(113-418)
⏰ 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). (5)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
middleware/pprof/pprof.go (1)
74-81: Redirect loses prefix when trimming trailing slash — produce incorrect targetAfter CutPrefix (Lines 42–46), path contains only the suffix (e.g., "/heap/"). In the default branch, when the suffix ends with "/", you trim it and then redirect to path directly, which drops the prefix and redirects to "/heap" instead of "/debug/pprof/heap". This is a functional bug and can lead to wrong redirects.
Fix: always build the redirect target with the full prefix.
Apply this diff:
- if strings.HasSuffix(path, "/") { - path = utils.TrimRight(path, '/') - } else { - path = prefix + "/" - } - - return c.Redirect().To(path) + target := prefix + "/" + if strings.HasSuffix(path, "/") { + target = prefix + utils.TrimRight(path, '/') + } + return c.Redirect().To(target)Optionally, consider adding a small test to assert that GET /heap/ redirects to /heap, and that GET (without trailing slash) redirects to /.
middleware/csrf/csrf.go (1)
328-341: Trusted-origin check for Referer incorrectly compares full URL (with path/query) instead of just scheme://hostThis makes entries in trustedOrigins (which are normalized origins) fail to match typical Referer values that include paths, and may wrongly reject legitimate requests when Origin is absent (HTTPS fallback path).
Apply this diff to compare only the origin portion and to pass that same origin to subdomain matching:
- referer = refererURL.String() - - if slices.Contains(trustedOrigins, referer) { + // Compare only the origin (scheme + host), not the full URL with path/query. + refererOrigin := refererURL.Scheme + "://" + refererURL.Host + + if slices.Contains(trustedOrigins, refererOrigin) { return nil } - for _, trustedSubOrigin := range trustedSubOrigins { - if trustedSubOrigin.match(referer) { + for _, trustedSubOrigin := range trustedSubOrigins { + if trustedSubOrigin.match(refererOrigin) { return nil } }log/default.go (1)
55-59: Bug: privateLogf prints nothing when no fmtArgs are providedWhen
len(fmtArgs) == 0, the code writesfmtArgs(empty) instead of theformatstring, producing blank log messages for calls likeInfof("hello").Apply this fix:
if len(fmtArgs) > 0 { _, _ = fmt.Fprintf(buf, format, fmtArgs...) } else { - _, _ = fmt.Fprint(buf, fmtArgs...) + buf.WriteString(format) }client/client_test.go (1)
1178-1182: Fix copy-paste in struct param test: assert int_slice, not TSliceThis block duplicates the TSlice assertions. It should verify the tagged
int_slicefield and its stringified values.Suggested change:
- tint := p.Param("TSlice") - require.Len(t, tint, 2) - require.Equal(t, "foo", tint[0]) - require.Equal(t, "bar", tint[1]) + tint := p.Param("int_slice") + require.Len(t, tint, 2) + require.Equal(t, "1", tint[0]) + require.Equal(t, "2", tint[1])
🧹 Nitpick comments (22)
middleware/cache/manager_test.go (1)
9-9: Prefer require over assert (fail-fast in tests)Project convention uses testify/require for assertions. Switching improves signal and simplifies failure handling in parallel tests.
-import "github.com/stretchr/testify/assert" +import "github.com/stretchr/testify/require" -assert.Nil(t, cacheManager.get(context.Background(), utils.UUID())) +require.Nil(t, cacheManager.get(context.Background(), utils.UUID())) -assert.NotNil(t, cacheManager.get(context.Background(), id)) +require.NotNil(t, cacheManager.get(context.Background(), id))Also applies to: 17-17, 25-25
binder/query.go (3)
23-30: Avoid extra allocation by iterating with QueryArgs.VisitAll instead of All()
All()allocates a map that you immediately range over. Iterating in-place avoids that intermediate allocation and reduces GC pressure, especially for large query strings.Suggested change confined to this block:
- for key, val := range reqCtx.URI().QueryArgs().All() { - k := utils.UnsafeString(key) - v := utils.UnsafeString(val) - err = formatBindData(b.Name(), out, data, k, v, b.EnableSplitting, true) - if err != nil { - break - } - } + reqCtx.URI().QueryArgs().VisitAll(func(key, val []byte) { + if err != nil { + return + } + k := utils.UnsafeString(key) + v := utils.UnsafeString(val) + if e := formatBindData(b.Name(), out, data, k, v, b.EnableSplitting, true); e != nil { + err = e + } + })Note: We can't early-break the visitor, but after the first error the callback becomes a no-op, which is usually an acceptable trade-off for the allocation savings.
20-20: Preallocate the map with known capacityWe know the number of query args up front. Preallocating avoids rehashes on the hot path.
- data := make(map[string][]string) + data := make(map[string][]string, reqCtx.URI().QueryArgs().Len())
24-26: Recheck immutability/aliasing expectations for UnsafeString hereThis binder converts query args with
utils.UnsafeString, which intentionally avoids copying. If the underlying fasthttp buffers back these values and the bound struct escapes the request scope, this can alias request memory. Given this PR’s new conditional copy helpers, consider whether binder should also offer a detach path when immutability is enabled (even if not in scope for this PR).If that’s desired in a follow-up:
- Option A: add a binder variant that accepts a lightweight “string detacher” func (e.g.,
func([]byte) string) so the caller (e.g., ctx) can injectctx.CopyStringvsutils.UnsafeString.- Option B: document that binder outputs may alias request memory and must not escape the request lifecycle.
I can draft either approach if you want it queued as a separate change.
Would you like me to scan the repo for binder usages that persist bound structs beyond the request lifecycle to assess actual risk?
middleware/csrf/csrf.go (2)
243-246: Make cookie deletion robust across browsers: set Max-Age and avoid Expires for session cookiesSetting only Expires works most of the time, but some browsers rely on Max-Age for deletion. Also, when SessionOnly is true, it’s cleaner to omit Expires/Max-Age entirely.
Apply this diff:
func expireCSRFCookie(c fiber.Ctx, cfg Config) { - setCSRFCookie(c, cfg, "", -time.Hour) + // negative duration signals deletion + setCSRFCookie(c, cfg, "", -time.Hour) } func setCSRFCookie(c fiber.Ctx, cfg Config, token string, expiry time.Duration) { - cookie := &fiber.Cookie{ + cookie := &fiber.Cookie{ Name: cfg.CookieName, Value: token, Domain: cfg.CookieDomain, Path: cfg.CookiePath, Secure: cfg.CookieSecure, HTTPOnly: cfg.CookieHTTPOnly, SameSite: cfg.CookieSameSite, - SessionOnly: cfg.CookieSessionOnly, - Expires: time.Now().Add(expiry), + SessionOnly: cfg.CookieSessionOnly, } - // Set the CSRF cookie to the response + // For session cookies, omit explicit expiry/max-age. + if !cfg.CookieSessionOnly { + if expiry < 0 { + // Explicitly mark for deletion + cookie.Expires = time.Now().Add(expiry) + cookie.MaxAge = -1 + } else { + cookie.Expires = time.Now().Add(expiry) + cookie.MaxAge = int(expiry.Seconds()) + } + } + + // Set the CSRF cookie to the response c.Cookie(cookie) }Also applies to: 247-262
59-61: Preallocate trustedOrigins slices to avoid reallocationsMinor allocation win on startup when many trusted origins are configured.
- trustedOrigins := []string{} - trustedSubOrigins := []subdomain{} + trustedOrigins := make([]string, 0, len(cfg.TrustedOrigins)) + trustedSubOrigins := make([]subdomain, 0, len(cfg.TrustedOrigins))internal/storage/memory/memory.go (2)
166-170: Minor inconsistency in expiry comparison (<vs<=).In the GC scan loop you use
< tsto collect expired keys, but in the double-checked deletion you use<= ts.Get()uses<=. This is harmless (the exactly-at-tskeys will be removed on the next tick), but using a consistent comparator improves readability and reasoning.Apply if you want consistent semantics:
- for id, v := range s.db { - if v.expiry != 0 && v.expiry < ts { + for id, v := range s.db { + if v.expiry != 0 && v.expiry <= ts { expired = append(expired, id) } }Also applies to: 176-179
186-191: Conn exposes internal map without protection; consider documenting or returning a snapshot.
Conn()returns the underlying map after releasing the read lock, allowing external callers to iterate/modify concurrently with writers—risking data races unless callers self-synchronize. If this is intentional, add a clear doc comment; otherwise, consider returning a read-only view or a shallow copy.Possible safer approach:
-func (s *Storage) Conn() map[string]entry { - s.mux.RLock() - defer s.mux.RUnlock() - return s.db -} +// Conn returns a snapshot of the storage map. The returned map is safe +// to read without holding s.mux but is a shallow copy. +func (s *Storage) Conn() map[string]entry { + s.mux.RLock() + defer s.mux.RUnlock() + out := make(map[string]entry, len(s.db)) + for k, v := range s.db { + out[k] = v + } + return out +}mount.go (2)
49-54: Avoid shadowing the subApp parameter inside the range loopThe loop reuses the name subApp, shadowing the function parameter and reducing readability. Renaming the loop variable clarifies intent and prevents accidental misuse.
- for mountedPrefixes, subApp := range subApp.mountFields.appList { + for mountedPrefixes, childApp := range subApp.mountFields.appList { - subApp.mountFields.mountPath = path - app.mountFields.appList[path] = subApp + childApp.mountFields.mountPath = path + app.mountFields.appList[path] = childApp
79-84: Same shadowing in Group.mountThe same shadowing pattern appears here; apply the same rename for consistency and clarity.
- for mountedPrefixes, subApp := range subApp.mountFields.appList { + for mountedPrefixes, childApp := range subApp.mountFields.appList { - subApp.mountFields.mountPath = path - grp.app.mountFields.appList[path] = subApp + childApp.mountFields.mountPath = path + grp.app.mountFields.appList[path] = childAppmiddleware/session/session.go (4)
411-411: Verify Header.SetBytesV key type; consider using Set when key is a stringIf Extractor.Key is a string (as implied by usages in Lines 447-451), passing it to SetBytesV may rely on a non-standard signature. fasthttp’s ResponseHeader typically exposes Set(key, value string) and SetBytesV(key, value []byte). If your key is a string, prefer Set to avoid ambiguity and ensure type correctness.
If Extractor.Key is indeed []byte, keep SetBytesV but ensure all call sites consistently pass []byte for both key and value.
Suggested change (only if Key is string):
- s.ctx.Response().Header.SetBytesV(ext.Key, []byte(s.id)) + s.ctx.Response().Header.Set(ext.Key, s.id)
504-507: Defer ordering clarity: use a single defer to avoid subtle LIFO pitfallsCurrent order is correct due to LIFO, but not immediately obvious to readers. Encapsulating Reset + Put into one defer improves readability and prevents future reorder bugs.
Apply to both decodeSessionData and encodeSessionData:
- byteBuffer := byteBufferPool.Get().(*bytes.Buffer) //nolint:forcetypeassert,errcheck // We store nothing else in the pool - defer byteBufferPool.Put(byteBuffer) - defer byteBuffer.Reset() + byteBuffer := byteBufferPool.Get().(*bytes.Buffer) //nolint:forcetypeassert,errcheck // We store nothing else in the pool + defer func() { + byteBuffer.Reset() + byteBufferPool.Put(byteBuffer) + }()Also applies to: 527-529
30-33: Fix misleading comment for absExpirationKeyThe comment references “sessionIDContextKey”, but the constant is absExpirationKey and is used for absolute expiration metadata, not session ID or context locals.
- // sessionIDContextKey is the key used to store the session ID in the context locals. + // absExpirationKey is the key used to store the absolute expiration timestamp in session data.
570-579: Docstring naming mismatch: setAbsExpiration vs “setAbsoluteExpiration”The function is named setAbsExpiration; adjust the comment to match for consistency and to aid grepability.
-// setAbsoluteExpiration sets the absolute session expiration time. +// setAbsExpiration sets the absolute session expiration time.binder/msgpack.go (1)
27-33: Nit: clarify panic messages grammar and casing.The current text is slightly ungrammatical and mixes “Msgpack/MsgPack.” Suggest tightening the message.
Apply this diff:
func UnimplementedMsgpackMarshal(_ any) ([]byte, error) { - panic("Must explicit setup Msgpack, please check docs: https://docs.gofiber.io/next/guide/advance-format#msgpack") + panic("MsgPack support is not configured. Please see the docs: https://docs.gofiber.io/next/guide/advance-format#msgpack") } func UnimplementedMsgpackUnmarshal(_ []byte, _ any) error { - panic("Must explicit setup Msgpack, please check docs: https://docs.gofiber.io/next/guide/advance-format#msgpack") + panic("MsgPack support is not configured. Please see the docs: https://docs.gofiber.io/next/guide/advance-format#msgpack") }middleware/csrf/storage_manager.go (2)
38-41: Typos in comments (“too” → “to”, “crsf” → “CSRF”).Minor nits that slightly reduce readability.
Apply this diff:
- // Fallback too memory storage + // Fallback to memory storage @@ - // the key is crucial in crsf and sometimes a reference to another value which can be reused later(pool/unsafe values concept), so a copy is made here + // The key is crucial in CSRF and may reference another value reused later (pool/unsafe values concept), so we copy it to detach from the original memory.Also applies to: 60-62
48-51: Follow-up: stop ignoring storage errors (has TODOs already).The three storage calls still ignore errors; consider surfacing or logging them to avoid silent failures in token lifecycle.
If helpful, I can open a follow-up issue and sketch a small refactor to return or aggregate these errors (opt-in via config), so existing behavior stays default-noise-free.
Also applies to: 58-59, 68-68
client/request.go (1)
19-19: Alias Usage Verified in Code; Documentation Examples OutliersAll Go code files consistently import
github.com/gofiber/utils/v2with theutilsalias—no non-aliased imports remain in the codebase. Call sites useutils.CopyString, so there are no ambiguous bare references.However, two Markdown documentation files include non-aliased imports of utils/v2. To maintain consistency between code and examples, consider updating these:
• docs/middleware/cache.md at line 53
• docs/middleware/session.md at line 455No action is required in code files. Optional refactor of the docs will align examples with the project convention.
middleware/basicauth/basicauth.go (1)
51-57: Unify credentials conversion with central copy helpersThe manual
Config().Immutablebranch here predates the copy helpers now available inutils(CopyString/CopyBytes), which centralize detach-if-aliased semantics. We can simplify and future-proof this code by using those helpers instead of rolling our own conditional:- // Get the credentials - var creds string - if c.App().Config().Immutable { - creds = string(raw) - } else { - creds = utils.UnsafeString(raw) - } + // Detach via utils.CopyString (handles aliasing consistently) + creds := utils.CopyString(utils.UnsafeString(raw))If you need the bytes form instead, you can similarly call:
credsBytes := utils.CopyBytes(raw)This aligns with usages elsewhere (e.g. in
res.go,app.go, middleware proxy, cache, etc.) and removes bespokeImmutablechecks here. Other manualConfig().Immutable/UnsafeStringbranches remain in middleware and can be consolidated in a follow-up:
- middleware/static/static.go
- middleware/session/data_msgp.go
- middleware/proxy/proxy.go
- middleware/logger/default_logger.go, utils.go, template_chain.go
- middleware/cache/config.go, cache.go, manager_msgp.go
- middleware/idempotency/idempotency.go, response_msgp.go
- middleware/limiter/manager_msgp.go
- middleware/csrf/storage_manager.go, storage_manager_msgp.go
- middleware/basicauth/basicauth.go
Consider refactoring these in a subsequent pass for full consistency.
middleware/logger/template_chain.go (1)
49-51: Clarify error message for unknown tag with parameterWhen a function with parameters isn’t found, the error “No parameter found…” is misleading. Consider changing to an actionable message.
Proposed tweak:
- if !ok { - return nil, nil, errors.New("No parameter found in \"" + utils.UnsafeString(templateB[:currentPos]) + "\"") - } + if !ok { + return nil, nil, errors.New("unknown template tag with parameter: " + utils.UnsafeString(templateB[:currentPos])) + }app.go (2)
639-649: Doc precision + consistency with utils: prefer utils.CopyString over strings.Clone
- Doc says “detached copy (strings.Clone)”. Consider referencing “copy” generically and using utils.CopyString for consistency with the rest of the project and easier future tuning.
- The read-only detection hinges on runtime rodata checks; add a short caveat that when provenance cannot be determined, this may conservatively copy.
Apply within this hunk:
-// GetString returns s unchanged when Immutable is off or s is read-only (rodata). -// Otherwise it returns a detached copy (strings.Clone). +// GetString returns s unchanged when Immutable is off or s is read-only (rodata). +// Otherwise it returns a detached copy. When provenance cannot be determined, +// it may conservatively copy to uphold immutability. func (app *App) GetString(s string) string { if !app.config.Immutable || len(s) == 0 { return s } if isReadOnly(unsafe.Pointer(unsafe.StringData(s))) { //nolint:gosec // pointer check avoids unnecessary copy return s // literal / rodata → safe to return as-is } - return strings.Clone(s) // heap-backed / aliased → detach + return utils.CopyString(s) // heap-backed / aliased → detach (project-consistent) }
651-661: Bytes rodata detection is likely redundant; consider simpler always-copy under ImmutableFor []byte, “read-only” memory doesn’t typically apply (rodata-backed slices aren’t a normal construct without unsafe). The isReadOnly check adds an unsafe call and complexity with little benefit. Under Immutable, returning a fresh copy for any non-empty slice is simpler and safer.
Apply within this hunk:
func (app *App) GetBytes(b []byte) []byte { if !app.config.Immutable || len(b) == 0 { return b } - if isReadOnly(unsafe.Pointer(unsafe.SliceData(b))) { //nolint:gosec // pointer check avoids unnecessary copy - return b // rodata → safe to return as-is - } - return utils.CopyBytes(b) // detach when backed by request/response memory + return utils.CopyBytes(b) // detach under Immutable }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (58)
app.go(5 hunks)app_test.go(4 hunks)bind.go(1 hunks)binder/cbor.go(1 hunks)binder/cookie.go(1 hunks)binder/form.go(1 hunks)binder/header.go(1 hunks)binder/json.go(1 hunks)binder/mapping.go(1 hunks)binder/msgpack.go(1 hunks)binder/query.go(1 hunks)binder/resp_header.go(1 hunks)binder/xml.go(1 hunks)client/client.go(1 hunks)client/client_test.go(1 hunks)client/cookiejar.go(1 hunks)client/hooks.go(1 hunks)client/request.go(1 hunks)client/response.go(1 hunks)ctx.go(7 hunks)ctx_test.go(4 hunks)helpers.go(7 hunks)helpers_test.go(1 hunks)internal/memory/memory.go(1 hunks)internal/memory/memory_test.go(1 hunks)internal/storage/memory/memory.go(1 hunks)listen_test.go(1 hunks)log/default.go(1 hunks)middleware/adaptor/adaptor.go(1 hunks)middleware/basicauth/basicauth.go(1 hunks)middleware/cache/cache.go(1 hunks)middleware/cache/cache_test.go(1 hunks)middleware/cache/config.go(1 hunks)middleware/cache/manager_test.go(1 hunks)middleware/cors/cors.go(1 hunks)middleware/csrf/config.go(1 hunks)middleware/csrf/csrf.go(1 hunks)middleware/csrf/csrf_test.go(1 hunks)middleware/csrf/storage_manager.go(1 hunks)middleware/idempotency/idempotency.go(1 hunks)middleware/limiter/limiter_fixed.go(1 hunks)middleware/limiter/limiter_sliding.go(1 hunks)middleware/logger/default_logger.go(1 hunks)middleware/logger/template_chain.go(1 hunks)middleware/logger/utils.go(1 hunks)middleware/pprof/pprof.go(1 hunks)middleware/proxy/proxy.go(1 hunks)middleware/requestid/config.go(1 hunks)middleware/session/config.go(1 hunks)middleware/session/session.go(1 hunks)middleware/static/static.go(1 hunks)mount.go(1 hunks)path.go(1 hunks)redirect.go(3 hunks)req.go(16 hunks)res.go(8 hunks)router.go(1 hunks)router_test.go(16 hunks)
✅ Files skipped from review due to trivial changes (25)
- client/client.go
- internal/memory/memory_test.go
- internal/memory/memory.go
- binder/resp_header.go
- middleware/cors/cors.go
- path.go
- binder/header.go
- middleware/adaptor/adaptor.go
- middleware/limiter/limiter_sliding.go
- middleware/csrf/config.go
- middleware/limiter/limiter_fixed.go
- middleware/cache/config.go
- client/hooks.go
- client/cookiejar.go
- binder/cookie.go
- router.go
- middleware/proxy/proxy.go
- binder/json.go
- client/response.go
- binder/xml.go
- middleware/static/static.go
- binder/form.go
- binder/cbor.go
- helpers_test.go
- middleware/logger/utils.go
🚧 Files skipped from review as they are similar to previous changes (8)
- res.go
- redirect.go
- req.go
- router_test.go
- ctx.go
- ctx_test.go
- app_test.go
- helpers.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
middleware/basicauth/basicauth.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/cache/manager_test.goclient/client_test.golisten_test.gomiddleware/cache/cache_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/cache/manager_test.gomiddleware/csrf/csrf_test.goclient/client_test.golisten_test.gomiddleware/cache/cache_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/cache/manager_test.golisten_test.go
📚 Learning: 2024-09-25T16:17:00.969Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:16-26
Timestamp: 2024-09-25T16:17:00.969Z
Learning: In the session middleware `Config` struct, `Store` is backed by `fiber.Storage`; they are different entities serving distinct purposes in session management.
Applied to files:
middleware/session/config.go
⏰ 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 (31)
middleware/requestid/config.go (1)
5-5: Ready to merge: allgithub.com/gofiber/utils/v2imports are consistently aliased asutils
The verification script confirms there are no unaliased single-line or block imports ofgithub.com/gofiber/utils/v2anywhere in the repository.binder/mapping.go (1)
12-12: Approve:utils/v2alias consistency confirmedRan the provided verification script across the repository:
- No imports of the bare
github.com/gofiber/utilswere found.- Every
github.com/gofiber/utils/v2import is aliased asutils.binder/mapping.gouses the same alias and import path as all other call sites.All checks passed—no further changes needed.
middleware/pprof/pprof.go (1)
8-8: Import alias LGTM and consistent with codebase patternsAliasing github.com/gofiber/utils/v2 to utils is correct (default import name would be v2) and matches usage at Line 75 (utils.TrimRight). No issues here.
middleware/cache/manager_test.go (2)
8-8: Alias utils/v2 as utils — LGTMConsistent with repo-wide aliasing; keeps call sites clean (utils.UUID()).
12-27: The scripts are running; I’ll use the details onsetand the in-memory storage’s implementation to confirm thread safety. [waiting for output]middleware/idempotency/idempotency.go (3)
9-10: Import aliasing to utils/v2 looks goodAliasing
github.com/gofiber/utils/v2asutilsmatches the codebase convention and keeps call sites unchanged.
78-83: Incorrect suggestion:Ctx.CopyStringdoesn’t exist
It looks like there is noCopyStringmethod defined onCtxanywhere in the repo, so switching toc.CopyString(...)would not compile. You can ignore this refactor suggestion unless you add aCopyStringhelper on your context type.Likely an incorrect or invalid review comment.
118-123: I’ve initiated searches for any existingCopyBytesmethods and pulled in the top of the idempotency middleware for context. Let me know the results so we can determine ifc.CopyBytesactually exists and whether the suggested refactor is valid.binder/query.go (1)
4-4: Alias of utils/v2 to "utils" — consistent and clearUsing an explicit alias keeps usage uniform (
utils.UnsafeString) across the codebase. No functional changes; good to go.middleware/csrf/csrf.go (1)
11-11: Consistent alias for utils — LGTMImport alias matches in-file usage (utils.Trim) and aligns with repo-wide convention.
internal/storage/memory/memory.go (2)
10-10: Import bump to utils/v2 looks good and consistent.Switching to
github.com/gofiber/utils/v2with aliasutilsaligns with the repo-wide convention and preserves call sites (utils.Timestamp,utils.StartTimeStampUpdater). No functional change observed.
39-42: EnsureStartTimeStampUpdaterwon’t spawn duplicate updatersWe verified that all imports reference
github.com/gofiber/utils/v2and there are multiple calls toutils.StartTimeStampUpdater()(ininternal/storage/memory/memory.go,internal/memory/memory.go, and both limiter implementations). However, we haven’t confirmed whetherStartTimeStampUpdateritself is guarded to run only once.Please verify that its implementation in the
utilsmodule usessync.Once(or an equivalent guard) to ensure idempotence. If it does not, you have two options:
- Add a package-level
var once sync.Onceinside theutilsrepo around the goroutine spawn.- Wrap each call locally in this repo with a
sync.Onceguard.Call sites to review and potentially wrap:
internal/storage/memory/memory.goat theutils.StartTimeStampUpdater()invocationinternal/memory/memory.goat itsStartTimeStampUpdater()callmiddleware/limiter/limiter_fixed.goat line 25middleware/limiter/limiter_sliding.goat line 26mount.go (2)
12-12: Alias import to utils — LGTMImporting github.com/gofiber/utils/v2 as utils is consistent with existing call sites (e.g., utils.TrimRight at Line 43 and Line 73). No behavior change introduced.
12-12: utils aliasing verification – All clearPerformed repo-wide sanity checks:
- No imports of
github.com/gofiber/utils(v1) remain- All
github.com/gofiber/utils/v2imports are aliased asutils- No local variables, constants, or function parameters named
utilsthat could shadow the importEverything is consistent.
middleware/session/session.go (1)
12-12: Consistent utils aliasing — LGTMAliasing github.com/gofiber/utils/v2 as utils keeps this file consistent with the rest of the PR. No functional impact.
middleware/session/config.go (2)
8-8: Consistent utils aliasing — LGTMUsing utils "github.com/gofiber/utils/v2" matches the repository-wide aliasing pattern in this PR.
11-22: Defaults applied in constructors: no validation needed on Config fieldsBoth
StorageandStoreare initialized with safe defaults in the session constructors, so theConfigstruct itself does not require additional checks:
- In NewStore (middleware/session/store.go:47–50), if
cfg.Storageisnilit’s set tomemory.New(), guaranteeing a non-nilStorage.- In NewWithStore (middleware/session/middleware.go:79–82), if
cfg.Storeisnilit’s set toNewStore(cfg), guaranteeing a non-nilStore.Since each constructor ensures required fields are populated, explicit validation in
Configis unnecessary.binder/msgpack.go (1)
4-4: Explicit alias for utils is fine and consistent.No behavioral change; aligns with the repo-wide aliasing pattern.
middleware/csrf/storage_manager.go (1)
10-10: Alias change LGTM.Behavior is unchanged; consistent with the codebase’s utils aliasing.
listen_test.go (1)
19-19: Import alias consistency acknowledged.Test behavior unchanged; the explicit alias avoids accidental shadowing and is consistent with other files.
middleware/csrf/csrf_test.go (1)
13-13: Alias change is fine and aligns tests with the project-wide convention.No behavioral changes; tests still rely on utils helpers as before.
middleware/basicauth/basicauth.go (1)
8-8: LGTM: import alias standardizationSwitching to an explicit alias for utils is consistent with the project-wide pattern and avoids future import name collisions.
log/default.go (1)
10-10: LGTM: utils alias importThe alias clarifies usage and matches the rest of the codebase refactor.
client/client_test.go (1)
19-19: LGTM: utils alias import in testsConsistent with project conventions; no behavior change.
bind.go (1)
10-10: LGTM: explicit utils aliasNo functional change; aligns imports across the repo and keeps call sites clear.
middleware/logger/template_chain.go (1)
7-7: LGTM: import alias alignmentAlias matches broader refactor; no runtime impact.
middleware/cache/cache.go (1)
15-15: Consistent utils aliasing — good cleanupImporting github.com/gofiber/utils/v2 as utils aligns with the repo-wide convention and keeps call sites uniform. No functional impact.
middleware/cache/cache_test.go (1)
19-19: Import alias matches project conventionAliasing utils in tests mirrors the production code and avoids churn in references like utils.CopyString. Looks good.
middleware/logger/default_logger.go (1)
10-10: Alias import matches repo-wide patternSwitching to utils alias improves consistency across the codebase. No behavior change.
app.go (2)
78-81: Immutable conversion functions wiring verified
- toStringImmutable and toBytesImmutable are defined and reachable in helpers.go (around lines 783–789).
- App’s toBytes/toString pointers are correctly reassigned when
Config.Immutableis enabled in app.go (lines 575–577).- All DefaultCtx methods (e.g.
OriginalURL,Path,XHR, etc.), DefaultReq/DefaultRes helpers, and other utilities consistently call throughc.app.toString/c.app.toBytesorApp.GetString/App.GetBytes, ensuring the immutability contract is honored.No further changes needed here.
639-649: The script will locate and print the first 200 lines of the generatedctx_interface_gen.goto confirm whetherCopyString/CopyBytesmethods exist onfiber.Ctx. Once we have that, we can determine if the review’s suggestion to add Ctx-level helpers is valid or if the comment should instead note that App-level methods are intentional and docs are correct.
| "unsafe" | ||
|
|
||
| "github.com/gofiber/utils/v2" | ||
| utils "github.com/gofiber/utils/v2" | ||
| "github.com/valyala/fasthttp" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Audit and contain unsafe usage behind a single gate
unsafe is pulled in solely for read-only memory probes used by GetString/GetBytes. Ensure:
- The isReadOnly implementation lives in a dedicated file (e.g., readonly.go) with clear build tags and a safe fallback.
- All uses are annotated (you already have //nolint:gosec) and are not persisted beyond the call (no pointer retention).
Please verify the existence and coverage of isReadOnly across platforms, and that a fallback always compiles.
If you’d like, I can propose a minimal readonly.go fallback that always returns false (copy-on-Immutable) when platform support is unknown.
🏁 Script executed:
#!/bin/bash
# Verify isReadOnly exists and has a portable fallback
set -euo pipefail
echo "Definitions of isReadOnly:"
rg -nC2 -g '!**/vendor/**' $'^func\\s+isReadOnly\\s*\\(' || true
echo -e "\nFiles guarded by build tags (sanity check):"
rg -n '^//go:build' -g '!**/vendor/**' readonly.go || true
# Confirm the only unsafe callsites in this file are the two guarded checks below
rg -nP -C1 'unsafe\\.(StringData|SliceData)\\(' app.goLength of output: 763
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List all readonly*.go files (excluding vendor)
mapfile -t files < <(find . -type f -name 'readonly*.go' ! -path './vendor/*')
echo "Detected readonly files:"
for f in "${files[@]}"; do
echo " - ${f#./}"
done
for f in "${files[@]}"; do
echo -e "\n== Inspecting ${f#./} =="
echo "Build tags:"
grep -n '^//go:build' "$f" || echo " (none detected)"
echo "First 30 lines of implementation:"
head -n 30 "$f"
done
# Search for any other definitions of isReadOnly (fallbacks or duplicates)
echo -e "\nSearching for all isReadOnly definitions:"
rg -nH 'func\s+isReadOnly' -g '!**/vendor/**'Length of output: 209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Detected readonly files:"
find . -type f -name 'readonly*.go' ! -path './vendor/*' | sed 's|^\./||'
# Inspect each readonly file
find . -type f -name 'readonly*.go' ! -path './vendor/*' | while IFS= read -r f; do
echo -e "\n== Inspecting ${f#./} =="
echo "Build tags:"
grep -n '^//go:build' "$f" || echo " (none detected)"
echo "First 30 lines of implementation:"
head -n 30 "$f"
done
# Find all isReadOnly definitions across the repo
echo -e "\nSearching for all isReadOnly definitions:"
rg -nH 'func\s\+isReadOnly' -g '!**/vendor/**' || echo "(none found)"Length of output: 1292
Audit and contain unsafe usage behind a single gate
Our inspection confirms that:
isReadOnlyis defined exactly once in readonly.go and lives in its own file.- There are no
//go:buildtags on readonly.go, so this unsafe‐based implementation is compiled on all platforms. - No alternative (safe) fallback is provided for platforms or Go versions where
runtime.rodata/runtime.erodatalinkage may not exist.
To satisfy the original intent, please:
- Add explicit build tags to readonly.go (e.g.
//go:build linux || darwin) so that the unsafe probe only compiles where it’s known to work. - Create a companion file (e.g. readonly_stub.go) with complementary build tags (e.g.
//go:build !linux && !darwin) that definesisReadOnly(p unsafe.Pointer) boolto unconditionally returnfalse. - Ensure every callsite is annotated with
//nolint:gosecand that no pointers obtained byunsafe.StringData/unsafe.SliceData(or similar) are stored or passed beyond their immediate, read-only checks.
I’m happy to draft a minimal stub fallback file if that would help.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
readonly.go (2)
13-18: Document the contract and add minimal tests to lock behavior.The logic looks good: nil is handled (returns false), lower-inclusive/upper-exclusive bounds are correct, and we avoid uintptr round-tripping back to pointers. Please add a short doc comment and a focused unit test to ensure we don’t regress the semantics when refactoring.
Apply this diff to add a function comment:
func isReadOnly(p unsafe.Pointer) bool { + // isReadOnly reports whether p points into the Go runtime's read-only + // data segment [runtime.rodata, runtime.erodata). This is used to avoid + // unnecessary copies for string literals and other immutable data. start := uintptr(unsafe.Pointer(&runtimeRodata)) //nolint:gosec // converting runtime symbols end := uintptr(unsafe.Pointer(&runtimeErodata)) //nolint:gosec // converting runtime symbols addr := uintptr(p) return addr >= start && addr < end }Suggested test (same package to access the unexported helper):
// readonly_test.go package fiber import ( "testing" "unsafe" ) func TestIsReadOnly_LiteralsVsHeap(t *testing.T) { // String literal should be in rodata. if !isReadOnly(unsafe.Pointer(unsafe.StringData("abc"))) { t.Fatal("expected literal string to be detected as read-only") } // Heap-backed slice should not be read-only. b := []byte("abc") if len(b) > 0 && isReadOnly(unsafe.Pointer(&b[0])) { t.Fatal("expected heap-backed []byte to not be read-only") } // Nil is not read-only. if isReadOnly(nil) { t.Fatal("expected nil pointer to not be read-only") } }
14-15: Consolidate gosec suppression or make the intent explicit.The inline nolint markers are acceptable, but consider consolidating them via a short comment above the function that explains why converting runtime sentinels is intentional and safe in this context. This reduces per-line noise without broadening the suppression scope.
Example:
// This function intentionally converts addresses of runtime rodata sentinels to uintptr. // The conversions are localized and never round-tripped back to pointers. // nolint:gosec func isReadOnly(p unsafe.Pointer) bool { ... }If you keep the per-line form, that’s fine too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
readonly.go(1 hunks)
⏰ 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). (6)
- GitHub Check: codecov/patch
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
🔇 Additional comments (2)
readonly.go (2)
3-5: Lint fixes applied correctly (grouped import).The import is now grouped and addresses the previous golangci-lint complaint. Nothing else to change here.
7-11: Verify cross-version/arch compatibility for internal runtime symbols stubThe two
//go:linknamestubs inreadonly.go(lines 7–11 and 13–18) pull in unexported runtime symbols. To avoid breakage on less-common platforms, you should:• Run a GC-only build from your module root for:
- GOOS={linux, darwin, windows, freebsd, openbsd} × GOARCH={amd64, arm64}
- Example:
GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build . GOOS=darwin GOARCH=arm64 CGO_ENABLED=0 go build . …etc.• Verify the stub compiles under:
- GOOS=js GOARCH=wasm
- GOOS=wasip1 GOARCH=wasm
• Ensure you’re invokinggo buildfrom the directory containing yourgo.mod(the failure “pattern ./…: directory prefix . does not contain main module” indicates it wasn’t run in a module root).Please perform these manual checks in a proper Go module context and confirm all targets build without errors.
| //go:linkname runtimeRodata runtime.rodata | ||
| var runtimeRodata byte | ||
|
|
||
| //go:linkname runtimeErodata runtime.erodata | ||
| var runtimeErodata byte |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard go:linkname usage with build tags and provide a stub for unsupported targets.
Linking to runtime.rodata/runtime.erodata is a pragmatic solution, but it is inherently non-portable and may fail on non-gc toolchains or restricted targets (e.g., js/wasm, wasip1) where those internal symbols may not exist or be linkname-accessible. To avoid hard build breaks downstream, restrict this file to gc and non-js/non-wasi platforms, and add a stub that compiles everywhere else.
Apply this diff at the top of this file:
+//go:build gc && !js && !wasip1
package fiberAnd add a small note for future maintainers:
//go:linkname runtimeRodata runtime.rodata
var runtimeRodata byteAdd a stub file to gracefully degrade on excluded targets (always returns false; safe fallback that simply treats inputs as non-rodata):
// readonly_stub.go
//go:build !gc || js || wasip1
package fiber
import "unsafe"
// isReadOnly returns false on unsupported targets/toolchains.
// This avoids build/link errors where runtime rodata sentinels are unavailable.
func isReadOnly(_ unsafe.Pointer) bool { return false }🤖 Prompt for AI Agents
In readonly.go around lines 7 to 11, the use of //go:linkname to reference
runtime.rodata/runtime.erodata is unguarded and breaks non-gc or restricted
targets (js/wasm/wasip1); wrap this file with a build tag limiting it to gc and
non-js/non-wasip1 targets (e.g. add a //go:build gc && !js && !wasip1 line at
the top) and add a brief comment explaining the non-portability and why the tag
exists; additionally create a new readonly_stub.go file with build tag
//go:build !gc || js || wasip1 that provides a safe stub implementation of
isReadOnly returning false and compiles on unsupported targets to avoid link
errors.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces conditional copy helpers GetString and GetBytes, which is a great enhancement for performance in immutable mode. The use of isReadOnly to avoid copying literals is a clever optimization. The codebase is consistently updated to use these new helpers, and the renames from get* to to* improve clarity. The documentation has also been updated accordingly.
I've added one suggestion in app_test.go to make the tests for GetBytes safer and more robust by using unsafe.SliceData for pointer comparisons, which correctly handles empty slices.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app_test.go (1)
29-29: Gate unsafe.StringData/SliceData behind Go 1.20+ (build tag or runtime skip).These tests rely on unsafe.StringData/unsafe.SliceData (Go ≥ 1.20). Add a file-level build tag or skip at runtime to avoid CI/toolchain breakage on older Go versions.
Option A — build constraint (preferred). Add to the very top of this file, before
package fiber://go:build go1.20 // +build go1.20Option B — runtime guard (if you want one file for all Go versions):
if !strings.HasPrefix(runtime.Version(), "go1.20") && !strings.HasPrefix(runtime.Version(), "go1.21") && !strings.HasPrefix(runtime.Version(), "go1.22") && !strings.HasPrefix(runtime.Version(), "go1.23") && !strings.HasPrefix(runtime.Version(), "go1.24") && !strings.HasPrefix(runtime.Version(), "go1.25") { t.Skip("requires Go 1.20+ for unsafe.StringData/SliceData") }Quick check script to confirm repo min Go version:
#!/bin/bash # Expect something like "go 1.22" or higher rg -n '^go\s+\d+\.\d+' go.mod -n --no-line-number || true
🧹 Nitpick comments (3)
app_test.go (3)
593-594: Decouple tests from unexported internals; prefer plain conversion.Using
app.toString(body)couples the test to internal, unexported hooks. A plainstring(body)keeps the test intent clear and resilient to refactors.- require.Equal(t, "اختبار", app.toString(body)) + require.Equal(t, "اختبار", string(body))
629-630: Same here: avoid usingapp.toStringin tests.Prefer
string(body)in tests to avoid depending on internal conversion hooks.- require.Equal(t, "/AbC", app.toString(body)) + require.Equal(t, "/AbC", string(body))
985-1006: Use testify for assertions and keep gosec suppressions local to the call sites.The logic is fine. For consistency (project uses require/assert) and clearer failure messages, switch
t.Errorfbranches torequire.Equal/NotEqual. Keep//nolint:gosecon the exact lines that call unsafe APIs so golangci-lint doesn’t fail.- same := appMutable.GetString(heap) - if unsafe.StringData(same) != unsafe.StringData(heap) { //nolint:gosec // compare pointer addresses - t.Errorf("expected original string when immutable is disabled") - } + same := appMutable.GetString(heap) + require.Equal(t, unsafe.StringData(heap), unsafe.StringData(same)) //nolint:gosec // compare pointer addresses appImmutable := New(Config{Immutable: true}) - copied := appImmutable.GetString(heap) - if unsafe.StringData(copied) == unsafe.StringData(heap) { //nolint:gosec // compare pointer addresses - t.Errorf("expected a copy for heap-backed string when immutable is enabled") - } + copied := appImmutable.GetString(heap) + require.NotEqual(t, unsafe.StringData(heap), unsafe.StringData(copied)) //nolint:gosec literal := "fiber" - sameLit := appImmutable.GetString(literal) - if unsafe.StringData(sameLit) != unsafe.StringData(literal) { //nolint:gosec // compare pointer addresses - t.Errorf("expected original literal when immutable is enabled") - } + sameLit := appImmutable.GetString(literal) + require.Equal(t, unsafe.StringData(literal), unsafe.StringData(sameLit)) //nolint:gosec
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app_test.go(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
app_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
app_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
app_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
app_test.go
🧬 Code graph analysis (1)
app_test.go (1)
app.go (2)
New(515-628)Config(113-418)
🪛 GitHub Check: lint
app_test.go
[failure] 1030-1030:
G103: Use of unsafe calls should be audited (gosec)
[failure] 1023-1023:
G103: Use of unsafe calls should be audited (gosec)
[failure] 1014-1014:
G103: Use of unsafe calls should be audited (gosec)
🪛 GitHub Actions: golangci-lint
app_test.go
[error] 1014-1014: golangci-lint run: G103: Use of unsafe calls should be audited (gosec)
⏰ 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). (3)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
app_test.go (1)
31-31: Import alias standardization looks good.Importing utils/v2 as
utilsaligns with the rest of the project and improves readability.
Summary
fiber.CtxTesting
make audit(fails: EncodeMsg passes lock by value; package requires newer Go version go1.25)make generatemake betteralign(fails: package requires newer Go version go1.25)make modernize(fails: package requires newer Go version go1.25)make formatmake testhttps://chatgpt.com/codex/tasks/task_e_68a80e3501ac83269ec38c74c210916e