search

remotestorage / spec

remoteStorage Protocol Specification
https://tools.ietf.org/html/draft-dejong-remotestorage
87 stars 5 forks source link

Optionally support byte ranges support on PUT requests. #124

Open michielbdejong opened 8 years ago

michielbdejong commented 8 years ago

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.

ghost commented 8 years ago

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.

ghost commented 8 years ago

117

michielbdejong commented 8 years ago

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 commented 8 years ago

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

ghost commented 8 years ago

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 commented 8 years ago

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.

ghost commented 8 years ago

Now we're getting somewhere :)

michielbdejong commented 8 years ago

https://developers.google.com/drive/web/manage-uploads#resumable refers to http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.16 for Content-Range headers on uploads.

ghost commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.

ghost commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

ghost commented 8 years ago

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 commented 8 years ago

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:

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: https://github.com/remotestorage/spec/issues/124#issuecomment-159996612

untitaker commented 8 years ago

FWIW I've added a test to the api spec suite that asserts that PUT with Content-Range fails: https://github.com/remotestorage/api-test-suite/commit/5a4bc19adb8c87e4d4403709e6d3a9181ce55844

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