ServeDir & ServeFile last modified headers handling #145

zenria · 2021-09-17T15:45:55Z

~~Note: This PR also includes #137, I will modify it when it will be merged~~

Motivation

ServeDir & ServeFile should respect http caching best practice by setting Last-Modified response header and handling If-Modified_since & If-Unmodified-Since requests headers.

Solution

The Last-Modified header is added on responses of ServeFile & ServeDir.

If-Modified_since & If-Unmodified-Since requests headers are handled so the service will respond NOT_MODIFIED or PRECONDITION_FAILED depending on values set into the headers.

davidpdrsn · 2021-11-08T15:22:36Z

@zenria do you merge/rebase the latest master? Seems some things have changed making this hard to review.

zenria · 2021-11-08T16:03:30Z

@zenria do you merge/rebase the latest master? Seems some things have changed making this hard to review.

I've just rebased my branch on master :)

davidpdrsn · 2021-11-08T16:17:35Z

Thank you! Given the size of this change I'm pushing it to 0.2 as I would like to get 0.1.2 our relatively soon.

tower-http/Cargo.toml

davidpdrsn · 2021-11-15T10:50:07Z

We made some more changes to CI end of last week so you'll have to rebase master again to get CI working.

Just a heads up there are also some other PRs landing this week which touch these files so you'll probably get some merge conflicts soon.

zenria · 2021-11-16T17:14:50Z

I've rebased my branch, you were right, there was some merge conflicts ;)

I've got rid of headers crate, but I was forced to introduce a dependency on httpdate to parse and generate headers.

davidpdrsn · 2021-11-16T17:19:51Z

Thanks! I'll take a look some time this week.

davidpdrsn · 2021-11-24T13:39:35Z

Just a heads up. I'm waiting on #173 being merged before diving into this.

davidpdrsn · 2021-11-25T13:23:42Z

Removing this from the 0.2 milestone. It wont require breaking changes so we can always ship it in 0.2.1

shepmaster · 2022-01-01T17:45:11Z

Thank you for your work on this; I'm excited to have this functionality in tower-http!

davidpdrsn

Looks good overall! Using httpdate for parsing is fine. Hyper is also using it.

I'm sorry but it looks like you'll have to rebase master again 😞 At least ServeFile has been simplified quite a bit which should make this simpler as well.

tower-http/src/services/fs/serve_dir.rs

zenria · 2022-01-07T09:55:54Z

I've rebased my branch :)

shepmaster

My comments don’t carry any weight in this repository, just actively reading through to see what changed.

shepmaster · 2022-01-07T12:19:21Z

tower-http/src/services/fs/mod.rs

+    fn from_header_value(value: &http::header::HeaderValue) -> Option<IfModifiedSince> {
+        String::from_utf8(value.as_bytes().to_vec())
+            .ok()
+            .map(|value| httpdate::parse_http_date(&value).ok())


map followed by flatten should be flat_map

yes! you're right. (btw this is an option, as there is no flat_map on that type, I always forget I can use and_then to do that.)

shepmaster · 2022-01-07T12:19:53Z

tower-http/src/services/fs/mod.rs

+        String::from_utf8(value.as_bytes().to_vec())
+            .ok()
+            .map(|value| httpdate::parse_http_date(&value).ok())
+            .flatten()


shepmaster · 2022-01-07T12:20:49Z

tower-http/src/services/fs/mod.rs

+}
+
+fn check_modified_headers(
+    modified: &Option<LastModified>,


Usually you don’t want references to options, but an optional reference. Option::as_ref Can be used

in that case this does not matter, but I agree.

shepmaster · 2022-01-07T12:22:21Z

tower-http/src/services/fs/serve_dir.rs

+            .headers()
+            .get(header::IF_MODIFIED_SINCE)
+            .map(IfModifiedSince::from_header_value)
+            .flatten();


Ditto here x2

shepmaster · 2022-01-07T12:23:43Z

tower-http/src/services/fs/serve_dir.rs

@@ -424,7 +459,7 @@ fn append_slash_on_path(uri: Uri) -> Uri {
 enum Output {
    File(FileRequest),
    Redirect(HeaderValue),
-    NotFound,
+    StatusCode(StatusCode),


Feels unfortunate that this went from one possibility to hundreds when it seemingly only needed two possibilities.

~~Yes, you're right!~~

the check_modified_headers can output 2 different status codes: 304 Not Modified and 412 Precondition Failed this is why I'm not using 2 more enum variants.

shepmaster · 2022-01-07T12:24:29Z

tower-http/src/services/fs/serve_dir.rs

@@ -749,7 +787,6 @@ mod tests {
            .unwrap();
        let res = svc.oneshot(request).await.unwrap();

-        assert_eq!(res.headers()["content-type"], "text/plain");


Why’s this get removed?

Sorry, this is a merge error!

shepmaster · 2022-01-07T12:25:27Z

tower-http/src/services/fs/serve_file.rs

@@ -1,8 +1,7 @@
-//! Service that serves a file.


Why’s this get removed?

this too :(

Nehliin

Nicely done! Thanks for the PR! I have some comments regarding the tests but those extra checks should still pass with this implementation. Also have a comment regarding some unnecessary heap allocations.

I will approve this since I personally think my comments can be easily fixed in a follow up pr by either you or me and this has been in the works for some time now. But it's up to @davidpdrsn if he wants to block this on my comments.

Nehliin · 2022-01-14T11:58:34Z

tower-http/src/services/fs/mod.rs

+        String::from_utf8(value.as_bytes().to_vec())
+            .ok()
+            .and_then(|value| httpdate::parse_http_date(&value).ok())
+            .map(|time| IfModifiedSince(time.into()))


Ok, I was not aware of this function, thanks a lot!

Nehliin · 2022-01-14T12:00:01Z

tower-http/src/services/fs/mod.rs

+        String::from_utf8(value.as_bytes().to_vec())
+            .ok()
+            .and_then(|value| httpdate::parse_http_date(&value).ok())
+            .map(|time| IfUnmodifiedSince(time.into()))


Nehliin · 2022-01-14T12:12:25Z

tower-http/src/services/fs/serve_file.rs

+            .unwrap();
+
+        let res = svc.oneshot(req).await.unwrap();
+        assert_eq!(res.status(), StatusCode::NOT_MODIFIED);


Could we please also check that the body is empty here like the spec states?

Nehliin · 2022-01-14T12:12:56Z

tower-http/src/services/fs/serve_file.rs

+            .unwrap();
+
+        let res = svc.oneshot(req).await.unwrap();
+        assert_eq!(res.status(), StatusCode::OK);


Let's check that the file is actually part of the body here as a sanity check.

Nehliin · 2022-01-14T12:13:04Z

tower-http/src/services/fs/serve_file.rs

+            .unwrap();
+
+        let res = svc.oneshot(req).await.unwrap();
+        assert_eq!(res.status(), StatusCode::OK);


Nehliin · 2022-01-14T12:13:19Z

tower-http/src/services/fs/serve_file.rs

+            .unwrap();
+
+        let res = svc.oneshot(req).await.unwrap();
+        assert_eq!(res.status(), StatusCode::PRECONDITION_FAILED);


And this shouldn't return the body.

Nehliin · 2022-01-14T12:13:37Z

tower-http/src/services/fs/serve_dir.rs

@@ -1124,4 +1160,63 @@ mod tests {
            &format!("bytes */{}", file_contents.len())
        )
    }
+    #[tokio::test]
+    async fn last_modified() {


Same comments here like in the tests for serve_file

Nehliin · 2022-01-14T15:58:44Z

Ci issue should be fixed by this: #206

zenria · 2022-01-17T07:45:24Z

Ok, thank you @Nehliin I'll update the PR with the CI & suggested fixes :)

photino · 2023-09-23T11:08:59Z

I cannot find the Last-Modified header in the response for tower-http 0.4.4.

davidpdrsn · 2023-09-23T11:23:51Z

Please open a new issue instead of resurrecting such and old PR.

zenria force-pushed the servedir-lastmod-handling branch from 8e83667 to 6b737a4 Compare September 18, 2021 15:05

zenria force-pushed the servedir-lastmod-handling branch from 6b737a4 to d64c656 Compare November 8, 2021 16:02

davidpdrsn added this to the 0.2.0 milestone Nov 8, 2021

davidpdrsn requested changes Nov 15, 2021

View reviewed changes

tower-http/Cargo.toml Outdated Show resolved Hide resolved

zenria force-pushed the servedir-lastmod-handling branch 2 times, most recently from a837668 to cef7a29 Compare November 16, 2021 17:10

davidpdrsn mentioned this pull request Nov 22, 2021

Implement ServeFile in terms of ServeDir #184

Closed

davidpdrsn removed this from the 0.2.0 milestone Nov 25, 2021

davidpdrsn requested changes Jan 3, 2022

View reviewed changes

tower-http/src/services/fs/serve_dir.rs Outdated Show resolved Hide resolved

zenria force-pushed the servedir-lastmod-handling branch from cef7a29 to cc21606 Compare January 7, 2022 09:49

shepmaster reviewed Jan 7, 2022

View reviewed changes

Nehliin approved these changes Jan 14, 2022

View reviewed changes

zenria added 6 commits January 17, 2022 08:46

Handle last modified conditional requests

0ab0bad

revert merge error

feb9b6a

revert merge error

7915c04

remove useless imports

0b6e22e

use and_then() instead of map().flatten()

d7bedfd

Use Option<&LastModifed> intead of &Option<LastModified>

d7eda5a

zenria added 3 commits January 17, 2022 08:46

use and_then() instead of map().flatten()

a2e00dd

avoid heap allocation while parsing headers value

57fa805

test body content

a373bcd

zenria force-pushed the servedir-lastmod-handling branch from 19f2f8d to a373bcd Compare January 17, 2022 08:01

davidpdrsn approved these changes Jan 17, 2022

View reviewed changes

Nehliin merged commit 83b1f24 into tower-rs:master Jan 17, 2022

ServeDir & ServeFile last modified headers handling #145

ServeDir & ServeFile last modified headers handling #145

Conversation

zenria commented Sep 17, 2021 • edited Loading

Motivation

Solution

davidpdrsn commented Nov 8, 2021

zenria commented Nov 8, 2021

davidpdrsn commented Nov 8, 2021

davidpdrsn commented Nov 15, 2021

zenria commented Nov 16, 2021

davidpdrsn commented Nov 16, 2021

davidpdrsn commented Nov 24, 2021

davidpdrsn commented Nov 25, 2021

shepmaster commented Jan 1, 2022

davidpdrsn left a comment

Choose a reason for hiding this comment

zenria commented Jan 7, 2022

shepmaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenria Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nehliin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nehliin commented Jan 14, 2022

zenria commented Jan 17, 2022

photino commented Sep 23, 2023

davidpdrsn commented Sep 23, 2023

zenria commented Sep 17, 2021 •

edited

Loading

zenria Jan 7, 2022 •

edited

Loading