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

Add redirect_path_prefix option #486

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

Conversation

maxi0604
Copy link

@maxi0604 maxi0604 commented Apr 27, 2024

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
Copy link
Collaborator

jplatte commented May 2, 2024

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
Copy link
Author

maxi0604 commented May 2, 2024

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 maxi0604 changed the title Add prepend_path option Add redirect_path_prefix option May 2, 2024
@maxi0604
Copy link
Author

maxi0604 commented May 2, 2024

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

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.

ServeDir does not redirect correctly for directories without trailing / when nested
2 participants