♻️ refactor: Change c.Redirect() default status #3415
Conversation
Closes gofiber#3405 In some browsers, redirect status 302 Found sometimes is used to change the HTTP verb of the response from what the user set to what was used in the request. Changing to 303 SeeOther in the default works more like expected: it defaults to GET and can be overriden by the user.
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe changes update the default HTTP status code for redirects in the Fiber framework from 302 (Found) to 303 (See Other). This update is reflected in the documentation, code comments, struct field defaults, and all related unit and benchmark tests. The method signatures and core logic remain unchanged, with only the default status code and associated documentation/comments being modified to use 303 instead of 302. Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🪧 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 (
|
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the default redirection status code from 302 Found to 303 See Other to prevent potential HTTP verb change issues in some browsers. Key changes include:
- Updating all redirect tests to expect 303 instead of 302.
- Changing the default status value in redirect.go and ensuring it is reset accordingly.
- Revising documentation in docs/api/redirect.md to reflect the default change.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| redirect_test.go | Updated test assertions to check for 303 status code. |
| redirect.go | Changed default status to StatusSeeOther and updated reset. |
| docs/api/redirect.md | Updated documentation to reference 303 See Other. |
Comments suppressed due to low confidence (1)
redirect_test.go:25
- [nitpick] Consider using the defined StatusSeeOther constant instead of the literal 303 in the test assertions to improve consistency with the rest of the codebase.
require.Equal(t, 303, c.Response().StatusCode())
gaby
changed the title
|
Tests passed on my linux host with no issues, not sure what caused the error in |
|
@andradei How are you running the tests? What's your command? |
|
@gaby just |
…. Reflect that in docs/whats_new.md
![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
🔭 Outside diff range comments (1)
middleware/redirect/config.go (1)
28-34:⚠️ Potential issueInconsistent default status code: comment and code mismatch.
The comment specifies "Default: 303 See Other", but
ConfigDefault.StatusCoderemains set tofiber.StatusFound(302). This discrepancy needs to be resolved by updating the default value tofiber.StatusSeeOther(303).Suggested diff:
var ConfigDefault = Config{ - StatusCode: fiber.StatusFound, + StatusCode: fiber.StatusSeeOther, }
🧹 Nitpick comments (2)
docs/whats_new.md (1)
832-834: Grammar refinement: remove stray article
In “allowing you to define a custom conditions”, drop the “a” before the plural noun “conditions” to read “…define custom conditions…”.🧰 Tools
🪛 LanguageTool
[grammar] ~832-~832: Do not use the singular ‘a’ before the plural noun ‘conditions’.
Context: ...ache management, allowing you to define a custom conditions for invalidating cache entries. Additio...(VB_A_JJ_NNS)
redirect_test.go (1)
25-27: Tests correctly expect 303 See Other but use magic number
Allrequire.Equal(..., 303, ...)assertions align with the new default redirect code. For clarity and maintainability, consider replacing the literal303with thefiber.StatusSeeOtherconstant.Also applies to: 42-44, 71-73, 91-93, 115-117, 130-132, 149-151, 164-166, 185-187, 214-217, 236-238, 274-276, 309-311, 353-355, 484-486, 511-514, 539-541
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ctx.go(1 hunks)ctx_interface_gen.go(1 hunks)docs/api/redirect.md(5 hunks)docs/extra/internal.md(1 hunks)docs/whats_new.md(5 hunks)middleware/expvar/expvar_test.go(1 hunks)middleware/pprof/pprof_test.go(2 hunks)middleware/redirect/config.go(1 hunks)redirect.go(3 hunks)redirect_test.go(17 hunks)
🧰 Additional context used
🧠 Learnings (1)
docs/whats_new.md (1)
Learnt from: ckoch786
PR: gofiber/fiber#3230
File: docs/whats_new.md:944-951
Timestamp: 2024-12-15T19:56:45.935Z
Learning: Detailed usage examples and explanations for new methods like `RemoveRoute` and `RemoveRouteByName` are documented in `docs/api/app.md`, so it's unnecessary to duplicate them in `docs/whats_new.md`.
🧬 Code Graph Analysis (3)
middleware/expvar/expvar_test.go (1)
constants.go (1)
MethodGet(5-5)
middleware/pprof/pprof_test.go (1)
constants.go (2)
MethodGet(5-5)StatusSeeOther(68-68)
redirect.go (1)
constants.go (1)
StatusSeeOther(68-68)
🪛 LanguageTool
docs/whats_new.md
[grammar] ~832-~832: Do not use the singular ‘a’ before the plural noun ‘conditions’.
Context: ...ache management, allowing you to define a custom conditions for invalidating cache entries. Additio...
(VB_A_JJ_NNS)
🔇 Additional comments (13)
middleware/expvar/expvar_test.go (1)
84-84: Test expectations updated to match new default redirect status.The test for
/debug/vars/303now correctly assertsresp.StatusCode == 303, aligning with the change from 302 to 303.Also applies to: 86-86
docs/extra/internal.md (1)
346-346: Documentation updated to reflect new default redirect status.The description now mentions "defaulting to 303 See Other", which is consistent with the framework-wide update.
ctx.go (1)
1316-1317: Verify implementation aligns with updated comment.The comment for
DefaultCtx.Redirect()now states the default status is 303 See Other. Please confirm thatAcquireRedirect()(inredirect.go) initializesRedirect.StatusCodetofiber.StatusSeeOtherrather than the previous 302.ctx_interface_gen.go (1)
254-255: Interface comment updated: validate underlying behavior.The generated
Ctx.Redirect()interface documentation now states the default status is 303 See Other. Ensure that the concrete implementation honors this new default status code.middleware/pprof/pprof_test.go (1)
156-159: Tests updated to 303 See Other as expected default redirect status
Testing for/debug/pprof/303now correctly assertsfiber.StatusSeeOther. Matches the framework change.docs/api/redirect.md (3)
17-19: Documentation default status updated to 303 See Other
The info block now correctly indicates that when no status is specified, redirects default to 303 See Other.
37-44: Example comments reflect new default redirect code
The inline examples under “More examples” have been updated from 302 to 303, aligning sample output with the new default.
94-96: Back method documentation default adjusted
TheBacksection’s info box now correctly shows 303 See Other as the default status.docs/whats_new.md (1)
580-582: Release notes updated for default redirect status change
The “Redirect” section now mentions the default status change from 302 Found to 303 See Other for more predictable behavior.redirect.go (4)
19-23: Default redirect status updated in pool initializer
Thesync.Poolnow instantiatesRedirectwithStatusSeeOther(303) instead of 302, aligning with the new default.
93-97: Reset behavior updated to new default status
Therelease()method correctly resetsr.statustoStatusSeeOther(303), ensuring reused instances start with the updated default.
62-65: Struct comment reflects new default status
The comment on thestatusfield now accurately documents the default as303 StatusSeeOther.
99-102: Status() comment updated to reflect new default
The doc comment forStatus()now states defaulting to 303 See Other when unset.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3415 +/- ##
==========================================
+ Coverage 83.87% 83.93% +0.06%
==========================================
Files 119 119
Lines 11892 11892
==========================================
+ Hits 9974 9982 +8
+ Misses 1488 1480 -8
Partials 430 430
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:
|
| @@ -577,6 +577,9 @@ In this example, the `Bind` method is used to bind the request body to the `User | |||
|
|
|||
| Fiber v3 enhances the redirect functionality by introducing new methods and improving existing ones. The new redirect methods provide more flexibility and control over the redirection process. | |||
|
|
|||
| The default response status is changes from 302 Found to 303 See Other for more | |||
There was a problem hiding this comment.
This statement needs work. It should be a separate section.
| @@ -256,7 +256,7 @@ The route method is now like [`Express`](https://expressjs.com/de/api.html#app.r | |||
|
|
|||
| ```diff | |||
| - Route(prefix string, fn func(router Router), name ...string) Router | |||
| + Route(path string) Register | |||
| require.NoError(t, err) | ||
| require.Equal(t, 302, resp.StatusCode) | ||
| require.Equal(t, 303, resp.StatusCode) |
|
|
||
| err := c.Redirect().To("http://default.com") | ||
| require.NoError(t, err) | ||
| require.Equal(t, 302, c.Response().StatusCode()) | ||
| require.Equal(t, 303, c.Response().StatusCode()) |
| @@ -39,7 +40,7 @@ func Test_Redirect_To_WithFlashMessages(t *testing.T) { | |||
|
|
|||
| err := c.Redirect().With("success", "2").With("success", "1").With("message", "test", 2).To("http://example.com") | |||
| require.NoError(t, err) | |||
| require.Equal(t, 302, c.Response().StatusCode()) | |||
| require.Equal(t, 303, c.Response().StatusCode()) | |||
| @@ -68,7 +69,7 @@ func Test_Redirect_Route_WithParams(t *testing.T) { | |||
| }, | |||
| }) | |||
| require.NoError(t, err) | |||
| require.Equal(t, 302, c.Response().StatusCode()) | |||
| require.Equal(t, 303, c.Response().StatusCode()) | |||
Fixes #3405
In some browsers, redirect status 302 Found sometimes is used to change the HTTP verb of the response from what the user set to what was used in the request. Changing to 303 SeeOther in the default works more like expected: it defaults to GET and can be overriden by the user.