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

Allow Range and Content-Range headers to contain any valid range unit #180

Closed
wants to merge 1 commit into from

Conversation

ltvolks
Copy link
Contributor

@ltvolks ltvolks commented Nov 25, 2014

  • Range units other than 'bytes' may be provided as part of a Range
    or Content-Range header
  • Updated repr() tests for Range and ContentRange to actually test
    the return value of the repr call (previous test was only asserting
    that repr() returned a 'truthy' value)
  • Updated Range repr to return a string representation enclosed in angle
    brackets, matching the style of most other WebOb classes
    (vs. a string that may be able to be passed to eval)
  • Fixes Range header should parse Range-Units other than 'bytes' #177

@ltvolks ltvolks force-pushed the 177-range-units branch 2 times, most recently from b10550e to 6fae1ac Compare November 25, 2014 23:03
- Range units other than 'bytes' may be provided as part of a Range
  or Content-Range header
- Updated __repr__() tests for Range and ContentRange to actually test
  the return value of the repr call (previous test was only asserting
  that __repr__() returned a 'truthy' value)
- Updated Range repr to return a string representation enclosed in angle
  brackets, matching the style of most other WebOb classes
  (vs. a string that may be able to be passed to `eval`)
- Fixes Pylons#177
@ltvolks
Copy link
Contributor Author

ltvolks commented May 23, 2015

@bertjwregeer - Would you be able to review this PR at your convenience? See #177 for rationale. Thanks!

@digitalresistor digitalresistor added this to the Version 1.6 milestone Dec 29, 2015
@digitalresistor
Copy link
Member

I have been really conflicted on whether I wanted to merge this or not to be completely honest. It is not part of any of the standard specs, but at the same time I completely understand the utility and think it would help when I am building my own applications.

In reviewing this though, how does this interact with https://github.com/Pylons/webob/blob/master/webob/response.py#L1079? How does this work with sending a conditional response (WebOb conditional response that is).

I need more time to understand the implications, and possibly even add a safeguard to the response code that the range unit is bytes.

@digitalresistor digitalresistor removed this from the Version 1.6 milestone Jan 29, 2016
@ltvolks
Copy link
Contributor Author

ltvolks commented Jan 29, 2016

@bertjwregeer - The only range unit defined by the spec is "bytes", but the grammar allows for other range units:

range-unit = bytes-unit | other-range-unit
bytes-unit = "bytes"
other-range-unit = token

The Range Requests memo elaborates:

Although the range request mechanism is designed to allow for
extensible range types, this specification only defines requests for
byte ranges.

2.2. Other Range Units.

Range units are intended to be extensible. New range units ought to
be registered with IANA, as defined in Section 5.1.

 other-range-unit = token

@ltvolks
Copy link
Contributor Author

ltvolks commented Jan 29, 2016

@bertjwregeer - I do see your concern with https://github.com/Pylons/webob/blob/master/webob/response.py#L1079

The conditional app iterates over a range of actual bytes derived from content_length. The response will not match client expectations if an alternate range unit were specified.

In other words, if the client requests Range items=0-9, the conditional app would return the first 10 bytes of the response body. Also the Content-Range header is set using the content_length, so the response implicitly assumes bytes and results in a mismatch:

Content-Range: items 0-9/{{content_length}}

I don't use the conditional app, so we haven't had an issue with the patch because we handle generating the Content-Range and response codes in our pagination helper.

One approach might be to explicitly disallow anything but "bytes" as a range unit when using the conditional app.

@digitalresistor
Copy link
Member

I am closing this pull request for now. Until WebOb's Content-Range is fixed so that conditional_response continues to work I am not sure this is the right change to make. Please re-open if the concerns noted above are fixed.

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.

Range header should parse Range-Units other than 'bytes'
2 participants