🔥 feat: Implement UpdateParam override behavior for DefaultCtx #3962
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Ctx.UpdateParam(name, value string) (interface + DefaultCtx implementation), tests and benchmarks, API docs, and a defensive route registration panic when parameter count exceeds max; UpdateParam normalizes wildcards, respects CaseSensitive, updates c.values when a route is present, and no-ops otherwise. (≤50 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2024-10-08T19:06:06.583ZApplied to files:
📚 Learning: 2025-05-08T08:14:37.302ZApplied to files:
📚 Learning: 2024-11-15T07:56:21.623ZApplied to files:
📚 Learning: 2024-10-16T12:12:30.506ZApplied to files:
📚 Learning: 2025-12-07T15:07:23.885ZApplied to files:
🧬 Code graph analysis (1)ctx.go (1)
🔇 Additional comments (1)
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. Comment |
Summary of ChangesHello @TheAspectDev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new UpdateParam method to the context, allowing for the modification of route parameters. The implementation is straightforward and includes comprehensive tests, benchmarks, and documentation. My main feedback is to improve the clarity and correctness of the new documentation for UpdateParam.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/api/ctx.md (1)
506-508: Usefiber.Ctxas receiver type for consistency.The signature shows
*DefaultCtxas the receiver type, but for consistency with other method signatures in this documentation (e.g., lines 49, 608, 703), it should usefiber.Ctx.🔎 Suggested fix
-```go title="Signature" -func (c *DefaultCtx) UpdateParam(name, value string) -``` +```go title="Signature" +func (c fiber.Ctx) UpdateParam(name string, value string) +```ctx_test.go (1)
8006-8027: Consider a more focused benchmark for UpdateParam.The current benchmark measures the entire HTTP request/response cycle (
app.Test(req)includes routing, handler execution, response serialization, etc.), which adds significant overhead unrelated toUpdateParamitself.🔎 More focused benchmark measuring UpdateParam directly
func Benchmark_Ctx_UpdateParam(b *testing.B) { app := New(Config{ CBOREncoder: cbor.Marshal, CBORDecoder: cbor.Unmarshal, }) + c := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) + c.route = &Route{Params: []string{"name", "id"}} + c.values = [maxParams]string{"original", "123"} - app.Get("/user/:name/:id", func(c Ctx) error { - c.UpdateParam("name", "overridden") - return nil - }) - - req, err := http.NewRequest(http.MethodGet, "/user/original/123", http.NoBody) - require.NoError(b, err) - b.ReportAllocs() + b.ResetTimer() for b.Loop() { - resp, err := app.Test(req) - require.NoError(b, err) - require.NoError(b, resp.Body.Close()) + c.UpdateParam("name", "overridden") + c.values[0] = "original" // reset for next iteration } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx.goctx_interface_gen.goctx_test.godocs/api/ctx.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_interface_gen.goctx.goctx_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/api/ctx.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/api/ctx.md
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (1)
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
docs/api/ctx.md
🧬 Code graph analysis (2)
ctx.go (1)
req.go (1)
Params(626-632)
ctx_test.go (3)
ctx_interface_gen.go (1)
Ctx(18-435)req.go (1)
Params(626-632)router.go (1)
Route(47-68)
🔇 Additional comments (2)
ctx.go (1)
386-399: LGTM! Well-implemented defensive programming.The implementation correctly handles the nil router case and safely updates route parameters. The bounds check
i < maxParamsis technically redundant sincec.valuesis already sized atmaxParams, but it's good defensive coding that adds negligible overhead.ctx_test.go (1)
7481-7524: LGTM! Comprehensive test coverage for UpdateParam.The test properly validates:
- Overriding existing route parameters (
name→overridden)- Ignoring updates to non-existing parameters (
existing)- Preserving unmodified parameters (
idremains123)- Safe handling when no route is set (nil router case)
The test structure follows best practices with
t.Parallel()calls and clear subtests.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ctx_test.go (2)
7481-7524: Good coverage for UpdateParam; consider tightening assertions in the handler.The new test does exercise both overriding an existing param and the nil-router/no-op behavior, and
t.Parallel()is correctly used on the test and both subtests as per the test guidelines. As per coding guidelines, this is good.If you want to strengthen the behavioral guarantees a bit more:
- In the
"route_params"subtest handler, you already have access totvia closure. You could explicitly assert that a non-existent param remains empty and that the route param keys are unchanged, e.g.:app.Get("/user/:name/:id", func(c Ctx) error { c.UpdateParam("name", "overridden") c.UpdateParam("existing", "ignored") require.Equal(t, "overridden", c.Params("name")) require.Equal(t, "123", c.Params("id")) // or assert inside the test via JSON decode require.Empty(t, c.Params("existing")) require.Equal(t, []string{"name", "id"}, c.Route().Params) return c.JSON(map[string]any{ "name": c.Params("name"), "id": c.Params("id"), "all": c.Route().Params, }) })
- With those direct assertions in place, you can either keep or drop the
"all"field from the JSON, and therequire.NotContains(t, body, "existing")check becomes purely a safety net instead of the only signal for the ignored param.Not mandatory, but it would make the test more direct about the exact invariants you care about (existing key overridden, unknown key ignored, route metadata unchanged).
8005-8023: Benchmark is focused and consistent; you can drop the per-iteration reset if not needed.The benchmark setup mirrors existing patterns in this file (manual
DefaultCtx+ fixedRoute.Params/values) and gives a focused view onUpdateParamitself.If
UpdateParamdoesn’t depend on the previous value ofc.values[0](which it typically shouldn’t for a simple overwrite-by-name API), you can simplify and make the benchmark slightly cleaner by removing the reset from the hot loop:c := app.AcquireCtx(&fasthttp.RequestCtx{}).(*DefaultCtx) c.values = [maxParams]string{"original", "12345"} c.route = &Route{Params: []string{"name", "id"}} b.ReportAllocs() b.ResetTimer() for b.Loop() { c.UpdateParam("name", "changed") }That way you only measure the call itself, without the extra assignment cost on every iteration.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctx_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
🧬 Code graph analysis (1)
ctx_test.go (3)
ctx_interface_gen.go (1)
Ctx(18-435)req.go (1)
Params(626-632)router.go (1)
Route(47-68)
…ram, simplify benchmark behaviour
|
/gemini summary |
Summary of ChangesThis pull request introduces a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ctx_test.go (1)
8007-8023: Tighten the UpdateParam benchmark setup (optional)Two small nits you may want to address:
- The
CBOREncoder/CBORDecoderconfiguration onNewisn’t exercised byUpdateParam; you can simplify toapp := New()unless you intentionally want CBOR wired in here.- The forcible type assertion
.(*DefaultCtx)is consistent with other benchmarks but currently lacks the//nolint:errcheck,forcetypeassert // not neededcomment used elsewhere in this file; adding it would keep linters quiet and match the existing style.Neither affects correctness of the benchmark; they’re just minor cleanliness tweaks.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ctx_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_test.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
🧬 Code graph analysis (1)
ctx_test.go (3)
ctx_interface_gen.go (1)
Ctx(18-435)req.go (1)
Params(626-632)router.go (1)
Route(47-68)
🔇 Additional comments (1)
ctx_test.go (1)
7481-7525: UpdateParam tests cover core behaviors wellThe two subtests validate the intended semantics: overriding an existing route param, ignoring unknown keys, and leaving Params unchanged when using a context on an app with no routing state. Use of
t.Parallel()is correct for both the parent test and subtests, and the in-handlerrequirechecks give precise guarantees aroundParamsandRoute().Params.
There was a problem hiding this comment.
Code Review
This pull request introduces the UpdateParam feature, which allows for overriding route parameters. The implementation is well-structured and includes comprehensive tests and benchmarks. My review focuses on a correctness issue related to case-sensitivity in UpdateParam that could lead to inconsistent behavior. I've also provided a suggestion to improve the new benchmark by simplifying it and ensuring proper resource cleanup. Overall, this is a great addition to the framework.
|
@gaby Done! 🫡 |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ctx_test.go (1)
7481-7627: Good UpdateParam coverage; add+/ multi-wildcard cases to fully exercise normalizationThe new tests and benchmark nicely cover overriding existing params, ignoring non-existing ones, wildcard
"*"behavior,maxParamsbounds, case sensitivity, and nil-route safety. To fully validate the wildcard normalization logic (*/+→*1/+1) and multiple-wildcard handling, consider adding:
- A
+wildcard case (e.g. route like/files/+andc.UpdateParam("+", "changed")assertingc.Params("+") == "changed"), and- A route with multiple wildcards (e.g.
/files/*/*) verifying updates to the second wildcard viaUpdateParam("*2", ...).This keeps tests aligned with the implementation and docs around unnamed wildcard parameters.
Also applies to: 8110-8129
🧹 Nitpick comments (1)
ctx.go (1)
386-415: UpdateParam behavior looks correct; consider a small branch/alloc optimizationThe implementation now correctly guards on
Matched(), normalizes"*"/"+", respectsCaseSensitive, and avoids out-of-bounds writes whenlen(Route.Params) > maxParams. One optional improvement: avoid re-checkingCaseSensitiveand re-wrappingnameinto[]byteon every iteration by branching once outside the loop and cachingnameBytesfor the case-insensitive path.Possible refactor sketch (same behavior)
func (c *DefaultCtx) UpdateParam(name, value string) { // If no route is matched, there are no parameters to update if !c.Matched() { return } // Normalize wildcard (*) and plus (+) tokens to their internal // representations (*1, +1) used by the router. if name == "*" || name == "+" { name += "1" } - for i, param := range c.route.Params { - // Prevent out-of-bounds access if route params exceed allocated values - if i >= maxParams { - return - } - if c.app.config.CaseSensitive { - if param == name { - c.values[i] = value - return - } - } else if utils.EqualFold(utils.UnsafeBytes(param), utils.UnsafeBytes(name)) { - c.values[i] = value - return - } - } + if c.app.config.CaseSensitive { + for i, param := range c.route.Params { + if i >= maxParams { + return + } + if param == name { + c.values[i] = value + return + } + } + return + } + + nameBytes := utils.UnsafeBytes(name) + for i, param := range c.route.Params { + if i >= maxParams { + return + } + if utils.EqualFold(utils.UnsafeBytes(param), nameBytes) { + c.values[i] = value + return + } + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.goctx_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx.goctx_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Applied to files:
ctx.goctx_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
ctx_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ctx_test.go (1)
7481-7645: Refactor handler assertions out of HTTP handlers in UpdateParam tests; consider a '+' wildcard caseThe new subtests do a nice job covering route params, wildcards, maxParams overflow, case sensitivity, and nil-router behavior, but two points are worth adjusting:
Avoid
require.*inside HTTP handlersIn
route_params,wildcard_params,multi_wildcard_params,case_sensitive, andcase_insensitive, the route handlers callrequire.*directly. Per the existing project guidance, assertions from within HTTP handlers (including net/http-style handlers) are discouraged and enforced by linters; prefer usingtesting.Tmethods or asserting after the request on the test goroutine instead. For example:
- Have handlers write their observable state into the response body/headers (or a channel) and assert with
requirein the outer test function, or- If you really need in-handler checks, switch to
t.Fatalf/t.Errorfstyle checks.This keeps test failures properly associated with the parent test and avoids linter violations. Based on learnings, this repo’s linter specifically flags
requireusage inside handlers.Optional: add explicit
+wildcard coverageYou’ve covered
*and multiple wildcards; for completeness with the implementation that normalizes"+"to"+1", you might add a small subtest such as:t.Run("plus_wildcard_params", func(t *testing.T) { t.Parallel() app := New() app.Get("/files+/+", func(c Ctx) error { c.UpdateParam("+", "changed") // assert via response / test-side checks return c.SendStatus(StatusOK) }, ) req, err := http.NewRequest(http.MethodGet, "/files+/original", http.NoBody) require.NoError(t, err) resp, err := app.Test(req) require.NoError(t, err) defer func() { require.NoError(t, resp.Body.Close()) }() require.Equal(t, StatusOK, resp.StatusCode) })(Exact route shape can be whatever matches your router’s
+semantics.)
🧹 Nitpick comments (1)
ctx_test.go (1)
8127-8147: Benchmark isn’t exercising the main UpdateParam code path (matched flag stays false)In
Benchmark_Ctx_UpdateParam,c.valuesandc.route.Paramsare initialized, butc.matchedis left at its zero value. SinceUpdateParamnow early-returns when!c.Matched(), the benchmark only measures the short-circuit path instead of the param-lookup/update path you likely care about.Set the matched flag before the loop so the benchmark exercises the full implementation:
Proposed benchmark adjustment
func Benchmark_Ctx_UpdateParam(b *testing.B) { app := New() ctx := app.AcquireCtx(&fasthttp.RequestCtx{}) c, ok := ctx.(*DefaultCtx) if !ok { b.Fatal("AcquireCtx did not return *DefaultCtx") } defer app.ReleaseCtx(c) c.values = [maxParams]string{"original", "12345"} c.route = &Route{Params: []string{"name", "id"}} + c.setMatched(true) b.ReportAllocs() b.ResetTimer() for b.Loop() { c.UpdateParam("name", "changed") } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ctx.goctx_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx.goctx_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.go
🧠 Learnings (11)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Applied to files:
ctx.goctx_test.go
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
ctx_test.go
🧬 Code graph analysis (1)
ctx_test.go (2)
req.go (1)
Params(626-632)router.go (1)
Route(47-68)
🔇 Additional comments (1)
ctx.go (1)
386-425: UpdateParam implementation aligns with Params semantics and looks solidThe method correctly:
- Short-circuits when no route is matched via
c.Matched().- Normalizes
"*"/"+"to*1/+1to match internal wildcard naming.- Respects
CaseSensitiveand usesutils.EqualFoldfor the insensitive path.- Defensively guards against
len(route.Params) > maxParamsto avoid touchingc.valuesout of bounds.No changes needed here from a correctness or style standpoint.
|
Hey @gaby |
Doesn't seem to be related to your PR. |
|
@TheAspectDev, go ahead as I’ll address the issue I raised with the other maintainers elsewhere. |
…maximum parameters on router register
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ctx_test.go (1)
8119-8140: Consider asserting the final param value after the benchmark loopTo mirror other benchmarks in this file and catch regressions in
UpdateParam, you can add a quick post‑loop check that the parameter was actually updated; this won’t materially affect timing.Proposed post-loop assertion
func Benchmark_Ctx_UpdateParam(b *testing.B) { @@ c.setMatched(true) b.ReportAllocs() b.ResetTimer() for b.Loop() { c.UpdateParam("name", "changed") } + + // Verify that UpdateParam actually updated the value + require.Equal(b, "changed", c.Params("name")) }router_test.go (1)
11-11: Route param max-exceed test is good; you may want to assert the panic message tooThe new test correctly forces
maxParams+1parameters and ensuresregisterpanics, and thefmtimport is scoped to that usage. If you want the test to lock in the formatted error (count, limit, and path), you could switch torequire.PanicsWithValue(or inspectrecover()inside the closure) and assert that the message contains the expected details.Also applies to: 736-750
ctx.go (1)
386-419: Implementation is correct, but outdated comments should be removed.The core logic is sound:
- Uses
c.Matched()to verify route existence- Correctly normalizes wildcard tokens (
*→*1,+→+1)- Properly handles case-sensitive and case-insensitive parameter matching using
utils.EqualFoldHowever, the comments on lines 402 and 413 stating "Prevent out-of-bounds access if route params exceed allocated values" are now outdated. With the new validation at route registration (router.go:550-552), routes cannot exceed
maxParams, so the iteration overc.route.Paramswill never go out of bounds. These comments should be removed to avoid confusion.🔎 Suggested cleanup
if c.app.config.CaseSensitive { for i, param := range c.route.Params { - // Prevent out-of-bounds access if route params exceed allocated values if param == name { c.values[i] = value return } } return } nameBytes := utils.UnsafeBytes(name) for i, param := range c.route.Params { - // Prevent out-of-bounds access if route params exceed allocated values if utils.EqualFold(utils.UnsafeBytes(param), nameBytes) { c.values[i] = value return } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ctx.goctx_test.gorouter.gorouter_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
ctx_test.gorouter_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
ctx_test.gorouter.goctx.gorouter_test.go
🧠 Learnings (15)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
📚 Learning: 2025-12-07T15:07:23.885Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T15:07:23.885Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.goctx.gorouter_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 2864
File: ctx_test.go:4816-4816
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The `ParamsInt` function's behavior, including handling empty string parameters, will be addressed in future PRs as per ReneWerner87's guidance.
Applied to files:
ctx_test.goctx.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
ctx_test.gorouter_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
ctx_test.go
📚 Learning: 2024-09-25T17:09:03.756Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
ctx_test.go
📚 Learning: 2025-05-08T08:14:37.302Z
Learnt from: mdelapenya
Repo: gofiber/fiber PR: 3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Applied to files:
ctx.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
ctx.go
📚 Learning: 2024-10-16T12:12:30.506Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3170
File: ctx_test.go:1721-1724
Timestamp: 2024-10-16T12:12:30.506Z
Learning: In the Go unit tests in `ctx_test.go`, it is acceptable to use invalid CIDR notation such as `"0.0.0.1/31junk"` for testing purposes.
Applied to files:
ctx.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
router_test.go
🔇 Additional comments (2)
ctx_test.go (1)
7481-7637: Comprehensive UpdateParam test coverage looks solidThe subtests nicely cover overriding vs non‑existing params,
*and+wildcards (including multi‑wildcard), case‑sensitive vs case‑insensitive configs, and nil‑route safety. The structure (t.Parallel on test and subtests, proper body closes) matches existing patterns in this file and should give good confidence in the new behavior.router.go (1)
550-552: LGTM! Defensive max parameter check is well-placed.The validation at route registration ensures fail-fast behavior during startup and prevents runtime issues. The panic is consistent with existing validation panics in the same function and provides a clear error message.
|
4 participants
Description
Allows
Ctx.UpdateParamto override existing route parameters while ignoring non-existing ones, and ensures proper behavior when using a nil router.This PR reflects the mutable req.params in
expressjsand adds corresponding tests and benchmarks to validate correctness and measure performance.Changes introduced
UpdateParam:UpdateParamctx.goandctx_interface_gen.goto satisfy lint rulesc.UpdateParamFixes #2118
Type of change
UpdateParamoverride behavior)🧾 Checklist