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 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 84.35% 84.24% -0.11%
==========================================
Files 120 120
Lines 12161 12184 +23
==========================================
+ Hits 10258 10264 +6
- Misses 1475 1504 +29
+ Partials 428 416 -12
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 |
| @@ -251,7 +165,7 @@ func (app *App) customRequestHandler(rctx *fasthttp.RequestCtx) { | |||
| defer app.ReleaseCtx(ctx) | |||
|
|
|||
| // Check if the HTTP method is valid | |||
| if app.methodInt(ctx.Method()) == -1 { | |||
| if ctx.getMethodInt() == -1 { | |||
| _ = ctx.SendStatus(StatusNotImplemented) //nolint:errcheck // Always return nil | |||
There was a problem hiding this comment.
@ReneWerner87 Probably related to this
There was a problem hiding this comment.
ok thx for the hint, i will check
There was a problem hiding this comment.
was unfortunately not the reason
![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: 4bacc1a | Previous: 5eb1432 | Ratio |
|---|---|---|---|
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
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 <[email protected]>
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.
| func (app *App) AcquireCtx(fctx *fasthttp.RequestCtx) CustomCtx { | ||
| ctx, ok := app.pool.Get().(CustomCtx) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Context unification looks good, but verify performance impact.
The signature change from Ctx to CustomCtx aligns well with the unified context handling approach. The type assertion ensures type safety at runtime.
However, based on the PR objectives, there are performance concerns (~10ns regression) with this new implementation. Consider monitoring the performance impact of this type assertion in hot paths.
🏁 Script executed:
#!/bin/bash
# Description: Verify that all callers of AcquireCtx have been updated to work with CustomCtx return type
# Expected: All usages should handle CustomCtx properly
echo "Searching for AcquireCtx usage patterns..."
rg -A 3 -B 1 "AcquireCtx" --type goLength of output: 70365
🏁 Script executed:
#!/bin/bash
# Show the AcquireCtx implementation around the assertion
rg -n "AcquireCtx" -A 5 -B 5 ctx_interface.goLength of output: 350
Update panic message in AcquireCtx to reflect CustomCtx
- In
ctx_interface.go(line 50), the panic readsbut the code is asserting topanic(errors.New("failed to type-assert to Ctx"))
CustomCtx. Please change the message to something like:panic(errors.New("failed to type-assert to CustomCtx"))
- All callers of
AcquireCtxalready handle theCustomCtxreturn (verified across router.go, tests, etc.). - As noted in the PR description, it may be worth adding or updating micro-benchmarks around hot paths to ensure the extra interface assertion doesn’t introduce unacceptable regressions.
🤖 Prompt for AI Agents
In ctx_interface.go around lines 46 to 47 and specifically at line 50, update
the panic message in the AcquireCtx method to accurately reflect the type
assertion to CustomCtx by changing the error text from "failed to type-assert to
Ctx" to "failed to type-assert to CustomCtx". Additionally, consider adding or
updating micro-benchmarks for AcquireCtx to monitor any performance impact
caused by the type assertion in hot paths.
Summary
NewWithCustomCtxfor creating apps with custom contextsTesting
go test ./...