-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
🐛 [Bug]: c.Redirect doesn't respect c.Method #3405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
If I make the change below, it works. Is this a bug? func putHandler(c *fiber.Ctx) error {
// ...
c.Redirect("/get", fiber.StatusSeeOther) // <---- this change
} |
Is this really a bug? |
What does net/http do? |
here https://cs.opensource.google/go/go/+/refs/tags/go1.24.2:src/net/http/server.go;l=2416-2425 @coderabbitai What does net/http do? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 30227 🏁 Script executed:
Length of output: 2184 🏁 Script executed:
Length of output: 324 @ReneWerner87 Looking at Go's standard library ( The func Redirect(w ResponseWriter, r *Request, url string, code int) {
// Redirect logic here
// ...
} Go's In contrast, Fiber defaults to status code 302 (Found) for redirects when no status code is specified: func (r *Redirect) release() {
r.status = 302
r.messages = r.messages[:0]
r.c = nil
} This explains the issue - browsers interpret 302 inconsistently. Some preserve the original method (like PUT), while others change to GET. Using 303 (See Other) explicitly tells the browser to use GET for the new request, which is why changing to For 307 (Temporary Redirect) and 308 (Permanent Redirect), as you mentioned, browsers must preserve the original HTTP method. |
@coderabbitai should we change our behavior for the method? would that be better for the consumers? or is the extension of the documentation better for that? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 9081 🏁 Script executed:
Length of output: 6402 @ReneWerner87 After analyzing Fiber's redirect implementation, I believe you have two good options:
My recommendation is a hybrid approach:
This approach preserves backward compatibility while giving developers clear options for specific use cases and better guidance. |
@coderabbitai does not like helper methods for v2 |
Tip For best results, initiate chat on the files or code changes. A summary of the edits CodeRabbit can apply:
A plan of each step that CodeRabbit will take in order to execute the edits for each file (with inline rough draft snippets): ✏️ docs/api/redirect.md
:::info
If **not** specified, status defaults to **302 Found**.
✏️ docs/api/redirect.md
:::info
If **not** specified, status defaults to **302 Found**.
➕ docs/api/redirect.md
## Future Improvements (v3)
➕ docs/guide/routing.mdInsert a new section with the heading
at the end of the file. Add the following documentation content:
## Redirect Behavior
Ensure that the new section integrates with the rest of the document's style and formatting. Review the formatted output in preview mode to confirm consistency and readability. ➕ docs/extra/faq.mdAppend the following FAQ entry to the end of the file:
## How does Fiber handle HTTP redirects and what should I be aware of?
✏️ docs/api/redirect_best_practices.mdAdd the following markdown sections at the end of the file.
## Redirect Best Practices in v2
Review the updated sections and ensure that any related guidelines in the documentation index are updated accordingly. A summary of the context that CodeRabbit is considering across the codebase:
|
Changing default to 303 (at least on v3) and adding warnings in the docs about the 302 default is a satisfactory solution, I think. |
@ReneWerner87 I can work on this. |
Would be nice V2: docs update |
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.
Two PRs created:
|
I'm confused as to why we are changing all the places from 302 to 303? They are very different things, see https://en.m.wikipedia.org/wiki/HTTP_302 |
Bug Description
Calling
c.Method("GET")
beforec.Redirect("/my/route")
doesn't set the method andc.Redirect
still uses the method that the handler was originally called with.How to Reproduce
Steps to reproduce the behavior:
app.Put("/put", putHandler)
app.Get("/get", getHandler)
putHandler
havec.Method("GET"); return c.Redirect("/get")
/put
with "PUT" HTTP method/verbGET /get
butPUT /get
despite method being set in item 3 aboveExpected Behavior
c.Method("GET")
is respected and overrides the method/verb of the original request (PUT, in this example).Fiber Version
v2.52.6
Code Snippet (optional)
Checklist:
The text was updated successfully, but these errors were encountered: