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 ServeDir::fallback #243

Merged
merged 12 commits into from Apr 24, 2022
Merged

Add ServeDir::fallback #243

merged 12 commits into from Apr 24, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Apr 4, 2022

Fixes #240

Example usage:

let service = ServeDir::new("assets")
    // respond with `index.html` for missing files
    .fallback(ServeFile::new("assets/index.html"));

This will call ServeFile if the file is missing.

You can also achieve this using combinators from tower::ServiceExt but several have asked for this so probably makes sense to add.

Note this is different from axum_extra::routing::SpaRouter which requires assets to be nested (for example under /assets/*). This doesn't require that and thus allows serving assets at the top level (i.e. GET /style.css or smth like that).

TODO

  • Fix remaining todos in the code
  • Make sure its compatible with axum
  • Think about setting status codes for fallback responses
  • Changelog

@jplatte
Copy link
Collaborator

jplatte commented Apr 4, 2022

This also allows showing a custom 404 page for a ServeDir, right? I think that really calls for a convenience API to override ServeFiles success status code.

@davidpdrsn davidpdrsn added the breaking change A PR that makes a breaking change. label Apr 4, 2022
@jplatte jplatte marked this pull request as ready for review April 4, 2022 21:32
@jplatte
Copy link
Collaborator

jplatte commented Apr 4, 2022

Oops

@davidpdrsn
Copy link
Member Author

This also allows showing a custom 404 page for a ServeDir, right?

Yep it does!

I think that really calls for a convenience API to override ServeFiles success status code.

Hm what do you mean? Like not responding with 200 Ok if the file is found and served?

@davidpdrsn davidpdrsn marked this pull request as draft April 4, 2022 21:33
@davidpdrsn
Copy link
Member Author

hehe not quite ready for review yet :P

@jplatte
Copy link
Collaborator

jplatte commented Apr 4, 2022

Misclicked in mobile UI, not sure how to change back 😅

Edit: Ah you did already.

@davidpdrsn
Copy link
Member Author

I've fixed it :)

@jplatte
Copy link
Collaborator

jplatte commented Apr 4, 2022

I think that really calls for a convenience API to override ServeFiles success status code.

Hm what do you mean? Like not responding with 200 Ok if the file is found and served?

Yeah, since you'd want the custom not-found page to still have the 404 status code even when served from an on-disk file successfully.

@davidpdrsn
Copy link
Member Author

Ahh I see. So like .fallback(ServeFile::new("not_found.html").success_status(StatusCode::NOT_FOUND)) or something along those lines 🤔 Have to think about this.

@davidpdrsn
Copy link
Member Author

Maybe

ServeDir::new("...").fallback_with_status(ServeFile::new("not_found.html"), StatusCode::NOT_FOUND)

That way we wouldn't need anything new in ServeFile, or whatever other service you might be using 🤔

@jplatte
Copy link
Collaborator

jplatte commented Apr 5, 2022

Maybe

ServeDir::new("...").fallback_with_status(ServeFile::new("not_found.html"), StatusCode::NOT_FOUND)

That way we wouldn't need anything new in ServeFile, or whatever other service you might be using 🤔

I guess the question there would be when do you apply the custom status code? Probably if the status code from the inner service is anything 2xx?

Also, if this is not attached to ServeFile, it should probably be its own service adapter, and then maybe instead of .fallback_with_status(inner_svc, status_code) as an alias for .fallback(inner_svc.layer(SetStatus::layer(status_code)) we should have .not_found_service(inner_svc) as an alias for .fallback(inner_svc.layer(SetStatus::layer(StatusCode::NOT_FOUND))? That would provide full flexibility using the adapter, and a very succinct helper method for the common use case.

@davidpdrsn
Copy link
Member Author

I'll try and find some time to work on this soon but might just make a breaking release next week to get #237 out regardless if this has landed or not.

@davidpdrsn davidpdrsn mentioned this pull request Apr 24, 2022
@davidpdrsn
Copy link
Member Author

Made a PR to add a SetStatus middleware #248. When that is merged I'll update this to include helpers that also set the status from the fallback to 404. Once thats done I think we can merge this!

I don't think a more general fallback mechanism is possible since it requires cloning the request (to call the fallback). ServeDir and ServeFile doesn't have that issue since they don't require the body or extensions so they're a special case.

type Response = Response<ResponseBody>;
type Error = io::Error;
type Future = ResponseFuture;
type Future = ResponseFuture<ReqBody, F>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new type params makes this a breaking change.

@davidpdrsn davidpdrsn marked this pull request as ready for review April 24, 2022 13:02
@davidpdrsn
Copy link
Member Author

This is finally ready for review!

tower-http/src/services/fs/serve_dir.rs Outdated Show resolved Hide resolved
tower-http/src/services/fs/serve_dir.rs Outdated Show resolved Hide resolved
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@davidpdrsn davidpdrsn requested a review from jplatte April 24, 2022 13:31
@davidpdrsn davidpdrsn requested a review from jplatte April 24, 2022 13:36
tower-http/src/services/fs/serve_dir.rs Outdated Show resolved Hide resolved
tower-http/src/services/fs/serve_dir.rs Show resolved Hide resolved
}

fn call(&mut self, req: Request<ReqBody>) -> Self::Future {
let mut full_path = match self.variant.full_path(&self.base, req.uri().path()) {
// `ServeDir` doesn't care about the request body but the fallback might. So move out the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All methods that are reasonable for asset routes don't have a body anyways, right? By the way, does ServeDir still serve files for non-GET requests? I think it did that at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All methods that are reasonable for asset routes don't have a body anyways, right?

Yes. At least I can't think of a use case for serving a static file and having a request body.

By the way, does ServeDir still serve files for non-GET requests? I think it did that at some point.

It still accepts all methods. Perhaps we should change that to respond with 405 for non-GET requests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is proper handling of HEAD now, right? That should probably stay. Not sure about OPTIONS, probably okay to have it return 405 if the user doesn't have a cors middleware set up.

Copy link
Collaborator

@jplatte jplatte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from latest comments, looks good!

@davidpdrsn davidpdrsn enabled auto-merge (squash) April 24, 2022 16:17
@davidpdrsn davidpdrsn merged commit 1737566 into master Apr 24, 2022
@davidpdrsn davidpdrsn deleted the serve-dir-fallback branch April 24, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A PR that makes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add special support for single-page applications which does its own routing
2 participants