🔥 feat: Add support for embedded Koa-Style Req and Res structs #3533
Conversation
WalkthroughThis change refactors the core HTTP context ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultCtx
participant DefaultReq
participant DefaultRes
participant fasthttp
Client->>DefaultCtx: HTTP request received
DefaultCtx->>DefaultReq: Delegate request-specific methods (headers, body, params)
DefaultCtx->>DefaultRes: Delegate response-specific methods (headers, cookies, send)
DefaultReq->>fasthttp: Access request data
DefaultRes->>fasthttp: Manipulate response data
DefaultCtx->>fasthttp: Reset or release context as needed
DefaultRes->>Client: Send HTTP response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150+ minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the linked issue #3347. Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @grivera64, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant refactoring to the core context handling by embedding DefaultReq and DefaultRes structs directly into the Ctx interface. The primary goal is to enhance developer experience and code maintainability by allowing request and response-specific methods to be defined in their respective files, which are then automatically promoted to the main context interface. This change reduces boilerplate and simplifies future API extensions.
Highlights
- Core Refactoring: The
DefaultReqandDefaultResstructs are now directly embedded into theDefaultCtxstruct, simplifying theCtxstructure and promoting a more cohesive design. This removes the need for explicitreqandrespointer fields withinDefaultCtx. - Method Relocation: Numerous request-related methods (e.g.,
Accepts,Body,Cookies,FormFile,Get,Host,IP,Params,Query,Range,Subdomains,XHR,IsProxyTrusted,IsFromLocal) and response-related methods (e.g.,Append,Attachment,ClearCookie,Cookie,Download,Format,GetRespHeader,GetRespHeaders,JSON,CBOR,JSONP,XML,Links,Location,Render,Send,SendFile,SendStatus,SendString,SendStream,SendStreamWriter,Set,Status,Type,Vary,Write,Writef,WriteString,Drop,End) have been moved fromctx.goto their respectivereq.goandres.gofiles. - Automated Interface Generation: The
go:generate ifacemakercommand inctx.gohas been updated to includereq.goandres.goas source files, along with the--promotedflag. This ensures that methods defined on the embeddedDefaultReqandDefaultResstructs are automatically promoted and included in theCtxinterface, maintaining a consistent public API. - Improved Developer Experience: This refactoring centralizes the implementation of request and response functionalities within their dedicated files. This makes it significantly easier to add or modify methods without needing to duplicate code across
ctx.go, thereby enhancing maintainability and streamlining the developer workflow.
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 is currently in preview and 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 to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the DefaultReq and DefaultRes structs into view-like structs onto the Ctx interface, aiming to improve DX when adding new Ctx methods. The changes involve modifying the DefaultCtx struct to embed DefaultReq and DefaultRes, updating the ifacemaker command, and adjusting method calls to use the embedded structs. The code review identified areas where documentation and comments should be updated to reflect the changes, and where potential side effects of modifying the request context should be considered.
…d-koa-style-req-res
Changes expected output from `<h1><no value></h1>` to `<h1></h1>`
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3533 +/- ##
==========================================
+ Coverage 91.12% 91.89% +0.77%
==========================================
Files 113 113
Lines 11388 11348 -40
==========================================
+ Hits 10377 10428 +51
+ Misses 752 662 -90
+ Partials 259 258 -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:
|
|
Warning I updated the |
|
@grivera64 Since res.go and req.go are generated, should we remove those from the coverage reports? |
@gaby Do you mean to only check coverage for the There is an exception for overridden methods (like |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
req_interface_gen.go (1)
29-30: Internal methods should not be part of the public interface.Similar to the
Resinterface, theReqinterface exposes internal implementation methods liketryDecodeBodyInOrder(),release(),extractIPsFromHeader(),extractIPFromHeader(), andgetBody(). These should be kept unexported.Also applies to: 50-51, 90-97, 174-175
res.go (1)
117-124: This method doesn't belong in the response struct.Reading cookies from the request is a request operation, not a response operation. This method should be in
DefaultReqinstead ofDefaultRes. TheCookiemethod (singular) for setting cookies makes sense here, butCookies(plural) for reading doesn't.
🧹 Nitpick comments (2)
res_interface_gen.go (1)
64-65: Consider keeping internal methods unexported in the interface.The interface exposes internal methods like
release(),setCanonical(), andrenderExtensions()which appear to be implementation details. This could lead to API stability issues if these methods need to change.Consider using a separate internal interface or keeping these methods unexported to maintain a cleaner public API surface.
Also applies to: 117-118, 138-139
ctx_interface_gen.go (1)
124-145: Large interface with exposed internals needs refinement.The
Ctxinterface has become quite large and exposes many internal methods that should remain unexported:
- Configuration methods:
configDependentPaths()- Internal state accessors:
getMethodInt(),setMethodInt(),getIndexRoute(), etc.- Implementation details:
tryDecodeBodyInOrder(),setCanonical(),keepOriginalPath()Consider:
- Using composition with smaller, focused interfaces
- Keeping internal methods unexported
- Using an internal interface for framework-specific operations
Also applies to: 229-229, 247-247, 267-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Makefile(1 hunks)app_test.go(1 hunks)ctx.go(9 hunks)ctx_interface.go(1 hunks)ctx_interface_gen.go(4 hunks)ctx_test.go(1 hunks)req.go(1 hunks)req_interface_gen.go(1 hunks)res.go(1 hunks)res_interface_gen.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ctx.go
[warning] 341-341: ctx.go#L341
Added line #L341 was not covered by tests
[warning] 603-604: ctx.go#L603-L604
Added lines #L603 - L604 were not covered by tests
[warning] 634-635: ctx.go#L634-L635
Added lines #L634 - L635 were not covered by tests
req.go
[warning] 48-48: req.go#L48
Added line #L48 was not covered by tests
[warning] 130-131: req.go#L130-L131
Added lines #L130 - L131 were not covered by tests
[warning] 316-316: req.go#L316
Added line #L316 was not covered by tests
[warning] 415-415: req.go#L415
Added line #L415 was not covered by tests
[warning] 504-504: req.go#L504
Added line #L504 was not covered by tests
[warning] 517-517: req.go#L517
Added line #L517 was not covered by tests
[warning] 718-719: req.go#L718-L719
Added lines #L718 - L719 were not covered by tests
[warning] 796-797: req.go#L796-L797
Added lines #L796 - L797 were not covered by tests
res.go
[warning] 122-123: res.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 155-157: res.go#L155-L157
Added lines #L155 - L157 were not covered by tests
[warning] 188-188: res.go#L188
Added line #L188 was not covered by tests
[warning] 278-279: res.go#L278-L279
Added lines #L278 - L279 were not covered by tests
[warning] 289-290: res.go#L289-L290
Added lines #L289 - L290 were not covered by tests
[warning] 415-416: res.go#L415-L416
Added lines #L415 - L416 were not covered by tests
[warning] 421-422: res.go#L421-L422
Added lines #L421 - L422 were not covered by tests
[warning] 477-478: res.go#L477-L478
Added lines #L477 - L478 were not covered by tests
[warning] 590-591: res.go#L590-L591
Added lines #L590 - L591 were not covered by tests
[warning] 593-594: res.go#L593-L594
Added lines #L593 - L594 were not covered by tests
[warning] 641-641: res.go#L641
Added line #L641 was not covered by tests
[warning] 748-749: res.go#L748-L749
Added lines #L748 - L749 were not covered by tests
[warning] 758-759: res.go#L758-L759
Added lines #L758 - L759 were not covered by tests
🔇 Additional comments (8)
Makefile (1)
64-64: Verify the ifacemaker tool commit hash and compatibility.The commit hash has been updated to support the new interface generation requirements. Please verify that this specific commit exists and is compatible with the current codebase requirements.
#!/bin/bash # Description: Verify the ifacemaker commit exists and check for any breaking changes # Check if the new commit hash exists in the repository echo "Checking if commit c0371201a75b1c79e69cda79a6f90b8ae5799b37 exists..." git ls-remote https://github.com/vburenin/ifacemaker.git c0371201a75b1c79e69cda79a6f90b8ae5799b37 # Get commit information echo "Getting commit details..." curl -s https://api.github.com/repos/vburenin/ifacemaker/commits/c0371201a75b1c79e69cda79a6f90b8ae5799b37 | jq '.commit.message, .commit.author.date' # Check if there are any breaking changes between the old and new commits echo "Checking commits between old and new versions..." curl -s "https://api.github.com/repos/vburenin/ifacemaker/compare/975a95966976eeb2d4365a7fb236e274c54da64c...c0371201a75b1c79e69cda79a6f90b8ae5799b37" | jq '.commits[].commit.message'app_test.go (1)
318-322: Verify that removing the govet nolint comment doesn't introduce warnings.The
//nolint:govetcomment was removed from the struct definition. Please ensure this doesn't introduce any govet warnings or errors during the build process.#!/bin/bash # Description: Check if removing the nolint comment introduces govet issues # Run go vet specifically on this test file to check for issues echo "Running go vet on app_test.go..." go vet ./app_test.go # Run go vet on the entire package to check for broader issues echo "Running go vet on the entire package..." go vet . # Check if there are any specific struct alignment or field issues echo "Checking for struct field issues..." go run golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest .ctx_test.go (1)
4062-4062: [web_search]What is the default missing key behavior for Go's html/template package? Does it render missing variables as "<no value>" or empty string?ctx_interface.go (1)
39-40: LGTM! Proper initialization of embedded structs.The direct assignment of context references to the embedded
DefaultReqandDefaultResstructs is the correct approach for this refactoring pattern.ctx.go (4)
49-50: Good architectural choice with struct embedding.The direct embedding of
DefaultReqandDefaultResaligns well with the Koa-style architecture and simplifies the API by promoting methods to the parent type.
407-414: Good use of lazy initialization for Redirect.The lazy initialization pattern for the Redirect object is efficient, only allocating when needed.
336-346: Add test coverage for custom context in RestartRouting.The
RestartRoutingmethod lacks test coverage for the custom context path (line 341). This functionality should be tested to ensure it works correctly with custom context implementations.#!/bin/bash # Description: Check if there are any tests for RestartRouting with custom contexts # Search for RestartRouting tests rg -A 10 "TestCtx.*RestartRouting|Test.*RestartRouting" --type go # Search for tests that use newCtxFunc rg -A 10 "newCtxFunc.*test|test.*newCtxFunc" --type go
634-636: ```shell
#!/bin/bashList all Go test files in the repository
rg --files --glob "*_test.go"
Search for direct tests of the unexported setter
rg "setMethodInt" -g "*_test.go"
Search for tests of the public method that should call setMethodInt
rg "SetMethod" -g "_test.go"
rg "WithMethod" -g "_test.go"</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| func (c *DefaultCtx) renderExtensions(bind any) { | ||
| if bindMap, ok := bind.(Map); ok { | ||
| // Bind view map | ||
| c.viewBindMap.Range(func(key, value any) bool { | ||
| keyValue, ok := key.(string) | ||
| if !ok { | ||
| return true | ||
| } | ||
| if _, ok := bindMap[keyValue]; !ok { | ||
| bindMap[keyValue] = value | ||
| } | ||
| return true | ||
| }) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
Add test coverage and handle type assertion failure.
The renderExtensions method has uncovered lines (603-604) in the type assertion failure path. Additionally, the method silently continues when type assertion fails, which might hide issues.
Consider logging or handling the type assertion failure:
- keyValue, ok := key.(string)
- if !ok {
- return true
- }
+ keyValue, ok := key.(string)
+ if !ok {
+ // Log the issue or handle it appropriately
+ if c.app.config.EnableDebug {
+ log.Printf("renderExtensions: non-string key in viewBindMap: %T", key)
+ }
+ return true
+ }Add test coverage and handle type assertion failure.
The renderExtensions method has uncovered lines (603-604) in the type assertion failure path. Additionally, the method silently continues when type assertion fails, which might hide issues.
Consider logging or handling the type assertion failure:
- keyValue, ok := key.(string)
- if !ok {
- return true
- }
+ keyValue, ok := key.(string)
+ if !ok {
+ // Log the issue or handle it appropriately
+ if c.app.config.EnableDebug {
+ log.Printf("renderExtensions: non-string key in viewBindMap: %T", key)
+ }
+ return true
+ }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 603-604: ctx.go#L603-L604
Added lines #L603 - L604 were not covered by tests
🤖 Prompt for AI Agents
In ctx.go around lines 597 to 610, the renderExtensions method does not handle
the case when the type assertion of bind to Map fails, and this path is not
covered by tests. Add handling for the type assertion failure by logging a
warning or error message to indicate the failure, and add test coverage to
verify this behavior. This will ensure that type assertion failures are visible
and tested.
| func (r *DefaultReq) Port() string { | ||
| tcpaddr, ok := r.RequestCtx().RemoteAddr().(*net.TCPAddr) | ||
| if !ok { | ||
| panic(errors.New("failed to type-assert to *net.TCPAddr")) | ||
| } | ||
| return strconv.Itoa(tcpaddr.Port) | ||
| } |
There was a problem hiding this comment.
Avoid panic - handle non-TCP addresses gracefully
The type assertion failure will panic if the connection is not TCP (e.g., Unix sockets). This should return an empty string or a default value instead of crashing the application.
Apply this diff to handle the error gracefully:
func (r *DefaultReq) Port() string {
tcpaddr, ok := r.RequestCtx().RemoteAddr().(*net.TCPAddr)
if !ok {
- panic(errors.New("failed to type-assert to *net.TCPAddr"))
+ return ""
}
return strconv.Itoa(tcpaddr.Port)
}📝 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.
| func (r *DefaultReq) Port() string { | |
| tcpaddr, ok := r.RequestCtx().RemoteAddr().(*net.TCPAddr) | |
| if !ok { | |
| panic(errors.New("failed to type-assert to *net.TCPAddr")) | |
| } | |
| return strconv.Itoa(tcpaddr.Port) | |
| } | |
| func (r *DefaultReq) Port() string { | |
| tcpaddr, ok := r.RequestCtx().RemoteAddr().(*net.TCPAddr) | |
| if !ok { | |
| return "" | |
| } | |
| return strconv.Itoa(tcpaddr.Port) | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 316-316: req.go#L316
Added line #L316 was not covered by tests
🤖 Prompt for AI Agents
In req.go around lines 313 to 319, the Port() method panics if the RemoteAddr is
not a *net.TCPAddr, which can happen with non-TCP connections like Unix sockets.
Modify the code to check the type assertion and if it fails, return an empty
string instead of panicking, ensuring the application does not crash on non-TCP
addresses.
gaby
changed the title
Summary of ChangesThis pull request significantly refactors the core Highlights
Changelog
Activity
|
…d-koa-style-req-res
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
res_interface_gen.go (1)
129-129: Internal methods should be privateThe
setCanonicalandrelease()methods appear to be internal implementation details that shouldn't be part of the publicResinterface contract.Also applies to: 145-145
res.go (1)
936-950: Improve error handling in End() methodThe End() method should wrap errors to provide better context about where failures occur, as suggested in previous reviews.
🧹 Nitpick comments (1)
res_interface_gen.go (1)
108-108: Consider making renderExtensions privateThe
renderExtensionsmethod appears to be an internal helper method without documentation comments, suggesting it's not intended for public use. Consider whether this should be part of the public interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx_interface_gen.go(3 hunks)ctx_test.go(7 hunks)res.go(1 hunks)res_interface_gen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ctx_test.go
- ctx_interface_gen.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
res_interface_gen.go (7)
Learnt from: ReneWerner87
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.
Learnt from: gaby
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.
Learnt from: gaby
PR: #3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, Context() is being renamed to RequestCtx(), and UserContext() to Context() to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
Learnt from: sixcolors
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.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
res.go (8)
Learnt from: gaby
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.
Learnt from: gaby
PR: #3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, Context() is being renamed to RequestCtx(), and UserContext() to Context() to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
Learnt from: sixcolors
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.
Learnt from: efectn
PR: #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.
Learnt from: ReneWerner87
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.
🧬 Code Graph Analysis (1)
res_interface_gen.go (5)
res.go (3)
Cookie(101-113)ResFmt(116-119)SendFile(23-59)redirect.go (1)
Redirect(62-66)app.go (1)
Map(41-41)router.go (1)
Route(43-62)ctx_interface_gen.go (1)
Ctx(17-401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: repeated
🔇 Additional comments (11)
res_interface_gen.go (1)
1-152: Generated interface looks comprehensiveThe interface definition is well-documented with detailed method comments and covers all necessary response-related functionality. The expansion of methods aligns well with the refactoring objectives.
res.go (10)
121-124: Well-designed struct embeddingThe
DefaultResstruct with genericCtxinterface embedding is well-designed and supports the refactoring objectives by allowing direct access to context methods while maintaining loose coupling.
22-99: Comprehensive SendFile configurationThe
SendFilestruct and supporting functionality provide comprehensive file serving options with proper caching, compression, and byte-range support. The manual config comparison incompareConfigis a good performance optimization.
194-271: Robust cookie handling with validationThe
Cookiemethod implements proper validation usingnet/http.Cookie.Valid()and handles all cookie attributes correctly, including SameSite modes and partitioned cookies. The fallback to ignoring invalid cookies matchesnet/httpbehavior.
132-150: Efficient header appending logicThe
Appendmethod correctly handles header deduplication and formatting. The string checking logic prevents duplicate values while maintaining proper comma separation.
311-353: Well-implemented content negotiationThe
Formatmethod properly implements content negotiation with Accept header handling, default handler support, and appropriate error responses. The stack allocation optimization for the types slice is a nice touch.
586-646: Sophisticated template renderingThe
Rendermethod handles multiple mounted apps correctly and includes fallback to raw template parsing when no engine is configured. The buffer management is efficient.
664-801: Complex but correct file servingThe
SendFileimplementation handles multiple configuration options, caching, path normalization, and status code management correctly. The mutex-protected cache of handlers is well-designed.
875-896: Smart charset handlingThe
shouldIncludeCharsethelper function implements intelligent charset detection for MIME types, properly handling text types and JSON/XML variants.
847-849: Internal method correctly implementedThe
setCanonicalmethod is properly implemented for internal use, though it should ideally be private as noted in the interface review.
923-925: Simple and correct cleanupThe
releasemethod correctly resets the context reference for object pooling.
| // getLocationFromRoute get URL location from route using parameters | ||
| getLocationFromRoute(route Route, params Map) (string, error) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify visibility of getLocationFromRoute method
This method is lowercase (suggesting private) but is part of the public interface. If it's intended to be private, it should be removed from the interface. If public, consider adding proper documentation.
🤖 Prompt for AI Agents
In res_interface_gen.go around lines 101 to 102, the getLocationFromRoute method
is defined with a lowercase first letter, indicating it is private, yet it is
included in a public interface. Decide if this method should be public or
private: if public, rename it to start with an uppercase letter and add proper
documentation; if private, remove it from the public interface to maintain
correct visibility and interface design.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
ctx.go (3)
166-171: Add test coverage for GetHeaders method.The new
GetHeaders()method lacks test coverage. Please add unit tests covering:
- No headers (expect empty map)
- Single header key/value
- Multiple values for the same header key
237-251: Verify path override behavior with request context modification.The path override logic directly modifies the request context which could have side effects if other parts of the application rely on the original path.
459-472: Add test coverage and handle type assertion failure.The
renderExtensionsmethod has uncovered lines in the type assertion failure path. Additionally, the method silently continues when type assertion fails, which might hide issues.Consider logging or handling the type assertion failure:
- keyValue, ok := key.(string) - if !ok { - return true - } + keyValue, ok := key.(string) + if !ok { + // Log the issue or handle it appropriately + if c.app.config.EnableDebug { + log.Printf("renderExtensions: non-string key in viewBindMap: %T", key) + } + return true + }res.go (1)
958-973: Improve error handling to avoid masking connection errors.The End() method should wrap errors to provide better context about where failures occur.
bw := bufio.NewWriter(conn) if err := ctx.Response.Write(bw); err != nil { - return err + return fmt.Errorf("failed to write response: %w", err) } if err := bw.Flush(); err != nil { - return err //nolint:wrapcheck // unnecessary to wrap it + return fmt.Errorf("failed to flush response: %w", err) } - return conn.Close() //nolint:wrapcheck // unnecessary to wrap it + if err := conn.Close(); err != nil { + return fmt.Errorf("failed to close connection: %w", err) + } + return nilreq.go (5)
75-122: Add safeguards for body decompression to prevent resource exhaustion.The
tryDecodeBodyInOrdermethod modifies the request body in-place and lacks size limits, which could cause issues if called multiple times or with large bodies.Consider adding these safeguards:
func (r *DefaultReq) tryDecodeBodyInOrder( originalBody *[]byte, encodings []string, ) ([]byte, uint8, error) { + // Add size limit check + if len(r.Request().Body()) > r.App().config.BodyLimit { + return nil, 0, ErrRequestEntityTooLarge + } + var ( err error body []byte decodesRealized uint8 ) // ... rest of implementation }
129-178: Remove automatic response status setting from Body() method.The Body() method directly modifies the response status code when decompression fails, which violates separation of concerns and could interfere with other handlers.
Apply this diff to let the caller handle errors appropriately:
if err != nil { - switch { - case errors.Is(err, ErrUnsupportedMediaType): - _ = r.c.SendStatus(StatusUnsupportedMediaType) //nolint:errcheck // It is fine to ignore the error - case errors.Is(err, ErrNotImplemented): - _ = r.c.SendStatus(StatusNotImplemented) //nolint:errcheck // It is fine to ignore the error - } return []byte(err.Error()) }
336-342: Handle non-TCP addresses gracefully to avoid panic.The type assertion failure will panic if the connection is not TCP (e.g., Unix sockets). This should return an empty string instead of crashing the application.
Apply this diff to handle the error gracefully:
func (r *DefaultReq) Port() string { tcpaddr, ok := r.RequestCtx().RemoteAddr().(*net.TCPAddr) if !ok { - panic(errors.New("failed to type-assert to *net.TCPAddr")) + return "" } return strconv.Itoa(tcpaddr.Port) }
514-526: Consider relocating generic Locals function.The generic Locals function conceptually belongs to general context operations rather than request-specific operations. Consider moving this to a more appropriate location or the main context file.
728-800: Separate parsing logic from response modification in Range method.The Range method contains complex parsing logic and modifies the response, which violates separation of concerns. The response modification should be handled by the caller.
Consider refactoring to separate parsing from response modification:
func (r *DefaultReq) Range(size int) (Range, error) { var rangeData Range // Parse the range header err := r.parseRangeHeader(size, &rangeData) if err != nil { return rangeData, err } if len(rangeData.Ranges) < 1 { - r.c.Status(StatusRequestedRangeNotSatisfiable) - r.c.Set(HeaderContentRange, "bytes */"+strconv.Itoa(size)) return rangeData, ErrRequestedRangeNotSatisfiable } return rangeData, nil }
🧹 Nitpick comments (2)
res.go (2)
679-824: Comprehensive SendFile implementation with proper caching.The SendFile method provides a feature-rich implementation with:
- Thread-safe handler caching for performance
- Proper path handling across different OS
- Good error handling and status code preservation
- Support for compression, byte ranges, and cache control
Consider adding documentation about the caching behavior for maintainability.
600-665: Complex but necessary template rendering implementation.The Render method handles multiple scenarios including mounted apps, layouts, and fallback rendering. The implementation is correct but complex.
Consider adding detailed documentation about the rendering order and app selection logic for future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ctx.go(6 hunks)req.go(1 hunks)res.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
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`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
ctx.go (17)
Learnt from: gaby
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.
Learnt from: gaby
PR: #3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, Context() is being renamed to RequestCtx(), and UserContext() to Context() to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: ReneWerner87
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.
Learnt from: sixcolors
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.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
Learnt from: ckoch786
PR: #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.
Learnt from: sixcolors
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.
Learnt from: mdelapenya
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.
Learnt from: mdelapenya
PR: #3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Fiber currently supports square bracket notation in form data with 'application/x-www-form-urlencoded' content type, but this feature is not available for multipart form data. The implementation for form-urlencoded is in ctx.go (BodyParser method), while multipart form data handling needs enhancement to support this feature.
Learnt from: sixcolors
PR: #3446
File: docs/middleware/logger.md:44-44
Timestamp: 2025-05-13T00:19:16.407Z
Learning: In documentation files for the Fiber framework, code examples are often partial and don't repeat import statements that were shown in earlier examples, focusing instead on demonstrating specific usage patterns.
Learnt from: ReneWerner87
PR: #3564
File: client/cookiejar.go:128-130
Timestamp: 2025-07-17T11:39:38.265Z
Learning: In the fasthttp library, the Cookie.CopyTo(src *Cookie) method copies the source cookie (parameter) to the receiver cookie, so nc.CopyTo(c) copies cookie c into cookie nc.
Learnt from: sigmundxia
PR: #3151
File: middleware/adaptor/adaptor_test.go:226-230
Timestamp: 2024-09-26T03:08:50.917Z
Learning: In Fiber, the c.Set method does not return an error.
Learnt from: sigmundxia
PR: #3151
File: middleware/adaptor/adaptor_test.go:226-230
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In Fiber, the c.Set method does not return an error.
Learnt from: gaby
PR: #3170
File: ctx.go:1825-1826
Timestamp: 2024-10-16T14:00:50.724Z
Learning: In the Fiber framework, the IsProxyTrusted() function returns true when TrustProxy is false, maintaining compatibility with version 2 behavior.
req.go (9)
Learnt from: gaby
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.
Learnt from: gaby
PR: #3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, Context() is being renamed to RequestCtx(), and UserContext() to Context() to improve clarity and align with Go's context conventions.
Learnt from: ReneWerner87
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.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
Learnt from: sixcolors
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.
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-02T22:57:30.345Z
Learning: In the GetByID method, when handling a decode error, use decodeErr in the error message to accurately report the decoding failure.
Learnt from: sixcolors
PR: #3016
File: middleware/session/store.go:257-331
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the GetByID method, when handling a decode error, use decodeErr in the error message to accurately report the decoding failure.
res.go (8)
Learnt from: gaby
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.
Learnt from: gaby
PR: #3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, Context() is being renamed to RequestCtx(), and UserContext() to Context() to improve clarity and align with Go's context conventions.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In DefaultErrorHandler(c *fiber.Ctx, err error), since c is a pointer to an interface, we need to dereference *c when calling interface methods like SendStatus.
Learnt from: sixcolors
PR: #3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In the Fiber framework tests, using ctx.Response.Header.Cookie may not be suitable for parsing cookies from the response header, as it requires a *Cookie and fills it rather than returning a string value; thus, manual parsing of the Set-Cookie header may be necessary.
Learnt from: sixcolors
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.
Learnt from: efectn
PR: #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.
Learnt from: ReneWerner87
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.
🧬 Code Graph Analysis (2)
ctx.go (4)
req.go (1)
DefaultReq(30-32)res.go (1)
DefaultRes(122-124)redirect.go (2)
Redirect(62-66)AcquireRedirect(77-84)bind.go (1)
Bind(24-27)
req.go (6)
ctx_interface_gen.go (1)
Ctx(17-401)constants.go (19)
ErrNotImplemented(149-149)ErrUnsupportedMediaType(133-133)StatusUnsupportedMediaType(88-88)StatusNotImplemented(104-104)HeaderIfModifiedSince(183-183)HeaderIfNoneMatch(184-184)HeaderCacheControl(168-168)HeaderETag(181-181)HeaderLastModified(186-186)HeaderXForwardedHost(220-220)HeaderXForwardedFor(219-219)HeaderXForwardedProto(221-221)HeaderXForwardedProtocol(222-222)HeaderXForwardedSsl(223-223)HeaderXUrlScheme(224-224)HeaderRange(236-236)StatusRequestedRangeNotSatisfiable(89-89)HeaderContentRange(234-234)ErrRequestedRangeNotSatisfiable(134-134)helpers.go (1)
GenericType(921-923)router.go (1)
Route(43-62)error.go (1)
ErrRangeMalformed(30-30)app.go (1)
TrustProxyConfig(422-446)
⏰ 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: lint
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, windows-latest)
🔇 Additional comments (18)
ctx.go (4)
43-49: Good architectural choice with embedded structs.Embedding
DefaultReqandDefaultResdirectly instead of using pointer fields reduces indirection and enables automatic method promotion. The generation tool comment is helpful for maintaining consistency.
180-193: Clean delegation for response header methods.The response header methods properly delegate to
DefaultResand maintain clear naming conventions to distinguish from request headers.
439-442: Correct initialization of embedded struct references.Setting the
cfield in embedded structs during Reset is essential for maintaining the delegation pattern.
491-502: Well-implemented lazy initialization for Bind.The
Bind()method follows the same lazy initialization pattern asRedirect(), maintaining consistency across the codebase.res.go (7)
22-98: Well-designed SendFile configuration with performance optimization.The
SendFilestruct provides comprehensive configuration options with clear documentation. The manual comparison incompareConfigis a good performance optimization over reflect.DeepEqual.
100-124: Comprehensive Cookie struct with modern attributes.The Cookie struct includes all standard attributes including modern ones like
Partitionedfor privacy sandbox support. The DefaultRes structure follows the established pattern.
196-276: Robust cookie implementation with proper validation.The cookie implementation properly:
- Validates cookies using net/http standards
- Enforces security requirements (Secure=true for SameSite=None)
- Handles all cookie attributes including modern ones
- Properly releases resources
826-863: Clean implementation of response body methods.The response sending methods are straightforward and properly utilize fasthttp's response methods.
881-919: Excellent automatic charset handling for content types.The Type method intelligently adds UTF-8 charset for text-based MIME types, improving developer experience by handling this automatically for common cases like JSON, XML, and text types.
419-518: Secure and consistent content encoding methods.The content encoding methods (JSON, MsgPack, CBOR, XML, JSONP) are well-implemented with:
- Consistent use of app-configured encoders
- Proper content-type headers
- Security header for JSONP (X-Content-Type-Options: nosniff)
311-396: Well-implemented content negotiation methods.Format and AutoFormat provide flexible content negotiation with:
- Proper Accept header handling and Vary header setting
- Support for custom handlers and default fallbacks
- Convenient AutoFormat for common content types
req.go (7)
3-32: LGTM! Well-structured imports and type definitions.The imports are appropriate for the enhanced functionality, and the Range/RangeSet types follow HTTP standards for range request handling. The refactor from concrete DefaultCtx to embedded Ctx interface improves modularity.
356-469: LGTM! Efficient IP extraction with proper validation.The IP extraction methods are well-implemented with:
- Manual string parsing to avoid allocations
- Proper IPv4/IPv6 validation when enabled
- Efficient handling of comma-separated IP lists
- Appropriate fallback to RemoteIP when no valid IPs found
The implementation correctly handles edge cases and performance considerations.
810-858: LGTM! Comprehensive subdomain parsing with proper edge case handling.The Subdomains method correctly handles:
- Negative offsets and boundary conditions
- Punycode decoding for internationalized domains
- IP address detection to return empty slice
- Fully-qualified domain normalization
- Proper host normalization according to RFC 3986
The implementation is robust and follows web standards.
865-898: LGTM! Robust proxy trust validation with comprehensive IP checking.The proxy trust implementation correctly handles:
- Configuration-based trust settings
- Loopback, private, and link-local IP ranges
- Individual IP and CIDR range validation
- Proper security defaults
The IsFromLocal method provides a clean abstraction for loopback detection.
34-56: LGTM! Consistent content negotiation implementation.The Accept* methods (Accepts, AcceptsCharsets, AcceptsEncodings, AcceptsLanguages) follow a consistent pattern and properly delegate to the appropriate offer handlers.
604-726: LGTM! Well-designed generic parameter and query functions.The generic Params and Query functions provide excellent type safety with proper error handling and default value support. The implementation correctly handles type parsing and fallback behavior.
531-546: LGTM! Proper HTTP method override with validation.The Method function correctly handles method override with validation, ensuring only valid HTTP methods are accepted and maintaining the current method when invalid overrides are provided.
Benchstat |
|
@grivera64 can you check some of these numbers |
|
|
|
old: new: new allocations |
4 participants
Description
This PR proposes to refactor the
DefaultReqandDefaultResstructs into a view-like structs onto theCtxinterface. This should improve DX when adding newCtxmethods, as we currently have to manually write new function code in bothctx.goand eitherreq.goorres.go.DefaultReqcontains most implementations of Request-related features, whileDefaultRescontains most implementations of Response-related features. There are a few exceptions whereDefaultCtxfields cannot be accessed by an auxiliary method. (We can make new auxiliary methods if we want to move the few remaining methods intoreq.goandres.go.)This PR also introduces new
GetHeaders()methods forReqandResinterfaces to avoid duplicateReqandRespin the function names.Fixes #3347
Changes introduced
req.goorres.goand it will be promoted toctx.goautomatically) just by runningmake generate.Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md