♻️ refactor: Change c.Redirect() default status#3415
♻️ refactor: Change c.Redirect() default status#3415ReneWerner87 merged 7 commits intogofiber:mainfrom
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 from 302 (Found) to 303 (See Other) throughout the codebase and documentation. This includes updating comments, documentation, tests, and example usages to reflect the new default. No changes were made to method signatures or core redirect logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FiberApp
participant RedirectHandler
Client->>FiberApp: HTTP Request (any method)
FiberApp->>RedirectHandler: Route match
RedirectHandler->>FiberApp: c.Redirect("/some/path")
FiberApp-->>Client: HTTP 303 See Other with Location header
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 🪧 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 (
|
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())
|
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
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.97% 84.07% +0.09%
==========================================
Files 119 119
Lines 11897 11897
==========================================
+ Hits 9991 10002 +11
+ Misses 1479 1469 -10
+ Partials 427 426 -1
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:
|
gaby
left a comment
There was a problem hiding this comment.
Use fiber.StatusSeeOther instead of "302"
middleware/expvar/expvar_test.go
Outdated
| resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/debug/vars/303", nil)) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 302, resp.StatusCode) | ||
| require.Equal(t, 303, resp.StatusCode) |
|
@andradei can you fix the review hints |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/whats_new.md (1)
837-838: Correct grammar in caching descriptionThe phrase “define a custom conditions” is grammatically incorrect. Consider one of the following revisions:
- ... allowing you to define a custom conditions for invalidating cache entries. + ... allowing you to define custom conditions for invalidating cache entries.or
- ... allowing you to define a custom conditions for invalidating cache entries. + ... allowing you to define a custom condition for invalidating cache entries.🧰 Tools
🪛 LanguageTool
[grammar] ~837-~837: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/middleware/redirect.md(1 hunks)docs/whats_new.md(5 hunks)middleware/expvar/expvar_test.go(1 hunks)redirect_test.go(17 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/middleware/redirect.md
🚧 Files skipped from review as they are similar to previous changes (2)
- middleware/expvar/expvar_test.go
- redirect_test.go
🧰 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`.
🪛 LanguageTool
docs/whats_new.md
[grammar] ~837-~837: 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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit (1.23.x, macos-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
- GitHub Check: Compare
🔇 Additional comments (3)
docs/whats_new.md (3)
601-608: Approve updated redirect behavior documentationThe new "Changed behavior" section clearly communicates the switch from
302 Foundto303 See Otherand aligns with the code changes and cross-browser expectations.
300-303: Skip formatting-only changeThis removal of trailing whitespace in the
app.Useexample does not affect the content or functionality.
957-959: Skip formatting-only changeThis whitespace cleanup in the logger middleware example has no impact on the documentation’s meaning.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/whats_new.md (1)
259-259: Removed extraneous '+' in the route signature example.
The stray plus sign in this code snippet has been removed, improving consistency with the surrounding diff blocks.
🧹 Nitpick comments (1)
docs/whats_new.md (1)
849-849: Grammar: remove extraneous 'a' before plural noun.
In the caching middleware description, update “define a custom conditions” to “define custom conditions” (or “define a custom condition”).🧰 Tools
🪛 LanguageTool
[grammar] ~849-~849: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/whats_new.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/whats_new.md
[grammar] ~849-~849: 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)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Compare
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (1)
docs/whats_new.md (1)
601-608: Validate the new “Changed behavior” section for redirects.
This section succinctly explains the default status change from302 Foundto303 See Other. Please verify that all related documentation—particularlydocs/api/redirect.mdand any code examples—are updated to reflect this new default.
* Set default redirect response status to 303 SeeOther 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. * Add tests to Redirect default status change. * Update docs. * Fix remaining tests to reflect redirect 303 status as the new default. Reflect that in docs/whats_new.md * Update redirect_test.go * Fix code review hints --------- Co-authored-by: Juan Calderon-Perez <835733+gaby@users.noreply.github.com> Co-authored-by: René <rene@gofiber.io>
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.