En/http redirects #1640
En/http redirects #1640
Conversation
|
@Umang01-hash , Can we also check and add tests for url with path and query params as well as request Bodies, headers etc.....get handled successfully through the redirect. I feel that is not the case right now. |
| app.GET("/old-page", func(ctx *gofr.Context) (any, error) { | ||
| // Redirect to a new URL with 301 Moved Permanently status | ||
| return ctx.Redirect("/new-page", http.StatusMovedPermanently) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Let's not have a redirect method. We can just expose response of redirect Type ..... Also, we must ensure the traces are in hierarchy.
There was a problem hiding this comment.
I agree. Till now, all responses in Gofr are a type which is returned. ctx.Redirect is coming from other framework's behaviour where things like ctx.JSON etc are method. Also, we do not ask people to set manual status codes - but, we automatically decide. We should do the same and use a proper status code. The usecase in APIs for redirects are never going to be 301 - I think.
There was a problem hiding this comment.
In redirects, on the client-side :
- For 301, 302, 303 → body is usually dropped; method becomes GET
- For 307, 308 → body is preserved; method is unchanged
I think instead of setting internally, we should allow mentioning it in the response itself so that the client browser can process the urls accordingly.
@vikash , @aryanmehrotra — would love to hear your thoughts on this.
There was a problem hiding this comment.
I think, we will not find usecase for 307,308. Most of the usecase in API context can actually be of 303 like payment flow etc.
@Umang01-hash, Can we add test for these cases ? I feel this implementation can fall short in fitting these usecases. |
|
|
||
| // Set up redirect with specific URL and status code | ||
| redirectURL := "/new-location" | ||
| statusCode := http.StatusFound // 301 |
There was a problem hiding this comment.
| statusCode := http.StatusFound // 301 | |
| statusCode := http.StatusFound // 302 |
|
|
||
| func TestResponder_RedirectResponse(t *testing.T) { | ||
| recorder := httptest.NewRecorder() | ||
| r := NewResponder(recorder, http.MethodGet) |
There was a problem hiding this comment.
You should also test at least POST and HEAD
There was a problem hiding this comment.
@Umang01-hash cc @coolwednesday
I got not feedback on these
- En/http redirects #1640 (comment)
- En/http redirects #1640 (comment)
- En/http redirects #1640 (comment)
so I'm moving my comment to "request changes"
Pull Request Template
Description:
Example:
Additional Information:
Checklist:
goimportandgolangci-lint.Thank you for your contribution!