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

Optionally support byte ranges support on PUT requests. #124

Open
michielbdejong opened this Issue Nov 26, 2015 · 19 comments

Comments

3 participants
@michielbdejong
Copy link
Member

michielbdejong commented Nov 26, 2015

In specs 02 and 03 we had the option to announce support for byte ranges on GET requests through WebFinger.

In spec 06 we're moving this to the Accept-Ranges response header, to better comply with HTTP.

For GET requests this is fine, since a client can just try, and get the whole resource if its request header was ignored. For PUT requests this is a bit more complicated (also, how would you announce "supported for GET but not for PUT" in an OPTIONS response?).

So we should see if we can find a better way to announce byte range support for GET / PUT / both / none.

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

I'd advise removing the whole range request stuff from the specification. It is of no consequence to the client(s) and there is no standard for PUT, so just remove it all.

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

@michielbdejong

This comment has been minimized.

Copy link
Member

michielbdejong commented Nov 26, 2015

In specs 02 and 03 we had ...

Correction: it's still in there: https://github.com/remotestorage/spec/blob/master/source.txt#L453-L456

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

We might as well just say that servers "MAY implement rfc7233" and be done with it...

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

But but but. You guys don't understand :)

This RFC you mention is, correct me if I am wrong, only for retrieving data, i.e. GET, not for sending it from client to server, i.e. PUT.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

Sorry, didn't realize that.

I've found this remark in RFC7233:

A server MUST ignore a Range header field received with a request method other than GET.

So we can't define PUT ranges without violating the HTTP RFC.

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

Now we're getting somewhere :)

@michielbdejong

This comment has been minimized.

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

Is this also part of the newer HTTP RFCs? If not, let's explicitly mention RFC 2616 section 14 in the RS spec when mentioning PUT range requests.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

Nevermind my comment! It seems that Content-Range is used in PUT as a request header, and in GET as a response header. In other words, Content-Range is for describing the payload's range of a request or a response, Range is for requesting a particular range of the payload.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

If not, let's explicitly mention RFC 2616 section 14 in the RS spec when mentioning PUT range requests.

Not sure it's not a good idea to reference both document X, and the document that made X obsolete.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

Is this also part of the newer HTTP RFCs?

So, no idea. It seems that the authors of RFC 7233 themselves have not thought of this usecase.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

And then, Mark Nottingham himself says that Content-Range is not valid in requests. So I guess that the Google Drive devs have exploited a loophole in RFC 2616 to implement a very useful feature. I'm saying loophole here because the definition of Content-Range in RFC 2616 doesn't seem to talk about it being a request header either. I see now how Content-Range is valid under RFC 2616, but it doesn't change anything on the fact that RFC 7233 doesn't mention it being a request header at all.

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

There are also alternative approaches:

Especially resumablejs is interesting! It looks like something I'd like to implement on the server. I don't really like the Google API because you have to send a PUT/POST first with metadata and then only start the upload and is not at all compatible with RFC 2616...

But I guess we should agree on something, maybe the RFC 2616 variant would be okay, but I don't understand then why everyone reinvents the wheel instead of using this RFC approach?

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

For now I think we should remove any mention of partial-content PUT, and consider re-inclusion at a later point.

but I don't understand then why everyone reinvents the wheel instead of using this RFC approach?

Because there is no way it is legal under RFC 7233

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 26, 2015

RFC 7231 has this:

An origin server that allows PUT on a given target resource MUST send a 400 (Bad Request) response to a PUT request that contains a Content-Range header field (Section 4.2 of [RFC7233]), since the payload is likely to be partial content that has been mistakenly PUT as a full representation. Partial content updates are possible by targeting a separately identified resource with state that overlaps a portion of the larger resource, or by using a different method that has been specifically defined for partial updates (for example, the PATCH method defined in [RFC5789]).

https://tools.ietf.org/html/rfc7231#section-4.3.4

@fkooman

This comment has been minimized.

Copy link
Member

fkooman commented Nov 26, 2015

Yeah, it seems PATCH is the only way to do this properly. But that would require defining some update message to update binary files, ideally without base64 encoding them ;)

I also agree on removing PUT with range from the spec for now as noone implemented it, and if they did it would not be interoperable anyway.

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 27, 2015

For completeness:

We could also use a custom HTTP method for the partial PUT as Google Drive
supports it. That'd be completely conformant, but almost certainly not a good
idea for practical reasons:

  • For WebDAV's custom methods, you have to explicitly enable them in nginx, and
    likely other servers. This will also be required here.
  • PHP's builtin server shits all over unknown HTTP methods, see
    https://bugs.php.net/bug.php?id=69655

On Thu, Nov 26, 2015 at 01:39:03PM -0800, François Kooman wrote:

Yeah, it seems PATCH is the only way to do this properly. But that would require defining some update message to update binary files, ideally without base64 encoding them ;)

I also agree on removing PUT from the spec for now as noone implemented it, and if they did it would not be interoperable anyway.


Reply to this email directly or view it on GitHub:
#124 (comment)

untitaker added a commit to remotestorage/api-test-suite that referenced this issue Nov 27, 2015

@untitaker

This comment has been minimized.

Copy link
Member

untitaker commented Nov 27, 2015

FWIW I've added a test to the api spec suite that asserts that PUT with Content-Range fails: remotestorage/api-test-suite@5a4bc19

This seems like a non-controversial step to me since the HTTP spec requires it with a MUST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment