feat(headers): add `Range` header #560

lame-nickname · 2015-06-09T18:05:40Z

There are 2 TODOs left in the code. I wasn't sure what is the best way to handle these cases and I'd like to hear your feedback on that.

What I also wasn't sure about, are the names of values inside the RangeSpec enum, as well as the name of enum itself. Maybe you'll be able to come up with better ones ;)

seanmonstar · 2015-06-09T18:21:25Z

Woo, thanks! I'll look this over. Also pinging @pyfisch, since he's done a ton of work on the headers.

seanmonstar · 2015-06-09T18:24:15Z

src/header/common/range.rs

+/// Each 'Range' header can contain one or more RangeSpecs.
+/// Each RangeSpec defines a range of units to fetch
+#[derive(PartialEq, Clone, Debug)]
+pub enum RangeSpec {


I wonder, would it work out nicely to use std::ops::{Range, RangeFrom, RangeTo}? This could allow usage like let ranges = vec![ 0..1, 30..40];

I'm not sure if that makes sense. The thing is that Range and RangeFrom have their equivalent in the RangeSpec struct, but RangeTo doesn't. So we could implement Into<RangeSpec> for RangeTo and for Range, but that would leave out RangeFrom (and RangeSpec::Last), which IMO isn't very clean.

You are right, I hadn't read the Range spec before suggesting that. I didn't realize that bytes=-500 means the last 500 bytes, I assumed it means 0-500.

I've made the same assumption when I've started implementing it ;)

lame-nickname · 2015-06-09T19:57:53Z

Ok, so in the new commit, I've included all your suggestions regarding iterators and unnecessary allocations.

I've also made some decisions regarding the TODOs:
1). I think it makes sense to allow Range without any ranges (empty vector). In that case it will be transmitted as "bytes=" and it will be up to a server do decide what to do with it.
2) Now instead of skipping bad ranges during parsing, we'll return an Err, which basically means that the whole Range header is invalid.

seanmonstar · 2015-06-09T22:13:02Z

src/header/common/range.rs

+    headers.set(Range {unit: RangeUnit::Bytes, ranges: vec![]});
+
+    assert_eq!(&headers.to_string(), "Range: bytes=\r\n");
+}


mind adding the bench_header! macro usage down here?

seanmonstar · 2015-06-10T16:38:45Z

This looks good to go. Thanks! Would you mind squashing into a single commit? The changelog only really needs to report 1 new feature: added the Range header.

pyfisch · 2015-06-10T16:45:31Z

src/header/common/range.rs

+    type Err = ();
+
+    fn from_str(s: &str) -> Result<RangeSpec, ()> {
+        let mut parts = s.split("-");


You should probably use splitn(2) here. Imagine a malformed RangeSpec like 200-300-foobar. It would pass as valid.

pyfisch · 2015-06-10T17:13:16Z

Sorry for the late comments, some are a bit nitpicky but they should help to archieve a higher conformance with the spec 😉

lame-nickname · 2015-06-15T16:47:46Z

No worries. I'm glad you've reviewed the commits, as there were several things I've clearly missed reading the HTTP spec. Will do better next time! ;)

GitCop · 2015-06-15T20:11:48Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 287dc39
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 808ccdd
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

lame-nickname · 2015-06-15T20:25:52Z

Ok guys, I've made the requested changes. I wouldn't be surprised if they were unidiomatic, so let me know if that's the case.

There are 2 open issues:

i.e. bytes=-1,-1,-1 or bytes=1-,1-. According to the spec, there should be only one range spec of {RangeSpec::AllFrom, RangeSpec::Last} and it should always be at the end of the range set. Currently, it's not enforced and it's up to the application code to check that.
i.e. bytes=1-100,abc,200-. Should we just drop the invalid range specs and mark the whole range as valid (as long as there is at least one valid range spec) or mark the range as invalid, if there is at least one invalid range spec? Currently, we do the former.

pyfisch · 2015-06-15T20:41:23Z

src/header/common/range.rs

+use header::{Header, HeaderFormat, RangeUnit};
+use header::parsing::{from_one_raw_str, from_one_comma_delimited};
+
+/// `Range` header, defined in [RFC7233](https://tools.ietf.org/html/rfc7233#section-3.1)


GitCop · 2015-06-15T21:31:41Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 287dc39
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 808ccdd
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

GitCop · 2015-06-15T22:07:48Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 287dc39
- Commits must be in the following format: %{type}(%{scope}): %{description}
Commit: 808ccdd
- Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md

This message was auto-generated by https://gitcop.com

pyfisch · 2015-06-16T05:04:56Z

+1

feat(headers): add `Range` header

seanmonstar · 2015-06-16T17:44:03Z

Excellent work! Thanks for sticking with it.

seanmonstar reviewed Jun 9, 2015
View reviewed changes

pyfisch reviewed Jun 10, 2015
View reviewed changes

pyfisch reviewed Jun 15, 2015
View reviewed changes

feat(headers): add Range header

05c3199

lame-nickname force-pushed the master branch from ab0b0a1 to 05c3199 Compare June 16, 2015 16:31

seanmonstar added a commit that referenced this pull request Jun 16, 2015

Merge pull request #560 from lame-nickname/master

41823ca

feat(headers): add `Range` header

seanmonstar merged commit 41823ca into hyperium:master Jun 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(headers): add `Range` header #560

feat(headers): add `Range` header #560

lame-nickname commented Jun 9, 2015

seanmonstar commented Jun 9, 2015

seanmonstar Jun 9, 2015

lame-nickname Jun 9, 2015

seanmonstar Jun 9, 2015

lame-nickname Jun 9, 2015

lame-nickname commented Jun 9, 2015

seanmonstar Jun 9, 2015

lame-nickname Jun 9, 2015

seanmonstar commented Jun 10, 2015

pyfisch Jun 10, 2015

pyfisch commented Jun 10, 2015

lame-nickname commented Jun 15, 2015

GitCop commented Jun 15, 2015

lame-nickname commented Jun 15, 2015

pyfisch Jun 15, 2015

GitCop commented Jun 15, 2015

GitCop commented Jun 15, 2015

pyfisch commented Jun 16, 2015

seanmonstar commented Jun 16, 2015

feat(headers): add Range header #560

feat(headers): add Range header #560

Conversation

lame-nickname commented Jun 9, 2015

seanmonstar commented Jun 9, 2015

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

lame-nickname commented Jun 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar commented Jun 10, 2015

Choose a reason for hiding this comment

pyfisch commented Jun 10, 2015

lame-nickname commented Jun 15, 2015

GitCop commented Jun 15, 2015

lame-nickname commented Jun 15, 2015

Choose a reason for hiding this comment

GitCop commented Jun 15, 2015

GitCop commented Jun 15, 2015

pyfisch commented Jun 16, 2015

seanmonstar commented Jun 16, 2015

feat(headers): add `Range` header #560

feat(headers): add `Range` header #560