🧹 chore: delay routing error creation #3683
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 ad-hoc 404 message construction and the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router
participant H as Handler
C->>R: Request(method, path)
alt route and method match
R->>H: Invoke handler
H-->>R: Handler response
R-->>C: 200 (handler response)
else route exists but method mismatch
R-->>C: ErrMethodNotAllowed (405)
else no matching route
R-->>C: ErrNotFound (404)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the routing error handling by delaying the creation of 404 "Not Found" errors and centralizing their construction. This change aims to prevent unnecessary HTML escaping of paths and reduce code duplication within the router's "next" and "nextCustom" methods.
Highlights
- Optimized 404 Error Generation: The creation of the 404 "Not Found" error object is now deferred until it's actually needed, preventing premature and potentially redundant HTML escaping of the request path.
- Consolidated Error Construction: A new helper function, "cannotRouteError", has been introduced to centralize the logic for generating 404 errors, thereby eliminating duplicate code across different routing paths.
- Streamlined Router Logic: The "next" and "nextCustom" functions within the router have been updated to utilize a local "notFound" closure, which leverages the new "cannotRouteError" function, simplifying error return statements.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request effectively centralizes the creation of 404 errors into a new cannotRouteError helper function and delays its creation to avoid unnecessary work in the routing logic. This is a solid improvement for both code clarity and performance. My review includes a suggestion to further optimize the implementation by removing the intermediate notFound closure in the next and nextCustom functions to avoid allocations in these performance-critical paths.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3683 +/- ##
=======================================
Coverage ? 91.98%
=======================================
Files ? 114
Lines ? 11484
Branches ? 0
=======================================
Hits ? 10564
Misses ? 661
Partials ? 259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This pull request optimizes error handling in the router by deferring 404 error creation until it's actually needed. The changes centralize 404 error construction to eliminate code duplication and avoid unnecessary HTML escaping operations when the error might not be used.
Key changes:
- Introduced a shared
cannotRouteErrorfunction to centralize 404 error creation - Replaced immediate error creation with lazy evaluation using closure functions
- Removed duplicate error handling code between
nextandnextCustommethods
![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 (3)
router.go (3)
161-166: Avoid per-request closure allocation; inline lazy 404 creationThe notFound closure is instantiated on every request, even when not used (e.g., 405 cases). Inline the call to cannotRouteError at the return sites to keep the “delay” semantics without closure overhead and capture.
Apply this diff:
- // Moved from app.handler because middleware may break the route chain - notFound := func() error { - return cannotRouteError(c.Method(), c.getPathOriginal()) - } if c.matched { - return false, notFound() + return false, cannotRouteError(c.Method(), c.getPathOriginal()) }
208-211: Inline final 404 creation to remove dependency on local closureConsistent with removing the closure above, inline the 404 error creation here as well.
- return false, notFound() + return false, cannotRouteError(c.Method(), c.getPathOriginal())
306-309: Inline the final 404 in nextCustom for consistency and performanceReplace the last notFound() call with direct construction.
- return false, notFound() + return false, cannotRouteError(c.Method(), c.getPathOriginal())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
router.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
router.go (2)
app.go (1)
NewError(894-903)constants.go (2)
StatusNotFound(77-77)ErrMethodNotAllowed(123-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: repeated
🔇 Additional comments (2)
router.go (2)
109-112: Consolidated 404 error construction with proper HTML-escaping — good changeCentralizing 404 creation via cannotRouteError improves consistency, removes duplication, and ensures HTML-escaping is applied only when the error is actually produced.
259-264: Inline error construction in router.go to avoid closure allocation
CustomCtxalready exposes bothgetPathOriginal()andMethod()via the embeddedCtx, so it’s safe to remove the extra closure.• Remove the
notFoundclosure (and its comment) around line 259 inrouter.go.
• Inline thecannotRouteErrorcall directly in thereturn.Apply this diff:
--- a/router.go +++ b/router.go @@ -259,8 +259,6 @@ - // Moved from app.handler because middleware may break the route chain - notFound := func() error { - return cannotRouteError(c.Method(), c.getPathOriginal()) - } if c.getMatched() { - return false, notFound() + return false, cannotRouteError(c.Method(), c.getPathOriginal()) }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
utils/escape_path.go (3)
12-27: Confirm encoding semantics: spaces become '+' and '%' is re-encodedEscapePath uses fasthttp.AppendQuotedArg per segment, which applies query-arg style escaping: spaces are encoded as '+' and any literal '%' is encoded to '%25'. For path segments per RFC 3986, spaces are typically encoded as '%20' and existing percent-encodings are usually preserved. Since this is used for error message display only, this might be acceptable, but please confirm this behavior is intentional to avoid surprises if callers reuse it for URL construction.
If the '+' behavior is intended, consider clarifying it in the doc comment to prevent misuse.
Apply this doc update to set expectations:
-// EscapePath returns a URL-escaped path while preserving slashes. -// It pre-allocates extra capacity to minimize allocations. +// EscapePath returns a URL-escaped path while preserving slashes. +// Each segment is escaped via fasthttp.AppendQuotedArg: +// - spaces become '+' +// - literal '%' becomes '%25' (existing percent-encodings are re-encoded) +// This helper is intended for safe display (e.g., error messages), not for generating canonical URLs. +// It pre-allocates extra capacity to minimize allocations. func EscapePath(path string) string {
16-16: Avoid potential reallocations in worst-case (non-ASCII-heavy inputs)Preallocating len+len/2 may still trigger reallocations when most bytes need escaping (e.g., non-ASCII, which grow to ~3x). Consider reserving up to 3x to minimize reallocations in worst cases. This is a micro-optimization and optional.
- buf := make([]byte, 0, len(b)+len(b)/2) + // In the worst case, every byte expands to 3 bytes ("%XX"). + // Pre-allocate up to 3x to reduce reallocations on non-ASCII-heavy inputs. + buf := make([]byte, 0, len(b)*3)
14-17: Optional guard for empty path inputsIf an empty path ever reaches this function, it currently returns an empty string. For better UX in error messages, returning "/" might be preferable. Only apply if consistent with upstream behavior of getPathOriginal.
func EscapePath(path string) string { + if path == "" { + return "/" + } b := gutils.UnsafeBytes(path)utils/escape_path_test.go (1)
5-17: Optional: add trailing-slash and multi-slash casesSince EscapePath preserves slashes and encodes segments only, it’s useful to lock that in with tests like "/users/" and "///". Not required for this PR but helps prevent regressions.
Here are example additions you could append to cases:
"/users/": "/users/", "///": "///",router.go (1)
204-208: Method-not-allowed path: ensure Allow header uniqueness (optional)When multiple routes across methods match, Allow may accumulate duplicates. Rare in practice due to deduping and route structure, but consider de-duplicating before returning 405 for spec cleanliness.
You could track seen methods in a small bool array keyed by index to avoid duplicates before appending.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
router.go(13 hunks)router_test.go(1 hunks)utils/escape_path.go(1 hunks)utils/escape_path_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
utils/escape_path_test.go (1)
utils/escape_path.go (1)
EscapePath(14-27)
router.go (1)
utils/escape_path.go (1)
EscapePath(14-27)
🪛 GitHub Check: lint
utils/escape_path_test.go
[failure] 3-3:
should only use grouped 'import' declarations (grouper)
🪛 GitHub Actions: golangci-lint
utils/escape_path_test.go
[error] 3-3: should only use grouped 'import' declarations (grouper)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (12)
router_test.go (2)
415-417: LGTM: 404 body now percent-encodes the injected contentAssertion updated to expect a percent-encoded path in the 404 body, matching the new centralized escaping. This closes the HTML-escaping gap and is safer for plain-text responses.
419-423: LGTM: cannotRouteError escaping is coveredGreat addition. This directly validates that cannotRouteError uses EscapePath for percent-encoding.
router.go (10)
13-16: LGTM: import split clarifies responsibilitiesUsing fiber/v3/utils for EscapePath and aliasing github.com/gofiber/utils/v2 as gutils avoids naming collisions and improves readability.
109-111: Centralized 404 creation improves consistency and reduces duplicationcannotRouteError standardizes NotFound construction and defers escaping to utils.EscapePath. This avoids scattered HTML-escaping and makes the path handling uniform.
137-139: LGTM: zero-copy string conversion during matchingUsing gutils.UnsafeString on detectionPath and path here avoids allocations during hot path matching.
161-164: LGTM: return standardized 404 when a route matched earlier but yielded no handler chainReturning cannotRouteError here keeps behavior consistent with requestHandler’s default error handling.
193-194: LGTM: consistent matching in alternate-method scanThe same zero-copy matching logic is applied when scanning other methods, maintaining performance characteristics.
351-357: LGTM: gutils for case/trailing-slash normalizationSwitch to gutils.ToLower/TrimRight aligns with the rest of the refactor and reduces bespoke utility usage.
396-401: LGTM: normalizePath refactor mirrors register logicNormalization is consistent across register/remove flows, reducing edge-case drift.
446-447: LGTM: uppercase methods via gutils.ToUpperKeeps method normalization consistent and avoids ad-hoc case handling.
497-501: LGTM: registration normalization with gutilsUsing gutils.ToLower/TrimRight in register mirrors normalizePath, ensuring pairwise consistency.
509-510: LGTM: method normalization at registrationUppercasing here prevents surprises when mixing casing in API usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
helpers.go (2)
141-157: Clarify escaping semantics (+ for spaces) and verify double-encoding is desiredescapePath uses fasthttp.AppendQuotedArg per segment, which encodes spaces as '+' and percent-encodes other bytes. For paths, '+' isn’t the standard representation of spaces (usually %20), but if the intent is “display-only” for 404 messages and to avoid HTML escaping, this is fine. However, please clarify this in the docstring and confirm that double-encoding existing '%' sequences (e.g., "/a%2Fb" -> "/a%252Fb") is intended.
Apply this diff to improve the comment:
-// escapePath returns a URL-escaped path while preserving slashes. -// It pre-allocates extra capacity to minimize allocations. +// escapePath returns a URL-escaped path while preserving '/' separators. +// Note: spaces are encoded as '+' (fasthttp.AppendQuotedArg semantics), and all +// non-ASCII or reserved bytes are percent-encoded. Existing '%' bytes will be +// encoded (i.e., already-escaped sequences are not decoded or preserved). +// It pre-allocates extra capacity to minimize allocations.
141-157: Optional: Add a fast path to avoid allocation when no escaping is neededRight now we always allocate and re-encode even for safe ASCII paths like "/foo/bar". Since this runs on 404s, a trivial pre-check could allow returning the original string unchanged and avoid one allocation/copy in the common case.
If desired, I can propose a minimal “needs-escape” pre-scan consistent with fasthttp’s rules (ALPHA / DIGIT / '-' / '.' / '_' / '~' and '/').
helpers_test.go (1)
484-498: Benchmark representative inputs (+, UTF-8, pre-escaped) to catch regressionsAdding a couple more samples makes the benchmark more representative and will flag regressions in cases we explicitly support in tests.
func Benchmark_EscapePath(b *testing.B) { samples := []string{ "/foo/bar", "/does/not/exist<script>alert('foo');</script>", + "/foo+bar", + "/créer", + "/a%2Fb", } for _, sample := range samples { b.Run(sample, func(b *testing.B) { b.ReportAllocs() for b.Loop() { _ = escapePath(sample) } }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
helpers.go(1 hunks)helpers_test.go(1 hunks)router.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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). (2)
- GitHub Check: Compare
- GitHub Check: repeated
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
router_test.go (1)
914-914: Good addition: allocation reporting for the NotFound benchmarkAdding b.ReportAllocs() here is useful to observe heap activity for 404 paths.
Consider also resetting the timer to exclude setup costs from benchmark timing.
func Benchmark_Router_NotFound(b *testing.B) { - b.ReportAllocs() + b.ReportAllocs() + b.ResetTimer()router.go (1)
300-303: CustomCtx 405 vs 404 split is cleanMethod-not-allowed returns the shared sentinel error; not-found goes through the sanitizer. Looks good.
There’s test coverage for DefaultCtx 404 escaping. Consider adding a parallel CustomCtx test asserting the escaped message to guard against regressions in the custom path. I can draft it if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
router.go(5 hunks)router_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
router.go (2)
app.go (1)
NewError(894-903)constants.go (2)
StatusNotFound(77-77)ErrMethodNotAllowed(123-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
router.go (4)
162-163: 404 path now created lazily with centralized helperReturning cannotRouteError only when needed avoids unnecessary HTML escaping on hot paths. Nice cleanup.
205-206: 405 handling remains explicit and fastEarly return of ErrMethodNotAllowed is clear and avoids formatting/escaping work. Header population happens just above this, as before.
207-208: Final not-found fallback uses centralized helperConsistent with the earlier branch; keeps all 404 strings sanitized and uniform.
257-258: CustomCtx path: same 404 behavior via helperParity with the DefaultCtx path is preserved; escaping is handled identically.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
router.go (1)
160-203: Avoid mutating ctx.indexRoute on 405 path; (optional) consider advertising HEAD when GET is allowedShort: mutating c.indexRoute/c.setIndexRoute while building an Allow header for a 405 is unnecessary and pollutes ctx state. Remove that mutation in both next() and nextCustom(). Optionally, you can append HEAD when GET is present (this is a behavior change — update tests if you adopt it).
Files to change:
- router.go — DefaultCtx next() (removes c.indexRoute assignment while scanning other methods; optionally add HEAD) — currently c.indexRoute = indexRoute (router.go:197)
- router.go — nextCustom() (same change for CustomCtx) — currently c.setIndexRoute(indexRoute) (router.go:292)
Apply the following (showing both places). The HEAD-related code is optional; keep it out if you want to avoid changing existing Allow header behavior and test expectations.
@@ - exists := false - methods := app.config.RequestMethods + exists := false + methods := app.config.RequestMethods + // Track GET/HEAD to optionally include HEAD when GET is allowed. + foundGet, foundHead := false, false @@ - if route.match(utils.UnsafeString(c.detectionPath), utils.UnsafeString(c.path), &c.values) { - // We matched - exists = true - // Add method to Allow header - c.Append(HeaderAllow, methods[i]) - // Break stack loop - break - } + if route.match(utils.UnsafeString(c.detectionPath), utils.UnsafeString(c.path), &c.values) { + // We matched + exists = true + m := methods[i] + if m == "GET" { + foundGet = true + } + if m == "HEAD" { + foundHead = true + } + // Add method to Allow header + c.Append(HeaderAllow, m) + // Break stack loop + break + } @@ - c.indexRoute = indexRoute + // Do not persist indexRoute while only collecting Allow header values for a 405. } - if exists { - return false, ErrMethodNotAllowed - } + if exists { + // Optional: advertise HEAD when GET is allowed (common server behavior). + if foundGet && !foundHead { + c.Append(HeaderAllow, "HEAD") + } + return false, ErrMethodNotAllowed + } return false, ErrNotFound }And mirror the same edits in nextCustom():
@@ - exists := false - methods := app.config.RequestMethods + exists := false + methods := app.config.RequestMethods + foundGet, foundHead := false, false @@ - if route.match(c.getDetectionPath(), c.Path(), c.getValues()) { - // We matched - exists = true - // Add method to Allow header - c.Append(HeaderAllow, methods[i]) - // Break stack loop - break - } + if route.match(c.getDetectionPath(), c.Path(), c.getValues()) { + // We matched + exists = true + m := methods[i] + if m == "GET" { + foundGet = true + } + if m == "HEAD" { + foundHead = true + } + // Add method to Allow header + c.Append(HeaderAllow, m) + // Break stack loop + break + } @@ - c.setIndexRoute(indexRoute) + // Do not persist indexRoute while only collecting Allow header values for a 405. } - if exists { - return false, ErrMethodNotAllowed - } + if exists { + if foundGet && !foundHead { + c.Append(HeaderAllow, "HEAD") + } + return false, ErrMethodNotAllowed + } return false, ErrNotFound }Tests to consider:
- Changing Allow to auto-include HEAD will alter existing assertions (e.g. app_test.go Test_App_MethodNotAllowed expects "POST, OPTIONS" or "GET, POST, OPTIONS" depending on registration). If you adopt HEAD auto-inclusion, update those expectations.
- To find all places asserting Allow headers: run a repo search such as:
rg -n -A3 -B3 'Allow' --type goRecommendation:
- I suggest applying the index mutation removal in both next() and nextCustom() now (safe, reduces ctx state pollution).
- Keep the HEAD auto-addition as an opt-in change — only enable if you want to change public behavior and update tests accordingly.
🧹 Nitpick comments (1)
router.go (1)
255-298: Mirror the 405 refinements in nextCustom; consider factoring shared logic
- Same two tweaks as in next: don’t mutate c.setIndexRoute on an error path; optionally include HEAD in Allow when GET is allowed.
- Consider extracting the “scan other methods and build Allow” logic into a small helper to keep next and nextCustom from drifting.
Apply this diff to align behavior and remove the unnecessary state mutation:
@@ - exists := false - methods := app.config.RequestMethods + exists := false + methods := app.config.RequestMethods + // Track GET/HEAD to optionally include HEAD when GET is allowed. + foundGet, foundHead := false, false @@ - if route.match(c.getDetectionPath(), c.Path(), c.getValues()) { + if route.match(c.getDetectionPath(), c.Path(), c.getValues()) { // We matched exists = true // Add method to Allow header - c.Append(HeaderAllow, methods[i]) + m := methods[i] + if m == "GET" { + foundGet = true + } + if m == "HEAD" { + foundHead = true + } + c.Append(HeaderAllow, m) // Break stack loop break } } - c.setIndexRoute(indexRoute) + // No need to persist indexRoute when returning an error afterwards. } - if exists { - return false, ErrMethodNotAllowed - } + if exists { + // Optionally advertise HEAD when GET is allowed (common server behavior). + if foundGet && !foundHead { + c.Append(HeaderAllow, "HEAD") + } + return false, ErrMethodNotAllowed + } return false, ErrNotFoundIf you prefer a helper to avoid duplication:
- Extract a method like app.buildAllowHeaderAndDecide(cCtx, methodInt, treeHash) that returns (exists bool).
- Use an interface satisfied by both DefaultCtx and CustomCtx for the minimal calls used here (Append, Path getters, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
router.go(4 hunks)router_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
router.go (1)
constants.go (2)
ErrNotFound(122-122)ErrMethodNotAllowed(123-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (2)
router.go (2)
156-158: LGTM: using sentinel ErrNotFound avoids per-request formatting and allocationsReturning ErrNotFound here keeps the hot path lean and defers response construction to the centralized error handler. Matches the PR goal of delaying 404 creation.
251-253: LGTM: consistent 404 sentinel usage in CustomCtx pathMirrors the DefaultCtx code path and preserves behavior while avoiding unnecessary string work.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/guide/routing.md (2)
42-47: Nit: Capitalize “Express.js”.Minor editorial fix to keep brand naming consistent and professional.
-As with the expressJs framework, the order of the route declaration plays a role. +As with the Express.js framework, the order of the route declaration plays a role.
229-232: Clarify escaping note for datetime constraints (optional).Consider explicitly listing that 404s will use the uniform “Not Found” body when constraints fail to match, which ties back to the new behavior.
middleware/static/static_test.go (1)
965-972: Fix mismatched assertion message in else-branch (minor).The assertion checks for “Are you a hacker?” but the failure message still mentions “Not Found”. Update the message for clarity.
- // If we got a 404, we expect the "Not Found" message because that's how fiber handles NotFound by default. + // If we got a 404, we expect the "Not Found" message because that's how Fiber handles NotFound by default. if status == 404 { require.Contains(t, string(body), "Not Found", "Blocked traversal should have a \"Not Found\" message for %s", path) } else { - require.Contains(t, string(body), "Are you a hacker?", - "Blocked traversal should have a \"Not Found\" message for %s", path) + require.Contains(t, string(body), "Are you a hacker?", + "Blocked traversal should have an \"Are you a hacker?\" message for %s", path) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/core_test.go(3 hunks)docs/guide/routing.md(5 hunks)middleware/keyauth/keyauth_test.go(2 hunks)middleware/logger/logger_test.go(3 hunks)middleware/static/static_test.go(4 hunks)router_test.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- router_test.go
🧰 Additional context used
📓 Path-based instructions (1)
docs/**
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/guide/routing.md
🧠 Learnings (4)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/keyauth_test.gomiddleware/logger/logger_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
middleware/static/static_test.goclient/core_test.go
📚 Learning: 2024-12-15T19:56:45.935Z
Learnt from: ckoch786
PR: gofiber/fiber#3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like `RemoveRoute` and `RemoveRouteByName` are documented in `docs/api/app.md`, so it's unnecessary to duplicate them in `docs/whats_new.md`.
Applied to files:
docs/guide/routing.md
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/logger/logger_test.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). (5)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (12)
docs/guide/routing.md (2)
183-184: Docs correctly reflect the new 404 body (“Not Found”).All updated examples now align with the centralized 404 handling. Good consistency across constraints and regex examples.
Also applies to: 197-201, 216-221, 246-247
1-356: No "Cannot GET" occurrences found — resolvingSearched docs/** (case-sensitive) and the repo (case-insensitive) for "Cannot GET" using ripgrep; no matches were found. No further action required.
middleware/logger/logger_test.go (3)
24-28: Import reflow is fine.No functional impact; grouping is clean.
468-474: Update to “Not Found” matches new router behavior.Expected line content reflects centralized 404 handling. Looks good.
1039-1045: Colorized “Not Found” expectation aligns with new default message.Assertion matches the ForceColors/DisableColors test setup.
client/core_test.go (2)
13-15: Import order tweak is fine.No runtime effect.
170-173: “Not Found” expected body is correct.These subtests now align with the standardized 404 response body.
Also applies to: 186-190
middleware/keyauth/keyauth_test.go (2)
12-15: Import reordering looks fine.No behavioral change.
122-126: Param-source empty key case updated to “Not Found” is correct.When the token is part of the path and missing, the router never reaches the handler; 404 body is now standardized.
middleware/static/static_test.go (3)
60-63: 404 body assertion updated to “Not Found” is correct.This matches the framework’s standardized 404 body.
179-182: “Not Found” expectation after deletion is correct.Consistent with updated NotFound handler behavior.
774-809: Address flakiness/timeouts in compression subtests — verification incompleteTried to verify by running the test in the sandbox. rg found the test (middleware/static/static_test.go:774), but go test failed here due to module/environment limits ("go: cannot find main module" + telemetry sidecar warning). I couldn't run the tests to confirm flakiness or timeout improvements.
What I inspected
- File: middleware/static/static_test.go — Test_Static_Compress (lines ~774-809) — subtests call t.Parallel() and use the same app/FS instance.
Suggested fixes (choose one)
Option A — run subtests sequentially (simplest)
for _, algo := range algorithms { t.Run(algo+"_compression", func(t *testing.T) { - t.Parallel() // request non-compressible file (less than 200 bytes), Content Length will remain the sameOption B — isolate state per subtest (each subtest gets its own app/FS)
for _, algo := range algorithms { t.Run(algo+"_compression", func(t *testing.T) { - t.Parallel() - // request non-compressible file (less than 200 bytes), Content Length will remain the same + // Dedicated app per subtest to avoid concurrent compression of the same files. + app := fiber.New() + app.Get("/*", New(dir, Config{ Compress: true })) + + // request non-compressible file (less than 200 bytes), Content Length will remain the sameAlso consider increasing testConfig.Timeout for compression tests if CI remains near the 10s limit.
Please verify locally (run from the repository root where go.mod is present):
rg -n '^func Test_Static_Compress' middleware/static/static_test.go go test ./middleware/static -run Test_Static_Compress -count=5 -timeout=120s -vI cannot complete runtime verification in the sandbox due to the missing module; please run the commands above and report back if you want me to re-evaluate based on results.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
hooks_test.go(2 hunks)middleware/keyauth/keyauth_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/keyauth/keyauth_test.gohooks_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/keyauth/keyauth_test.gohooks_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
middleware/keyauth/keyauth_test.gohooks_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
hooks_test.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). (5)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (4)
hooks_test.go (1)
12-14: LGTM: import placement for log aligns with usage in this testImport of github.com/gofiber/fiber/v3/log is correctly grouped and used later for capturing hook error logs.
middleware/keyauth/keyauth_test.go (3)
15-17: LGTM: import block reordering is fineThe reordering/groups are consistent; no functional impact.
169-173: No-op whitespace changeNo issues.
194-197: Approve: update 404 expectation to "Not Found" for missing param tokenMatches the centralized 404 behavior and the router change away from "Cannot GET /…".
- middleware/keyauth/keyauth_test.go — lines 194–197 (expect 404 and "Not Found")
- Repo search for "Cannot GET" returned no matches.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
hooks_test.go (1)
320-331: Avoid t usage in goroutine; assert in main goroutine for prefork case tooThe same testing pattern applies here for prefork. This also helps standardize on testify/require in the main goroutine.
- go func() { - time.Sleep(1000 * time.Millisecond) - assert.NoError(t, app.Shutdown()) - }() + done := make(chan error, 1) + go func() { + time.Sleep(1 * time.Second) + done <- app.Shutdown() + }() app.Hooks().OnFork(func(pid int) error { require.Equal(t, 1, pid) return nil }) - require.NoError(t, app.prefork(":0", nil, ListenConfig{DisableStartupMessage: true, EnablePrefork: true})) + require.NoError(t, app.prefork(":0", nil, ListenConfig{DisableStartupMessage: true, EnablePrefork: true})) + require.NoError(t, <-done)
♻️ Duplicate comments (2)
hooks_test.go (2)
281-286: Good move to :0; avoid using testing.T from a goroutine and prefer require in main goroutineNice fix switching to an ephemeral port (as previously suggested). To avoid racy writes to t from goroutines and to fail fast, capture Shutdown errors in a channel and assert in the main goroutine.
Apply this minimal refactor:
- go func() { - time.Sleep(1000 * time.Millisecond) - assert.NoError(t, app.Shutdown()) - }() - require.NoError(t, app.Listen(":0")) + done := make(chan error, 1) + go func() { + time.Sleep(1 * time.Second) + done <- app.Shutdown() + }() + require.NoError(t, app.Listen(":0")) + require.NoError(t, <-done)
304-311: Use channel handshake instead of asserting from a goroutine; keep :0Same rationale as above: avoid calling assert/require on t in a goroutine; assert in the main goroutine and keep the ephemeral port.
- go func() { - time.Sleep(1000 * time.Millisecond) - assert.NoError(t, app.Shutdown()) - }() - - require.NoError(t, app.Listen(":0", ListenConfig{DisableStartupMessage: true, EnablePrefork: true})) + done := make(chan error, 1) + go func() { + time.Sleep(1 * time.Second) + done <- app.Shutdown() + }() + require.NoError(t, app.Listen(":0", ListenConfig{DisableStartupMessage: true, EnablePrefork: true})) + require.NoError(t, <-done) require.Equal(t, "ready", buf.String())
🧹 Nitpick comments (1)
hooks_test.go (1)
12-14: LGTM on importing fiber/log; minor test hygiene: restore logger output after redirectionThe added log import is appropriate for capturing output in tests. However, tests that call log.SetOutput risk leaking global state across the suite. Consider restoring the original writer via t.Cleanup to avoid cross-test interference.
Example pattern:
// after creating your buffer // import "os" log.SetOutput(&buf) t.Cleanup(func() { log.SetOutput(os.Stderr) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
hooks_test.go(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
hooks_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
hooks_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
hooks_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
hooks_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
hooks_test.go
🧬 Code Graph Analysis (1)
hooks_test.go (2)
listen.go (1)
ListenConfig(42-125)hooks.go (1)
ListenData(38-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, macos-13)
🔇 Additional comments (1)
hooks_test.go (1)
422-424: LGTM: using Port "0" in ListenData for hook error pathSwitching to port "0" removes privilege/port-collision concerns in this unit test while preserving semantics of the error-path check.
gaby
changed the title
Summary
Testing
make audit(fails: lock by value in generated middleware files)make generatemake betteralignmake modernizemake formatmake test(fails: Test_Static_Compress/zstd_compression, Test_Static_Compress/br_compression i/o timeout)https://chatgpt.com/codex/tasks/task_e_689ca563225c8326a9677a7887cfdb07