♻️ refactor: Replace string functions#3923
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. WalkthroughReplaced many string-splitting/containment patterns with safer/faster alternatives (strings.Cut, strings.IndexByte, bytes.Cut), introduced preallocated byte-slices and utils helpers, adjusted import ordering, and replaced some allocations with utils.Unsafe*/slices.Clone; no exported API signatures changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-16T07:19:52.418ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @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 focuses on a significant refactoring effort across the codebase to modernize string and byte manipulation. By consistently adopting Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3923 +/- ##
==========================================
- Coverage 91.64% 91.62% -0.02%
==========================================
Files 119 119
Lines 10185 10187 +2
==========================================
Hits 9334 9334
- Misses 539 540 +1
- Partials 312 313 +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 is a nice refactoring that simplifies string and byte slice manipulation across the codebase by using functions like strings.Cut and strings.Contains. These changes generally improve code readability and maintainability. I've identified a couple of areas for improvement. One is a logic issue in address parsing that isn't fully resolved by the refactoring, and I've suggested a more robust approach using the net package. The other is a minor performance regression in one of the refactored lines due to unnecessary memory allocation in a loop, for which I've proposed reverting to the previous, more efficient implementation.
There was a problem hiding this comment.
Pull request overview
This PR refactors string and byte searching operations throughout the codebase by replacing manual index-based operations with more idiomatic strings.Cut/bytes.Cut calls and replacing some IndexByte checks with Contains. The changes improve code readability and add validation checks in CORS and CSRF middleware.
- Replaced
strings.Index/bytes.Indexpatterns withstrings.Cut/bytes.Cutfor cleaner string splitting - Replaced some
IndexBytechecks withContains/ContainsRunefor readability - Added validation for normalized origins in CORS and CSRF middleware
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| res.go | Refactored greedy parameter check to use bytes.Contains |
| req.go | Refactored Host(), Is(), Scheme(), and Range() methods to use strings.Cut |
| path.go | Refactored parameter length finding to use strings.Cut and strings.Contains |
| middleware/static/static.go | Refactored sanitizePath() to use strings.Contains and prefix handling with strings.Cut |
| middleware/proxy/proxy.go | Simplified scheme extraction using bytes.Cut |
| middleware/logger/template_chain.go | Refactored template parsing to use bytes.Cut for cleaner tag extraction |
| middleware/csrf/csrf.go | Refactored origin parsing with strings.Cut and added post-normalization validation, plus constant usage for scheme comparison |
| middleware/cors/utils.go | Refactored scheme matching and domain normalization to use strings.Cut |
| middleware/cors/cors.go | Refactored origin parsing with strings.Cut and added post-normalization validation |
| middleware/basicauth/basicauth.go | Simplified credential parsing using strings.Cut |
| helpers.go | Refactored MIME type and language acceptance checks to use strings.Cut and strings.Contains |
| ctx.go | Refactored transfer encoding parsing to use strings.Cut |
| client/core.go | Simplified port checking to use strings.ContainsRune |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
middleware/cors/cors.go (1)
49-77: Wildcard origin normalization is more robust; minor panic message nitThe new
strings.Cut(trimmedOrigin, "://*.")+normalizeOriginflow produces a canonicalscheme://hostbefore constructingsubdomain{prefix: scheme + "://", suffix: host}, which integrates cleanly with thesubdomain.matchlogic and ensures invalid wildcard origins are rejected early.One small readability nit: the panic string for the post-normalization failure is missing a space before the redacted value.
- panic("[CORS] Invalid origin format after normalization:" + maskValue(trimmedOrigin)) + panic("[CORS] Invalid origin format after normalization: " + maskValue(trimmedOrigin))path.go (1)
545-584: Param length calculation now enforces segment boundaries more strictlyThe updated
findParamLenbehavior:
- Still respects precomputed
segment.Lengthand greedy handling.- Uses
strings.Cut(s, segment.ComparePart)to locate the following constant, and for non‑greedy params rejects matches where the prefix contains'/', so routes like/api/:param/fixedEndwon’t accept/api/123/456/fixedEnd. This is a correctness improvement for non‑greedy parameters.
findParamLenForLastSegment’s switch tostrings.Cuton"/"keeps the existing semantics (first slash splits the last segment) while simplifying the code.If you want to make the invariants more explicit and guard against future changes that might produce a parameter with an empty
ComparePartreaching theCutbranch, you could add a cheap early return:func findParamLen(s string, segment *routeSegment) int { @@ - if before, _, found := strings.Cut(s, segment.ComparePart); found { + if segment.ComparePart == "" { + return len(s) + } + if before, _, found := strings.Cut(s, segment.ComparePart); found { // if the compare part was found, but contains a slash although this part is not greedy, then it must not match // example: /api/:param/fixedEnd -> path: /api/123/456/fixedEnd = no match , /api/123/fixedEnd = match if !segment.IsGreedy && strings.Contains(before, "/") { return 0 } return len(before) }Not strictly required with current route construction, but it clarifies expectations and avoids relying on
Cut’s empty‑separator behavior.helpers.go (1)
286-323: Cut-based MIME parsing inacceptsOfferTypepreserves wildcard semanticsSwitching to
strings.Cutfor splittingspecandmimetypearound'/', guarded bymimeFound && specFound, avoids index math and panics while keeping behavior for exact matches and<type>/*wildcards intact; allowingmimeAfter == "*"also safely supports wildcard offers. One minor nit is that the comment mentioning only<MIME_type>/*is now slightly narrower than what the code accepts.req.go (1)
321-331: X-Forwarded-Host first-value extraction viastrings.Cutis correctUsing
strings.Cut(host, ",")to return the substring before the first comma correctly selects the leftmost host in multi-hop proxy lists while preserving existing behavior for single-valued headers; optionally trimming spaces aroundbeforewould make it a bit more robust against headers like"example.com, proxy".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client/core.go(1 hunks)ctx.go(1 hunks)helpers.go(2 hunks)middleware/basicauth/basicauth.go(1 hunks)middleware/cors/cors.go(1 hunks)middleware/cors/utils.go(2 hunks)middleware/csrf/csrf.go(2 hunks)middleware/logger/template_chain.go(1 hunks)middleware/proxy/proxy.go(1 hunks)middleware/static/static.go(4 hunks)path.go(2 hunks)req.go(4 hunks)res.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/basicauth/basicauth.goctx.gomiddleware/static/static.gomiddleware/cors/utils.gopath.gomiddleware/logger/template_chain.gores.goreq.gohelpers.gomiddleware/csrf/csrf.gomiddleware/cors/cors.goclient/core.gomiddleware/proxy/proxy.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
path.goreq.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 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:
req.go
🧬 Code graph analysis (2)
middleware/logger/template_chain.go (1)
middleware/logger/errors.go (1)
ErrTemplateParameterMissing(8-8)
req.go (1)
error.go (1)
ErrRangeMalformed(32-32)
⏰ 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: Agent
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (18)
middleware/static/static.go (4)
45-45: LGTM: Clearer loop condition.The refactor to
strings.Containsis more idiomatic and improves readability. The loop semantics remain unchanged—it continues unescaping while percent-encoded sequences exist.
57-57: LGTM: More explicit null-byte validation.The change to
strings.Contains(s, "\x00")is clearer and more explicit about checking for null bytes. This maintains the security validation while improving code clarity.
119-121: LGTM: Safer and more idiomatic wildcard handling.The refactor to
strings.Cutis excellent. This modern Go approach (Go 1.18+) is:
- Safer than manual Index + slicing
- More efficient (single pass)
- Clearer in intent
The functional behavior is preserved—extracting the substring before the first asterisk for partial wildcard routes.
31-31: LGTM: Improved idiom for backslash detection.The change from
bytes.IndexBytetobytes.Containsis more idiomatic and clearer. The semantics remain equivalent, and thegithub.com/gofiber/utils/v2package does not provide a Contains-like helper for this operation, making the stdlib approach appropriate per coding guidelines.res.go (1)
617-621: Greedy parameter detection change keeps behavior intactUsing
bytes.Contains(greedyParameters, []byte{key[0]})preserves the original “single‑byte greedy key in the whitelist” semantics; given the tinygreedyParametersset and non–hot-path usage ingetLocationFromRoute, the extra single‑byte slice construction is acceptable here.client/core.go (1)
33-44: Simplified port detection remains correctSwitching to
strings.ContainsRune(addr, ':')for deciding whether to append the default port keeps the original behavior while making the intent clearer and avoiding manual index checks.middleware/cors/utils.go (1)
8-28: Scheme and port handling semantics are preserved and clearer
matchSchemenow cleanly extracts schemes viastrings.Cut(domain, ":")/strings.Cut(pattern, ":")and only matches when both sides actually have a scheme, which aligns with the function’s contract. ThenormalizeDomainchange tostrings.Cut(input, ":")under the non‑IPv6 condition keeps the previous “strip first port separator” behavior forhost:portwhile avoiding manual index math.ctx.go (1)
386-425: Transfer-Encoding parsing now correctly ignores parametersThe added
strings.Cut(token, ";")+ trim before checking for"identity"ensures that values likeidentity;q=0.8are treated as identity (i.e., no body), while any non‑identity encodings still sethasEncoding = true. This bringshasTransferEncodingBodycloser to the HTTP spec without changing behavior for simple cases.middleware/basicauth/basicauth.go (1)
84-104: Cut-based credential split is safer and clearerUsing
strings.Cut(creds, ":")both simplifies the"username:password"parsing and properly rejects malformed credentials that lack a colon, while preserving support for empty passwords and colon-containing passwords.middleware/proxy/proxy.go (1)
204-210: getScheme refactor preserves URI handlingRefactoring
getSchemetobytes.Cut(uri, []byte("://"))keeps the previous behavior—returning the bytes before the first"://"ornilif absent—while removing manual index arithmetic and making the intent of the code self-evident.middleware/csrf/csrf.go (2)
74-87: Trusted origin wildcard normalization viastrings.Cutlooks solidThe new
strings.Cut(trimmedOrigin, "://*.")path correctly rewritesscheme://*.hosttoscheme://hostfor normalization, then safely re-splits the normalized value intoschemeandhostwith a secondCut, panicking on invalid formats. This keepstrustedSubOriginsconsistent and improves validation without changing the matching semantics.
139-150: MissingOriginon non-HTTPS now bypasses referer check — confirm this is intentionalThe updated logic only falls back to
refererMatchesHostwhenc.Scheme() == schemeHTTPS, and otherwise clearserrOriginNotFound, effectively relaxing CSRF origin/referer enforcement for plain HTTP requests. That can be a deliberate trade-off (since referer is less reliable on HTTP), but it is a security-behavior change; please confirm this matches the intended CSRF policy and that tests/docs reflect it.helpers.go (2)
207-220: Language range wildcard handling remains consistent with intentUsing
strings.Contains(spec, "*")(after the special-casespec == "*") still enforces that only a standalone"*"is accepted as a wildcard and any other occurrence of*makes the range invalid, matching the documented behavior.
704-737:parseAddrhost/port splitting correctly handles IPv6 and edge casesThe function now prefers bracketed IPv6 (
[::1]:8080), usesLastIndexBytefor generichost:port, and falls back to treating unbracketed IPv6 literals containing additional:as pure hosts with no port, which is appropriate for its use inHostname().req.go (3)
488-502: Content-Type parameter stripping inIsis safely simplifiedReplacing the index/slice logic with
strings.Cut(ct, ";")to drop parameters, followed byutils.TrimSpaceandutils.EqualFold, keeps the same matching semantics while making the code clearer and less error-prone.
635-652: Scheme determination from X-Forwarded headers benefits from Cut-based comma handlingFor
X-Forwarded-Proto/X-Forwarded-Protocol, taking the substring before the first comma viastrings.Cutensures only the leftmost forwarded scheme is considered when multiple values are present, which aligns with typical proxy header semantics and avoids manual index management.
741-822: Range header Cut-based parsing looks sound; verify behavior for missing/malformed headersThe new
strings.Cutusage to separate<type>=<ranges>and to split each range on,and-keeps support fornnn-,-nnn, and multiple ranges while enforcing a single'='in the header and simplifying the control flow. Note that when theRangeheader is empty or lacks'=', you now returnErrRangeMalformedimmediately via the!foundcheck; please confirm all call sites treat this as expected (e.g., differentiating between “no Range header” and “malformed Range” when choosing status codes).middleware/logger/template_chain.go (1)
17-70:buildLogFuncChainCut-based parsing keeps func/param alignment and improves clarityThe refactor to use
bytes.CutaroundstartTagandendTagcleanly separates fixed segments from function blocks, and thebytes.Cut(before, paramSeparatorB)path correctly distinguishes parameterized tags (tagFunctions[tag+paramSeparator]with the param stored infixParts) from non-parameter tags, while preserving the existing behavior for missing end tags and maintainingfixParts/funcChainalignment.
|
I will change back the strings.Contains to the index functions |
… performance and readability
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
middleware/cors/cors.go (1)
66-66: Missing space after the colon in the error message.For consistency with the error message on line 62 and line 73, add a space after the colon.
- panic("[CORS] Invalid origin format after normalization:" + maskValue(trimmedOrigin)) + panic("[CORS] Invalid origin format after normalization: " + maskValue(trimmedOrigin))
🧹 Nitpick comments (2)
middleware/cors/utils.go (1)
92-92: Misleading performance comment.The nolint comment states
strings.Index is faster than strings.Contains, but in Go's standard library,strings.Containsis literally implemented asreturn Index(s, substr) >= 0. They have identical performance sinceContainsis just a thin wrapper aroundIndex.If the intent is to use index-based functions for consistency (per PR objectives), consider updating the comment to reflect that reason rather than an inaccurate performance claim.
- if sub == "" || sub[0] == '.' || strings.Index(sub, "..") >= 0 { //nolint:gocritic,staticcheck // strings.Index is faster than strings.Contains + if sub == "" || sub[0] == '.' || strings.Index(sub, "..") >= 0 { //nolint:gocritic,staticcheck // using Index for consistency with codebase stylemiddleware/compress/compress.go (1)
115-119: Weak ETag check is correct; consider optional readability helperThe new condition
tag != "" && (len(tag) < 2 || tag[0] != 'W' || tag[1] != '/')is logically equivalent to the old!strings.HasPrefix(tag, "W/")and remains safe (short‑circuiting prevents out‑of‑bounds). This looks good and keeps things allocation‑free. If you want to make it more self‑documenting, you could optionally extract this into a smallisWeakETag(tag string) boolhelper and negate that here, but it’s not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app.go(2 hunks)binder/mapping.go(2 hunks)helpers.go(5 hunks)listen.go(1 hunks)middleware/cache/cache.go(1 hunks)middleware/compress/compress.go(1 hunks)middleware/cors/cors.go(2 hunks)middleware/cors/utils.go(5 hunks)middleware/csrf/helpers.go(3 hunks)middleware/idempotency/idempotency.go(1 hunks)middleware/logger/logger.go(1 hunks)middleware/redirect/redirect.go(2 hunks)middleware/static/static.go(3 hunks)path.go(2 hunks)req.go(5 hunks)res.go(4 hunks)services.go(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- middleware/logger/logger.go
- middleware/cache/cache.go
🚧 Files skipped from review as they are similar to previous changes (4)
- middleware/static/static.go
- path.go
- res.go
- helpers.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
app.gomiddleware/redirect/redirect.goreq.gomiddleware/cors/cors.gobinder/mapping.gomiddleware/compress/compress.gomiddleware/idempotency/idempotency.gomiddleware/csrf/helpers.gomiddleware/cors/utils.goservices.golisten.go
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
app.gomiddleware/redirect/redirect.gomiddleware/idempotency/idempotency.gomiddleware/cors/utils.goservices.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 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:
middleware/redirect/redirect.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
middleware/redirect/redirect.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 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:
req.gomiddleware/cors/cors.gomiddleware/idempotency/idempotency.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/idempotency/idempotency.go
📚 Learning: 2025-05-07T13:07:33.899Z
Learnt from: mdelapenya
Repo: gofiber/fiber PR: 3434
File: docs/api/services.md:39-43
Timestamp: 2025-05-07T13:07:33.899Z
Learning: When documenting Go interface methods in the Fiber project, avoid showing method signatures with the interface type as the receiver (e.g., `func (d *Service) Method()`) since interfaces cannot be used as receivers in Go. Instead, show just the method signature without a receiver or use a placeholder implementation name.
Applied to files:
services.go
🧬 Code graph analysis (3)
app.go (2)
log/fiberlog.go (1)
Error(14-16)constants.go (1)
ErrNotImplemented(149-149)
req.go (1)
error.go (1)
ErrRangeMalformed(32-32)
middleware/compress/compress.go (1)
constants.go (1)
HeaderETag(181-181)
⏰ 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: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: lint
- GitHub Check: Compare
🔇 Additional comments (22)
middleware/idempotency/idempotency.go (1)
54-56: LGTM! Follows coding guidelines with proper safety measures.The combination of
utils.ToLowerandutils.CopyStringcorrectly ensures map keys are immutable copies, addressing the unsafe string usage mentioned in the comment. The approach aligns with the coding guideline to prefergithub.com/gofiber/utils/v2helpers for string manipulation. The added comment clearly documents the rationale, and the pattern is consistently applied (note that line 154's lookup correctly usesutils.ToLowerwithoutCopyStringsince it's only a temporary comparison value, not a stored key).As per coding guidelines, this properly uses utils helpers for string operations.
binder/mapping.go (2)
340-340: LGTM!Using
strings.IndexBytefor single-character presence checks is more efficient thanstrings.Contains.
379-386: LGTM!The refactored
assignBindDatafunction correctly usesstrings.IndexBytefor comma detection and properly handles the else branch to append single values.middleware/csrf/helpers.go (3)
61-63: LGTM!The change to
strings.IndexBytefor detecting colons in IPv6 hostnames is correct and more efficient for single-byte checks.
86-88: LGTM!Using
strings.IndexBytefor wildcard detection is appropriate and efficient.
129-132: LGTM!Using
strings.Indexfor the multi-character substring".."is correct. The nolint comment appropriately documents the performance rationale.app.go (2)
673-673: LGTM!Using
strings.IndexBytefor detecting the CIDR slash character is correct and efficient.
1350-1353: LGTM!The refactor to
strings.Indexfor error message detection preserves existing behavior. The nolint comments appropriately document the performance rationale.req.go (6)
324-327: LGTM!Using
strings.Cutfor comma-separated header parsing is cleaner and correctly returns the portion before the first comma.
497-501: LGTM!Using
strings.Cutto strip Content-Type parameters after the semicolon is cleaner and maintains the same behavior.
647-651: LGTM!The refactor to
strings.Cutfor extracting the scheme from comma-separated forwarded headers is correct and more readable.
760-791: LGTM!The Range parsing has been cleanly refactored to use
strings.Cut:
- Line 761 correctly validates no extra
=characters- Lines 776-778 properly iterate through comma-separated ranges
- Lines 786-791 correctly parse start-end bounds with hyphen separator
The logic is equivalent and more readable than the previous index-based approach.
852-856: LGTM!Using
strings.Indexfor the multi-character"xn--"punycode prefix check is correct. The nolint comment documents the performance rationale.
860-862: LGTM!The length-based bracket check is more efficient than
HasPrefix/HasSuffixfor single-character checks at known positions.middleware/cors/cors.go (2)
58-68: LGTM on the refactored wildcard origin parsing.The use of
strings.Cutto detect and process wildcard subdomain patterns (://*.) is cleaner. The subsequent normalization and scheme/host extraction is correct.
101-101: LGTM!Using
utils.ToLoweraligns with the coding guidelines to prefergithub.com/gofiber/utils/v2helpers.middleware/cors/utils.go (5)
6-7: LGTM!Adding the utils import aligns with the coding guidelines to prefer
github.com/gofiber/utils/v2helpers for common operations.
12-14: LGTM!Using
strings.Cutis idiomatic for extracting the scheme portion, and the logic correctly ensures both inputs contain a scheme separator before comparing.
24-26: LGTM!Clean use of
strings.Cutto extract the host portion before the port separator.
45-45: LGTM!Using
strings.IndexBytefor single-character search is appropriate and consistent with the PR's objective of using index-based functions.
57-59: LGTM!Good use of
utils.ToLowerper coding guidelines, with appropriateutils.CopyStringwrapper to ensure safety when the result is stored. The comment clearly explains the rationale.middleware/redirect/redirect.go (1)
9-9: LGTM: Import aligns with coding guidelines.Adding the utils/v2 package follows the established guideline to prefer Fiber's utility helpers for string manipulation.
As per coding guidelines and learnings, prefer
github.com/gofiber/utils/v2helpers when performing common operations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
router.go (1)
331-349: Flash-cookie detection refactor is behaviorally equivalentUsing
bytes.Index(rawHeaders, flashCookieNameBytes) >= 0(with the length guard) preserves the priorbytes.Containsbehavior for bothDefaultCtxandCustomCtxpaths while matching the PR’s index‑based pattern. The duplication between the two branches is acceptable and keeps their behavior in lockstep.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
middleware/etag/etag.go(1 hunks)middleware/static/static.go(2 hunks)res.go(4 hunks)router.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- middleware/static/static.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/etag/etag.gores.gorouter.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 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:
router.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 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:
router.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). (3)
- GitHub Check: repeated
- GitHub Check: Compare
- GitHub Check: lint
🔇 Additional comments (4)
middleware/etag/etag.go (1)
105-110: Index-based ETag match preserves previous behaviorSwitching from
bytes.Containstobytes.Index(...) >= 0keeps the same containment semantics, is nil/empty‑safe forclientEtag, and doesn’t change the weak/strong ETag handling logic. The added//nolintis appropriate given the explicit perf intent.res.go (3)
619-619: LGTM! Performance issue correctly resolved.The change from
bytes.Contains(greedyParameters, []byte{key[0]})tobytes.IndexByte(greedyParameters, key[0]) >= 0properly addresses the past review comments about memory allocation. This avoids creating a temporary byte slice on each loop iteration.
660-660: Intentional optimization per PR objectives.The use of
strings.Indexwith>= 0check instead ofstrings.Containsis consistent with this PR's performance optimization goals. The nolint comment appropriately documents the rationale.
1022-1024: Manual prefix check is correct and safe.The length-based prefix check properly guards against out-of-bounds access and is functionally equivalent to
strings.HasPrefix.
…stency and performance
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 (1)
middleware/csrf/config.go (1)
5-5: Remove unusedstringsimport to fix build failureThe
stringsimport at line 5 is no longer used after switching toutils.EqualFold. Go treats unused imports as compilation errors and must be removed.import ( "fmt" - "strings" "time" "github.com/gofiber/fiber/v3" "github.com/gofiber/fiber/v3/extractors" "github.com/gofiber/fiber/v3/log" "github.com/gofiber/fiber/v3/middleware/session" "github.com/gofiber/utils/v2" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/cookiejar.go(3 hunks)ctx.go(2 hunks)helpers.go(8 hunks)middleware/csrf/config.go(1 hunks)req.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/csrf/config.goreq.goclient/cookiejar.gohelpers.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/csrf/config.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 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:
req.go
🪛 GitHub Actions: Modernize Lint
middleware/csrf/config.go
[error] 5-5: "strings" imported and not used
[error] 5-5: Go build failed due to unused import while running: go run golang.org/x/tools/gopls/internal/analysis/modernize/cmd/modernize@latest -test=false ./...
🪛 GitHub Actions: Run govulncheck
middleware/csrf/config.go
[error] 5-5: "strings" imported and not used
⏰ 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: lint
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (17)
middleware/csrf/config.go (1)
217-220: EqualFold change looks good and matches project utilitiesUsing
utils.EqualFoldhere is consistent with the rest of the middleware and the guideline to prefergithub.com/gofiber/utils/v2helpers for string ops, while preserving the existing case‑insensitive warning behavior.client/cookiejar.go (2)
56-56: LGTM! Usages are correct.Both call sites correctly use the pre-allocated
httpsSchemeslice, eliminating inline allocations while preserving the original logic.Also applies to: 218-218
21-22: LGTM! Good performance optimization.Pre-allocating the
httpsSchemebyte slice eliminates repeated allocations at the comparison sites (lines 56 and 218), improving performance without changing behavior. The read-only usage pattern is thread-safe and appropriate for this performance-critical path.helpers.go (8)
45-50: LGTM! Good performance optimization.Preallocating byte slices for common header patterns avoids repeated allocations during header parsing.
218-218: LGTM! Addresses past review feedback.The switch from
strings.Containstostrings.IndexByte(as suggested in past review) improves performance. Using>= 0instead of!= -1is equally performant and more idiomatic in Go.
320-327: LGTM! Safer MIME type parsing.Using
strings.Cutfor splitting MIME types is more idiomatic and eliminates slice bounds concerns compared to manualIndex+ slicing.
524-524: LGTM! Efficient quality value parsing.Using the preallocated
semicolonQEqualsbyte slice withHasPrefixavoids repeated allocations in the hot path.
566-568: LGTM! Consistent use of preallocated byte slices.Using preallocated
wildcardAllandwildcardSuffixfor wildcard checks maintains consistency with the optimization pattern and reduces allocations.
639-639: LGTM! More explicit weak ETag detection.The explicit length and index checks are clear and perform the same validation as
HasPrefix("W/")while being slightly more direct.
719-719: LGTM! Safer IPv6 bracket detection.Adding the explicit
raw != ""check before indexing prevents potential panics on empty input, though the function already returns early for empty strings at line 712.
736-736: LGTM! Optimized single-character search.Using
IndexBytefor single-character searches is more efficient thanContainsand aligns with the PR's optimization strategy.req.go (6)
18-26: LGTM! Well-organized header constants.Preallocating byte slices for frequently-checked header patterns reduces allocations in request parsing hot paths.
334-336: LGTM! Cleaner comma-separated host parsing.Using
strings.Cutto extract the first host from comma-separatedX-Forwarded-Hostvalues is more readable than manual index-based slicing.
507-509: LGTM! Cleaner Content-Type parameter stripping.Using
strings.Cutto separate the MIME type from parameters is more idiomatic than index-based slicing.
653-667: LGTM! Optimized scheme detection with preallocated byte slices.The refactored scheme detection uses preallocated byte slices and
strings.Cutfor cleaner parsing of comma-separated proxy headers.
870-870: LGTM! Efficient IPv6 bracket detection.The explicit length check and index comparison is more direct than
HasPrefix/HasSuffixand clearly conveys the intent of detecting bracketed IPv6 addresses.
770-801: Range header parsing correctly usesstrings.Cutfor safer parsing.The refactor properly handles:
- Type/ranges separation on "=" with validation for multiple "=" characters
- Comma-separated range segments parsing
- Start/end bounds extraction with "-" delimiter
The code appropriately uses
utils.ToLower()andutils.TrimSpace()from the utils package per the coding guidelines.Recommend verifying test coverage exists for edge cases: multiple ranges (e.g., "bytes=0-100,200-300"), suffix ranges (e.g., "bytes=-500"), open-ended ranges (e.g., "bytes=500-"), and malformed inputs (e.g., "bytes==0-100").
… readability and consistency
…onsistency and performance
…r consistency and performance
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 25de9c5 | Previous: a331406 | Ratio |
|---|---|---|---|
Benchmark_Ctx_Write |
30.67 ns/op 76 B/op 0 allocs/op |
18.04 ns/op 70 B/op 0 allocs/op |
1.70 |
Benchmark_Ctx_Write - ns/op |
30.67 ns/op |
18.04 ns/op |
1.70 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
_CBORBinder_Bind-12 149.7n ± 3% 143.8n ± 4% -3.94% (p=0.011 n=6)
_CookieBinder_Bind-12 1.634µ ± 7% 1.604µ ± 0% -1.84% (p=0.002 n=6)
_FormBinder_Bind-12 1.634µ ± 2% 1.645µ ± 1% ~ (p=0.567 n=6)
_FormBinder_BindMultipart-12 1.668µ ± 2% 1.654µ ± 5% ~ (p=0.286 n=6)
_HeaderBinder_Bind-12 1.636µ ± 1% 1.626µ ± 1% ~ (p=0.061 n=6)
_QueryBinder_Bind-12 1.611µ ± 2% 1.603µ ± 2% -0.50% (p=0.037 n=6)
_RespHeaderBinder_Bind-12 1.711µ ± 1% 1.716µ ± 3% ~ (p=0.318 n=6)There was a problem hiding this comment.
│ client_old.txt │ client_new.txt │
│ sec/op │ sec/op vs base │
_Client_Request-12 5.487µ ± 1% 5.434µ ± 3% ~ (p=0.310 n=6)
_Client_Request_Parallel-12 2.766µ ± 8% 2.747µ ± 8% ~ (p=0.818 n=6)
_Client_Request_Send_ContextCancel-12 5.577µ ± 2% 5.550µ ± 1% ~ (p=0.288 n=6)
geomean 4.390µ 4.359µ -0.71%
│ client_old.txt │ client_new.txt │
│ B/op │ B/op vs base │
_Client_Request-12 81.00 ± 1% 81.00 ± 0% ~ (p=1.000 n=6)
_Client_Request_Parallel-12 82.50 ± 3% 83.00 ± 2% ~ (p=1.000 n=6)
_Client_Request_Send_ContextCancel-12 386.5 ± 2% 387.0 ± 3% ~ (p=0.861 n=6)
geomean 137.2 137.5 +0.24%There was a problem hiding this comment.
│ basicauth_old.txt │ basicauth_new.txt │
│ sec/op │ sec/op vs base │
_Middleware_BasicAuth-12 287.1n ± 10% 286.3n ± 2% ~ (p=0.288 n=6)
_Middleware_BasicAuth_Upper-12 294.9n ± 4% 286.4n ± 13% ~ (p=0.065 n=6)
geomean 290.9n 286.4n -1.57%There was a problem hiding this comment.
│ cache_old.txt │ cache_new.txt │
│ sec/op │ sec/op vs base │
_Cache-12 51.59µ ± 15% 50.60µ ± 9% ~ (p=0.093 n=6)
_Cache_Miss-12 56.45µ ± 5% 54.48µ ± 3% -3.50% (p=0.009 n=6)
_Cache_Storage-12 50.83µ ± 5% 50.34µ ± 3% ~ (p=0.240 n=6)
_Cache_AdditionalHeaders-12 100.2µ ± 17% 102.5µ ± 4% ~ (p=0.065 n=6)
_Cache_MaxSize/Disabled-12 404.4n ± 1% 403.4n ± 1% ~ (p=0.290 n=6)
_Cache_MaxSize/Unlim-12 404.2n ± 1% 403.4n ± 1% ~ (p=0.225 n=6)
_Cache_MaxSize/LowBounded-12 404.7n ± 0% 403.5n ± 1% ~ (p=0.223 n=6)There was a problem hiding this comment.
│ compress_old.txt │ compress_new.txt │
│ sec/op │ sec/op vs base │
_Compress/Gzip-12 242.2n ± 7% 243.1n ± 2% ~ (p=0.372 n=6)
_Compress/Deflate-12 238.4n ± 2% 243.9n ± 2% +2.33% (p=0.002 n=6)
_Compress/Brotli-12 241.7n ± 2% 244.4n ± 1% ~ (p=0.091 n=6)
_Compress/Zstd-12 240.1n ± 1% 245.2n ± 16% +2.10% (p=0.002 n=6)
_Compress_Levels/Gzip_LevelDisabled-12 81.57n ± 1% 83.42n ± 1% +2.26% (p=0.009 n=6)
_Compress_Levels/Gzip_LevelDefault-12 242.9n ± 1% 248.3n ± 6% +2.24% (p=0.002 n=6)
_Compress_Levels/Gzip_LevelBestSpeed-12 241.4n ± 1% 244.9n ± 0% +1.45% (p=0.002 n=6)
_Compress_Levels/Gzip_LevelBestCompression-12 242.8n ± 2% 245.0n ± 1% ~ (p=0.065 n=6)
_Compress_Levels/Deflate_LevelDisabled-12 81.33n ± 1% 82.15n ± 25% +1.01% (p=0.011 n=6)
_Compress_Levels/Deflate_LevelDefault-12 242.4n ± 1% 244.4n ± 1% +0.80% (p=0.009 n=6)
_Compress_Levels/Deflate_LevelBestSpeed-12 241.4n ± 1% 245.3n ± 1% +1.66% (p=0.004 n=6)
_Compress_Levels/Deflate_LevelBestCompression-12 242.4n ± 1% 244.5n ± 0% +0.85% (p=0.037 n=6)
_Compress_Levels/Brotli_LevelDisabled-12 81.45n ± 1% 81.83n ± 0% ~ (p=0.058 n=6)
_Compress_Levels/Brotli_LevelDefault-12 242.7n ± 1% 243.1n ± 1% ~ (p=0.455 n=6)
_Compress_Levels/Brotli_LevelBestSpeed-12 247.4n ± 4% 244.3n ± 1% ~ (p=0.126 n=6)
_Compress_Levels/Brotli_LevelBestCompression-12 254.7n ± 5% 252.2n ± 0% ~ (p=0.121 n=6)
_Compress_Levels/Zstd_LevelDisabled-12 82.35n ± 3% 81.87n ± 2% ~ (p=0.589 n=6)
_Compress_Levels/Zstd_LevelDefault-12 242.7n ± 1% 244.4n ± 1% +0.70% (p=0.030 n=6)
_Compress_Levels/Zstd_LevelBestSpeed-12 243.5n ± 18% 247.4n ± 2% ~ (p=0.223 n=6)
_Compress_Levels/Zstd_LevelBestCompression-12 246.4n ± 2% 252.6n ± 13% +2.50% (p=0.041 n=6)
_Compress_Parallel/Gzip-12 30.26n ± 4% 30.50n ± 8% ~ (p=1.000 n=6)
_Compress_Parallel/Deflate-12 31.05n ± 6% 30.54n ± 5% ~ (p=0.394 n=6)
_Compress_Parallel/Brotli-12 33.48n ± 14% 33.24n ± 17% ~ (p=0.937 n=6)
_Compress_Parallel/Zstd-12 30.09n ± 17% 28.87n ± 5% ~ (p=0.065 n=6)
_Compress_Levels_Parallel/Gzip_LevelDisabled-12 10.41n ± 7% 10.08n ± 12% ~ (p=0.310 n=6)
_Compress_Levels_Parallel/Gzip_LevelDefault-12 29.41n ± 6% 30.11n ± 24% ~ (p=0.240 n=6)
_Compress_Levels_Parallel/Gzip_LevelBestSpeed-12 28.93n ± 6% 29.46n ± 8% ~ (p=0.589 n=6)
_Compress_Levels_Parallel/Gzip_LevelBestCompression-12 30.78n ± 22% 29.34n ± 8% ~ (p=0.180 n=6)
_Compress_Levels_Parallel/Deflate_LevelDisabled-12 9.822n ± 5% 10.170n ± 9% ~ (p=0.093 n=6)
_Compress_Levels_Parallel/Deflate_LevelDefault-12 29.77n ± 7% 31.03n ± 5% ~ (p=0.180 n=6)
_Compress_Levels_Parallel/Deflate_LevelBestSpeed-12 28.65n ± 4% 33.85n ± 32% +18.15% (p=0.009 n=6)
_Compress_Levels_Parallel/Deflate_LevelBestCompression-12 29.47n ± 6% 28.41n ± 5% ~ (p=0.065 n=6)
_Compress_Levels_Parallel/Brotli_LevelDisabled-12 10.40n ± 4% 10.03n ± 5% ~ (p=0.071 n=6)
_Compress_Levels_Parallel/Brotli_LevelDefault-12 29.21n ± 12% 29.02n ± 32% ~ (p=0.699 n=6)
_Compress_Levels_Parallel/Brotli_LevelBestSpeed-12 29.35n ± 11% 29.85n ± 7% ~ (p=1.000 n=6)
_Compress_Levels_Parallel/Brotli_LevelBestCompression-12 31.52n ± 10% 30.34n ± 7% ~ (p=0.240 n=6)
_Compress_Levels_Parallel/Zstd_LevelDisabled-12 10.54n ± 10% 10.29n ± 5% ~ (p=0.240 n=6)
_Compress_Levels_Parallel/Zstd_LevelDefault-12 29.39n ± 16% 29.87n ± 9% ~ (p=0.937 n=6)
_Compress_Levels_Parallel/Zstd_LevelBestSpeed-12 28.84n ± 24% 28.46n ± 6% ~ (p=0.485 n=6)
_Compress_Levels_Parallel/Zstd_LevelBestCompression-12 29.43n ± 4% 30.01n ± 10% ~ (p=0.699 n=6)
geomean 68.78n 69.20n +0.60%- revert compress adjustement
There was a problem hiding this comment.
│ cors_old.txt │ cors_new.txt │
│ sec/op │ sec/op vs base │
_CORS_NewHandler-12 237.1n ± 6% 231.8n ± 1% -2.21% (p=0.024 n=6)
_CORS_NewHandler_Parallel-12 35.93n ± 5% 34.72n ± 7% ~ (p=0.180 n=6)
_CORS_NewHandlerSingleOrigin-12 239.8n ± 1% 232.0n ± 1% -3.23% (p=0.002 n=6)
_CORS_NewHandlerSingleOrigin_Parallel-12 35.24n ± 8% 34.72n ± 5% ~ (p=0.240 n=6)
_CORS_NewHandlerWildcard-12 140.6n ± 1% 131.0n ± 1% -6.83% (p=0.002 n=6)
_CORS_NewHandlerWildcard_Parallel-12 16.74n ± 7% 15.74n ± 4% -5.94% (p=0.041 n=6)
_CORS_NewHandlerPreflight-12 641.2n ± 1% 639.5n ± 19% ~ (p=1.000 n=6)
_CORS_NewHandlerPreflight_Parallel-12 117.6n ± 4% 117.9n ± 4% ~ (p=0.669 n=6)
_CORS_NewHandlerPreflightSingleOrigin-12 643.8n ± 1% 632.2n ± 1% -1.79% (p=0.002 n=6)
_CORS_NewHandlerPreflightSingleOrigin_Parallel-12 116.5n ± 1% 115.0n ± 1% ~ (p=0.061 n=6)
_CORS_NewHandlerPreflightWildcard-12 579.7n ± 1% 572.0n ± 1% -1.33% (p=0.015 n=6)
_CORS_NewHandlerPreflightWildcard_Parallel-12 109.7n ± 7% 111.7n ± 3% ~ (p=0.665 n=6)
_CORS_SubdomainMatch-12 6.534n ± 0% 6.530n ± 2% ~ (p=0.732 n=6)
geomean 111.2n 109.0n -2.00%…arity and performance
# Conflicts: # client/core.go # helpers.go # middleware/basicauth/basicauth.go # middleware/cors/utils.go # req.go
…readability in MIME type handling
…readability in MIME type handling
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
req.go (1)
864-868: LGTM!Using
strings.Containsfor the multi-character punycode prefix detection ("xn--") is appropriate here. As discussed in previous reviews with benchmark data,strings.Containsandstrings.Indexhave identical performance since Contains is implemented asIndex(...) >= 0. For multi-character substrings, both are equally efficient.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app.goclient/core.goclient/transport.goctx.goextractors/extractors.gohelpers.golisten.gomiddleware/basicauth/config.gomiddleware/cache/cache.gomiddleware/cors/utils.gomiddleware/csrf/config.gomiddleware/csrf/csrf.gomiddleware/csrf/helpers.gomiddleware/encryptcookie/encryptcookie.gomiddleware/etag/etag.gomiddleware/pprof/pprof.gomiddleware/proxy/proxy.gomiddleware/redirect/redirect.gomiddleware/session/session.gomiddleware/static/static.goreq.gores.go
🚧 Files skipped from review as they are similar to previous changes (8)
- middleware/etag/etag.go
- middleware/cors/utils.go
- app.go
- middleware/proxy/proxy.go
- middleware/csrf/csrf.go
- ctx.go
- listen.go
- client/core.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
middleware/basicauth/config.gomiddleware/session/session.goclient/transport.goreq.gomiddleware/encryptcookie/encryptcookie.gomiddleware/csrf/helpers.gomiddleware/pprof/pprof.gomiddleware/redirect/redirect.gohelpers.gomiddleware/cache/cache.goextractors/extractors.gomiddleware/csrf/config.gores.gomiddleware/static/static.go
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 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`.
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
middleware/basicauth/config.gomiddleware/pprof/pprof.gomiddleware/redirect/redirect.gomiddleware/csrf/config.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 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:
middleware/basicauth/config.gomiddleware/encryptcookie/encryptcookie.gomiddleware/pprof/pprof.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
middleware/basicauth/config.gomiddleware/csrf/helpers.gomiddleware/csrf/config.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
middleware/basicauth/config.gomiddleware/encryptcookie/encryptcookie.gomiddleware/pprof/pprof.gomiddleware/csrf/config.go
📚 Learning: 2024-10-02T15:16:39.868Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:272-293
Timestamp: 2024-10-02T15:16:39.868Z
Learning: In the `middleware/session/session.go` file, the `saveSession()` method returns either `nil` or an error, so additional error wrapping in the `Save()` method is unnecessary.
Applied to files:
middleware/session/session.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/session.go:46-61
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In this codebase, the `sessionPool` only contains `Session` instances, so type assertions without additional checks are acceptable.
Applied to files:
middleware/session/session.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 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:
middleware/session/session.gomiddleware/cache/cache.go
📚 Learning: 2025-07-27T17:28:53.403Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3625
File: middleware/session/config.go:57-58
Timestamp: 2025-07-27T17:28:53.403Z
Learning: In the session middleware `Config` struct, the `Extractor` field uses function closures (like `FromCookie(key)`), making it impossible to introspect extractor parameters at runtime for validation purposes without complex reflection techniques.
Applied to files:
middleware/session/session.gomiddleware/csrf/config.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
middleware/encryptcookie/encryptcookie.gomiddleware/pprof/pprof.gomiddleware/redirect/redirect.gomiddleware/csrf/config.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 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:
middleware/pprof/pprof.gomiddleware/redirect/redirect.go
📚 Learning: 2025-09-18T00:32:59.671Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2025-09-18T00:32:59.671Z
Learning: In Go 1.22, manual copying of loop variables (like `x := x`) is no longer necessary due to the fix for variable capture in closures. Each loop iteration now creates its own instance of the loop variable automatically.
Applied to files:
extractors/extractors.go
🧬 Code graph analysis (2)
req.go (1)
constants.go (4)
HeaderXForwardedProto(221-221)HeaderXForwardedProtocol(222-222)HeaderXForwardedSsl(223-223)HeaderXUrlScheme(224-224)
middleware/cache/cache.go (1)
constants.go (1)
HeaderCacheControl(168-168)
⏰ 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 (30)
extractors/extractors.go (1)
511-512: LGTM: Performance optimization for token validation.The change from
range []byte(token)to index-based iteration eliminates an allocation by avoiding the byte slice copy. Since token68 only allows ASCII characters per RFC 7235/9110, direct string indexing is safe and maintains identical validation behavior.middleware/csrf/config.go (1)
216-216: LGTM! Refactor aligns with coding guidelines.The replacement of
strings.EqualFoldwithutils.EqualFoldis appropriate and consistent with the project's preference for usinggithub.com/gofiber/utils/v2helpers for string operations.As per coding guidelines, prefer
github.com/gofiber/utils/v2helpers for string manipulation.middleware/basicauth/config.go (2)
15-15: LGTM! Import supports utils helper migration.The addition of the
github.com/gofiber/utils/v2import is necessary for the refactor to useutils.EqualFoldbelow.
116-116: LGTM! Refactor aligns with coding guidelines.The replacement of
strings.EqualFoldwithutils.EqualFoldcorrectly maintains the case-insensitive charset validation behavior while aligning with the project's preference for using utility helpers.As per coding guidelines, prefer
github.com/gofiber/utils/v2helpers for string manipulation.middleware/redirect/redirect.go (1)
34-34: Good performance optimization usingutils.UnsafeString.The switch to
utils.UnsafeStringavoids a string allocation by directly referencing the underlying query string bytes. This is safe here because the string is immediately consumed to construct the redirect URL within the same request handler, and fasthttp guarantees the query string bytes remain valid for the request duration.Based on coding guidelines, using utils/v2 helpers for string operations is preferred.
middleware/cache/cache.go (2)
246-246: LGTM! Good performance optimization.Using
utils.UnsafeStringhere avoids allocation while safely converting the header byte slice to a string for read-only operations. This aligns with the coding guidelines to prefer utils package helpers.
327-327: LGTM! Consistent optimization.The use of
utils.UnsafeStringis appropriate here for the same reasons as line 246 - the header value is only read byparseMaxAgeand not retained or modified.middleware/session/session.go (1)
412-412: LGTM! Safe and consistent optimization.The use of
utils.UnsafeBytesis appropriate here. SinceSetBytesVcopies the byte data when setting the header, ands.idremains stable during this operation, this optimization safely reduces allocations while following the coding guidelines to prefer utils package helpers.middleware/pprof/pprof.go (1)
9-10: LGTM! Import ordering follows Go conventions.The import block correctly separates same-module imports (
fiber/v3) from external dependencies, following standard Go import organization (stdlib → external → internal/same-module).middleware/encryptcookie/encryptcookie.go (1)
7-8: LGTM! Import organization is correct.The import reordering properly separates the same-module
fiber/v3import from external dependencies, consistent with Go conventions and the changes inpprof.go.client/transport.go (1)
323-323: LGTM: Proper usage of the precomputed scheme constant.The scheme validation logic correctly uses
httpSchemeto restrict redirects to HTTP/HTTPS schemes only, preventing potential security issues from arbitrary protocol handlers.res.go (1)
619-619: LGTM!The change from
!= -1to>= 0is semantically equivalent and maintains the efficientbytes.IndexByteapproach that avoids the allocation issue previously flagged withbytes.Contains.middleware/csrf/helpers.go (4)
20-22: LGTM - Safe use of UnsafeBytes for zero-allocation comparison.Using
utils.UnsafeByteshere is safe becausesubtle.ConstantTimeCompareonly reads the byte slices and doesn't store or modify them. This avoids two allocations per comparison. Based on learnings, this aligns with fasthttp's zero-allocation design philosophy.
61-63: LGTM!Using
strings.IndexBytefor single-byte colon detection is consistent with the PR's performance optimization pattern.
86-88: LGTM!Consistent use of
strings.IndexBytefor single-character wildcard detection.
129-132: LGTM!The refactored condition
sub == "" || sub[0] == '.'is functionally equivalent to the previousstrings.HasPrefix(sub, ".")check but avoids the function call overhead. The empty string check safely guards against index out of bounds.helpers.go (4)
45-50: LGTM - Good use of pre-allocated byte slices.Pre-allocating these common byte patterns at package level avoids repeated allocations in the hot
getOfferpath. Since they're only used for read-only comparisons, there's no concurrency concern.
218-220: LGTM!The change from
!= -1to>= 0is semantically equivalent and maintains consistency with the PR's style.
524-524: LGTM!Good use of the pre-allocated byte slices for prefix/suffix/equality checks in the parsing logic, reducing allocations in this frequently called function.
Also applies to: 566-568
736-738: LGTM!Using
strings.IndexBytefor single-character colon detection is consistent with the PR's optimization pattern.req.go (4)
18-26: LGTM - Good use of pre-allocated byte slices for header comparison.Pre-allocating these header name byte slices at package level is an effective optimization for the
Scheme()method's hot path. The slices are initialized from the existing string constants and used only for read comparisons.
334-337: LGTM!Using
strings.Cutis cleaner than manual index-based slicing for extracting the first comma-separated value. The logic correctly returns the portion before the comma when found, or the full host otherwise.
653-670: LGTM!The refactored
Scheme()method effectively uses the pre-allocated byte slices for header comparisons and applies consistentstrings.Cutparsing for comma-separated header values. The logic is preserved with improved efficiency.
770-778: LGTM!Using
strings.Cutfor range header parsing is cleaner than manual index-based slicing. The additional checkstrings.IndexByte(after, '=') >= 0correctly rejects malformed range headers containing multiple=characters.middleware/static/static.go (6)
61-63: LGTM!Using
strings.IndexByte(s, '\x00')is the correct efficient approach for null-byte detection, addressing the performance concern from past reviews.
69-71: LGTM!Returning the precomputed
rootPathBytesavoids allocation. The shared slice concern is covered in the earlier comment.
123-126: Clean use ofstrings.Cutfor wildcard parsing.This is more readable than manual
strings.Index+ slicing while maintaining the same behavior of extracting the prefix before the first*.
162-172: LGTM!Using
rootPathBytesfor the file-at-root case is a good optimization. The shared slice concern from the earlier comment applies here as well sincesanitizePathmay returnrootPathBytesdirectly.
175-180: LGTM!Explicit slice construction with pre-sized allocation is clearer than append-based approach and avoids potential slice capacity surprises.
182-187: LGTM!Using the precomputed
invalidPathRespavoids allocation in the error path. The shared slice concern is lower risk here since it's intentionally an invalid path that triggers a 404.
… and improving byte slice usage in static.go
…in cookiejar.go and transport.go
Uh oh!
There was an error while loading. Please reload this page.