Add `range` submodule by ririsoft · Pull Request #180 · http-rs/http-types

ririsoft · 2020-06-09T21:17:00Z

This introduce a new 'range' module
to support encoding, decoding and validation of HTTP range requests.

This could be a helper to implement http-rs/tide#63 and enhance tide's serve_dir with range requests.

This is also an experiment toward typed headers discussed in #99.

I am using a simple design where the typed header implements TryFrom and ToHeaderValues. This has the advantage to be fully compatible with the current header APIs. However I believe we could push Rust type system a little further.

For instance we could enhance the Headers type with a new method and trait:

trait TypedHeader: ToHeaderValues {
    const HeaderName: &'static str;
}

impl Headers {
    // TODO: find a better name
    pub fn get_typed<T>(&self) -> Option<T>
    where
            T: TypedHeader,
    {
        // TODO: implementation
    }
}

One could then retrieve a typed value directly from the headers :

let range: Option<Range> = headers.get_typed();

I am not sure if it works, but I could add a few commits on this pull request if you want a try.

Thanks in advance for your comments !
Cheers.

Fishrock123 · 2020-06-10T04:39:35Z

We should probably do it in a separate PR, and I'm not sure how idiomatic this would be, but Headers::get::<TypedHeader>() would seem reasonable to me.

ririsoft · 2020-06-10T06:46:00Z

We should probably do it in a separate PR, and I'm not sure how idiomatic this would be, but Headers::get::<TypedHeader>() would seem reasonable to me.

Indeed this is orthogonal to this PR and deserve a separate discussion.

ririsoft · 2020-08-24T12:23:13Z

Hello @yoshuawuyts,

In the scope of #99 I see you are progressing well on #222, #224 , #225, #220.
What do you expect from me at this point: You would be happy that I rework this pull request once the above PRs are merged and reuse the same conditional APIs ? Or you plan to implement this yourself ?

Some part of this PR need to be reworked, I identified a few bugs (regarding RFCs), I prefer to wait for your comments before going forward.

Cheers,

yoshuawuyts · 2020-10-17T15:49:37Z

@ririsoft it's been a few weeks since we've actually released the patches you linked above, and we're definitely on track in that direction. Apologies that it took a while to figure out how we wanted the headers structured. If you're still interested in adding typed headers for Range that'd be a very welcome contribution! It'd be so good to have http-rs/tide#687 too!

ririsoft · 2020-10-19T07:54:17Z

@ririsoft it's been a few weeks since we've actually released the patches you linked above, and we're definitely on track in that direction. Apologies that it took a while to figure out how we wanted the headers structured. If you're still interested in adding typed headers for Range that'd be a very welcome contribution! It'd be so good to have http-rs/tide#687 too!

Sure, I followed the discussions and will rewrite this PR with latest design. I am just a bit busy at work those days, I plan to come back on this next week.
Cheers.

ririsoft · 2020-10-30T16:07:21Z

I have resumed the work here: https://github.com/ririsoft/http-types/tree/headers-range-wip
and will squash/rebase/merge back to this PR once ready for review (a couple of days).

ririsoft · 2020-11-22T13:44:12Z

Sorry guys, I worked less on this than initially expected but I am progressing and almost there.

ririsoft · 2020-11-24T17:46:12Z

Hi @yoshuawuyts,

The work is ready for review on my side.
You might want to flag this as unstable so that we can experiment the API with Tide before freezing it.

I am very open to any suggestions of course. Also the RFC specification is not straightforward, you might want to have a 4 eyes check on my implementation.

Cheers.

ririsoft · 2020-11-25T09:16:30Z

src/range/accept_ranges.rs

+    const NONE: &'static str = "none";
+
+    /// Create a new AcceptRange which does not accept range requests.
+    pub fn new() -> Self {


new setting the value to "none" might be confusing here since the widely used default is "bytes". Maybe we should name it new_none or a better idea ?

ririsoft · 2020-11-25T09:18:15Z

src/range/accept_ranges.rs

+    }
+
+    /// Returns the supported unit if any.
+    pub fn other(&self) -> &Option<String> {


I am not a fan of returning &Option<String>>. Maybe Option<&str> is better ?

ririsoft · 2020-11-25T09:19:28Z

src/range/accept_ranges.rs

+        let s = match self.other {
+            Some(ref other) => other.as_str(),
+            None if self.bytes => Self::BYTES,
+            None => Self::NONE,
+        };


This could be factorized with the Display implementation. Let me known if you want that.

ririsoft · 2020-11-25T09:23:27Z

src/range/byte_content_range.rs

+
+    /// Create a ByteRanges from a string.
+    pub(crate) fn from_str(s: &str) -> crate::Result<Self> {
+        let fn_err = || {


There is no error code specified for invalid Content-Range header in a response. I chose to use the same error code as the one used for bad range requests. Maybe I should add this as a comment ?

ririsoft · 2020-11-25T09:26:28Z

src/range/byte_ranges.rs

+        self.ranges.get(0).copied()
+    }
+
+    /// Validates that the ranges are withing the expected document size.


Typo: within

yoshuawuyts

Hey ririsoft, thanks for updating this PR. I spent some time reviewing it today, and I have some thoughts.

Bytes Prefix

When reviewing this PR I was looking for the Range header, but instead I found ByteRange and ByteRanges. It wasn't immediately clear to me which is which, and for consistency with the other typed headers I would've expected the header name to exist without the Byte prefix.

This has some implications for the API, but I'd like to find a way for us to drop the Byte prefix from the header names.

Assume the `RangeUnit` is always bytes?

The RFC states that RangeUnits can be expected to be expanded in the future through RFC. Though in the past 6 years this has not happened, which makes it questionable to which degree we need to consider this. We could make the assumption that the only two types that may ever be sent are "none" and bytes.

What if we assume the units may be extensible?

If we don't create specific structs for BytesRange, etc. then this route means points us more directly towards enums. We could define a RangeDirective enum which is #[non_exhaustive] and contains None and Bytes members. That is the type that's communicated in the Range headers.

Additionally we can add a From<(u64, u64)> impl to construct a RangeDirective instance, and make the bound in Range::push take impl Into<RangeDirective>.

Extensibility of range units

However this leads to an issue: currently the range set of headers is hard-coded to only operate on bytes. Hence the prefix. I guess the intended path to add support for more range units is to add additional FooRange and BarRange headers.

Instead I would like to suggest switching it around: create a RangeUnits enum which is tagged #[non_exhaustive] and None and Bytes members, and then have Range , ContentRange, and AcceptRanges headers. This will allow us to add more range types in the future without breaking anything.

AcceptRanges constructor

It seems AcceptRanges is working around not having a RangeUnits enum. I think AcceptRanges::new should take a RangeUnits enum, and we can keep a AcceptRanges::new_bytes as a shorthand.

suffix-length

I was reading about ranges on mdn and I'm confused about how the suffixes are exposed in our API? Do you have a good sense of how they work, and how we could implement them?

Conclusion

Despite having feedback on some of the design, I think we're very well on our way to getting where we need to be! — I'm incredibly excited for this work, and how it'll enable us to implement http-rs/tide#687. Thanks so much for your work on this!

ririsoft · 2020-11-28T18:25:01Z

Hi @yoshuawuyts thank you very much for your review. I am pretty much in line globally.

Assume the RangeUnit is always bytes?

The RFC states that RangeUnits can be expected to be expanded in the future through RFC. Though in the past 6 years this has not happened, which makes it questionable to which degree we need to consider this. We could make the assumption that the only two types that may ever be sent are "none" and bytes.

I initially worked on a version with a RangeUnit enum, like the one you mentioned, opening the door to other range units if the RFC evolves in the future. But I realized that this is a lot of boilerplate for every day use since only bytes unite are used today in practice. So I went for a compromise with this Byte prefix thing which is not very much satisfying at the end, I agree.

Instead I would like to suggest switching it around: create a RangeUnits enum which is tagged #[non_exhaustive] and None and Bytes members, and then have Range , ContentRange, and AcceptRanges headers. This will allow us to add more range types in the future without breaking anything.

I would go for that design too, but I am not sure about having None as an enum variant: None is not a range unit type. It is a possible value for the AcceptRanges header to notify that range requests are not accepted, but it is not used otherwise for Range and ContentRange headers. We could have AcceptRanges::from_headers return Ok(None) if the Accept-Range header has the none value, without any impact client side.

However, by removing None from the enum we would have only one variant which looks strange too.

A third option would be something like:

enum RangeUnit {
    Bytes
    Other(String)
}

This allows for usage of non standard units, but not great either.

If we go for the RangeUnit design we will need to handle it with the Range and ContentRange headers too. Those type do not only returns a range unit but also a range set (ByteRanges in my current implementation) and a single Range. At the end I do not see how we can avoid several if let and match code for the end user using only bytes unit in practice. Do you have something in mind that could help here ?

suffix-length

I was reading about ranges on mdn and I'm confused about how the suffixes are exposed in our API? Do you have a good sense of how they work, and how we could implement them?

You can see them as relative seek from either start or end. So far I do not provide further helper over it, I am just exposing them with an optional value. In Tide we will have to match over them to either SeekFrom::Start or SeekFrom::End. We can extend the type with further helper functions if need be without a breaking change I believe.

Conclusion

Despite having feedback on some of the design, I think we're very well on our way to getting where we need to be! — I'm incredibly excited for this work, and how it'll enable us to implement http-rs/tide#687. Thanks so much for your work on this!

Thank you for your great feedback. This leaves us with the following choices:

We provide implementation for bytes unit only, and we fully assume a breaking change will probably need to be introduced if a new unit is added to the RFC. This is a limited risk at the benefit of simple code covering 100% of usage for the last 6 years.
We go for a RangeUnit enum #[non_exhaustive], at the benefit of allowing a source compatible introduction of a new unit, but at the risk to "complexify" the API for every day use.

Not an easy decision. A little daemon on my left shoulder says "come on, take the challenge and go with those damn enums, you are a rustacean or not ?", a little angel on my right shoulder says "come on, you know this is over-engineering, right ?"

Do you want me to have a try with the RangeUnit enum design to see how it goes, or you want to listen to the little angel ?
In the latter case I will rename ByteRanges to Range and ByteRange to RangeItem (or any better name you might have), and simplify the API to handle bytes only.

What do you think ?

After this we need to talk about this comment which could impact the design of the If-Range header. But one discussion at a time :)

yoshuawuyts · 2020-12-04T19:04:37Z

Assume the RangeUnit is always bytes?

However, by removing None from the enum we would have only one variant which looks strange too.

I think that's fine though, especially if we mark the enum as #[non_exhaustive] and maybe add a comment this may be extended in the future.

At the end I do not see how we can avoid several if let and match code for the end user using only bytes unit in practice. Do you have something in mind that could help here ?

Not necessarily; I think at this layer it's more important that the implementation is correct and understandable, than necessarily providing the best end-user ergonomics. Then in Tide we can use the implementation we've defined here to truly make it ergonomic.

suffix-length

We can extend the type with further helper functions if need be without a breaking change I believe.

Fantastic! Happy to punt this topic for now then (:

Conclusion

Do you want me to have a try with the RangeUnit enum design to see how it goes?

I think this is indeed the route I'd like to see explored. Thanks so much for taking the time to work through this; I feel I say this everytime it comes up, but I'm incredibly excited for this!

This introduce a new 'range' module to support encoding, decoding and validation of HTTP range requests.

ririsoft · 2020-12-07T15:56:36Z

I think this is indeed the route I'd like to see explored.

Here we are ! The new design is enum based but different than what we discussed above. I would like to apologies for that and hope you won't be too disappointed.

I came to this design because each header exposes different types that I could not unify across one single Unit enum.
In this design each header is an enum, all with a Bytes variant but they all differ in the inner types exposed. All bytes related implementation belongs to a dedicated bytes module.

There is a few points that I was not sure about, such having new constructors taking a impl Into<...> for each header.

Do not hesitate to challenge my proposal I am fully open for suggestions.

ririsoft · 2020-12-08T07:56:25Z

src/range/range.rs

+        let fn_err = || {
+            Error::from_str(
+                StatusCode::BadRequest,
+                "Invalid Range header for byte ranges",


In my previous commits I add several error cases to handle, let closure here is no more justified I should move this block to the match block below

ririsoft · 2020-12-08T07:59:13Z

src/range/bytes.rs

+    }
+}
+
+impl FromStr for BytesRange {


Maybe I should just provide a pub(crate) fn from_str implementation and not do it as impl FromStr to remain consistant with the rest of the code.

ririsoft · 2020-12-08T08:01:19Z

src/range/bytes.rs

+    /// # fn main() -> http_types::Result<()> {
+    /// #
+    /// use http_types::range::{BytesRange, BytesRangeSet, Range};
+    /// use http_types::{Method, Request, StatusCode, Url};
+    /// use std::convert::TryInto;
+    ///
+    /// let mut range_set = BytesRangeSet::new();
+    /// range_set.push(0, 500);
+    /// let range = Range::Bytes(range_set);
+    ///
+    /// let mut req = Request::new(Method::Get, Url::parse("http://example.com").unwrap());
+    /// range.apply(&mut req);
+    ///
+    /// let range = Range::from_headers(req)?.unwrap();
+    ///
+    /// if let Range::Bytes(range_set) = range {
+    ///     let err = range_set.match_size(350).unwrap_err();
+    ///     assert_eq!(err.status(), StatusCode::RequestedRangeNotSatisfiable);
+    /// }
+    /// #
+    /// # Ok(()) }


I could make this example shorter without using a Request

yoshuawuyts · 2020-12-18T14:52:55Z

@ririsoft In #180 (review) I proposed to go for RangeUnits + RangeDirective types so that one can hold the unit definition, and the other can work as the direct inputs to the headers.

In #180 (comment) it sounds like you attempted to unify these into a single type, which then didn't work out -- am I reading that correctly?

ririsoft · 2020-12-20T13:36:31Z

@ririsoft In #180 (review) I proposed to go for RangeUnits + RangeDirective types so that one can hold the unit definition, and the other can work as the direct inputs to the headers.

In #180 (comment) it sounds like you attempted to unify these into a single type, which then didn't work out -- am I reading that correctly?

Hello @yoshuawuyts ,
I do not understand. Were you able to review my latest updates with enum ?
Does it matches your expectations ? Otherwise would you be able to provide some code snippets for one of the header type please. I really don't see where you want to go, I am really sorry about that.

yoshuawuyts · 2021-01-19T08:26:40Z

@ririsoft that's all good! -- I think what I may do is fork your PR, and then file a patch against your PR. I think perhaps at this point that may be the easiest way to communicate what I mean 😅

ririsoft · 2021-01-19T08:48:17Z

@ririsoft that's all good! -- I think what I may do is fork your PR, and then file a patch against your PR. I think perhaps at this point that may be the easiest way to communicate what I mean 😅

Yeah we are reaching the limit of asynchronous talk here I am afraid :). A small patch says a lot more sometimes 👍

Also I had a look at what they did on hyperium: https://docs.rs/headers/0.3.3/headers/struct.Range.html.
Interestingly they are using the RangeBounds type from std. It may look like a good idea but I am not sure how you can represent a "negative bound" such as "the last five bytes" that you can have with http range requests such -5. To be further discussed with your patch ...

ririsoft · 2024-05-15T14:38:41Z

I am not using http-rs anymore and I am thus not interested in working on this PR further.

ririsoft force-pushed the headers-range branch from bf88870 to 03d02ec Compare June 9, 2020 21:37

ririsoft mentioned this pull request Jun 10, 2020

[tracking] typed headers #99

Open

72 tasks

jbr added the enhancement New feature or request label Jun 19, 2020

goto-bus-stop changed the base branch from master to main August 9, 2020 12:58

ririsoft mentioned this pull request Aug 26, 2020

Serve content with support for HTTP conditionals and range requests http-rs/tide#687

Closed

12 tasks

ririsoft force-pushed the headers-range branch from 03d02ec to c5fd4e7 Compare November 24, 2020 17:35

ririsoft commented Nov 25, 2020

View reviewed changes

yoshuawuyts changed the title ~~New range request module~~ Add range submodule Nov 25, 2020

yoshuawuyts requested changes Nov 27, 2020

View reviewed changes

ririsoft added 2 commits December 7, 2020 16:37

new range request module

6897151

This introduce a new 'range' module to support encoding, decoding and validation of HTTP range requests.

new conditional submodule for if_range

9e0b94c

ririsoft force-pushed the headers-range branch from c5fd4e7 to 9e0b94c Compare December 7, 2020 15:37

ririsoft commented Dec 8, 2020

View reviewed changes

ririsoft closed this May 15, 2024

Conversation

ririsoft commented Jun 9, 2020

Uh oh!

Fishrock123 commented Jun 10, 2020

Uh oh!

ririsoft commented Jun 10, 2020

Uh oh!

ririsoft commented Aug 24, 2020

Uh oh!

yoshuawuyts commented Oct 17, 2020

Uh oh!

ririsoft commented Oct 19, 2020

Uh oh!

ririsoft commented Oct 30, 2020

Uh oh!

ririsoft commented Nov 22, 2020

Uh oh!

ririsoft commented Nov 24, 2020

Uh oh!

ririsoft Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ririsoft Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ririsoft Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ririsoft Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ririsoft Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts left a comment

Choose a reason for hiding this comment

Bytes Prefix

Assume the RangeUnit is always bytes?

What if we assume the units may be extensible?

Extensibility of range units

AcceptRanges constructor

suffix-length

Conclusion

Uh oh!

ririsoft commented Nov 28, 2020

Assume the RangeUnit is always bytes?

suffix-length

Conclusion

Uh oh!

yoshuawuyts commented Dec 4, 2020

Assume the RangeUnit is always bytes?

suffix-length

Conclusion

Uh oh!

ririsoft commented Dec 7, 2020

Uh oh!

ririsoft Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

ririsoft Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

ririsoft Dec 8, 2020

Choose a reason for hiding this comment

Uh oh!

yoshuawuyts commented Dec 18, 2020

Uh oh!

ririsoft commented Dec 20, 2020

Uh oh!

yoshuawuyts commented Jan 19, 2021

Uh oh!

ririsoft commented Jan 19, 2021

Uh oh!

ririsoft commented May 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

ririsoft Nov 25, 2020 •

edited

Loading

Assume the `RangeUnit` is always bytes?

Assume the `RangeUnit` is always bytes?