🔥 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 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3533 +/- ##
==========================================
+ Coverage 91.04% 91.76% +0.72%
==========================================
Files 112 112
Lines 11289 11213 -76
==========================================
+ Hits 10278 10290 +12
+ Misses 754 666 -88
Partials 257 257
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
ctx.go (1)
459-472: Add test coverage and handle type assertion failure.The
renderExtensionsmethod has uncovered lines (463-466) 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 { + // Log the issue or handle it appropriately + if c.app.config.EnableDebug { + log.Printf("renderExtensions: non-string key in viewBindMap: %T", key) + } return true }req.go (1)
330-336: Avoid panic - handle non-TCP addresses gracefullyThe 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.
🧹 Nitpick comments (6)
ctx.go (1)
336-336: Consider using the embedded response struct for consistency.While this works correctly, for consistency with the refactoring pattern, consider delegating to the embedded response struct.
- c.Response().SetStatusCode(status) + c.DefaultRes.Status(status)Note: Since
StatusinDefaultResreturnsCtxfor chaining, this maintains the same behavior.res.go (1)
233-236: Consider logging invalid cookies instead of silently ignoring them.Invalid cookies are currently ignored without any indication to the developer, which could make debugging difficult.
if err := hc.Valid(); err != nil { // invalid cookies are ignored, same approach as net/http + if r.App().config.EnableDebug { + log.Printf("Invalid cookie: %s, error: %v", cookie.Name, err) + } return }req.go (4)
350-406: Performance concern: Manual string parsing in extractIPsFromHeaderThe IP extraction logic manually iterates through header values character by character. While this avoids allocations, it's complex and error-prone. Consider:
- The loop complexity makes it difficult to maintain
- IPv6 and IPv4 detection logic could be simplified
- The
maxEstimatedCountconstant (line 358) seems arbitraryConsider using a more readable approach that still maintains performance:
func (r *DefaultReq) extractIPsFromHeader(header string) []string { headerValue := r.Get(header) if headerValue == "" { return []string{} } + // Use strings.Split for better readability + parts := strings.Split(headerValue, ",") + ipsFound := make([]string, 0, len(parts)) + + for _, part := range parts { + ip := strings.TrimSpace(part) + if ip == "" { + continue + } + + if r.App().config.EnableIPValidation { + if !utils.IsIPv4(ip) && !utils.IsIPv6(ip) { + continue + } + } + + ipsFound = append(ipsFound, ip) + } + + return ipsFound - // ... complex manual parsing logic }
556-575: Potential performance issue: Inefficient parameter lookupThe Params method performs linear search through route parameters for each call. For routes with many parameters, this could be slow. The comparison logic (lines 562-566) is also complex.
Consider optimizing parameter lookup:
func (r *DefaultReq) Params(key string, defaultValue ...string) string { if key == "*" || key == "+" { key += "1" } route := r.Route() + // Consider using a map for O(1) lookup if route has many params + if len(route.Params) > 10 { + return r.getParamFromMap(key, defaultValue...) + } + for i := range route.Params { if len(key) != len(r.c.Route().Params[i]) { continue } if route.Params[i] == key || (!r.App().config.CaseSensitive && utils.EqualFold(route.Params[i], key)) { if len(r.c.getValues()) <= i || len(r.c.getValues()[i]) == 0 { break } return r.c.getValues()[i] } } return defaultString("", defaultValue) }
793-841: Complex subdomain extraction logicThe Subdomains method contains complex logic for handling punycode, IP addresses, and domain parsing. The IPv4/IPv6 detection and punycode handling could be error-prone.
Consider breaking down the logic into smaller, testable functions:
func (r *DefaultReq) Subdomains(offset ...int) []string { o := 2 if len(offset) > 0 { o = offset[0] } if o < 0 { return []string{} } + host := r.normalizeHost() + if r.isIPAddress(host) { + return []string{} + } + + parts := strings.Split(host, ".") + return r.extractSubdomains(parts, o) +} + +func (r *DefaultReq) normalizeHost() string { + host := r.Hostname() + // Trim trailing dot and convert to lowercase + if strings.HasSuffix(host, ".") { + host = utils.TrimRight(host, '.') + } + host = utils.ToLower(host) + + // Handle punycode + if strings.Contains(host, "xn--") { + if u, err := idna.Lookup.ToUnicode(host); err == nil { + host = utils.ToLower(u) + } + } + + return host +} + +func (r *DefaultReq) isIPAddress(host string) bool { + ip := host + if strings.HasPrefix(ip, "[") && strings.HasSuffix(ip, "]") { + ip = ip[1 : len(ip)-1] + } + return utils.IsIPv4(ip) || utils.IsIPv6(ip) +}
883-885: Incomplete cleanup in release methodThe release method only sets
r.c = nilbut doesn't clean up any other potential resources or state that might be held by the DefaultReq struct.Consider adding documentation about what resources are being cleaned up:
-// Release is a method to reset Req fields when to use ReleaseCtx() +// release is a method to reset Req fields when using ReleaseCtx() +// This method clears the context reference to prevent memory leaks func (r *DefaultReq) release() { r.c = nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ctx.go(7 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_test.go
🧰 Additional context used
🧠 Learnings (6)
📓 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_interface_gen.go (7)
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: mdelapenya
PR: gofiber/fiber#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: 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: 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.
req_interface_gen.go (5)
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: 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: 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.
req.go (7)
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: 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: 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-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: 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`.
ctx.go (12)
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: 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: 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`.
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: 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: 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`.
Learnt from: mdelapenya
PR: gofiber/fiber#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: gofiber/fiber#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: gaby
PR: gofiber/fiber#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.
res.go (8)
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: 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`.
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: 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.
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.
🧬 Code Graph Analysis (2)
ctx_interface_gen.go (6)
req_interface_gen.go (1)
Req(12-184)res_interface_gen.go (1)
Res(13-147)app.go (2)
Map(41-41)Storage(45-65)router.go (1)
Route(43-62)bind.go (1)
Bind(24-27)res.go (2)
Cookie(101-113)SendFile(23-59)
req_interface_gen.go (2)
req.go (4)
Locals(505-517)Params(592-598)Query(702-709)Range(18-21)router.go (1)
Route(43-62)
⏰ 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, ubuntu-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (1)
req_interface_gen.go (1)
13-184: Overall interface design looks comprehensiveThe
Reqinterface provides a well-structured and comprehensive API for HTTP request handling. The method documentation is detailed and helpful, following Go conventions. The interface covers all major aspects of request processing including content negotiation, headers, body handling, routing, and network information.The interface design aligns well with the PR objectives of refactoring to embed Koa-style request handling within the
Ctxinterface, improving developer experience and code organization.
| func (r *DefaultRes) End() error { | ||
| ctx := r.RequestCtx() | ||
| conn := ctx.Conn() | ||
|
|
||
| bw := bufio.NewWriter(conn) | ||
| if err := ctx.Response.Write(bw); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := bw.Flush(); err != nil { | ||
| return err //nolint:wrapcheck // unnecessary to wrap it | ||
| } | ||
|
|
||
| return conn.Close() //nolint:wrapcheck // unnecessary to wrap it | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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 nil🤖 Prompt for AI Agents
In res.go around lines 886 to 900, the End() method currently returns errors
directly without wrapping, which can mask the source of connection errors.
Modify the error returns to wrap them with contextual messages using fmt.Errorf
or a similar approach, so that each error clearly indicates the operation that
failed, such as writing the response, flushing the buffer, or closing the
connection.
| // Range returns a struct containing the type and a slice of ranges. | ||
| func (r *DefaultReq) Range(size int) (Range, error) { | ||
| var ( | ||
| rangeData Range | ||
| ranges string | ||
| ) | ||
| rangeStr := utils.Trim(r.Get(HeaderRange), ' ') | ||
|
|
||
| i := strings.IndexByte(rangeStr, '=') | ||
| if i == -1 || strings.Contains(rangeStr[i+1:], "=") { | ||
| return rangeData, ErrRangeMalformed | ||
| } | ||
| rangeData.Type = utils.ToLower(utils.Trim(rangeStr[:i], ' ')) | ||
| if rangeData.Type != "bytes" { | ||
| return rangeData, ErrRangeMalformed | ||
| } | ||
| ranges = utils.Trim(rangeStr[i+1:], ' ') | ||
|
|
||
| var ( | ||
| singleRange string | ||
| moreRanges = ranges | ||
| ) | ||
| for moreRanges != "" { | ||
| singleRange = moreRanges | ||
| if i := strings.IndexByte(moreRanges, ','); i >= 0 { | ||
| singleRange = moreRanges[:i] | ||
| moreRanges = utils.Trim(moreRanges[i+1:], ' ') | ||
| } else { | ||
| moreRanges = "" | ||
| } | ||
|
|
||
| singleRange = utils.Trim(singleRange, ' ') | ||
|
|
||
| var ( | ||
| startStr, endStr string | ||
| i int | ||
| ) | ||
| if i = strings.IndexByte(singleRange, '-'); i == -1 { | ||
| return rangeData, ErrRangeMalformed | ||
| } | ||
| startStr = utils.Trim(singleRange[:i], ' ') | ||
| endStr = utils.Trim(singleRange[i+1:], ' ') | ||
|
|
||
| start, startErr := fasthttp.ParseUint(utils.UnsafeBytes(startStr)) | ||
| end, endErr := fasthttp.ParseUint(utils.UnsafeBytes(endStr)) | ||
| if startErr != nil { // -nnn | ||
| start = size - end | ||
| end = size - 1 | ||
| } else if endErr != nil { // nnn- | ||
| end = size - 1 | ||
| } | ||
| if end > size-1 { // limit last-byte-pos to current length | ||
| end = size - 1 | ||
| } | ||
| if start > end || start < 0 { | ||
| continue | ||
| } | ||
| rangeData.Ranges = append(rangeData.Ranges, struct { | ||
| Start int | ||
| End int | ||
| }{ | ||
| Start: start, | ||
| End: end, | ||
| }) | ||
| } | ||
| if len(rangeData.Ranges) < 1 { | ||
| r.c.Status(StatusRequestedRangeNotSatisfiable) | ||
| r.c.Set(HeaderContentRange, "bytes */"+strconv.Itoa(size)) | ||
| return rangeData, ErrRequestedRangeNotSatisfiable | ||
| } | ||
|
|
||
| return rangeData, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Range header parsing complexity and error handling
The Range method contains complex parsing logic that could benefit from improvements:
- The method modifies the response (lines 777-778) which violates separation of concerns
- Multiple nested loops and string manipulations increase complexity
- Error handling could be more specific
Consider separating 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
}
+func (r *DefaultReq) parseRangeHeader(size int, rangeData *Range) error {
+ // Move parsing logic here
+ // ...
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In req.go around lines 711 to 783, the Range method mixes parsing logic with
response modification, increasing complexity and violating separation of
concerns. Refactor by extracting the parsing logic into a separate function that
only returns the parsed ranges and errors without modifying the response. Move
response status setting and header modification outside this parsing function to
the caller. Simplify error handling by returning specific errors from the parser
and handle them appropriately where the function is called.
| func (r *DefaultReq) tryDecodeBodyInOrder( | ||
| originalBody *[]byte, | ||
| encodings []string, | ||
| ) ([]byte, uint8, error) { | ||
| var ( | ||
| err error | ||
| body []byte | ||
| decodesRealized uint8 | ||
| ) | ||
|
|
||
| for idx := range encodings { | ||
| i := len(encodings) - 1 - idx | ||
| encoding := encodings[i] | ||
| decodesRealized++ | ||
| switch encoding { | ||
| case StrGzip, "x-gzip": | ||
| body, err = r.Request().BodyGunzip() | ||
| case StrBr, StrBrotli: | ||
| body, err = r.Request().BodyUnbrotli() | ||
| case StrDeflate: | ||
| body, err = r.Request().BodyInflate() | ||
| case StrZstd: | ||
| body, err = r.Request().BodyUnzstd() | ||
| case StrIdentity: | ||
| body = r.Request().Body() | ||
| case StrCompress, "x-compress": | ||
| return nil, decodesRealized - 1, ErrNotImplemented | ||
| default: | ||
| return nil, decodesRealized - 1, ErrUnsupportedMediaType | ||
| } | ||
|
|
||
| if err != nil { | ||
| return nil, decodesRealized, err | ||
| } | ||
|
|
||
| if i > 0 && decodesRealized > 0 { | ||
| if i == len(encodings)-1 { | ||
| tempBody := r.Request().Body() | ||
| *originalBody = make([]byte, len(tempBody)) | ||
| copy(*originalBody, tempBody) | ||
| } | ||
| r.Request().SetBodyRaw(body) | ||
| } | ||
| } | ||
|
|
||
| return body, decodesRealized, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex decompression logic needs validation
The tryDecodeBodyInOrder method handles multiple compression formats but has several concerns:
- The method modifies the request body in-place (line 116) which could cause issues if the method is called multiple times
- The
decodesRealizedcounter logic is complex and error-prone - Memory allocation and copying could be expensive for large bodies
Consider adding safeguards:
- Check if body has already been processed
- Add size limits to prevent excessive memory usage
- Document the side effects of body modification
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
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In req.go around lines 75 to 121, the tryDecodeBodyInOrder method modifies the
request body in-place and uses a complex decodesRealized counter, which can
cause issues if called multiple times and lead to high memory usage. To fix
this, add a check at the start to detect if the body has already been processed
to avoid redundant decoding, implement size limits before allocating memory to
prevent excessive usage, and clearly document the side effects of modifying the
request body within this method for future maintainers.
| // Body contains the raw body submitted in a POST request. | ||
| // This method will decompress the body if the 'Content-Encoding' header is provided. | ||
| // It returns the original (or decompressed) body data which is valid only within the handler. | ||
| // Don't store direct references to the returned data. | ||
| // If you need to keep the body's data later, make a copy or use the Immutable option. | ||
| func (r *DefaultReq) Body() []byte { | ||
| var ( | ||
| err error | ||
| body, originalBody []byte | ||
| headerEncoding string | ||
| encodingOrder = []string{"", "", ""} | ||
| ) | ||
|
|
||
| // Get Content-Encoding header | ||
| headerEncoding = utils.ToLower(utils.UnsafeString(r.Request().Header.ContentEncoding())) | ||
|
|
||
| // If no encoding is provided, return the original body | ||
| if len(headerEncoding) == 0 { | ||
| return r.getBody() | ||
| } | ||
|
|
||
| // Split and get the encodings list, in order to attend the | ||
| // rule defined at: https://www.rfc-editor.org/rfc/rfc9110#section-8.4-5 | ||
| encodingOrder = getSplicedStrList(headerEncoding, encodingOrder) | ||
| for i := range encodingOrder { | ||
| encodingOrder[i] = utils.ToLower(encodingOrder[i]) | ||
| } | ||
| if len(encodingOrder) == 0 { | ||
| return r.getBody() | ||
| } | ||
|
|
||
| var decodesRealized uint8 | ||
| body, decodesRealized, err = r.tryDecodeBodyInOrder(&originalBody, encodingOrder) | ||
|
|
||
| // Ensure that the body will be the original | ||
| if originalBody != nil && decodesRealized > 0 { | ||
| r.Request().SetBodyRaw(originalBody) | ||
| } | ||
| 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()) | ||
| } | ||
|
|
||
| if r.App().config.Immutable { | ||
| return utils.CopyBytes(body) | ||
| } | ||
| return body | ||
| } |
There was a problem hiding this comment.
Potential issue: Body() method modifying response status on error
The Body() method directly modifies the response status code when decompression fails (lines 164-167). This could cause unexpected behavior if:
- The method is called multiple times
- Other middleware or handlers expect to set their own status codes
- The error handling should be left to the caller
Consider removing the automatic status code setting and let the caller handle the error 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())
}🤖 Prompt for AI Agents
In req.go around lines 123 to 175, the Body() method sets the HTTP response
status code directly when decompression errors occur, which can cause unexpected
side effects if called multiple times or interfere with other handlers. To fix
this, remove the calls to r.c.SendStatus for error cases and instead return the
error information to the caller, allowing them to handle status codes and error
responses appropriately.
| release() | ||
| getBody() []byte |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internal lifecycle methods should be private
The release() and getBody() methods appear to be internal lifecycle and utility methods that shouldn't be part of the public Req interface contract.
Consider removing these methods from the public interface:
- // Release is a method to reset Req fields when to use ReleaseCtx()
- release()
- getBody() []byteCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In req_interface_gen.go around lines 182 to 183, the methods release() and
getBody() are currently part of the public Req interface but are intended as
internal lifecycle and utility methods. To fix this, remove these methods from
the public Req interface definition so they are not exposed externally, and
instead implement them as private methods within the package or struct where
they are used.
| BodyRaw() []byte | ||
| ClientHelloInfo() *tls.ClientHelloInfo | ||
| tryDecodeBodyInOrder(originalBody *[]byte, encodings []string) ([]byte, uint8, error) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making internal method private
The tryDecodeBodyInOrder method appears to be an internal utility for body decompression and shouldn't be part of the public Req interface. This method signature with pointer parameters suggests it's meant for internal use only.
Consider moving this method to a private interface or making it unexported in the implementation:
- tryDecodeBodyInOrder(originalBody *[]byte, encodings []string) ([]byte, uint8, error)📝 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.
| tryDecodeBodyInOrder(originalBody *[]byte, encodings []string) ([]byte, uint8, error) |
🤖 Prompt for AI Agents
In req_interface_gen.go at line 29, the method tryDecodeBodyInOrder is currently
part of the public Req interface but is intended as an internal utility for body
decompression. To fix this, remove tryDecodeBodyInOrder from the public Req
interface and instead make it a private method by renaming it to start with a
lowercase letter in its implementation or move it to a private interface used
internally. This will prevent it from being exposed publicly while retaining its
functionality internally.
| // When IP validation is enabled, any invalid IPs will be omitted. | ||
| extractIPsFromHeader(header string) []string | ||
| // extractIPFromHeader will attempt to pull the real client IP from the given header when IP validation is enabled. | ||
| // currently, it will return the first valid IP address in header. | ||
| // when IP validation is disabled, it will simply return the value of the header without any inspection. | ||
| // Implementation is almost the same as in extractIPsFromHeader, but without allocation of []string. | ||
| extractIPFromHeader(header string) string |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internal IP extraction methods should be private
The extractIPsFromHeader and extractIPFromHeader methods appear to be internal utilities for IP extraction and shouldn't be exposed in the public Req interface.
Consider removing these methods from the public interface:
- // extractIPsFromHeader will return a slice of IPs it found given a header name in the order they appear.
- // When IP validation is enabled, any invalid IPs will be omitted.
- extractIPsFromHeader(header string) []string
- // extractIPFromHeader will attempt to pull the real client IP from the given header when IP validation is enabled.
- // currently, it will return the first valid IP address in header.
- // when IP validation is disabled, it will simply return the value of the header without any inspection.
- // Implementation is almost the same as in extractIPsFromHeader, but without allocation of []string.
- extractIPFromHeader(header string) stringCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In req_interface_gen.go around lines 95 to 101, the methods extractIPsFromHeader
and extractIPFromHeader are internal utilities and should not be part of the
public Req interface. Remove these methods from the Req interface definition to
make them private and prevent exposing internal implementation details.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
req_interface_gen.go (3)
29-29: Consider making internal method privateThe
tryDecodeBodyInOrdermethod appears to be an internal utility for body decompression and shouldn't be part of the publicReqinterface. This method signature with pointer parameters suggests it's meant for internal use only.
95-101: Internal IP extraction methods should be privateThe
extractIPsFromHeaderandextractIPFromHeadermethods appear to be internal utilities for IP extraction and shouldn't be exposed in the publicReqinterface.
182-183: Internal lifecycle methods should be privateThe
release()andgetBody()methods appear to be internal lifecycle and utility methods that shouldn't be part of the publicReqinterface contract.res.go (1)
886-900: Improve error handling to avoid masking connection errors.The End() method should wrap errors to provide better context about where failures occur.
req.go (5)
133-133: Confusing array initializationInitializing
encodingOrderwith three empty strings is confusing and may introduce empty entries. It would be clearer to start with an empty slice and append as needed.
75-121: Complex decompression logic needs validationThe
tryDecodeBodyInOrdermethod handles multiple compression formats but has several concerns:
- The method modifies the request body in-place (line 116) which could cause issues if the method is called multiple times
- The
decodesRealizedcounter logic is complex and error-prone- Memory allocation and copying could be expensive for large bodies
123-175: Potential issue: Body() method modifying response status on errorThe Body() method directly modifies the response status code when decompression fails (lines 164-167). This could cause unexpected behavior if:
- The method is called multiple times
- Other middleware or handlers expect to set their own status codes
- The error handling should be left to the caller
330-336: Avoid panic - handle non-TCP addresses gracefullyThe 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.
711-783: Range header parsing complexity and error handlingThe Range method contains complex parsing logic that could benefit from improvements:
- The method modifies the response (lines 777-778) which violates separation of concerns
- Multiple nested loops and string manipulations increase complexity
- Error handling could be more specific
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefile(1 hunks)ctx_interface_gen.go(2 hunks)req.go(1 hunks)req_interface_gen.go(1 hunks)res.go(1 hunks)res_interface_gen.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_interface_gen.go
🧰 Additional context used
🧠 Learnings (5)
📓 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`.
res_interface_gen.go (5)
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: 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: 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: 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.
req_interface_gen.go (8)
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: 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: 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: mdelapenya
PR: gofiber/fiber#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: sixcolors
PR: gofiber/fiber#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: gofiber/fiber#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.
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.
res.go (8)
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: 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`.
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: 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: 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: 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.
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`.
req.go (9)
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: 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: 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-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: 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/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: gofiber/fiber#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.
🧬 Code Graph Analysis (1)
req_interface_gen.go (2)
req.go (4)
Locals(505-517)Params(592-598)Query(702-709)Range(18-21)router.go (1)
Route(43-62)
⏰ 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: repeated
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: Analyse
| Set(key, val string) | ||
| setCanonical(key, val string) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Internal methods should be private
The setCanonical and release() methods appear to be internal implementation details that shouldn't be part of the public Res interface contract.
Consider removing these methods from the public interface:
- setCanonical(key, val string)
...
- // Release is a method to reset Res fields when to use ReleaseCtx()
- release()Also applies to: 140-140
🤖 Prompt for AI Agents
In res_interface_gen.go at lines 124 and 140, the methods setCanonical and
release are currently part of the public Res interface but should be internal.
Remove these methods from the public interface definition to make them private
implementation details, ensuring they are not exposed as part of the interface
contract.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ctx.go (1)
463-466: Handle type assertion failure and add test coverage.The type assertion for
key.(string)silently continues on failure, which could hide issues. Additionally, these lines lack test coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(7 hunks)ctx_interface_gen.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_interface_gen.go
🧰 Additional context used
🧠 Learnings (2)
📓 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 (12)
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: 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: 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`.
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: 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: 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`.
Learnt from: mdelapenya
PR: gofiber/fiber#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: gofiber/fiber#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: gaby
PR: gofiber/fiber#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.
🪛 GitHub Check: codecov/patch
ctx.go
[warning] 169-170: ctx.go#L169-L170
Added lines #L169 - L170 were not covered by tests
[warning] 465-466: ctx.go#L465-L466
Added lines #L465 - L466 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (8)
ctx.go (8)
48-49: LGTM: Embedded structs align with refactoring goals.The change from pointer fields to embedded structs (
DefaultReqandDefaultRes) is well-aligned with the PR's objective to use embedded Koa-style structs. This enables method promotion and simplifies the API.
163-163: LGTM: Method delegation is correct.The delegation to
c.DefaultReq.Get()is consistent with the refactoring approach and maintains the same API surface.
184-186: LGTM: Response header delegation is correct.The
GetRespHeadermethod correctly delegates toc.DefaultRes.Get(), maintaining consistency with the refactoring pattern.
256-263: LGTM: Req() and Res() methods return embedded struct references.The methods now return references to the embedded structs (
&c.DefaultReq,&c.DefaultRes) instead of creating new instances. This is efficient and aligns with the refactoring goals.
440-441: LGTM: Reset method correctly initializes embedded struct context references.The reset logic properly sets the context reference for both embedded structs, ensuring they have access to the parent context.
455-456: LGTM: Release method delegates to embedded structs.The release method correctly calls the release methods on the embedded structs, maintaining proper cleanup.
494-502: LGTM: Bind method provides consistent API.The
Bind()method correctly initializes the bind reference with the context, maintaining the existing API pattern.
509-511: LGTM: Method setter follows convention.The
setMethodIntmethod follows the established pattern for internal setters in the context.
|
@grivera64 can you update the branch with the latest changes from master and also apply some hints from the AI |
…d-koa-style-req-res
This commit adds testing for: - GetHeaders - Path - (Res).Get
@ReneWerner87 For sure, I've added some unit tests per the latest few AI hints. Should I also apply the other AI hints?
I believe the remove the hidden methods from the public interface would be a good idea (should require the use of the |
|
@grivera64 is this ready for review? |
@gaby Yes, it should be ready to review. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Fiber framework to use embedded Koa-style Req and Res structs within the DefaultCtx, improving the developer experience when adding new context methods. This refactoring introduces a view-like architecture where request and response-related methods are organized in separate embedded structs.
- Embeds DefaultReq and DefaultRes structs directly into DefaultCtx structure to eliminate manual duplication
- Introduces GetHeaders() methods for both Req and Res interfaces to reduce naming redundancy
- Updates interface generation to use a newer version of ifacemaker tool with promoted field support
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| res_interface_gen.go | Generated response interface with comprehensive method documentation |
| res.go | Complete implementation of response methods moved from ctx.go |
| req_interface_gen.go | Generated request interface with comprehensive method documentation |
| req.go | Complete implementation of request methods moved from ctx.go |
| ctx_test.go | Updated tests to verify both old and new API methods work correctly |
| ctx_interface_gen.go | Regenerated main context interface using promoted fields |
| ctx_interface.go | Updated DefaultCtx constructor to use embedded field references |
| ctx.go | Streamlined context implementation with embedded req/res structs |
| Makefile | Updated ifacemaker tool version for promoted field support |
| AGENTS.md | Updated tool installation documentation |
Comments suppressed due to low confidence (2)
ctx_test.go:4599
- The test expectation has changed from "" to "" without any apparent code logic change in the template rendering. This suggests a potential behavioral change that should be investigated and documented.
require.Equal(t, "<h1></h1>", string(c.Response().Body()))
res.go:593
- [nitpick] The generic Params function is defined in req.go but the generic Query function is also defined in req.go. These utility functions might be better organized in a separate utilities file rather than being mixed with the struct methods.
bind = make(Map)
| // All the values are removed from ctx after returning from the top | ||
| // RequestHandler. Additionally, Close method is called on each value | ||
| // implementing io.Closer before removing the value from ctx. | ||
| func Locals[V any](c Ctx, key any, value ...V) V { |
There was a problem hiding this comment.
[nitpick] The Locals generic function is defined in req.go but conceptually belongs to general context operations rather than request-specific operations. Consider moving this to a more appropriate location or the main context file.
Copilot uses AI. Check for mistakes.
|
/gemini summary |
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.
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