Skip to content

New redirect inspection #3377

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

zeonzip
Copy link

@zeonzip zeonzip commented Jun 18, 2025

Motivation

This PR was made in response to the issue #3365.

Highlights that in axum most responses are strongly typed (and therefore inspectable) and is turned into HTTP at the IntoResponse stage.

Redirect does not have any methods to inspect StatusCode or the URI after being constructed, and constructs the http headers when created.

Solution

This PR adds testability and inspection of Redirect.
It adds status_code()which returns the current status code of the Redirect, and location() which returns the result of parsing the Http Header to a &str.
It also adds tests using the new inspection methods.

This PR should also be iterated on, but currently doesn't have any breaking changes to the internals of Redirect, but only adds new methods. We could still make these changes (discussed in the issue above):

  • Instead of Redirect constructing the HeaderValue when Redirect is constructed, it could be constructed when Redirect is made into a response in IntoResponse.
  • Make redirect a enum to ensure that StatusCode isn't invalid.

@zeonzip zeonzip mentioned this pull request Jun 18, 2025
1 task
@zeonzip
Copy link
Author

zeonzip commented Jun 18, 2025

Could also .expect the location() and return the &str since it's guaranteed to be a valid UTF-8 string. Also a better way to store the URI could be to store the raw &str given, and turn the URI into a header in IntoResponse.

@zeonzip
Copy link
Author

zeonzip commented Jun 18, 2025

(suggested in the issue above) We could introduce a RedirectKind which means we don't have to assert the StatusCode, and also turn that into the valid status code when using IntoResponse meaning we can eliminate any code that may in any way panic. And also we are going towards 0.9, and since breaking changes were allowed, it's a better time to do it before 0.9

@jplatte
Copy link
Member

jplatte commented Jun 24, 2025

I think we should just update the location field to be a String. I don't see what good the eager conversion to HeaderValue (and panic if that fails) does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants