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

Serve content with support for HTTP conditionals and range requests #687

Closed
wants to merge 2 commits into from

Conversation

ririsoft
Copy link
Contributor

@ririsoft ririsoft commented Aug 26, 2020

This PR introduces new utilities to serve content with support for HTTP conditionals as defined in RFC7232. It will allow to return 304 Not Modified (for instance) and handle HTTP Range requests (RFC7233) transparently.

Motivation and Context

Implementing RFC7232 and RFC7233 is not trivial: I believe it should be implemented at Tide level instead of leaving the users implementing it themselves. At the time I checked a few weeks ago Rocket, warp and actix all had limited (if any) support for such conditional requests (this might have changed). Support for conditional requests is provided by Go's standard http library.

This will help closing #63. Also the current implementation of tide::Route::serve_dir could benefit from it to handle last modification date and range requests.

I am already running a full implementation in production to serve a bunch of video files for internal use (building a media center for home).

This draft PR depends on changes that are not yet released (some not even merged) in http-types, see dependencies section below. I will update this PR as much as the situation evolves on http-types side. I will then commit the implementation, testing and more documentation.

Finally i will move out of draft status once everything is released on http-types side and implementation, tests and docs completed.

For now I am requesting comments for interest in such feature within Tide or if you prefer that I release this in an external crate. Also I would like you comments on the API, please.

Thanks in advance !
Cheers.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Dependencies:

This will allow to separate non related utilities in the future.
Allows to serve content
handling HTTP conditional requests as defined in RFC7232.
@Fishrock123
Copy link
Member

Fishrock123 commented Aug 26, 2020

I think if we have this, then ModState probably belongs in http-types?

I am not sure if that would be confusing or not, given most of the other things there represent http headers directly, but it feels like the correct place.

@ririsoft
Copy link
Contributor Author

I think if we have this, then ModState probably belongs in http-types?

I am not sure if that would be confusing or not, given most of the other things there represent http headers directly, but it feels like the correct place.

This is indeed an open question. It could be renamed in something like http_types::conditional::CondState, or a better name that suite more HTTP wording. But personally I am more on the it would be confusing side: I believe the ModState is closer to the application logic, than HTTP logic (the border is thin). It is just a convenient utility provided to the user to pass either a time or a version string associated to its resource.

Looking at what Go does they just allow a time in their serve_content function, and they force users to set the ETag if any at the response header level. This is where Rust enums are superior and allow to do more for the user.

Otherwise I have no strong opinion about it.


use crate::{Request, Response, Result, StatusCode};

/// A HTTP ressource modification state.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// A HTTP ressource modification state.
/// A HTTP resource modification state.

And other uses of the word

@ririsoft
Copy link
Contributor Author

This is indeed an open question. It could be renamed in something like http_types::conditional::CondState, or a better name that suite more HTTP wording. But personally I am more on the it would be confusing side: I believe the ModState is closer to the application logic, than HTTP logic (the border is thin). It is just a convenient utility provided to the user to pass either a time or a version string associated to its resource.

I might change my opinion here. When you look at the If-Range header implementation I have submitted in http-rs/http-types#180 it has exactly the same need. Having a shared type to handle both cases might be interesting, and in such case it would belong to http-types utils.

@u5surf
Copy link
Contributor

u5surf commented Feb 14, 2021

@Fishrock123 @ririsoft

Hi, I’m interested that conditional headers in tide framework. I tried to implements the middleware which can only verify about If-None-Match and content ETag. if they match, it returns the 304 NotModified.
https://github.com/u5surf/tide-condition

serve_file directive must know about file modified information through from_file_with_path of http-types
(see also
https://github.com/http-rs/http-types/blob/main/src/body.rs#L429
Fortunately, async_std::fs::Metadata has file modification time.

https://docs.rs/async-std/0.99.12/async_std/fs/struct.Metadata.html#method.modified

It is possible to be use this. Although, it has not implemented about this yet.
So now, it is hard to use modified information to serve the static file.
Moreover, how can be treated in case of dynamic generated contents?
That’s why I gave up implements the period verification such as using If-Modified-Since and Last-Modified

@ririsoft
Copy link
Contributor Author

I am closing this PR as I am not using/working on tide anymore and will thus not be able to work on this further on.

@ririsoft ririsoft closed this May 15, 2024
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.

3 participants