-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add FixNestedRedirect
middleware
#2230
base: main
Are you sure you want to change the base?
Conversation
This adds `FixNestedRedirectLayer` which uses `NestedPath` to change redirects from nested services to include the path they're nested at. Fixes #1731
Wouldn't it be better to fix this in tower-http? |
That would require moving axum's
|
Converting to draft because I just remember that this (and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit surprised this is in axum
as opposed to axum-extra
. There seem to be quite a few design choices to be made, for instance I would not have thought of including FixNestedRedirectOptOut
, but I was wondering whether there could be problems with people applying the middleware multiple times redundantly (more / less nested); if we wanted to solve this having extract
remove the NestedPath
or insert some other extension do tell that the middleware ran already could work?
@@ -45,6 +45,13 @@ impl NestedPath { | |||
pub fn as_str(&self) -> &str { | |||
&self.0 | |||
} | |||
|
|||
pub(crate) fn extract(parts: &mut Parts) -> Result<Self, NestedPathRejection> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub(crate) fn extract(parts: &mut Parts) -> Result<Self, NestedPathRejection> { | |
pub(crate) fn extract(parts: &Parts) -> Result<Self, NestedPathRejection> { |
return Some(()); | ||
} | ||
|
||
let nested_path = nested_path?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could be done earlier.
I wouldn't add NestedPath to tower-http, but give If |
Yeah I considered putting it in axum-extra. That probably is the better path. I'll move it.
I think I'll move forward with the path in this PR and then see what happens as people use it and give feedback. |
This adds
FixNestedRedirectLayer
which usesNestedPath
to change redirects from nested services to include the path they're nested at.Fixes #1731