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

Allow HEAD requests to be served by fs::dir() filter #835

Merged
merged 1 commit into from
May 3, 2021

Conversation

zenria
Copy link
Contributor

@zenria zenria commented Apr 19, 2021

This is a fix for feature request #834

@joseluisq
Copy link
Contributor

How about the empty body for HEAD responses? https://tools.ietf.org/html/rfc7231#section-4.3.2

@zenria
Copy link
Contributor Author

zenria commented Apr 26, 2021

The underlying HTTP server (not sure if it's hyper or tower) does not sent the body when handling HEAD requests.

This has already been done in the fs::file() filter:

warp/src/filters/fs.rs

Lines 48 to 57 in 4a03710

pub fn file(path: impl Into<PathBuf>) -> impl FilterClone<Extract = One<File>, Error = Rejection> {
let path = Arc::new(path.into());
crate::any()
.map(move || {
tracing::trace!("file: {:?}", path);
ArcPath(path.clone())
})
.and(conditionals())
.and_then(|path, conditionals| file_reply(path, conditionals))
}

@joseluisq
Copy link
Contributor

joseluisq commented Apr 26, 2021

Great.
Another question does the file filter need HEAD method support too or not?

@zenria
Copy link
Contributor Author

zenria commented Apr 26, 2021

I think, yes it needs it, but this is already working due to the any() filter.

I didn't want to use the any() filter for the fs:dir() as it appears cumbersome to respond to POST or PUT requests on static files. So I think we should also disable this on the fs:file() filter and use only get+head filter

@joseluisq
Copy link
Contributor

I think, yes it needs it, but this is already working due to the any() filter.

Do you mean this is working for your addition or do you mean this file filter is already working that's why you don't want to break someone else code?

But yeah I'm also wondering in what cases we want to support fs::file() with any method.

@zenria
Copy link
Contributor Author

zenria commented Apr 26, 2021

I think, yes it needs it, but this is already working due to the any() filter.

Do you mean this is working for your addition or do you mean this file filter is already working that's why you don't want to break someone else code?

the second option: I did not want to break someone else code!

But yeah I'm also wondering in what cases we want to support fs::file() with any method.

:-)

@jxs jxs merged commit bf8bfc4 into seanmonstar:master May 3, 2021
@jxs
Copy link
Collaborator

jxs commented May 4, 2021

thanks @zenria! and thanks @joseluisq for the reviewing :)

The underlying HTTP server (not sure if it's hyper or tower) does not sent the body when handling HEAD requests.

yeah it's hyper, see here

@amousset
Copy link

amousset commented Jan 6, 2022

FYI this change actually broke some code when updating from 0.3.1 to 0.3.2. We had a custom handler for HEAD but relied on fs::dir() for GET requests (something probably quite unusual I admit :) ). Adding a method::get() filter before the fs::dir() fixed the problem (and I think serving HEAD too makes more sense).

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.

4 participants