♻️ refactor: Replace string functions #3923
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughThis PR replaces many manual index/slice/contains patterns with safer, idiomatic parsing primitives (strings.Cut, strings.Index/IndexByte, bytes.Cut, length checks) and adopts small utils helpers across middleware and core packages. No exported/public API signatures were changed. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 10165 10165
==========================================
- Hits 9316 9314 -2
- Misses 538 539 +1
- Partials 311 312 +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:
|
ReneWerner87
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
client/core.go
Outdated
| if strings.ContainsRune(addr, ':') { | ||
| return addr | ||
| } |
There was a problem hiding this comment.
While this change is a valid refactoring, the underlying logic of checking for a colon to detect a port is not robust. It can lead to incorrect behavior with IPv6 addresses (e.g., [::1] or 2001:db8::1), which contain colons but may not have a port specified. This check would incorrectly assume a port is present and return the address as is.
A more reliable approach is to use net.SplitHostPort to parse the address and specifically check for a 'missing port' error.
if _, _, err := net.SplitHostPort(addr); err == nil {
return addr
} else if addrErr, ok := err.(*net.AddrError); !ok || !strings.Contains(addrErr.Err, "missing port") {
return addr
}
res.go
Outdated
| for key, val := range params { | ||
| isSame := key == segment.ParamName || (!app.config.CaseSensitive && utils.EqualFold(key, segment.ParamName)) | ||
| isGreedy := segment.IsGreedy && len(key) == 1 && bytes.IndexByte(greedyParameters, key[0]) != -1 | ||
| isGreedy := segment.IsGreedy && len(key) == 1 && bytes.Contains(greedyParameters, []byte{key[0]}) |
There was a problem hiding this comment.
The change from bytes.IndexByte to bytes.Contains with []byte{key[0]} introduces a small performance regression. The expression []byte{key[0]} causes a memory allocation for a new byte slice on every iteration of the loop. The previous implementation using bytes.IndexByte was more efficient as it avoids this allocation.
| isGreedy := segment.IsGreedy && len(key) == 1 && bytes.Contains(greedyParameters, []byte{key[0]}) | |
| isGreedy := segment.IsGreedy && len(key) == 1 && bytes.IndexByte(greedyParameters, key[0]) != -1 |
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 |
middleware/csrf/csrf.go
Outdated
| sd := subdomain{prefix: normalizedOrigin[:schemeSep], suffix: normalizedOrigin[schemeSep:]} | ||
| scheme, host, ok := strings.Cut(normalizedOrigin, "://") | ||
| if !ok { | ||
| panic("[CSRF] Invalid origin format after normalization:" + maskValue(origin)) |
There was a problem hiding this comment.
Missing space after the colon in the error message. For consistency with line 80 ("[CSRF] Invalid origin format in configuration:"), add a space: "[CSRF] Invalid origin format after normalization: " + maskValue(origin)
| panic("[CSRF] Invalid origin format after normalization:" + maskValue(origin)) | |
| panic("[CSRF] Invalid origin format after normalization: " + maskValue(origin)) |
| sd := subdomain{prefix: normalizedOrigin[:schemeSep], suffix: normalizedOrigin[schemeSep:]} | ||
| scheme, host, ok := strings.Cut(normalizedOrigin, "://") | ||
| if !ok { | ||
| panic("[CORS] Invalid origin format after normalization:" + maskValue(trimmedOrigin)) |
There was a problem hiding this comment.
Missing space after the colon in the error message. For consistency with line 80 ("[CSRF] Invalid origin format in configuration:") and line 62 in cors.go, add a space: "[CORS] Invalid origin format after normalization: " + maskValue(trimmedOrigin)
| panic("[CORS] Invalid origin format after normalization:" + maskValue(trimmedOrigin)) | |
| panic("[CORS] Invalid origin format after normalization: " + maskValue(trimmedOrigin)) |
![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 (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.
| if preData.BannerHeader != "" { | ||
| header := preData.BannerHeader | ||
| fmt.Fprint(out, header) | ||
| if !strings.HasSuffix(header, "\n") { | ||
| if header == "" || header[len(header)-1] != '\n' { | ||
| fmt.Fprintln(out) | ||
| } |
There was a problem hiding this comment.
Dead code: header == "" check is unreachable.
Since line 434 ensures preData.BannerHeader != "", the header variable assigned on line 435 is guaranteed to be non-empty. The header == "" check on line 437 can never be true.
Apply this diff to simplify the condition:
if preData.BannerHeader != "" {
header := preData.BannerHeader
fmt.Fprint(out, header)
- if header == "" || header[len(header)-1] != '\n' {
+ if header[len(header)-1] != '\n' {
fmt.Fprintln(out)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if preData.BannerHeader != "" { | |
| header := preData.BannerHeader | |
| fmt.Fprint(out, header) | |
| if !strings.HasSuffix(header, "\n") { | |
| if header == "" || header[len(header)-1] != '\n' { | |
| fmt.Fprintln(out) | |
| } | |
| if preData.BannerHeader != "" { | |
| header := preData.BannerHeader | |
| fmt.Fprint(out, header) | |
| if header[len(header)-1] != '\n' { | |
| fmt.Fprintln(out) | |
| } |
🤖 Prompt for AI Agents
In listen.go around lines 434 to 439, the check `header == ""` is unreachable
because the outer `if preData.BannerHeader != ""` guarantees non-empty header;
remove the redundant empty-string check and simplify the condition to only test
whether the last character is a newline (i.e., call fmt.Fprintln(out) when
header[len(header)-1] != '\n'), keeping the existing assignment and fmt.Fprint
call.
| func captureTokens(pattern *regexp.Regexp, input string) *strings.Replacer { | ||
| if len(input) > 1 { | ||
| input = strings.TrimSuffix(input, "/") | ||
| input = utils.TrimRight(input, '/') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check utils.TrimRight implementation/behavior
echo "=== Checking utils.TrimRight implementation ==="
rg -n -A 10 'func TrimRight' --type=go | head -30
# Find tests for captureTokens function
echo -e "\n=== Finding tests for captureTokens ==="
rg -n 'captureTokens|TestCapture' --type=go -g '*_test.go'
# Check the redirect test file to understand test coverage
echo -e "\n=== Checking redirect test file ==="
fd -e go -p '*redirect*test*' | head -5Repository: gofiber/fiber
Length of output: 494
🏁 Script executed:
# Find redirect test file with correct glob pattern
echo "=== Finding redirect test files ==="
fd -e go 'redirect.*test' --type f
# Check imports in redirect.go to understand utils source
echo -e "\n=== Checking imports in redirect.go ==="
head -20 middleware/redirect/redirect.go
# Search for TrimRight in entire codebase (might be in utils v2)
echo -e "\n=== Searching for TrimRight usage/definition ==="
rg -n 'TrimRight' --type=go | head -20
# Check if utils is a dependency
echo -e "\n=== Checking go.mod for utils dependency ==="
grep -n 'gofiber/utils' go.modRepository: gofiber/fiber
Length of output: 1774
🏁 Script executed:
# Check redirect_test.go for captureTokens tests
echo "=== Examining redirect_test.go for captureTokens tests ==="
cat middleware/redirect/redirect_test.goRepository: gofiber/fiber
Length of output: 7381
🏁 Script executed:
# View the captureTokens function implementation
echo "=== Viewing captureTokens function ==="
sed -n '45,60p' middleware/redirect/redirect.go
# Check if utils.TrimRight behavior is documented
echo -e "\n=== Searching for TrimRight usage patterns in path contexts ==="
rg -B 2 -A 2 'utils.TrimRight.*"/"' --type=go | head -40Repository: gofiber/fiber
Length of output: 626
🏁 Script executed:
# Check the git history to see what the previous implementation was
echo "=== Checking git history for this function ==="
git log --oneline -n 20 -- middleware/redirect/redirect.go
# Check if there's any documentation about trailing slash behavior
echo -e "\n=== Searching for documentation about trailing slash behavior ==="
rg -n 'trailing|slash' --type=go middleware/redirect/ -iRepository: gofiber/fiber
Length of output: 276
Confirm the behavioral change from TrimSuffix to TrimRight is intentional and add test coverage.
The change from strings.TrimSuffix(input, "/") to utils.TrimRight(input, '/') alters path normalization behavior:
strings.TrimSuffixremoves exactly one trailing "/"utils.TrimRightremoves all trailing "/" characters
For example, "/path//" changes from "/path/" to "/path". While using utils.TrimRight aligns with the coding guidelines for utils helper usage, the behavioral difference for paths with multiple trailing slashes is not covered by existing tests.
Ensure this normalization behavior is intentional for v3 and consider adding a test case for "/path//" patterns to verify the redirect matching works as expected.
🤖 Prompt for AI Agents
In middleware/redirect/redirect.go around lines 46 to 49, the change from
strings.TrimSuffix(input, "/") to utils.TrimRight(input, '/') changes path
normalization by removing all trailing slashes instead of just one; verify this
behavioral change is intentional for v3 and either revert to TrimSuffix if the
single-slash semantics must be preserved or keep TrimRight but add unit tests
covering inputs like "/path//" (and other multi-trailing-slash cases) to assert
expected redirect matching; update or add test cases in the redirect middleware
test file to codify the chosen behavior and add a short comment in the code
explaining the normalization decision.
| state = utils.ToUpper(state) | ||
| } | ||
| fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, strings.ToUpper(state), srv.String(), scheme.Reset) | ||
| fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, utils.ToUpper(state), srv.String(), scheme.Reset) |
There was a problem hiding this comment.
Redundant utils.ToUpper call on line 151.
The state variable is already uppercased on line 149. The second call to utils.ToUpper(state) on line 151 is unnecessary.
Apply this diff to remove the redundant call:
state = utils.ToUpper(state)
}
- fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, utils.ToUpper(state), srv.String(), scheme.Reset)
+ fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, state, srv.String(), scheme.Reset)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| state = utils.ToUpper(state) | |
| } | |
| fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, strings.ToUpper(state), srv.String(), scheme.Reset) | |
| fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, utils.ToUpper(state), srv.String(), scheme.Reset) | |
| state = utils.ToUpper(state) | |
| } | |
| fmt.Fprintf(out, "%sINFO%s 🥡 %s[ %s ] %s%s\n", scheme.Green, scheme.Reset, stateColor, state, srv.String(), scheme.Reset) |
🤖 Prompt for AI Agents
In services.go around lines 149 to 151, the code redundantly calls utils.ToUpper
on state twice; remove the second utils.ToUpper call in the fmt.Fprintf
invocation and use the already-uppercased state variable instead so the printed
state reuses state (e.g., replace utils.ToUpper(state) with state) to eliminate
the unnecessary function call.
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.
| if len(mimeType) >= 5 && (mimeType[len(mimeType)-5:] == "+json" || mimeType[len(mimeType)-4:] == "+xml") { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Edge case: 4-character strings ending in "+xml" won't be detected.
The condition requires len(mimeType) >= 5, which safely handles the "+json" suffix check (5 chars) but excludes MIME types of exactly 4 characters ending in "+xml". The original strings.HasSuffix(mimeType, "+xml") would have detected these.
In practice, valid MIME types are much longer (e.g., "application/foo+xml"), so this edge case is unlikely to occur. However, it's technically a behavior change.
If you want to preserve the original behavior, apply this diff:
- if len(mimeType) >= 5 && (mimeType[len(mimeType)-5:] == "+json" || mimeType[len(mimeType)-4:] == "+xml") {
+ if (len(mimeType) >= 5 && mimeType[len(mimeType)-5:] == "+json") || (len(mimeType) >= 4 && mimeType[len(mimeType)-4:] == "+xml") {
return true
}…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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: ff190ff | Previous: dd0b041 | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd - B/op |
1 B/op |
0 B/op |
+∞ |
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
Uh oh!
There was an error while loading. Please reload this page.