Add NewWithCustomCtx initialization helper #3476
Conversation
WalkthroughThis change refactors Fiber's custom context handling by introducing a new constructor Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant CustomCtxFactory
participant RequestHandler
participant RouteStack
User->>App: NewWithCustomCtx(customCtxFunc, config)
App->>CustomCtxFactory: Store customCtxFunc
User->>App: HTTP Request
App->>RequestHandler: requestHandler(rctx)
RequestHandler->>CustomCtxFactory: Create CustomCtx
RequestHandler->>RouteStack: next(CustomCtx)
RouteStack-->>RequestHandler: Handler execution result
RequestHandler-->>App: Return response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3476 +/- ##
==========================================
+ Coverage 90.53% 90.67% +0.13%
==========================================
Files 110 110
Lines 10949 10936 -13
==========================================
+ Hits 9913 9916 +3
+ Misses 777 769 -8
+ Partials 259 251 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new helper, NewWithCustomCtx, to simplify initializing an app with a custom context while hiding the context factory setter.
- Renames nextCustom to next and customRequestHandler to requestHandler for consistency.
- Updates tests, documentation, and helper functions to reflect the new initialization process.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| router.go | Renames methods and updates error message handling for custom contexts. |
| helpers.go | Renames methodExistCustom to methodExist for consistency. |
| docs/whats_new.md | Updates release notes to document NewWithCustomCtx. |
| docs/api/app.md | Updates API documentation to reflect the new helper. |
| ctx_test.go | Updates tests to use NewWithCustomCtx. |
| ctx.go | Simplifies context handling by using the new next implementation. |
| app.go | Introduces NewWithCustomCtx and refactors internal context handling. |
Comments suppressed due to low confidence (1)
router.go:110
- [nitpick] Renaming 'nextCustom' to 'next' may lead to ambiguity with similar methods handling DefaultCtx; consider a more descriptive name to differentiate custom and default context flows.
func (app *App) next(c CustomCtx) (bool, error) { //nolint:unparam // bool param might be useful for testing
|
|
old: new: this is bad, i have to investigate why it took so long |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
router.go (1)
193-198: 🛠️ Refactor suggestionClarify error handling flow - TODO needs resolution.
The TODO comment questions whether to return after error handling. Currently, the function continues execution even after
ErrorHandlerprocesses an error and sends a status. This could lead to unexpected behavior if handlers modify the response after an error has been handled.Consider returning immediately after error handling to prevent further processing:
if err != nil { if catch := ctx.App().ErrorHandler(ctx, err); catch != nil { _ = ctx.SendStatus(StatusInternalServerError) //nolint:errcheck // Always return nil } - // TODO: Do we need to return here? + return }
♻️ Duplicate comments (1)
router.go (1)
110-168: Well-structured unification of routing logic, but performance impact needs investigation.The consolidation of
nextandnextCustominto a single method using theCustomCtxinterface is clean and reduces code duplication. However, according to the PR objectives, there's a noted performance degradation that needs to be investigated.The interface method calls (
getMethodInt(),getTreePathHash(), etc.) may introduce overhead compared to direct field access in the previous implementation.
🧹 Nitpick comments (2)
docs/whats_new.md (2)
68-69: Link to API docs for NewWithCustomCtx
To help users quickly find the full API details, consider hyperlinking this new method to the API reference, e.g.:- **NewWithCustomCtx**: Initialize an app with a custom context in one step. See [API → App#NewWithCustomCtx](./api/app#newwithcustomctx).
93-98: Ensure consistent code fence metadata
This fence usesgo title="Signature", but other code blocks don’t include titles. For consistency, either remove thetitleattribute here or add similar titles elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app.go(4 hunks)ctx.go(1 hunks)ctx_test.go(2 hunks)docs/api/app.md(2 hunks)docs/whats_new.md(4 hunks)helpers.go(2 hunks)router.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: unit (1.23.x, macos-13)
- GitHub Check: repeated
🔇 Additional comments (10)
helpers.go (1)
106-150: LGTM! Clean interface abstraction.The refactoring from
*DefaultCtxtoCustomCtxinterface is well-executed and maintains the same functionality while providing better abstraction. The use of interface methods likegetMethodInt(),getTreePathHash(), andsetIndexRoute()properly encapsulates the context operations.ctx.go (2)
1048-1048: LGTM! Simplified routing logic.The removal of conditional logic and direct call to
c.app.next(c)aligns well with the unified request handling approach described in the PR summary.
1058-1058: LGTM! Consistent with the unified routing approach.The change to always call
c.app.next(c)maintains consistency with theNext()method and the broader refactoring goals.However, please monitor the performance impact as mentioned in the PR objectives, since this consolidation might affect request handling performance.
docs/whats_new.md (1)
121-126: Example aligns with signature
The factory function correctly returnsfiber.Ctx(satisfying the genericCustomCtxconstraint), and the example embedsDefaultCtxproperly. LGTM.ctx_test.go (2)
134-140: Consistent API usage with proper Config integration.The test correctly demonstrates using
NewWithCustomCtxwith additional configuration options, showing how the new API handles both custom context creation and app configuration in a unified manner.
112-116:❓ Verification inconclusive
LGTM! Clean migration to the new API pattern.
The test correctly uses the new
NewWithCustomCtxconstructor, which consolidates app creation and custom context function registration into a single step. This aligns well with the PR objectives to introduce the new helper function.However, given the performance concerns mentioned in the PR objectives, it would be valuable to verify that this change doesn't introduce performance regression in the test execution:
🏁 Script executed:
#!/bin/bash # Description: Run specific custom context tests and measure performance # Compare execution time of these tests to identify any performance impact echo "Running custom context tests with timing..." go test -run "Test_Ctx_CustomCtx" -v -count=5 -benchtime=1sLength of output: 375
Please verify performance of the new
NewWithCustomCtxtests
I’m unable to run the timing script in this environment (missing Go module), so please execute the following locally to ensure there’s no regression:# From your module root: go test -run "Test_Ctx_CustomCtx" -v -count=5 -benchtime=1sOnce you’ve confirmed performance is on par with the previous two-step initialization, this change can be considered fully approved.
app.go (4)
626-633: LGTM! Clean implementation of the custom context constructor.The new
NewWithCustomCtxfunction provides a convenient way to initialize an app with a custom context factory in a single call. The documentation clearly explains its purpose and relationship to the existing API.
654-663: Good refactoring: Method visibility and documentation are appropriate.The renaming from
NewCtxFunctosetCtxFuncand making it unexported aligns well with the new initialization pattern viaNewWithCustomCtx. The method correctly updates the server handler when the server is already initialized.
915-915: Excellent simplification of the handler logic.Removing the conditional logic and always returning
app.requestHandleraligns perfectly with the unified routing approach. This makes the code cleaner and more maintainable.
1126-1126: Consistent simplification in server initialization.The change to always set
app.server.Handler = app.requestHandleris consistent with the unified handler approach throughout the codebase.
| ## NewWithCustomCtx | ||
|
|
||
| `NewCtxFunc` allows you to customize the `ctx` struct as needed. | ||
| `NewWithCustomCtx` creates a new `*App` and sets the custom context factory | ||
| function at construction time. | ||
|
|
||
| ```go title="Signature" | ||
| func (app *App) NewCtxFunc(function func(app *App) CustomCtx) | ||
| func NewWithCustomCtx(fn func(app *App) CustomCtx, config ...Config) *App | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document generic type parameter in signature
The actual implementation of NewWithCustomCtx is generic over the custom context type (with a Ctx constraint), but the brackets around the type parameter are omitted here. Consider showing the full generic signature, for example:
func NewWithCustomCtx[CC Ctx](fn func(app *App) CC, config ...Config) *App🤖 Prompt for AI Agents
In docs/api/app.md around lines 515 to 522, the function signature for
NewWithCustomCtx is missing the generic type parameter declaration. Update the
signature to include the generic type parameter with its constraint, for
example: func NewWithCustomCtx[CC Ctx](fn func(app *App) CC, config ...Config)
*App, to accurately reflect the implementation.
| func main() { | ||
| app := fiber.New() | ||
|
|
||
| app.NewCtxFunc(func(app *fiber.App) fiber.CustomCtx { | ||
| app := fiber.NewWithCustomCtx(func(app *fiber.App) fiber.CustomCtx { | ||
| return &CustomCtx{ | ||
| DefaultCtx: *fiber.NewDefaultCtx(app), | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect return type in factory signature
The example’s factory function is declared as func(app *fiber.App) fiber.CustomCtx, but fiber.CustomCtx isn’t a defined type. It should match the local CustomCtx struct or the interface, e.g.:
app := fiber.NewWithCustomCtx(func(app *fiber.App) CustomCtx {
return &CustomCtx{ DefaultCtx: *fiber.NewDefaultCtx(app) }
})Or explicitly return the interface type:
func(app *fiber.App) fiber.Ctx { … }🤖 Prompt for AI Agents
In docs/api/app.md around lines 541 to 546, the factory function passed to
fiber.NewWithCustomCtx has an incorrect return type fiber.CustomCtx, which is
not defined. Change the return type to either the local CustomCtx struct or the
fiber.Ctx interface to match the expected signature. For example, update the
function signature to func(app *fiber.App) *CustomCtx or func(app *fiber.App)
fiber.Ctx and ensure the returned value matches this type.
|
IMO we could drop CustomContext in favor of #3382 |
not really, one is the context.context and the other is the fiber context |
Right, I totally mixed that up with v2 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 01c6c91 | Previous: e1a7c11 | Ratio |
|---|---|---|---|
BenchmarkUnmarshaldata |
10.42 ns/op 95.96 MB/s 0 B/op 0 allocs/op |
5.293 ns/op 188.92 MB/s 0 B/op 0 allocs/op |
1.97 |
BenchmarkUnmarshaldata - ns/op |
10.42 ns/op |
5.293 ns/op |
1.97 |
This comment was automatically generated by workflow using github-action-benchmark.
|
@ksw2000 Do you have any idea what the problem could be? |
|
Here some benchmarks for the functions type benchCustomCtx struct {
DefaultCtx
}
func (c *benchCustomCtx) Params(key string, defaultValue ...string) string { //revive:disable-line:unused-parameter
return c.DefaultCtx.Params(key)
}
func newCustomApp() *App {
return NewWithCustomCtx(func(app *App) CustomCtx {
return &benchCustomCtx{DefaultCtx: *NewDefaultCtx(app)}
})
}
func Benchmark_Next_Default(b *testing.B) {
app := New()
app.Get("/", func(_ Ctx) error { return nil })
app.startupProcess()
fctx := &fasthttp.RequestCtx{}
fctx.Request.Header.SetMethod(MethodGet)
fctx.Request.SetRequestURI("/")
c, _ := app.AcquireCtx(fctx).(*DefaultCtx) //nolint:forcetypeassert // not needed
b.ResetTimer()
for i := 0; i < b.N; i++ {
c.indexRoute = -1
_, _ = app.next(c)
}
}
func Benchmark_Next_Custom(b *testing.B) {
app := newCustomApp()
app.Get("/", func(_ Ctx) error { return nil })
app.startupProcess()
fctx := &fasthttp.RequestCtx{}
fctx.Request.Header.SetMethod(MethodGet)
fctx.Request.SetRequestURI("/")
c := app.AcquireCtx(fctx).(CustomCtx)
b.ResetTimer()
for i := 0; i < b.N; i++ {
c.setIndexRoute(-1)
_, _ = app.nextCustom(c)
}
} |
* perf: modify app.pool
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 36.26n ± 0% 34.51n ± 0% -4.83% (p=0.000 n=30)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
* perf: optimize defaultCtx in app.next
```
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ new.txt │ new4.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 34.51n ± 0% 30.02n ± 0% -13.01% (p=0.000 n=30)
│ new.txt │ new4.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ new.txt │ new4.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
```
---------
Co-authored-by: Kashiwa <13825170+ksw2000@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ctx_interface.go (1)
50-50: Fix inconsistent error message.The error message mentions "failed to type-assert to Ctx" but the actual assertion is to
CustomCtx. This inconsistency could confuse developers during debugging.- panic(errors.New("failed to type-assert to Ctx")) + panic(errors.New("failed to type-assert to CustomCtx"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(5 hunks)ctx_interface.go(2 hunks)helpers.go(0 hunks)router.go(1 hunks)
💤 Files with no reviewable changes (1)
- helpers.go
🚧 Files skipped from review as they are similar to previous changes (2)
- app.go
- router.go
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: unit (1.24.x, macos-13)
- GitHub Check: Compare
🔇 Additional comments (1)
ctx_interface.go (1)
58-58: Parameter type change aligns with unified context approach.The change from
CtxtoCustomCtxparameter is consistent with the overall refactoring goal and maintains type safety.
|
@efectn @gofiber/maintainers what do you think, should we go the old way with the double method (customRequestHandler + defaultRequestHandler, methodExist + methodExistCustom, nextCustom + next) or should we merge the current one, where the logic is merged in the method itself, but is a bit slower ? |
```
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ head.txt │ revert-1.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 30.04n ± 0% 28.74n ± 0% -4.33% (p=0.000 n=30)
│ head.txt │ revert-1.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ head.txt │ revert-1.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
```
|
If we keep Please check this: ksw2000@6600202#commitcomment-160231393 I can make a pull request!
|
|
Ok pls, with target to this branch |
|
@ksw2000 PR would be great |
…t-and-routing-interfaces-for-generics
* Make custom context setup via NewWithCustomCtx
* Update docs/api/app.md
* fix lint issues
* small optimizations
* perf: modify app.pool
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 36.26n ± 0% 34.51n ± 0% -4.83% (p=0.000 n=30)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
* perf: optimize defaultCtx in app.next
```
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ new.txt │ new4.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 34.51n ± 0% 30.02n ± 0% -13.01% (p=0.000 n=30)
│ new.txt │ new4.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ new.txt │ new4.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
```
* perf: revert nextCustom
```
goos: darwin
goarch: arm64
pkg: github.com/gofiber/fiber/v3
cpu: Apple M4
│ head.txt │ revert-1.txt │
│ sec/op │ sec/op vs base │
_Communication_Flow-10 30.04n ± 0% 28.74n ± 0% -4.33% (p=0.000 n=30)
│ head.txt │ revert-1.txt │
│ B/op │ B/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
│ head.txt │ revert-1.txt │
│ allocs/op │ allocs/op vs base │
_Communication_Flow-10 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=30) ¹
¹ all samples are equal
```
---------
Co-authored-by: RW <rene@gofiber.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
router.go (1)
209-305: Code duplication concern with next methodThe
nextCustommethod duplicates significant logic from thenextmethod, differing mainly in interface method calls vs direct field access. This creates maintenance overhead as both methods need to be updated when routing logic changes.Consider extracting the common routing logic into a shared function that accepts abstracted access methods. This would reduce duplication while maintaining the performance benefits of separate execution paths:
type routeAccessor interface { getMethodInt() int getTreePathHash() uint32 getIndexRoute() int // ... other access methods } func (app *App) routeMatch(accessor routeAccessor, setters routeSetters) (bool, error) { // Common routing logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx_interface.go(1 hunks)router.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ctx_interface.go
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.24.x, ubuntu-latest)
- GitHub Check: unit (1.24.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: Analyse
- GitHub Check: repeated
🔇 Additional comments (3)
router.go (3)
109-153: LGTM - Optimized route matching for default contextThe direct field access and unsafe string conversions are appropriate performance optimizations for the default context path. The local
indexRoutevariable management is more efficient than using getters/setters.
307-347: Unified request handler balances flexibility and performanceThe type assertion approach (
ctx.(*DefaultCtx)) effectively maintains the performance optimization for the common case while supporting custom contexts. The error handling and flash message processing are correctly duplicated for both paths.The design choice to keep separate
nextandnextCustommethods addresses the performance concerns mentioned in the PR objectives, where benchmarking showed a 10-nanosecond degradation when unifying the logic completely.
156-207: Verify HTML escaping consistency across error pathsThe error message properly escapes the path using
html.EscapeString, which is good for security. However, ensure this escaping approach is consistent with other error handling paths in the codebase.#!/bin/bash # Description: Check for consistent HTML escaping in error messages across the codebase # Expected: All error messages containing user input should use html.EscapeString echo "Checking for error messages with path/URL content:" rg -A 2 -B 2 "Cannot.*\+" --type go echo -e "\n\nChecking for html.EscapeString usage:" rg "html\.EscapeString" --type go echo -e "\n\nChecking for potential unescaped user input in error messages:" rg -A 2 -B 2 "NewError.*\+" --type go
Summary
NewWithCustomCtxfor creating apps with custom contextsTesting
go test ./...