🔥 Feature: Add Req and Res API #2894
Conversation
WalkthroughThe updates introduce modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 GitHub Check: codecov/patchreq.go[warning] 16-17: req.go#L16-L17 [warning] 20-21: req.go#L20-L21 [warning] 24-25: req.go#L24-L25 [warning] 28-29: req.go#L28-L29 [warning] 32-33: req.go#L32-L33 [warning] 44-45: req.go#L44-L45 [warning] 48-49: req.go#L48-L49 [warning] 52-53: req.go#L52-L53 [warning] 56-57: req.go#L56-L57 [warning] 60-61: req.go#L60-L61 [warning] 68-69: req.go#L68-L69 [warning] 76-77: req.go#L76-L77 [warning] 80-81: req.go#L80-L81 [warning] 84-85: req.go#L84-L85 [warning] 88-89: req.go#L88-L89 [warning] 92-93: req.go#L92-L93 [warning] 96-97: req.go#L96-L97 [warning] 100-101: req.go#L100-L101 [warning] 108-109: req.go#L108-L109 [warning] 112-113: req.go#L112-L113 [warning] 116-117: req.go#L116-L117 [warning] 120-121: req.go#L120-L121 [warning] 124-125: req.go#L124-L125 [warning] 128-129: req.go#L128-L129 ⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (16)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2894 +/- ##
==========================================
- Coverage 84.11% 83.64% -0.47%
==========================================
Files 116 118 +2
Lines 11557 11667 +110
==========================================
+ Hits 9721 9759 +38
- Misses 1405 1476 +71
- Partials 431 432 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@nickajacks1 Just so you are aware, the tests are very unstable/flaky right now in |
|
3/9: Moved some more methods from Ctx to Request. Rebased. |
|
3/17: Rebase. Moved more trivial methods to Request. |
|
Hi @gofiber/maintainers Sorry this is taking so long. |
|
Even though Express has Locals as part of the res API, I feel like it makes more sense to keep it as part of Ctx since it doesn't inherently have anything to do with the response (or request for that matter). I'm going to leave in Ctx for now. |
|
ok, sounds reasonable we can always adapt it later |
nickajacks1
force-pushed
the
reqres
branch
3 times, most recently
from
31fde6b to
d305ed8
Compare
|
Ok, the Response API is fully moved over. Some remaining work: Next week I'll start on either the docs or the interface part. |
|
I was thinking, is it actually that useful to make Request and Response interfaces if Ctx is already an interface and can thus be expanded instead of Request/Response? If we say that custom extensions should just go through Ctx, that might simplify both the API and implementation a bit. |
I am interested in learning more about the approach of expanding the In addition, can you help us by merging the changes added to the main branch, and updating your PR to use the new Bind() interface? Thank you, great work so far, and I look forward to your reply! |
The two primary points I have in mind are:
ye. will also probably be able to mark the PR ready for review soon. |
|
Marking as ready for review. If it is decided to make Request and Response interfaces, I will update the PR. |
|
Finished docs in time, so I added them to this PR after all. I'm not sure what the best way to document the whole " |
|
@nickajacks1 How about a table that shows both styles? |
I'm not sure about making Req and Resp not an interface. We're still able to update Ctx methods but it won't affect response for example. And it may cause some confusions as well. For example, if i've updated SendString, it means i'll have different behaving resp and ctx methods |
Yeah, and this would be a problem whether or not Response is an interface. If response is an interface and you only override Ctx.SendString, you still have different behavior between Response and Ctx. Gonna have to think about that one some more since we'll want to address that either way. Changing the subject a little...If Ctx implements both Request and Response, there's a corner case trap developers might hit if both Request and Response share method signatures, such as Get: printResponseContentType := func(r Response) {
fmt.Println(r.Get("Content-Type"))
}
printResponseContentType(c) // this prints the request's header, not the response'sThe only ways I've thought of to get around this so far are to explicitly add methods to Request and Response that Ctx does not implement so that Ctx does not implement those interfaces OR enforce that Request and Response do not have overlapping methods (which eliminates one of the benefits of splitting the APIs). I'll brainstorm for clean ways to deal with it, but feel free to spitball ideas. |
|
RE: overriding a method in Ctx but not Req/Res, resulting in differing behavior between e.g.,
I imagine the last two options are non-starters, so I will focus on the first. |
Maybe it's better if we don't make Req and Resp an interface and add a guide about customization |
|
Hi @nickajacks1 are there any update? |
|
Hi!!! I apologize for the extended absence. Both work and life got hectic at the same time and it threw me off the groove. |
|
definitely thanks for the PR
what if you leave the functionality in the ctx struct and the request response structs only refer to those of the ctx instead of then you could extend the functionality with the custom ctx and also inject your own request and response classes then you would only need a setResponse and setRequest function in the context and the subclasses then get the context injected with this behavior we would be able to logically separate the functionalities for the users in the 2 directions without moving the code and in my opinion we would have the best of both worlds |
|
@coderabbitai what do you think about it and how should we implement it |
|
Tip For best results, initiate chat on the files or code changes. @ReneWerner87, thank you for including me in this discussion. I've reviewed the proposed implementation options and would like to provide an analysis of each to help determine the best approach. Option 1: Make Advantages:
Disadvantages:
Option 2: Keep Advantages:
Disadvantages:
Option 3: Leave functionality in Advantages:
Disadvantages:
Option 4: Do not provide alias methods in Advantages:
Disadvantages:
Option 5: Implement Advantages:
Disadvantages:
Recommendation After weighing the options, Option 5 appears to offer the most balanced solution. It provides the flexibility to customize To implement this:
This approach combines the flexibility of interfaces with the cohesiveness of a shared context, offering a robust solution that can adapt to various needs without excessive complexity. Visual Representation Here's a simplified diagram to illustrate Option 5: classDiagram
class Ctx {
+SetRequest(Request)
+SetResponse(Response)
+OtherMethods()
}
class Request {
+Methods()
}
class Response {
+Methods()
}
Ctx o-- Request
Ctx o-- Response
Request <-- Ctx : injects
Response <-- Ctx : injects
Conclusion Implementing Option 5 should provide a scalable and maintainable architecture that meets the project's goals. It balances the need for flexibility with the desire to keep the API intuitive and consistent. Let me know your thoughts on this approach or if you'd like to explore any aspect further. |
|
@ReneWerner87 what do you think the usage should be like for customizing the request and response? I'm thinking about the following cases:
Where do you see |
|
Will try to answer tomorrow |
case number 3 would of course be best, but case number 1 would be okay to start with the setResponse and setRequest methods are initially called when the ctx is created. the ctx should be passed on internally to the two classes so that the methods of the context can then be used there and the request/response class is then only a class for forwarding and separation by topic area for the functionalities |
|
I've updated the PR with the more conservative approach that @ReneWerner87 suggested that keeps the implementation in Ctx. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
ctx_interface_gen.go (1)
268-273: Fix documentation comment for Req() method.The comment for
Req()incorrectly states "Res returns" instead of "Req returns". Please update the comment to:-// Res returns a convenience type whose API is limited to operations -// on the incoming request. +// Req returns a convenience type whose API is limited to operations +// on the incoming request.ctx.go (2)
1474-1476: Fix documentation typoThe comment has a typo - it says "Res returns" but is documenting the Req() method.
-// Res returns a convenience type whose API is limited to operations +// Req returns a convenience type whose API is limited to operations
57-58: Well-structured API split implementationThe implementation of separate Req and Res types through accessor methods provides a clean separation of concerns, aligning well with the PR objectives. This approach:
- Improves code organization by separating request and response operations
- Reduces ambiguity in method names
- Makes the API more intuitive for users
Consider documenting the migration path for users who are currently accessing Request() and Response() directly.
Also applies to: 1468-1478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ctx.go(2 hunks)ctx_interface.go(1 hunks)ctx_interface_gen.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] 1470-1471: ctx.go#L1470-L1471
Added lines #L1470 - L1471 were not covered by tests
[warning] 1476-1477: ctx.go#L1476-L1477
Added lines #L1476 - L1477 were not covered by tests
res.go
[warning] 8-9: res.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: res.go#L12-L13
Added lines #L12 - L13 were not covered by tests
req.go
[warning] 8-9: req.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: req.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 16-17: req.go#L16-L17
Added lines #L16 - L17 were not covered by tests
[warning] 20-21: req.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-25: req.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 28-29: req.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 32-33: req.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 36-37: req.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 40-41: req.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 44-45: req.go#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 48-49: req.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 52-53: req.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 56-57: req.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-61: req.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 64-65: req.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 68-69: req.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 72-73: req.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 76-77: req.go#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 80-81: req.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-85: req.go#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 88-89: req.go#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 92-93: req.go#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 96-97: req.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 100-101: req.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 104-105: req.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 108-109: req.go#L108-L109
Added lines #L108 - L109 were not covered by tests
🔇 Additional comments (7)
res_interface_gen.go (1)
1-27: Well-structured response interface with Express-like API
The interface provides a comprehensive set of response methods that align well with Express.js conventions while maintaining idiomatic Go patterns. The method chaining pattern (Status, Type) is particularly useful for fluent API usage.
Let's verify the Express.js alignment:
✅ Verification successful
Express-like API alignment confirmed with proper method chaining
The verification confirms that:
- The interface closely mirrors Express.js's response methods with Go-idiomatic naming (e.g.,
SendStatus,SendFile,JSON, etc.) - Method chaining is correctly implemented in
StatusandTypemethods, returningResinterface for fluent API usage - The implementation in
DefaultResproperly delegates to the context while maintaining the chaining pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare method names with Express.js response methods
# Create a list of Express response methods from their documentation
echo "Express.js response methods:" > express_methods.txt
curl -s https://expressjs.com/en/4x/api.html | grep -o 'res\.[a-zA-Z]*' | sort -u >> express_methods.txt
# List our response methods
echo "Our response methods:" > our_methods.txt
rg '^[[:space:]]+[A-Z][a-zA-Z]+\(' res_interface_gen.go | sed 's/(.*//' | tr -d '\t' >> our_methods.txt
# Compare the lists
diff express_methods.txt our_methods.txt
Length of output: 927
Script:
#!/bin/bash
# Let's verify the method chaining pattern mentioned in the review
ast-grep --pattern 'func ($var *$type) Status(status int) Res'
ast-grep --pattern 'func ($var *$type) Type(extension string, charset ...string) Res'
# Let's also check the actual implementation to verify Express-like behavior
rg -A 3 'func \(\w+ \*\w+\) (Status|Type|JSON|Send)' .
Length of output: 5872
req_interface_gen.go (1)
1-33: Well-designed request interface with comprehensive functionality
The interface provides a complete set of request handling methods with consistent signatures and good practices:
- Optional parameters using variadic arguments (defaultValue ...string)
- Clear method names that align with Express.js conventions
- Comprehensive coverage of HTTP request aspects (headers, body, params, etc.)
Let's verify the Express.js alignment:
✅ Verification successful
Interface aligns well with Express.js request API with appropriate Go adaptations
The verification confirms that the interface closely mirrors Express.js's request API with proper Go-style adaptations:
- Method names follow Go conventions (e.g.,
XHRinstead ofxhr,BaseURLinstead ofbaseUrl) - Core request methods are present (accepts, body, cookies, params, query, etc.)
- Intentionally omits Express.js-specific features not applicable to Go (e.g.,
req.app,req.connection,req.user) - Maintains consistent parameter patterns using Go idioms (variadic arguments for optional values)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare method names with Express.js request methods
# Create a list of Express request methods from their documentation
echo "Express.js request methods:" > express_methods.txt
curl -s https://expressjs.com/en/4x/api.html | grep -o 'req\.[a-zA-Z]*' | sort -u >> express_methods.txt
# List our request methods
echo "Our request methods:" > our_methods.txt
rg '^[[:space:]]+[A-Z][a-zA-Z]+\(' req_interface_gen.go | sed 's/(.*//' | tr -d '\t' >> our_methods.txt
# Compare the lists
diff express_methods.txt our_methods.txt
Length of output: 1120
ctx_interface.go (1)
38-39: Migration required: Update Request() and Response() method calls
The codebase still contains numerous references to the old Request() and Response() methods that need to be updated to use the new Req() and Res() methods. This is a breaking change that requires careful migration.
Let's verify the remaining usage:
#!/bin/bash
# Description: Find remaining usage of old methods
# Search for method calls
echo "Files still using old methods:"
rg --type go 'Request\(\)|Response\(\)' --glob '!ctx_interface.go'
# Count occurrences
echo -e "\nTotal occurrences:"
rg --type go 'Request\(\)|Response\(\)' --glob '!ctx_interface.go' | wc -lres.go (1)
4-6: LGTM! Clean implementation of the response API wrapper.
The DefaultRes struct correctly implements the delegation pattern, maintaining a clean separation of concerns while preserving the underlying functionality.
req.go (1)
4-6: LGTM! Clean implementation of the request API wrapper.
The DefaultReq struct correctly implements the delegation pattern, providing a clear separation between request-specific operations and the broader context.
ctx_interface_gen.go (1)
268-273: LGTM! Interface changes align with PR objectives.
The addition of Req() and Res() methods to the Ctx interface properly supports the new API structure, enabling better separation of concerns between request and response operations.
ctx.go (1)
57-58: LGTM! Well-structured field additions
The new fields are properly documented and follow Go naming conventions. Using pointer types is appropriate here for reference types.
|
|
||
| // Res | ||
| type Res interface { | ||
| Locals(key any, value ...any) any |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider moving Locals method back to Ctx interface
Based on the PR discussion, the Locals method was intended to remain in the Ctx interface as it doesn't inherently relate to the response or request.
ctx_interface.go
Outdated
| req: &DefaultReq{}, | ||
| res: &DefaultRes{}, |
There was a problem hiding this comment.
Fix initialization of req and res fields
The current initialization creates empty structs without setting the context reference. This will likely cause issues when methods try to access the context.
Apply this diff to properly initialize the fields:
- req: &DefaultReq{},
- res: &DefaultRes{},
+ req: &DefaultReq{ctx: c},
+ res: &DefaultRes{ctx: c},📝 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.
| req: &DefaultReq{}, | |
| res: &DefaultRes{}, | |
| req: &DefaultReq{ctx: c}, | |
| res: &DefaultRes{ctx: c}, |
res.go
Outdated
| package fiber | ||
|
|
||
| //go:generate ifacemaker --file res.go --struct DefaultRes --iface Res --pkg fiber --output res_interface_gen.go --not-exported true --iface-comment "Res" | ||
| type DefaultRes struct { | ||
| ctx *DefaultCtx | ||
| } | ||
|
|
||
| func (r *DefaultRes) Locals(key any, value ...any) any { | ||
| return r.ctx.Locals(key, value...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Append(field string, values ...string) { | ||
| r.ctx.Append(field, values...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Attachment(filename ...string) { | ||
| r.ctx.Attachment(filename...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) AutoFormat(body any) error { | ||
| return r.ctx.AutoFormat(body) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Cookie(cookie *Cookie) { | ||
| r.ctx.Cookie(cookie) | ||
| } | ||
|
|
||
| func (r *DefaultRes) ClearCookie(key ...string) { | ||
| r.ctx.ClearCookie(key...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Download(file string, filename ...string) error { | ||
| return r.ctx.Download(file, filename...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Format(handlers ...ResFmt) error { | ||
| return r.ctx.Format(handlers...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Get(key string, defaultValue ...string) string { | ||
| return r.ctx.GetRespHeader(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) JSON(body any) error { | ||
| return r.ctx.JSON(body) | ||
| } | ||
|
|
||
| func (r *DefaultRes) JSONP(data any, callback ...string) error { | ||
| return r.ctx.JSONP(data, callback...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Links(link ...string) { | ||
| r.ctx.Links(link...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Location(path string) { | ||
| r.ctx.Location(path) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Render(name string, bind Map, layouts ...string) error { | ||
| return r.ctx.Render(name, bind, layouts...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Send(body []byte) error { | ||
| return r.ctx.Send(body) | ||
| } | ||
|
|
||
| func (r *DefaultRes) SendFile(file string, config ...SendFile) error { | ||
| return r.ctx.SendFile(file, config...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) SendStatus(status int) error { | ||
| return r.ctx.SendStatus(status) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Set(key, val string) { | ||
| r.ctx.Set(key, val) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Status(status int) Res { | ||
| r.ctx.Status(status) | ||
| return r | ||
| } | ||
|
|
||
| func (r *DefaultRes) Type(extension string, charset ...string) Res { | ||
| r.ctx.Type(extension, charset...) | ||
| return r | ||
| } |
There was a problem hiding this comment.
Add unit tests for the response API methods.
The static analysis indicates that the new response API methods lack test coverage. This is critical for ensuring the delegation pattern works correctly and maintaining reliability during future changes.
Would you like me to help generate comprehensive unit tests for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: res.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: res.go#L12-L13
Added lines #L12 - L13 were not covered by tests
req.go
Outdated
|
|
||
| func (r *DefaultReq) AcceptsCharsets(offers ...string) string { | ||
| return r.ctx.AcceptsCharsets(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) AcceptsEncodings(offers ...string) string { | ||
| return r.ctx.AcceptsEncodings(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) AcceptsLanguages(offers ...string) string { | ||
| return r.ctx.AcceptsLanguages(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) BaseURL() string { | ||
| return r.ctx.BaseURL() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Body() []byte { | ||
| return r.ctx.Body() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Cookies(key string, defaultValue ...string) string { | ||
| return r.ctx.Cookies(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Fresh() bool { | ||
| return r.ctx.Fresh() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Get(key string, defaultValue ...string) string { | ||
| return r.ctx.Get(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Host() string { | ||
| return r.ctx.Host() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Hostname() string { | ||
| return r.ctx.Hostname() | ||
| } | ||
|
|
||
| func (r *DefaultReq) IP() string { | ||
| return r.ctx.IP() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Is(extension string) bool { | ||
| return r.ctx.Is(extension) | ||
| } | ||
|
|
||
| func (r *DefaultReq) IPs() []string { | ||
| return r.ctx.IPs() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Method() string { | ||
| return r.ctx.Method() | ||
| } | ||
|
|
||
| func (r *DefaultReq) OriginalURL() string { | ||
| return r.ctx.OriginalURL() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Params(key string, defaultValue ...string) string { | ||
| return r.ctx.Params(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Path() string { | ||
| return r.ctx.Path() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Protocol() string { | ||
| return r.ctx.Protocol() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Query(key string, defaultValue ...string) string { | ||
| return r.ctx.Query(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Range(size int) (Range, error) { | ||
| return r.ctx.Range(size) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Route() *Route { | ||
| return r.ctx.Route() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Secure() bool { | ||
| return r.ctx.Secure() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Stale() bool { | ||
| return r.ctx.Stale() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Subdomains(offset ...int) []string { | ||
| return r.ctx.Subdomains(offset...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) XHR() bool { | ||
| return r.ctx.XHR() | ||
| } |
There was a problem hiding this comment.
Add unit tests for the request API methods.
The static analysis indicates that the new request API methods lack test coverage. Comprehensive testing is essential for validating the delegation behavior.
Would you like me to help generate comprehensive unit tests for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: req.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: req.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 16-17: req.go#L16-L17
Added lines #L16 - L17 were not covered by tests
[warning] 20-21: req.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-25: req.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 28-29: req.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 32-33: req.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 36-37: req.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 40-41: req.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 44-45: req.go#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 48-49: req.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 52-53: req.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 56-57: req.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-61: req.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 64-65: req.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 68-69: req.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 72-73: req.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 76-77: req.go#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 80-81: req.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-85: req.go#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 88-89: req.go#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 92-93: req.go#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 96-97: req.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 100-101: req.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 104-105: req.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 108-109: req.go#L108-L109
Added lines #L108 - L109 were not covered by tests
Split the existing Ctx API into two separate APIs for Requests and Responses. There are two goals to this change: 1. Reduce cognitive load by making it more obvious whether a Ctx method interacts with the request or the response. 2. Increase API parity with Express.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
res_interface_gen.go (1)
6-30: Consider adding interface documentation.The
Resinterface would benefit from documentation describing its purpose and relationship with Express's response API. This would help users understand the new API structure.res.go (1)
44-46: Consider renaming method for clarity.The
Getmethod internally callsGetRespHeader, which is more descriptive. Consider renaming this method toGetHeaderto better reflect its purpose and maintain consistency with the internal implementation.-func (r *DefaultRes) Get(key string, defaultValue ...string) string { +func (r *DefaultRes) GetHeader(key string, defaultValue ...string) string { return r.ctx.GetRespHeader(key, defaultValue...) }req.go (2)
32-34: Add documentation for ambiguous parameter.The
defaultValueparameter in theCookiesmethod (and similar methods) would benefit from documentation explaining when it's used and what happens when multiple default values are provided.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-33: req.go#L32-L33
Added lines #L32 - L33 were not covered by tests
88-90: Consider error wrapping for better context.The
Rangemethod could benefit from wrapping the error with additional context about the request.func (r *DefaultReq) Range(size int) (Range, error) { - return r.ctx.Range(size) + rng, err := r.ctx.Range(size) + if err != nil { + return rng, fmt.Errorf("failed to parse range header: %w", err) + } + return rng, nil }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 88-89: req.go#L88-L89
Added lines #L88 - L89 were not covered by testsctx.go (1)
1468-1478: Consider adding examples in documentationThe documentation for both
Req()andRes()methods could be enhanced with usage examples to help developers understand how to use these new APIs effectively.Consider adding examples like:
// Example: // // app.Get("/", func(c fiber.Ctx) error { // // Access request-specific operations // body := c.Req().Body() // // // Access response-specific operations // c.Res().Status(200) // return nil // })🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1470-1471: ctx.go#L1470-L1471
Added lines #L1470 - L1471 were not covered by tests
[warning] 1476-1477: ctx.go#L1476-L1477
Added lines #L1476 - L1477 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ctx.go(2 hunks)ctx_interface.go(1 hunks)ctx_interface_gen.go(1 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 as they are similar to previous changes (3)
- ctx_interface_gen.go
- ctx_interface.go
- req_interface_gen.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
ctx.go
[warning] 1470-1471: ctx.go#L1470-L1471
Added lines #L1470 - L1471 were not covered by tests
[warning] 1476-1477: ctx.go#L1476-L1477
Added lines #L1476 - L1477 were not covered by tests
res.go
[warning] 8-9: res.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: res.go#L12-L13
Added lines #L12 - L13 were not covered by tests
req.go
[warning] 8-9: req.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: req.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 16-17: req.go#L16-L17
Added lines #L16 - L17 were not covered by tests
[warning] 20-21: req.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-25: req.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 28-29: req.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 32-33: req.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 36-37: req.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 40-41: req.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 44-45: req.go#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 48-49: req.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 52-53: req.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 56-57: req.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-61: req.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 64-65: req.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 68-69: req.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 72-73: req.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 76-77: req.go#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 80-81: req.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-85: req.go#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 88-89: req.go#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 92-93: req.go#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 96-97: req.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 100-101: req.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 104-105: req.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 108-109: req.go#L108-L109
Added lines #L108 - L109 were not covered by tests
🔇 Additional comments (2)
res_interface_gen.go (1)
7-7: Move Locals method back to Ctx interface.
Based on the PR discussion, the Locals method was intended to remain in the Ctx interface as it doesn't inherently relate to the response or request.
ctx.go (1)
57-58: LGTM! Fields are properly defined
The new fields req and res are correctly defined as pointers to DefaultReq and DefaultRes respectively, which aligns with the PR objective of splitting the Ctx API.
| type DefaultRes struct { | ||
| ctx *DefaultCtx | ||
| } | ||
|
|
||
| func (r *DefaultRes) Locals(key any, value ...any) any { | ||
| return r.ctx.Locals(key, value...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Append(field string, values ...string) { | ||
| r.ctx.Append(field, values...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Attachment(filename ...string) { | ||
| r.ctx.Attachment(filename...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) AutoFormat(body any) error { | ||
| return r.ctx.AutoFormat(body) | ||
| } | ||
|
|
||
| func (r *DefaultRes) CBOR(body any, ctype ...string) error { | ||
| return r.ctx.CBOR(body, ctype...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Cookie(cookie *Cookie) { | ||
| r.ctx.Cookie(cookie) | ||
| } | ||
|
|
||
| func (r *DefaultRes) ClearCookie(key ...string) { | ||
| r.ctx.ClearCookie(key...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Download(file string, filename ...string) error { | ||
| return r.ctx.Download(file, filename...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Format(handlers ...ResFmt) error { | ||
| return r.ctx.Format(handlers...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Get(key string, defaultValue ...string) string { | ||
| return r.ctx.GetRespHeader(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) JSON(body any, ctype ...string) error { | ||
| return r.ctx.JSON(body, ctype...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) JSONP(data any, callback ...string) error { | ||
| return r.ctx.JSONP(data, callback...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Links(link ...string) { | ||
| r.ctx.Links(link...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Location(path string) { | ||
| r.ctx.Location(path) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Render(name string, bind Map, layouts ...string) error { | ||
| return r.ctx.Render(name, bind, layouts...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Send(body []byte) error { | ||
| return r.ctx.Send(body) | ||
| } | ||
|
|
||
| func (r *DefaultRes) SendFile(file string, config ...SendFile) error { | ||
| return r.ctx.SendFile(file, config...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) SendStatus(status int) error { | ||
| return r.ctx.SendStatus(status) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Set(key, val string) { | ||
| r.ctx.Set(key, val) | ||
| } | ||
|
|
||
| func (r *DefaultRes) Status(status int) Res { | ||
| r.ctx.Status(status) | ||
| return r | ||
| } | ||
|
|
||
| func (r *DefaultRes) Type(extension string, charset ...string) Res { | ||
| r.ctx.Type(extension, charset...) | ||
| return r | ||
| } | ||
|
|
||
| func (r *DefaultRes) Vary(fields ...string) { | ||
| r.ctx.Vary(fields...) | ||
| } | ||
|
|
||
| func (r *DefaultRes) XML(data any) error { | ||
| return r.ctx.XML(data) | ||
| } |
There was a problem hiding this comment.
Add unit tests for the response API methods.
The static analysis indicates that the new response API methods lack test coverage. This is critical for ensuring the delegation pattern works correctly and maintaining reliability during future changes.
Would you like me to help generate comprehensive unit tests for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: res.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: res.go#L12-L13
Added lines #L12 - L13 were not covered by tests
req.go
Outdated
|
|
||
| func (r *DefaultReq) AcceptsCharsets(offers ...string) string { | ||
| return r.ctx.AcceptsCharsets(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) AcceptsEncodings(offers ...string) string { | ||
| return r.ctx.AcceptsEncodings(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) AcceptsLanguages(offers ...string) string { | ||
| return r.ctx.AcceptsLanguages(offers...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) BaseURL() string { | ||
| return r.ctx.BaseURL() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Body() []byte { | ||
| return r.ctx.Body() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Cookies(key string, defaultValue ...string) string { | ||
| return r.ctx.Cookies(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Fresh() bool { | ||
| return r.ctx.Fresh() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Get(key string, defaultValue ...string) string { | ||
| return r.ctx.Get(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Host() string { | ||
| return r.ctx.Host() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Hostname() string { | ||
| return r.ctx.Hostname() | ||
| } | ||
|
|
||
| func (r *DefaultReq) IP() string { | ||
| return r.ctx.IP() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Is(extension string) bool { | ||
| return r.ctx.Is(extension) | ||
| } | ||
|
|
||
| func (r *DefaultReq) IPs() []string { | ||
| return r.ctx.IPs() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Method() string { | ||
| return r.ctx.Method() | ||
| } | ||
|
|
||
| func (r *DefaultReq) OriginalURL() string { | ||
| return r.ctx.OriginalURL() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Params(key string, defaultValue ...string) string { | ||
| return r.ctx.Params(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Path() string { | ||
| return r.ctx.Path() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Protocol() string { | ||
| return r.ctx.Protocol() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Query(key string, defaultValue ...string) string { | ||
| return r.ctx.Query(key, defaultValue...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Range(size int) (Range, error) { | ||
| return r.ctx.Range(size) | ||
| } | ||
|
|
||
| func (r *DefaultReq) Route() *Route { | ||
| return r.ctx.Route() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Secure() bool { | ||
| return r.ctx.Secure() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Stale() bool { | ||
| return r.ctx.Stale() | ||
| } | ||
|
|
||
| func (r *DefaultReq) Subdomains(offset ...int) []string { | ||
| return r.ctx.Subdomains(offset...) | ||
| } | ||
|
|
||
| func (r *DefaultReq) XHR() bool { | ||
| return r.ctx.XHR() | ||
| } |
There was a problem hiding this comment.
Add unit tests for the request API methods.
The static analysis indicates that the new request API methods lack test coverage. Comprehensive testing is essential for validating the delegation behavior.
Would you like me to help generate comprehensive unit tests for these methods?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 8-9: req.go#L8-L9
Added lines #L8 - L9 were not covered by tests
[warning] 12-13: req.go#L12-L13
Added lines #L12 - L13 were not covered by tests
[warning] 16-17: req.go#L16-L17
Added lines #L16 - L17 were not covered by tests
[warning] 20-21: req.go#L20-L21
Added lines #L20 - L21 were not covered by tests
[warning] 24-25: req.go#L24-L25
Added lines #L24 - L25 were not covered by tests
[warning] 28-29: req.go#L28-L29
Added lines #L28 - L29 were not covered by tests
[warning] 32-33: req.go#L32-L33
Added lines #L32 - L33 were not covered by tests
[warning] 36-37: req.go#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 40-41: req.go#L40-L41
Added lines #L40 - L41 were not covered by tests
[warning] 44-45: req.go#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 48-49: req.go#L48-L49
Added lines #L48 - L49 were not covered by tests
[warning] 52-53: req.go#L52-L53
Added lines #L52 - L53 were not covered by tests
[warning] 56-57: req.go#L56-L57
Added lines #L56 - L57 were not covered by tests
[warning] 60-61: req.go#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 64-65: req.go#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 68-69: req.go#L68-L69
Added lines #L68 - L69 were not covered by tests
[warning] 72-73: req.go#L72-L73
Added lines #L72 - L73 were not covered by tests
[warning] 76-77: req.go#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 80-81: req.go#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 84-85: req.go#L84-L85
Added lines #L84 - L85 were not covered by tests
[warning] 88-89: req.go#L88-L89
Added lines #L88 - L89 were not covered by tests
[warning] 92-93: req.go#L92-L93
Added lines #L92 - L93 were not covered by tests
[warning] 96-97: req.go#L96-L97
Added lines #L96 - L97 were not covered by tests
[warning] 100-101: req.go#L100-L101
Added lines #L100 - L101 were not covered by tests
[warning] 104-105: req.go#L104-L105
Added lines #L104 - L105 were not covered by tests
[warning] 108-109: req.go#L108-L109
Added lines #L108 - L109 were not covered by tests
| type DefaultReq struct { | ||
| ctx *DefaultCtx | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for ctx field.
The DefaultReq struct should validate that the ctx field is not nil during initialization to prevent potential panics.
type DefaultReq struct {
ctx *DefaultCtx
}
+
+func NewDefaultReq(ctx *DefaultCtx) (*DefaultReq, error) {
+ if ctx == nil {
+ return nil, errors.New("context cannot be nil")
+ }
+ return &DefaultReq{ctx: ctx}, nil
+}Committable suggestion skipped: line range outside the PR's diff.
| // Res returns a convenience type whose API is limited to operations | ||
| // on the outgoing response. | ||
| func (c *DefaultCtx) Res() Res { | ||
| return c.res | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the Res() method
The review comment is correct. The codebase search reveals that while there are interface and implementation definitions for Res, there is no direct test coverage for the Res() method of DefaultCtx. A test should be added to verify that:
- The method returns the internal
resfield - The returned value implements the
Resinterface - Operations on the returned value affect the context's response
🔗 Analysis chain
Add test coverage for the Res() method
While the implementation is correct and the documentation is clear, test coverage is missing for this method. Please add unit tests to ensure the method correctly returns the res field.
Would you like me to help generate unit tests for this method?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for Res() method
# Test: Search for test cases covering Res(). Expect: Test cases for the method.
rg --type go 'func Test.*Res\(' app/
Length of output: 83
Script:
#!/bin/bash
# First, let's find the correct location of test files
fd -e go test
# Then search for any existing test coverage of Res() method
rg --type go 'func Test.*Res\('
# Also search for any usage of Res() in test files to catch potential indirect testing
rg --type go '\.Res\(' -g '*test.go'
Length of output: 2109
Script:
#!/bin/bash
# Let's check ctx_test.go specifically since Res() is defined in ctx.go
cat ctx_test.go | grep -A 10 -B 10 "Res()"
# Also check for any indirect testing of the response functionality
rg --type go "c\.res\b" ctx_test.go
# Let's also look for any tests that might be using the Res type
rg --type go "type.*Res\b" -A 5
Length of output: 620
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1476-1477: ctx.go#L1476-L1477
Added lines #L1476 - L1477 were not covered by tests
ctx.go
Outdated
| // Res returns a convenience type whose API is limited to operations | ||
| // on the incoming request. | ||
| func (c *DefaultCtx) Req() Req { | ||
| return c.req | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add test coverage for the Req() method
The method needs test coverage to ensure it correctly returns the req field. Here's a suggested test structure:
- Add a test case in
ctx_test.gothat:- Creates a new DefaultCtx instance
- Sets a known request value
- Calls Req() and verifies it returns the expected request
- Verifies that modifications to the returned request reflect in the context
🔗 Analysis chain
Add test coverage for the Req() method
While the implementation is correct and the documentation is clear, test coverage is missing for this method. Please add unit tests to ensure the method correctly returns the req field.
Would you like me to help generate unit tests for this method?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for Req() method
# Test: Search for test cases covering Req(). Expect: Test cases for the method.
rg --type go 'func Test.*Req\(' app/
Length of output: 83
Script:
#!/bin/bash
# First, let's find the correct location of test files
fd -e go test
# Then search for existing test cases for Req() method
rg --type go 'func Test.*Req\('
# Also search for any usage of Req() in test files to catch cases where it might be tested as part of other test cases
rg --type go -g '*_test.go' '\.Req\(\)'
Length of output: 2112
Script:
#!/bin/bash
# Let's check ctx_test.go specifically since it should contain tests for DefaultCtx
cat ctx_test.go | grep -A 20 -B 20 'Req()'
# Also let's verify if there are any indirect tests of Req() method
rg -g '*_test.go' -B 5 -A 5 'req :?= .*Req\(\)'
# Let's also check the implementation to understand the testing needs better
ast-grep --pattern 'func (c *DefaultCtx) Req() Req'
Length of output: 233
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1470-1471: ctx.go#L1470-L1471
Added lines #L1470 - L1471 were not covered by tests
thank you, is there anything else open here apart from the review? |
|
I was thinking about how to best update the docs. Right now the Ctx page has every method in alphabetical order. Perhaps it could be reordered a bit, something roughly like this Also, I was thinking that a future improvement could be to maybe auto generate req.go and res.go. I handwrote them, but it might not be that hard to write a tiny program to generate them based on some tag in the comments of Ctx methods or something. That could help prevent req and res from getting out of sync with Ctx. Other than those two things, in my opinion, there isn't much of a point of writing unit tests for the entire Req and Res APIs. Maybe just a couple that call some basic methods so that the Ctx.Req() and Ctx.Res() methods have greater than 0 coverage? |
ctx.go
Outdated
| @@ -1463,6 +1465,18 @@ func (c *DefaultCtx) renderExtensions(bind any) { | |||
| } | |||
| } | |||
|
|
|||
| // Res returns a convenience type whose API is limited to operations | |||
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.go(2 hunks)ctx_interface_gen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_interface_gen.go
🔇 Additional comments (3)
ctx.go (3)
57-58: LGTM! Field additions are well-documented and follow Go conventions.The new fields
reqandresare properly defined as pointers to their respective types and include clear documentation comments.
1470-1472: Add test coverage for the Req() method.The method needs test coverage to ensure it correctly returns the req field.
1474-1478: Add test coverage for the Res() method.The method needs test coverage to ensure it correctly returns the res field.
ctx.go
Outdated
| // Res returns a convenience type whose API is limited to operations | ||
| // on the incoming request. |
There was a problem hiding this comment.
Fix documentation comment for Req() method.
The comment incorrectly mentions "Res" instead of "Req". It should be:
"Req returns a convenience type whose API is limited to operations on the incoming request."
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
req.go (1)
3-3: Group import declarations.According to the static analysis and Go best practices, import declarations should be grouped.
-import "mime/multipart" +import ( + "mime/multipart" +)🧰 Tools
🪛 golangci-lint (1.62.2)
3-3: should only use grouped 'import' declarations
(grouper)
🪛 GitHub Check: lint
[failure] 3-3:
should only use grouped 'import' declarations (grouper)ctx_test.go (1)
971-1010: Refactor: Remove unnecessary string conversions in Cookie test casesThe test cases have been updated to use
Res().Cookie(), but there are unnecessary string conversions when getting response headers.- require.Equal(t, expect, string(c.Res().Get(HeaderSetCookie))) + require.Equal(t, expect, c.Res().Get(HeaderSetCookie))🧰 Tools
🪛 golangci-lint (1.62.2)
973-973: unnecessary conversion
(unconvert)
978-978: unnecessary conversion
(unconvert)
983-983: unnecessary conversion
(unconvert)
989-989: unnecessary conversion
(unconvert)
997-997: unnecessary conversion
(unconvert)
1005-1005: unnecessary conversion
(unconvert)
1010-1010: unnecessary conversion
(unconvert)
🪛 GitHub Check: lint
[failure] 973-973:
unnecessary conversion (unconvert)
[failure] 978-978:
unnecessary conversion (unconvert)
[failure] 983-983:
unnecessary conversion (unconvert)
[failure] 989-989:
unnecessary conversion (unconvert)
[failure] 997-997:
unnecessary conversion (unconvert)
[failure] 1005-1005:
unnecessary conversion (unconvert)
[failure] 1010-1010:
unnecessary conversion (unconvert)🪛 GitHub Actions: golangci-lint
[error] 973-973: Unnecessary conversion detected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ctx.go(2 hunks)ctx_interface.go(1 hunks)ctx_interface_gen.go(1 hunks)ctx_test.go(17 hunks)req.go(1 hunks)req_interface_gen.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ctx_interface.go
- ctx_interface_gen.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ctx_test.go
973-973: unnecessary conversion
(unconvert)
978-978: unnecessary conversion
(unconvert)
983-983: unnecessary conversion
(unconvert)
989-989: unnecessary conversion
(unconvert)
997-997: unnecessary conversion
(unconvert)
1005-1005: unnecessary conversion
(unconvert)
1010-1010: unnecessary conversion
(unconvert)
req.go
3-3: should only use grouped 'import' declarations
(grouper)
🪛 GitHub Check: lint
ctx_test.go
[failure] 973-973:
unnecessary conversion (unconvert)
[failure] 978-978:
unnecessary conversion (unconvert)
[failure] 983-983:
unnecessary conversion (unconvert)
[failure] 989-989:
unnecessary conversion (unconvert)
[failure] 997-997:
unnecessary conversion (unconvert)
[failure] 1005-1005:
unnecessary conversion (unconvert)
[failure] 1010-1010:
unnecessary conversion (unconvert)
req.go
[failure] 3-3:
should only use grouped 'import' declarations (grouper)
🪛 GitHub Actions: golangci-lint
ctx_test.go
[error] 973-973: Unnecessary conversion detected.
🪛 GitHub Check: codecov/patch
req.go
[warning] 14-15: req.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 18-19: req.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-23: req.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 26-27: req.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 30-31: req.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 42-43: req.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 46-47: req.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 50-51: req.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 54-55: req.go#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 58-59: req.go#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 66-67: req.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 74-75: req.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 78-79: req.go#L78-L79
Added lines #L78 - L79 were not covered by tests
[warning] 82-83: req.go#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-87: req.go#L86-L87
Added lines #L86 - L87 were not covered by tests
[warning] 90-91: req.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 94-95: req.go#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 98-99: req.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 106-107: req.go#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 110-111: req.go#L110-L111
Added lines #L110 - L111 were not covered by tests
[warning] 114-115: req.go#L114-L115
Added lines #L114 - L115 were not covered by tests
[warning] 118-119: req.go#L118-L119
Added lines #L118 - L119 were not covered by tests
[warning] 122-123: req.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 126-127: req.go#L126-L127
Added lines #L126 - L127 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: repeated
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (12)
req_interface_gen.go (1)
1-41: LGTM! Well-structured interface design.The
Reqinterface provides a comprehensive and logically organized set of methods for handling request-related operations. The interface aligns well with the PR objectives of splitting theCtxAPI.req.go (2)
6-8: Add validation for ctx field.The
DefaultReqstruct should validate that thectxfield is not nil during initialization to prevent potential panics.
10-128: Add unit tests for the request API methods.The static analysis indicates that the new request API methods lack test coverage. Comprehensive testing is essential for validating the delegation behavior.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 14-15: req.go#L14-L15
Added lines #L14 - L15 were not covered by tests
[warning] 18-19: req.go#L18-L19
Added lines #L18 - L19 were not covered by tests
[warning] 22-23: req.go#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 26-27: req.go#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 30-31: req.go#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 42-43: req.go#L42-L43
Added lines #L42 - L43 were not covered by tests
[warning] 46-47: req.go#L46-L47
Added lines #L46 - L47 were not covered by tests
[warning] 50-51: req.go#L50-L51
Added lines #L50 - L51 were not covered by tests
[warning] 54-55: req.go#L54-L55
Added lines #L54 - L55 were not covered by tests
[warning] 58-59: req.go#L58-L59
Added lines #L58 - L59 were not covered by tests
[warning] 66-67: req.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 74-75: req.go#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 78-79: req.go#L78-L79
Added lines #L78 - L79 were not covered by tests
[warning] 82-83: req.go#L82-L83
Added lines #L82 - L83 were not covered by tests
[warning] 86-87: req.go#L86-L87
Added lines #L86 - L87 were not covered by tests
[warning] 90-91: req.go#L90-L91
Added lines #L90 - L91 were not covered by tests
[warning] 94-95: req.go#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 98-99: req.go#L98-L99
Added lines #L98 - L99 were not covered by tests
[warning] 106-107: req.go#L106-L107
Added lines #L106 - L107 were not covered by tests
[warning] 110-111: req.go#L110-L111
Added lines #L110 - L111 were not covered by tests
[warning] 114-115: req.go#L114-L115
Added lines #L114 - L115 were not covered by tests
[warning] 118-119: req.go#L118-L119
Added lines #L118 - L119 were not covered by tests
[warning] 122-123: req.go#L122-L123
Added lines #L122 - L123 were not covered by tests
[warning] 126-127: req.go#L126-L127
Added lines #L126 - L127 were not covered by testsctx.go (3)
57-58: LGTM! Clear field declarations.The new fields
reqandresinDefaultCtxare well-defined and align with the PR objectives of splitting the API.
1468-1472: Add test coverage for the Req() method.The method needs test coverage to ensure it correctly returns the req field.
1474-1478: Add test coverage for the Res() method.The method needs test coverage to ensure it correctly returns the res field.
ctx_test.go (6)
48-66: LGTM: Accepts test cases updated to use Req() APIThe changes correctly update the test cases to use the new
Req().Accepts()method instead of directly callingAccepts(), aligning with the PR's objective to split context functionality.
942-942: LGTM: FormFile test case updated to use Req() APIThe test case correctly uses
Req().FormFile()instead of directly callingFormFile().
1168-1168: LGTM: AutoFormat test case updated to use Res() APIThe test case correctly uses
Res().AutoFormat()instead of directly callingAutoFormat().
785-785: LGTM: JSONP test case updated to use Res() APIThe test case correctly uses
Res().JSONP()instead of directly callingJSONP().
910-910: LGTM: Queries test case updated to use Req() APIThe test case correctly uses
Req().Queries()instead of directly callingQueries().
5058-5058: LGTM: IsFromLocal test case updated to use Req() APIThe test case correctly uses
Req().IsFromLocal()instead of directly callingIsFromLocal().
* Sprinkle in calls to Req() and Res() to a few unit tests * Fix improper initialization caught by ^ * Add a few missing methods
CURRENT STATUS
Description
Split the existing Ctx API into two separate APIs for Requests and Responses. Having distinct APIs for Request and Response types will allow developers to more easily write self-documenting code by avoiding ambiguous methods such as
Ctx.CookieandCtx.Cookies. Much of the existing Ctx API simply calls the corresponding Req or Res API. For example,Ctx.GetcallsRequest.GetandCtx.SetcallsResponse.Set.Because Express defines only res and req APIs with no context API, this change will make it easier to manage Fiber's adherence to Express's APIs. In addition, it will allow us to avoid needing to create certain one-off methods. For example,
Ctx.GetRespHeadercan now becomeCtx.Res().GetAlthough the majority of the Ctx API is unchanged,
Ctx.RequestandCtx.Responsehave been removed in favor ofCtx.ReqandCtx.Res. Now, instead of a rawfasthttp.Requestorfasthttp.Response, we returnfiber.Requestandfiber.Responseobjects, which implement their respective APIs. As such, this is a breaking change for users who accessCtx.RequestandCtx.Responsedirect.Inspired by Koa and fasthttp, both of which have individual request/response objects and a single context object whose methods are simply aliases for corresponding methods on the req/res APIs.
Deprecation:
Ctx.GetRespHeaderis replaced byCtx.Res().GetRelated to #2854
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.