Supporting Last-Modified Header for StaticFiles #1219

schrieveslaach · 2020-02-01T11:17:34Z

This commit adds basic support of Last-Modified and If-Modified-Since headers in order to enable HTTP caching while serving static files. #95

jebrosen

I am generally in favor of this proposal, and I agree that it makes sense to add it as an opt-in to NamedFile instead of unconditionally to File.

contrib/lib/src/serve.rs

core/lib/src/response/named_file.rs

schrieveslaach · 2020-02-03T19:56:10Z

@jebrosen, I addressed your comments and update the PR.

contrib/lib/src/serve.rs

core/lib/Cargo.toml

core/lib/src/response/named_file.rs

SergioBenitez · 2020-10-30T03:55:38Z

Besides needed a rebase, what is the status of this?

schrieveslaach · 2020-11-01T19:04:56Z

@SergioBenitez, the PR was stuck due to incompatible APIs (see comments above). I updated the PR and pushed a new version that use the headers crate (suggested by @jebrosen).

jebrosen

Overall LGTM; I requested a few minor changes. The encoding/decoding is a little bit wonky, but I think that would best be resolved by implementing #1067 with the headers crate.

core/lib/src/response/named_file.rs

jebrosen

This looks great to me, with a small expansion to the docs.

I think this PR already satisfies my concerns from #1376 (review) by being opt-in via a new constructor. So my vote is for this PR, which also includes tests already.

contrib/lib/src/serve.rs

core/lib/src/response/named_file.rs

schrieveslaach · 2020-11-27T13:35:48Z

@SergioBenitez, did you have the chance to review this pull request?

schrieveslaach · 2021-05-18T11:13:44Z

@SergioBenitez, @jebrosen do you have any idea why the build “Linux Debug (nightly)” after I rebased this PR? The error message seems unrelated to the changes in this PR.

SergioBenitez · 2021-05-18T18:41:51Z

Likely a change in how diagnostics are rendered. I'll take a close look now.

SergioBenitez · 2021-05-18T19:06:04Z

The unit tests should now pass.

This commit adds basic support of Last-Modified and If-Modified-Since headers in order to enable HTTP caching while serving static files.

schrieveslaach · 2021-05-20T15:51:19Z

@SergioBenitez, thanks. It works again.

LionsAd · 2021-06-09T12:09:44Z

contrib/lib/src/serve.rs

@@ -133,6 +133,12 @@ impl Options {
    /// that would be served is a directory.
    pub const NormalizeDirs: Options = Options(0b0100);

+    /// `Options` enabling caching based on the `Last-Modified` header. When
+    /// this is enabled, the [`StaticFiles`] handler will at the `Last-Modified`


Suggested change

/// this is enabled, the [`StaticFiles`] handler will at the `Last-Modified`

/// this is enabled, the [`StaticFiles`] handler will add the `Last-Modified`

LionsAd · 2021-06-09T12:10:07Z

contrib/lib/src/serve.rs

@@ -133,6 +133,12 @@ impl Options {
    /// that would be served is a directory.
    pub const NormalizeDirs: Options = Options(0b0100);

+    /// `Options` enabling caching based on the `Last-Modified` header. When
+    /// this is enabled, the [`StaticFiles`] handler will at the `Last-Modified`
+    /// header baesd on the modification datetime of the files in the served


Suggested change

/// header baesd on the modification datetime of the files in the served

/// header based on the modification datetime of the files in the served

SergioBenitez · 2021-06-29T12:20:16Z

I don't think the API or implementation here are ones that I'm in favor of. Specifically:

This API does not leave room for growth into headers which are not simply flags, that is, which require a value like Cache-Control.
This API reuses Options from StaticFiles for NamedFile when only one bitfield actually applies.
This implementation introduces a dependency on header for two types.
The constructor approach (with_last_modified_date()) results in lengthy invocations.
The Responder impl indexes into a vector without checking it contains a value and then unwrap()s. This can trivially lead to panics.
There are a many vec![], to_string() allocations. This functionality should require none.

Before revisting an implementation for this and #95 in general, I would like to agree on an API design and implementation strategy that has none of these drawbacks. Until then, I am closing this pull request.

jebrosen requested changes Feb 1, 2020

View reviewed changes

schrieveslaach force-pushed the static-file-cache branch from dd566f5 to 381b584 Compare February 3, 2020 19:47

jebrosen reviewed Feb 4, 2020

View reviewed changes

contrib/lib/src/serve.rs Outdated Show resolved Hide resolved

jebrosen requested changes Feb 4, 2020

View reviewed changes

core/lib/Cargo.toml Outdated Show resolved Hide resolved

core/lib/src/response/named_file.rs Outdated Show resolved Hide resolved

SergioBenitez force-pushed the master branch 2 times, most recently from 74113c3 to 95c981d Compare February 15, 2020 12:02

schrieveslaach force-pushed the static-file-cache branch from 381b584 to 962aa9e Compare February 23, 2020 12:17

SergioBenitez mentioned this pull request Oct 30, 2020

add support for Last-Modified and If-Modified-Since headers #1376

Closed

schrieveslaach force-pushed the static-file-cache branch from 962aa9e to 692f04e Compare November 1, 2020 19:02

schrieveslaach requested a review from jebrosen November 3, 2020 07:03

jebrosen requested changes Nov 3, 2020

View reviewed changes

core/lib/src/response/named_file.rs Outdated Show resolved Hide resolved

core/lib/src/response/named_file.rs Outdated Show resolved Hide resolved

schrieveslaach force-pushed the static-file-cache branch from 692f04e to 60f8133 Compare November 3, 2020 19:27

schrieveslaach requested a review from jebrosen November 3, 2020 19:28

schrieveslaach force-pushed the static-file-cache branch from 60f8133 to ca59e7e Compare November 10, 2020 21:36

jebrosen approved these changes Nov 11, 2020

View reviewed changes

contrib/lib/src/serve.rs Outdated Show resolved Hide resolved

core/lib/src/response/named_file.rs Show resolved Hide resolved

schrieveslaach force-pushed the static-file-cache branch 3 times, most recently from 27be497 to 9d1fe40 Compare November 13, 2020 03:25

jebrosen requested a review from SergioBenitez November 13, 2020 03:53

schrieveslaach force-pushed the static-file-cache branch from 9d1fe40 to efe0958 Compare November 22, 2020 19:20

schrieveslaach force-pushed the static-file-cache branch from efe0958 to 5b48e99 Compare January 27, 2021 18:13

jebrosen mentioned this pull request Feb 2, 2021

Cache-Control and other headers for static served files. #1522

Closed

schrieveslaach force-pushed the static-file-cache branch from 5b48e99 to d15a1d2 Compare April 2, 2021 17:44

schrieveslaach force-pushed the static-file-cache branch from d15a1d2 to 59032f3 Compare May 17, 2021 09:42

Supporting Last-Modified Header for StaticFiles

e9aea56

This commit adds basic support of Last-Modified and If-Modified-Since headers in order to enable HTTP caching while serving static files.

schrieveslaach force-pushed the static-file-cache branch from 59032f3 to e9aea56 Compare May 20, 2021 08:21

LionsAd reviewed Jun 9, 2021

View reviewed changes

SergioBenitez closed this Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting Last-Modified Header for StaticFiles #1219

Supporting Last-Modified Header for StaticFiles #1219

schrieveslaach commented Feb 1, 2020 •

edited

Loading

jebrosen left a comment

schrieveslaach commented Feb 3, 2020

SergioBenitez commented Oct 30, 2020

schrieveslaach commented Nov 1, 2020 •

edited

Loading

jebrosen left a comment

jebrosen left a comment

schrieveslaach commented Nov 27, 2020

schrieveslaach commented May 18, 2021

SergioBenitez commented May 18, 2021

SergioBenitez commented May 18, 2021 •

edited

Loading

schrieveslaach commented May 20, 2021

LionsAd Jun 9, 2021

LionsAd Jun 9, 2021

SergioBenitez commented Jun 29, 2021

	/// this is enabled, the [`StaticFiles`] handler will at the `Last-Modified`
	/// this is enabled, the [`StaticFiles`] handler will add the `Last-Modified`

	/// header baesd on the modification datetime of the files in the served
	/// header based on the modification datetime of the files in the served

Supporting Last-Modified Header for StaticFiles #1219

Supporting Last-Modified Header for StaticFiles #1219

Conversation

schrieveslaach commented Feb 1, 2020 • edited Loading

jebrosen left a comment

Choose a reason for hiding this comment

schrieveslaach commented Feb 3, 2020

SergioBenitez commented Oct 30, 2020

schrieveslaach commented Nov 1, 2020 • edited Loading

jebrosen left a comment

Choose a reason for hiding this comment

jebrosen left a comment

Choose a reason for hiding this comment

schrieveslaach commented Nov 27, 2020

schrieveslaach commented May 18, 2021

SergioBenitez commented May 18, 2021

SergioBenitez commented May 18, 2021 • edited Loading

schrieveslaach commented May 20, 2021

LionsAd Jun 9, 2021

Choose a reason for hiding this comment

LionsAd Jun 9, 2021

Choose a reason for hiding this comment

SergioBenitez commented Jun 29, 2021

schrieveslaach commented Feb 1, 2020 •

edited

Loading

schrieveslaach commented Nov 1, 2020 •

edited

Loading

SergioBenitez commented May 18, 2021 •

edited

Loading