tower-rs / tower-http

HTTP specific Tower utilities.
675 stars 156 forks source link

Add `redirect_path_prefix` option #486

Open maxi0604 opened 4 months ago

maxi0604 commented 4 months ago

Motivation

Currently ServeDir assumes that the files are served from /, which breaks trailing slash redirects in some configurations like a reverse proxy that redirects /somePath/* to the ServeDir server or a scenario where axum::Router::nest_service is used.

Solution

This allows the user to pass in a string that is prepended to the redirect path.

edit(jplatte): Closes #413

jplatte commented 3 months ago

Hey, thanks for the PR! I think we should have something like this (as indicated in my last comment on #413), but.. naming is hard. I've been considering something like .prefix("/static", PrefixMode::PrependOnRedirect) (where the other PrefixMode would be to strip the prefix on incoming requests before filesystem lookup and prepend it on redirects), but this would require users to import that enum, and I still don't know what name to give the other prefix mode 😄

I guess since axum .nest() is the most important use case and it's easy enough to implement something like it elsewhere, we could start with just this "mode", i.e. a function like what you added but with a clearer name.

So if you don't care much about the naming or what exactly the API looks like, I would recommend you rename prepend_path to redirect_path_prefix. Otherwise, I'm curious to hear your thoughts!

maxi0604 commented 3 months ago

Thanks for the review

I guess since axum .nest() is the most important use case and it's easy enough to implement something like it elsewhere, we could start with just this "mode", i.e. a function like what you added but with a clearer name. I think this one solves the more pressing issue with the redirects.

You could use a workaround like putting the files at base-folder/prefix/... if you are (somehow) getting requests for paths with the prefix; but as suggested in the issue, fixing the trailing slash redirects would require intercepting the response.

A separate option to strip an arbitrary prefix from the file path could also be added if that workaround doesn't work for someone. (Maybe strip_path_prefix) This would have to be explained in the documentation but ultimately provides more flexibility (With the two options combined, you could replace a prefix with another one.)

So if you don't care much about the naming or what exactly the API looks like, I would recommend you rename prepend_path to redirect_path_prefix. Otherwise, I'm curious to hear your thoughts!

I've renamed it to redirect_path_prefix, that does sound like a more reasonable name. I think this already solves an immediate issue while more flexibility could be added later.

maxi0604 commented 3 months ago

I don't know why the cargo-hack test is failing, it seems unrelated to me.