Re: I-D ACTION:draft-dusseault-http-patch-05.txt

On Sep 2, 2004, at 9:33 AM, Mark Nottingham wrote:

> Hi Lisa,
>
> Just a few notes (mostly nits) from reading -05. I haven't tracked the 
> entire PATCH discussion, so feel free to tell me that a subject has 
> already been discussed if I need to do more homework.
>
> * 3.1 - "The target resource's content type MUST be one to which the 
> patch format applies." The applicability of a patch format isn't 
> well-defined in this document. I'd suggest either not making this a 
> RFC2119-level requirement, or parenthetically clarifying what's meant 
> by "applies;" for example, "... format applies (e.g., some textual 
> patch formats can't operate upon binary resources)"

I guess what I'm trying to explain here is that the server can reject a 
PATCH request if it can't apply the patch format to the resource's 
content type -- e.g. it could reject a XSLT transform if the resource 
is not XML.  Actually, I think this is sufficiently covered in the 
section later on 'delta-format-forbidden-on-resource', so I could just 
delete this sentence.

>
> * 3.1 "A cache MAY mark the resource identified in the Request-URI as 
> stale if it sees a successful response to the PATCH request." First of 
> all, a minor nit; I think this should be "A cache MAY mark entities 
> associated with the resource identified..." Secondly, I'm 
> uncomfortable with the MAY here; is there any reason it isn't a 
> stronger requirement, if the cache is PATCH-aware? RFC2616 section 
> 13.10 sets the bar at SHOULD.

I don't think that would really help or matter, as I don't expect 
caches to be PATCH-aware.

>
> * How does PATCH interact with server-driven content negotiation? I 
> might expect that if I PATCH something with an Accept header that was 
> sufficiently specific, I'd be patching that particular representation.

An interesting thought, but authoring variants is so underspecified at 
this time that anything I could describe in PATCH would probably get in 
the way.

>
> * 3.2.1 "The server SHOULD provide a MD5 hash of the resource" -> "The 
> server SHOULD send the Content-MD5 header in responses to PATCH." (or, 
> is this intentionally vague?)

Not intentionally vague, no :)  I can make this change.

>
> * 3.2.2 "delta-format-unsupported: Used with 403 Forbidden status 
> code." Wouldn't 415 Unsupported Media Type be more appropriate, given 
> the current design?

Yes, you're right, and the XML element error is not needed for this 
case because it adds no more specificity.  nice catch.

>
> * 3.3 "OPTIONS * is not used to advertise support for PATCH because 
> the patch formats supported..." From context, it seems that you want 
> to say that OPTIONS * shouldn't return an Accept-Patch header. 
> However, the text above could be read as saying that OPTIONS * 
> shouldn't return PATCH in the Allow header. I'd suggest something like 
> "OPTIONS * is not used to advertise support for patch formats, because 
> those supported are likely to change from one resource to another."

This is a really tricky issue and has already been discussed on the 
list, with one outcome possibly that the next update to HTTP should 
clarify what OPTIONS * is all about.  The text is supposed to warn 
clients that a server might, or might not, include PATCH in the Allow 
header in response to OPTIONS *.  My intent is that clients will not 
rely on OPTIONS * to find out if PATCH is supported because in practice 
clients can't rely on OPTIONS * for extension discovery.

>
> * PATCH's safety and idempotency should be clearly specified.

I believe this depends on the patch format used.  I am not certain what 
the idempotency of 'gdiff' is, for example.  As for safety, the 
document does discuss (in security considerations) the risk that PATCH 
could corrupt a file and what safeguards are suggested to avoid this.

>
> * It might be helpful to point out in the introduction that whereas 
> PATCH is used to modify the body, PROPPATCH can be already used to 
> modify headers and other metadata (this gives a nice symmetry between 
> PATCH and delta encoding for the body, and then PROPPATCH and updates 
> of headers in the caching model).

This is an intentional omission.  I'm avoiding most mention of WebDAV 
so as not to give the impression that PATCH is a WebDAV extension.  
It's only a HTTP extension.

>
> Cheers,
>
Thanks,
Lisa

Received on Thursday, 2 September 2004 17:01:13 UTC