Skip to content
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

switch from 'redirect:' prefix to ResponseEntity for redirecting UI index #1104

Merged

Conversation

d3ns0n
Copy link
Contributor

@d3ns0n d3ns0n commented Mar 13, 2021

fixes #1100

@d3ns0n
Copy link
Contributor Author

d3ns0n commented Mar 13, 2021

I would like to add some tests for this, but I didn't manage to succeed. The existing tests, that check the Location header e.g. test.org.springdoc.ui.app1.SpringDocRedirectDefaultTest, already check for a relative path but I don't understand why these tests worked before. I suspect that it has something to do with how @SpringBootTest is spinning up the application server during the tests. Feedback and Suggestions are welcome as I would really like to make sure that the Location header is set properly.

@bnasslahsen
Copy link
Contributor

@d3ns0n,

Thank for your contribution.
I would prefer also the switch from redirect: prefix.

The tricky issue you had reported may need better integration tests.

As far as the existing tests are passing, i will accept PR.

@bnasslahsen bnasslahsen merged commit 4ad346e into springdoc:master Mar 14, 2021
@d3ns0n
Copy link
Contributor Author

d3ns0n commented Mar 15, 2021

I was wondering if HTTP 302 is the proper response code as springdoc-openapi-webflux-ui returns 307 for the same use case. I found this on MDN

The only difference between 307 and 302 is that 307 guarantees that the method and the body will not be changed when the redirected request is made. With 302, some old clients were incorrectly changing the method to GET: the behavior with non-GET methods and 302 is then unpredictable on the Web, whereas the behavior with 307 is predictable. For GET requests, their behavior is identical. -- https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307

So it wouldn't really change anything in this case since we only allow GET requests, but maybe we should make the return code consistent. What do you think?

@bnasslahsen
Copy link
Contributor

@d3ns0n,

I had the same thoughts. I think http 302 is much better.
About making the change consistent, i also identified this action, but if you can propose an additional PR for webflux as well, it will be great!

@d3ns0n d3ns0n deleted the relative-redirect-for-index-page branch March 15, 2021 08:51
@d3ns0n
Copy link
Contributor Author

d3ns0n commented Mar 15, 2021

I will propose a PR later tonight

This was referenced Mar 16, 2021
@bnasslahsen bnasslahsen added the enhancement New feature or request label Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants