PATCH method implemented incorrectly and documentation is inconsistent.

[unikmhz]

I'm currently implementing an integrated *DAV server, and decided to make it understand a SabreDAV PATCH request format. But docs have this to say on specifying byte ranges:

Just like the HTTP Range header, the specified bytes is a 1-based index. This means that 'bytes=0-3' is invalid, as there is no '0th byte'.

Here is my question though: According to RFC 2616 section 14.35.1 "Byte offsets start at zero". Which is not what current SabreDAV source does (from lib/Sabre/DAV/PartialUpdate/Plugin.php):

$etag = $node->putRange($body, $start-1);

So am I missing something here?

[unikmhz]

In addition to original question, I now see that implementation of putRange method (at least in FSExt backend) also decrements offset by 1. Is that a bug, or maybe I don't understand something?

Also, according to docs:

If the start byte is omitted, 1 is assumed

But code disagrees:

$start = ($range[0])?$range[0]:0;

[evert]

I messed this up, and pretty badly. This never worked correctly, and I don't know why the tests don't point this out.

The plugin is in line with the documentation, but then for some reason FSExt does indeed reduce the offset by 1.

If an offset of '1' would be specified, FSExt would turn this into -1, which is obviously wrong.

It's pretty great that you'd want to follow that document, but as you can see there's embarrassingly some problems with it, and it must not have been actively used by anyone so far.

Since that's the case, I will change the spec to be 0-indexed, and fix all the builds to fall in line with that.

[evert]

Bit of a WTF moment :(

[unikmhz]

Thank you for your clarification.

PS: If I read the code correctly, currently if range start is omitted, then final offset which is passed to fseek() will be -2

[unikmhz]

Also, I realized just now, that if you follow the HTTP spec, you can't really omit range start, because bytes=-10 means 10 bytes from current file end, and bytes=- is invalid.

[evert]

I'll re-read the entire specification and make 100% sure I got it right this time.

Note that I'll be using this doc: http://tools.ietf.org/html/draft-ietf-httpbis-p5-range-26

[evert]

@unikmhz i revamped the patch document to match the HTTP Range header. This is how I intend to fix the implementation. Frankly it was a bit of a mess, and it sucks to have to do this in a small release.

But I would love for you to review it and tell me that it matches your expectation. Just in case I messed it up:

http://sabre.io/dav/http-patch/

[unikmhz]

Seems to be good, I implemented mostly the same behaviour. Some notes:

• A case of -0 is ambiguous and is not supported by HTTP protocol libraries (i.e. python's WebOb parses this as 0-). Maybe add another syntax for appending, like X-Update-Range: append? I know it's not that pretty, but it can work.
• Can a spec explicitly forbid multiple ranges? Like bytes=2-3,5-10,20-. Because of poor library support, and more importantly because of several security issues.
• bytes=0 syntax is invalid according to both old and new stds, and doesn't parse with existing libraries. For all bytes starting with 0th one, use bytes=0-. For a single 0th byte use bytes=0-0.
• Maybe add a rule in the doc which specifies that "If both start and end offsets are given, than both must be non-negative, and the end offset must be greater or equal to the start offset".

[unikmhz]

Concerning append: alternative method is to maybe add another header, and require one or the other.

[evert]

Good ideas all around :) I will take a look at the validity of bytes=0 as well.

[unikmhz]

You know, the variant with a different header is more to my liking, because code/library which expects a range spec in a particular header will silently drop all non-conforming values. So separate header removes the need to access raw header data for users of "HTTP protocol parser libraries" and makes it more error-resistant.

EDIT: also, in this case the spec must prohibit sending both headers in one request.

[evert]

Yea I like the idea as well :)

[evert]

Updated the spec with all the suggestions.

I did end up with X-Update-Range: append because I felt it was more in line with how the Range header is also intended to be used.

I'm now starting work to ensure that sabre/dav also shows this behavior.

[unikmhz]

Thanks, I'm updating my code now.
Doc cosmetic fix: bytes=12- is not the very last example now ;)

Another question: what are header/status requirements for a server reply. I'm currently sending 204 status and an updated etag.

EDIT: grammar fail on my end - sorry - spec sentence that I quoted uses "than" instead of "then".

[evert]

I just updated the doc with the status codes I'm sending back. Had update ready minutes before your last comment ;)

Let me know if that works for you. This bug is now fixed, so I'm closing it.