Implement rewrite API #2789

the10thWiz · 2024-05-08T16:56:17Z

The goal of this PR is to implement a Rewrite API, following the basic outline presented in #2716.

The goal is to replace the existing Options on FileServer to instead use a rewrite based API. This will make FileServer pluggable, making it possible to implement things like cached compression implementable outside of Rocket.

The basic usage example is as follows:

FileServer::new("static")
   .rewrite(DotFiles)
   .rewrite(NormalizeDirs)
   .rewrite(Index("index.html"))

There are a couple of major limitations to the rewrite trait, specifically the file to be served must exist on the local file system, and the trait is synchronous, so it should not do any IO when rewriting paths. However, I would argue that if you want to move beyond these, you should just implement your own Handler, and fully replace FileServer.

the10thWiz · 2024-05-12T04:46:27Z

I've implemented a basic gzip compression rewriter, which provides an API similar to what was discussed in #2716.

FileServer::new("static", Options::None)
   .rewrite(NormalizeDirs)
   .rewrite(Index("index.txt"))
   .rewrite(CachedCompression::new()),

Full example, with implementation: https://github.com/the10thWiz/rocket-caching-layer

This particular implementation generates the compressed version of the file once it's been requested once, and responds with the uncompressed version in the meantime. It's not too hard to imagine different implementations, with different trade-offs. I built this as a proof-of-concept, to show that the new API actually worked for the intended use-case.

I've also marked most of the Options as deprecated, and implemented a quick check to generate an initial set of rewrites based on the Options provided.

SergioBenitez · 2024-05-21T05:02:58Z

I'm about to review this. Note that I tried to implement this API myself some time ago and ran into a bunch of issues. Here's my attempt, in case it helps with anything: https://paste.rs/wEcVu.rs

Hopefully you've manage to overcome the limitations I found!

the10thWiz · 2024-05-28T00:41:13Z

@SergioBenitez

Hopefully you've manage to overcome the limitations I found!

I'm not sure what the exact limitations you came across, but here are a few things I've done differently:

Rewrites can only produce the content of a file on disk. This simplifies the API somewhat, since a rewrite can only take one of a few specific actions. This also allows further modification in pretty much every case, so every rewrite is always applied in every case.
I've implemented DotFiles differently. Rather than adding a rewrite that either allows or disallows them, I have set up the FileServer to output a Hidden response, that the DotFiles rewrite converts to a normal File response.
Rather than using Cow<Path>, I'm using PathBuf. I'm pretty sure we always need to allocate the path, so we wouldn't gain much from Cow<Path>, but I'm open to switching if we can potentially remove some allocations.

There are a few points we need to decide on before this implementation can be finalized:

Is calling .is_dir() (internally std::fs::metadata) acceptable in an async environment? There is an async tokio version, because this is an IO call, but the current implementation uses .is_dir() internally already. My current version also uses .is_dir(), and I'm not sure if this is a big deal or not.
How do we want to handle Missing and IndexFile. These are an interesting case, because their primary function is a check on startup. We could extend the Rewrite API to include a startup check (and default to passing), but Missing would actually want us to ignore an error. For now, I've included IndexFile (which does nothing), and excluded Missing because I don't have a way to implement it.
How do we want to roll this out? My current implementation is technically backwards compatible, and outputs a deprecation warning. If we intend to introduce this as part of 0.6, we could make this a breaking change, but I'd to know your thoughts before I go in on either option.
Should we include a compression rewrite? I've created one (see the link in a previous comment) outside of Rocket, but compression is such a common requirement we should consider including it either in core, or as a contrib crate.

SergioBenitez · 2024-05-29T05:35:40Z

The entire patch for my rewrite API implementation is here: https://paste.rs/AbR41.patch

I'll be referring to it in my review.

SergioBenitez

The primary difference I see between this API and the one I implemented, at a more fundamental level, is the degree to which it seeks to make FileServer completely generic. In my implementation, FileServer becomes purely a sequence of rewrites:

+pub struct FileServer {
+    rewriters: Vec<Arc<dyn Rewriter>>,
+}

Whereas here, we still have the notion of a root and some options. I think to consider this a successful API, we need something that looks more like the former: we want to be able to take a path from the client and progressively apply rewrites until we have the response we're looking for, with those rewrites being controlled completely in a generic manner.

To that effect, I do think we need to make FileServer be:

pub struct FileServer {
    rewriters: Vec<Arc<dyn Rewriter>>,
}

And then be able to recover the existing functionality as a series of rewrites that we provide out of the box.

The second core difference I note is in the treatment of responses, in particular with respect to headers. In my changeset, I introduce changes to NamedFile in which I add a HeaderMap to the struct:

+pub struct NamedFile {
+    file: File,
+    path: PathBuf,
+    headers: HeaderMap<'static>,
+}

(In reality I also added Metadata, but I think that was a mistake.)

I think this makes sense as it creates parity between NamedFile and FileServer. My intent was to allow the rewriter to return a Path and optionally a HeaderMap and then use those to construct a NamedFile that gets used to generate the response. This proved to not be enough as we also need to support rewriting into redirects, and thus returning a Response was added.

In your implementation, you have an enum that effectively replaces the need for Response in my changeset.

I think the right approach is somewhere in-between.

At the end of my experiment, and after taking a look at the code here, I'm left with the impression that the rewrite API I suggested is fundamentally trying to be monadic in Option<FileResponse> of some sort, and that we should really approach that API a little more directly. That is, to boil things down to a sequence of calls of the form Option<FileResponse> -> Option<FileResponse>, where FileResponse looks something like FileServerResponse:

enum FileResponse<'a> {
    File(File<'a>),
    Redirect(Redirect<'a>),
}

struct File<'a> {
    path: Cow<'a, Path>,
    headers: HeaderMap<'a>,
}

Then, FileServer is really just an Option<FileResponse<'a>> that starts as a Some(FileResponse::File(File::from(req.uri()))) (or something morally equivalent) and proceeds through a series of and_then()s, map()s, and filter()s. I think this presentation would also make the ordering argument a bit clearer as we're already used to the ordering consequences between these operations.

If this path is followed, then the Rewrite trait becomes:

pub trait Rewrite: Send + Sync + Any {
    fn rewrite<'r>(&self, response: Option<FileResponse<'r>>) -> Option<FileResponse<'r>>;
}

It can be implemented for all function of the same form (for<'r> F: Fn(Option<FileResponse<'r>>) -> Option<FileResponse<'r>>) as well as the bind form (FileResponse<'r> -> Option<FileResponse<'r>>) and the map form (FileResponse<'r> -> FileResponse<'r>). We can also specialize implementations for common forms such as File<'a> -> File<'a>, File<'a> -> Option<File<'a>>, and File<'a> -> FileResponse<'r>.

This would mean we can implement the basic usage as something like:

FileServer::new()
    .filter_file(|file| !file.path.components().any(|c| c.starts_with('.')))
    .map_file(|file| if file.path.is_dir() && !file.path.ends_with('/') {
        Redirect(Redirect::to(file.path + '/'))
    } else {
        File(file)
    })
   .map_file(|file| if file.path.is_dir() && file.path.join("index.html").exists() {
        File(file.path.join("index.html"))
    } else {
        file
    })

Of course, we'd want to provide these as reusable components, but the point is that the implementation becomes significantly more trivial.

core/http/src/uri/segments.rs

the10thWiz · 2024-05-29T12:51:24Z

I think the APIs we have proposed (the two rewrite traits) are to some extent isomorphic. My trait has the request and file server root as additional parameters, but I'm pretty sure we could eliminate that without any issues. (I've already refactored away the need for root, and NormalizedDirs is the only one that still requires req, and this could be removed pretty easily). From there, FileServerResponse::NotFound is sort of equivalent to None.

The reason I opted to go with an explicit NotFound variant was to enable filtering dot files by default, i.e. FileServer::new("static") implies that dot files should not be served. To implement this, the None case needs to carry the path, enabling the DotFiles rewrite to convert it back to a Some case. This is also the reason I added to_path_buf_dotfiles.

I opted to retain Options for two reasons: first, to maintain backwards compatibility (and avoid the need to defer this to 0.6), but it sounds like you want this to be a breaking change. The second reason is that we can't mimic the behavior of Options::Missing and Options::IndexFile cleanly with a rewrite based API, since their primary function is a check during startup. If we are willing to lose this functionality, we can fully eliminate options from the FileServer struct.

With that in mind, FileServer becomes:

pub struct FileServer {
    root: PathBuf,
    rewrites: Vec<Arc<dyn Rewriter>>,
    rank: isize,
}

I think including root and rank is worthwhile. rank should be included, because it can't really be replicated by rewrites. I think we should include root, because we should restrict FileServer to a single root. This means handle becomes (roughly) File(self.root.join(req.uri().path()), a series of rewrites, and converting the final output to a Response.

Overall, I agree with most of the API changes you propose. The only changes I'm not willing to lose is hiding dot files by default, and some kind of startup check for whether the root directory exists.

SergioBenitez · 2024-05-29T19:15:51Z

The reason I opted to go with an explicit NotFound variant was to enable filtering dot files by default, i.e. FileServer::new("static") implies that dot files should not be served. To implement this, the None case needs to carry the path, enabling the DotFiles rewrite to convert it back to a Some case. This is also the reason I added to_path_buf_dotfiles.

We don't need to force filtering dot files by default: it would suffice to make the default constructor include a rewrite that filters dotfiles. This way we avoid negative logic.

I opted to retain Options for two reasons: first, to maintain backwards compatibility (and avoid the need to defer this to 0.6), but it sounds like you want this to be a breaking change.

0.6 is right around the corner. And in general, we shouldn't compromise on the API to avoid a breaking change unless there's a huge benefit to be had, which I don't feel is the case here.

The second reason is that we can't mimic the behavior of Options::Missing and Options::IndexFile cleanly with a rewrite based API, since their primary function is a check during startup. If we are willing to lose this functionality, we can fully eliminate options from the FileServer struct.

I think Options::Missing should become part of the functionality of specific rewrites. If a rewrite works from some directory, then it can/should be responsible for erroring as needed. This can be part of the constructor for that rewrite.

Options::IndexFile can and should be a rewrite, and it is in my implementation. In fact, it's one of the simplest of rewrites:

|_| File(self.0)

I think including root and rank is worthwhile. rank should be included, because it can't really be replicated by rewrites.

Absolutely.

I think we should include root, because we should restrict FileServer to a single root.

I think this is the case for FileServer today, but nothing should prevent a rewrite that looks in multiple roots, for example. Furthermore, we likely want to eventually extend FileServer to serve files included in the binary directly, and this would prevent that from being possible. In general, I don't think we gain anything from having root be endemic to FileServer but stand to lose quite a bit of flexibility.

Overall, I agree with most of the API changes you propose. The only changes I'm not willing to lose is hiding dot files by default, and some kind of startup check for whether the root directory exists.

I agree, and it doesn't need to be in conflict with the changes I propose. For the former, we can include it as part of the default constructor, not as part of FileServer directly. For the latter, a check can be done by the rewrite instead of by FileServer.

the10thWiz · 2024-05-31T00:15:29Z

0.6 is right around the corner
I was a bit disconnected from Rocket for the past couple months, so I wasn't sure when 0.6 would happen. Thanks for clarifying this for me.

I've implemented changes based on this discussion, and cleaned up the API quite a bit. There are a couple specific pain points with the new API, but they are some pretty minor nits, and should be fixable without any sweeping changes.

First, filter_file and map_file have weird trait bounds, since I wanted Root, BlockDotfiles and Index to work with these methods. However, I think I'm going to simplify this, and make them simple Fn(..) -> .. bounds, and convert all of the provided rewrites to just be functions with the correct signature. This will mean they aren't usable in and_rewrite, but I think this is a worthwhile sacrifice to make the API cleaner.

Second, I hoped to avoid any kind of remove_rewrites (to avoid reverse logic), but I've left it in for now. If we are willing to make allowing dot files more complicated, removing the remove_rewrites and allow_dotfiles methods forces users who want this functionality to use empty, and add Root (along with other rewrites) by hand. I'm on the fence about this, let me know what you think.

Finally, I'm going to need to rewrite the documentation and examples for FileServer. I wanted to nail down a design before I dove into this, but it's looking like it's time to work on this.

the10thWiz · 2024-06-02T21:53:27Z

@SergioBenitez I think I've got this to a pretty good place now. I'm pretty sure there aren't any subtle regressions left, but I'm not 100% sure. The test suite does pass now, so that's a good sign.

Looking forward, do we want to add a pre-computed compression rewrite to core or contrib? I'm happy to open another PR for an implementation. If not, I'll likely create and publish a separate crate.

core/lib/src/fs/server.rs

the10thWiz · 2024-06-30T23:29:59Z

I'm updated this PR (using the changes you've made), to pass CI. Overall, I like most of the changes. I'm not sure how I feel about Rewrite implementing Rewriter, and the change to using temporary redirects.

To that end, I've changed a few things:

Added FileServer::directory (One doc-test uses it).
Updated tests to reflect a change in TrailingDirs's behavior - it now generates temporary redirects, unlike the old file server which generated permanent redirects.
Re-exported Rewrite and Rewriter from rocket::fs.
Added From<File> and From<Redirect> impls for Option<Rewrite>.

SergioBenitez · 2024-07-01T01:57:56Z

core/lib/src/fs/rewrite.rs

+impl<'r> From<File<'r>> for Option<Rewrite<'r>> {
+    fn from(value: File<'r>) -> Self {
+        Some(Rewrite::File(value))
+    }
+}


What's the utility of this impl? I don't see it used anywhere, and I'm not sure we really want to hide the functionality under an opaque .into().

The other impl (From<Redirect> for Option<Rewrite>) is actually used in at least one doc-test.

This makes it possible for a rewrite to return File { .. }.into(), rather than Some(File { .. }.into()). I think it makes the API more succinct, without losing any information. I think it might be worth looking at the other doc-tests, and using this impl if appropriate.

When adding documentation, now both are used (in the doc-tests for FileServer::filter and FileServer::map respectively).

SergioBenitez · 2024-07-01T02:03:57Z

core/lib/src/fs/server.rs

+    /// Constructs a new `FileServer` that serves files from the file system
+    /// `path` with a default rank.
+    ///
+    /// Adds a set of default rewrites:
+    /// - [`filter_dotfiles`]: Hides all dotfiles.
+    /// - [`prefix(path)`](prefix): Applies the root path.
+    /// - [`normalize_dirs`]: Normalizes directories to have a trailing slash.
+    pub fn directory<P: AsRef<Path>>(path: P) -> Self {
+        Self::empty()
+            .filter(|f, _| f.is_visible())
+            .rewrite(Prefix::checked(path))
+            .rewrite(TrailingDirs)
+    }


So I had implemented this exact method but I removed it because I doubted its utility, given that this would make new() exactly Self::directory(path).rewrite(DirIndex::unconditional("index.html")). If we're going to keep this, we should document the two methods well with why/when you would use one or the other. And we should use it in new()'s implementation.

I assumed something like that might have happened. It was still used in one of the doc-tests (of DirIndex specifically). I think it's worth including, but I'm thinking it might be worth changing the name in addition to writing better docs.

SergioBenitez · 2024-07-01T02:05:41Z

core/lib/src/fs/rewrite.rs

+    /// # Windows Note
+    ///
+    /// This does *not* check the file metadata on any platform, so hidden files
+    /// on Windows will not be detected.


Could we make it so that it does check the metadata?

This is bit more complex than the dotfiles check. First, it would only work after Prefix (much like is_dir or exists), and the current dotfiles check is typically done before Prefix.

Second, to match the behavior of dotfiles, we would likely want to also check all parent folders, which would likely represent a non-trivial amount of work. Right now, each Rewrite requires at most one syscall, and I think that's a reasonable limit.

Overall, I'm not sure if it's worthwhile. We could provide an alternate method that does check fs metadata, but I'm not sure how much value it would provide. We would also likely need to create a more complex test for this functionality, since I'm not sure git preserves Windows file properties.

core/lib/src/fs/mod.rs

Apparently, no_compile doesn't prevent the doctest from being compiled

- Rewriter is now Monadic on `Option<FileResponse>` - `new` (+variants) now adds a proper set of default rewrites - filter_dotfiles now removed dotfiles - Compatibility has been broken

Changes `File` to contain the full `Origin` uri from the request, so normalize_dir doesn't have to deal with platform specific behavior. This is also a more convient way to handle Redirects, as well as provide more consistent access to the query parameters.

It doesn't do what I need it do.

- Add #[non_exaustive] to allow adding more FileResponses in the future - Improve readability of `FileServer::handle` - Take advantage of `NameFile`'s handling missing files - Add `strip_trailing_slash` to ensure files can be read from disk

- Change name of `missing_root` - Update documentation to reference non-panicing variant - Fix issue caused by attempting to open directories

- Introduce `FileMap` and `FileFilter` traits

- `Rewrite` and `Rewriter` are no longer re-exported from `fs`. - Renamed `FileServer::directory` to `FileServer::new_without_index`. - Added missing documentation in a number of places.

SergioBenitez · 2024-07-06T13:36:10Z

Merged! 65e3b87

the10thWiz marked this pull request as ready for review May 12, 2024 04:49

the10thWiz requested a review from SergioBenitez May 18, 2024 02:41

SergioBenitez reviewed May 29, 2024

View reviewed changes

core/http/src/uri/segments.rs Outdated Show resolved Hide resolved

the10thWiz force-pushed the rewrite-api branch from 7d08580 to 2be1e53 Compare June 2, 2024 21:00

the10thWiz requested a review from SergioBenitez June 4, 2024 00:02

the10thWiz force-pushed the rewrite-api branch from 3b46cfb to b637cfa Compare June 9, 2024 04:49

SergioBenitez reviewed Jun 11, 2024

View reviewed changes

the10thWiz force-pushed the rewrite-api branch from 048ce60 to 17d886d Compare June 12, 2024 00:13

the10thWiz requested a review from SergioBenitez June 12, 2024 01:18

SergioBenitez force-pushed the rewrite-api branch from 17d886d to 54b80b2 Compare June 30, 2024 02:26

SergioBenitez reviewed Jul 1, 2024

View reviewed changes

core/lib/src/fs/mod.rs Outdated Show resolved Hide resolved

the10thWiz requested a review from SergioBenitez July 2, 2024 00:02

the10thWiz mentioned this pull request Jul 2, 2024

Caching a background image with the Cache-Control header #2815

Closed

the10thWiz added 5 commits July 6, 2024 13:21

Implement basic rewrite API

101b245

Fix issues, and clean up implementation

6b23b49

Format for style changes

5f0164e

Make default rewrites public

86f148b

Clean up tests and remove unused member

bb47311

the10thWiz and others added 24 commits July 6, 2024 13:21

Minor cleanup and adding documentation

13bbfb8

Fix doctest

335e340

Apparently, no_compile doesn't prevent the doctest from being compiled

Update error message

7a6c355

Improve API and break backwards-compatibility

22ed995

Add map_uri function to Redirect

5b54e8a

Update Rewriter API based on discussion

424aa38

- Rewriter is now Monadic on `Option<FileResponse>` - `new` (+variants) now adds a proper set of default rewrites - filter_dotfiles now removed dotfiles - Compatibility has been broken

Add missing_root, and update docs to include panic

e282abe

Fix doctests

a37bdbb

Update tests to use new Rewrite API

59e7801

Stop using has_trailing_slash

e8a8c39

It doesn't do what I need it do.

Update to use tracing macros

16e2a5a

Update to_path_buf to ignore .s

9b23ec0

Update implementation

b42dbaf

- Add #[non_exaustive] to allow adding more FileResponses in the future - Improve readability of `FileServer::handle` - Take advantage of `NameFile`'s handling missing files - Add `strip_trailing_slash` to ensure files can be read from disk

Convert to proper tracing structure

e094091

Update naming and documentation

0c2bafe

- Change name of `missing_root` - Update documentation to reference non-panicing variant - Fix issue caused by attempting to open directories

Fix line length

9ead65c

Simplify standard rewrites type

4d7f98b

- Introduce `FileMap` and `FileFilter` traits

changes and improvements to rewrite PR

c179f56

streamline rewriters - use structs for consistency

3fbda80

Update doc-tests and add small pieces of functionality

1cfcbf5

Update examples and docs to use the new FileServer api

113b79c

Updates based on PR review

482a047

- `Rewrite` and `Rewriter` are no longer re-exported from `fs`. - Renamed `FileServer::directory` to `FileServer::new_without_index`. - Added missing documentation in a number of places.

Clean up docs. Rename 'FileServer::empty()'.

fb8df12

SergioBenitez force-pushed the rewrite-api branch from ae40009 to fb8df12 Compare July 6, 2024 13:22

SergioBenitez closed this Jul 6, 2024

SergioBenitez added the pr: merged This pull request was merged manually. label Jul 6, 2024

the10thWiz mentioned this pull request Jul 7, 2024

Alllow customizing HTTP headers for NamedFile, StaticFiles #95

Closed

8 tasks

SergioBenitez mentioned this pull request Aug 22, 2024

FileServer can serve pre-gzipped content if it exists on the filesystem #2718

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rewrite API #2789

Implement rewrite API #2789

the10thWiz commented May 8, 2024

the10thWiz commented May 12, 2024 •

edited

Loading

SergioBenitez commented May 21, 2024

the10thWiz commented May 28, 2024

SergioBenitez commented May 29, 2024

SergioBenitez left a comment

the10thWiz commented May 29, 2024

SergioBenitez commented May 29, 2024 •

edited

Loading

the10thWiz commented May 31, 2024

the10thWiz commented Jun 2, 2024

the10thWiz commented Jun 30, 2024

SergioBenitez Jul 1, 2024

the10thWiz Jul 1, 2024

the10thWiz Jul 1, 2024

SergioBenitez Jul 1, 2024 •

edited

Loading

the10thWiz Jul 1, 2024

SergioBenitez Jul 1, 2024

the10thWiz Jul 1, 2024

SergioBenitez commented Jul 6, 2024

Implement rewrite API #2789

Implement rewrite API #2789

Conversation

the10thWiz commented May 8, 2024

the10thWiz commented May 12, 2024 • edited Loading

SergioBenitez commented May 21, 2024

the10thWiz commented May 28, 2024

SergioBenitez commented May 29, 2024

SergioBenitez left a comment

Choose a reason for hiding this comment

the10thWiz commented May 29, 2024

SergioBenitez commented May 29, 2024 • edited Loading

the10thWiz commented May 31, 2024

the10thWiz commented Jun 2, 2024

the10thWiz commented Jun 30, 2024

SergioBenitez Jul 1, 2024

Choose a reason for hiding this comment

the10thWiz Jul 1, 2024

Choose a reason for hiding this comment

the10thWiz Jul 1, 2024

Choose a reason for hiding this comment

SergioBenitez Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

the10thWiz Jul 1, 2024

Choose a reason for hiding this comment

SergioBenitez Jul 1, 2024

Choose a reason for hiding this comment

the10thWiz Jul 1, 2024

Choose a reason for hiding this comment

SergioBenitez commented Jul 6, 2024

the10thWiz commented May 12, 2024 •

edited

Loading

SergioBenitez commented May 29, 2024 •

edited

Loading

SergioBenitez Jul 1, 2024 •

edited

Loading