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

From: Mark Nottingham <[email-masked]>
Date: Thu, 2 Sep 2004 09:33:05 -0700

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)"

* 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.

* 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.

* 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?)

* 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?

* 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."

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

* 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).

Cheers,

--
Mark Nottingham     http://www.mnot.net/

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 2 Sep 2004 10:00:32 -0700

One other thing;

* The nature of the Accept-Patch header should be explicitly specified 
in terms of the classifications defined in RFC2616; e.g.,

"The Accept-Patch entity-header field is used to advertise the patch 
formats supported by a resource, in response to an OPTIONS request. It 
is an end-to-end header, and its field-value..."

Cheers,

---

From: Lisa Dusseault <[email-masked]>
Date: Thu, 2 Sep 2004 10:01:02 -0700

> 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.

> 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.

> 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.

> is this intentionally vague?)

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

> code." Wouldn't 415 Unsupported Media Type be more appropriate, given 

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.

> 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

---

From: Mark Nottingham <[email-masked]>
Date: Thu, 2 Sep 2004 10:20:02 -0700

Thanks for the quick answers! A couple of responses below.

On Sep 2, 2004, at 10:01 AM, Lisa Dusseault wrote:

>> 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 

Typically not, but I can imagine people building PATCH-aware 
intermediaries that did some caching. It's not a big deal, I was just 
curious if there was any particular reason behind the (trival) 
inconsistency with 2616.

> this time that anything I could describe in PATCH would probably get 
> in the way.

Understood. Would it be helpful to state "The interaction between PATCH 
and server-driven content negotiation is not defined by this 
specification."?

>> 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."
>
> rely on OPTIONS * to find out if PATCH is supported because in 
> practice clients can't rely on OPTIONS * for extension discovery.

OK. My first reading of it was that servers shouldn't send Allow: PATCH 
in response to OPTIONS *. Maybe start the sentence with "Some servers 
may not advertise support for PATCH in response to an OPTIONS * request 
because..." ? Just a thought.

> I believe this depends on the patch format used.  I am not certain 
> what the idempotency of 'gdiff' is, for example.

If that's the case, I'd think that PATCH should be, by default, 
non-idempotent (in the same way that a sequence of POSTs on some 
particular resources might be idempotent, even when the method itself 
is not).

> 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.

Yes, but it should also use the language defined in RFC2616, so people 
can relate its operation to the concepts defined there.

Cheers,

---

From: Julian Reschke <[email-masked]>
Date: Thu, 02 Sep 2004 20:26:57 +0200

Lisa Dusseault wrote:

> This is the latest version of PATCH. Compared to the -04 draft:
> - Moved sections for interoperability with RFC3744 (ACL) and RFC3230 
> (Instance digests) to new section
> - now has Security Considerations and IANA Considerations sections
> - removed references in abstract
> - No longer uses "DAV:" namespace for any new syntax, instead defines 
> own namespace & registers with IANA
> 
> Once again, if I don't get feedback that requires changes beyond typos, 
> I intend to submit this to standards track for consideration as a 
> Proposed Standard in a week or two. Except for the fact that this is not 
> an official WG document for either WebDAV or HTTP, this constitutes kind 
> of a WG last call.
> 
> All the feedback so far in both groups has been great but because 
> there's been fairly close review I don't expect a lot more changes. I'm 
> still hoping to hear from people who intend to implement this if it 
> becomes an RFC.
> 
> Lisa

OK, here are my updated comments:

C-03-02, Section 2

"This specification only defines usage of the platform-portable gdiff
[9] format identified as 'application/gdiff'."

As far as I can tell, there is no registration for "application/gdiff". 
Options:

1) do the registration in this RFC (appendix)
2) have a separate RFC for the gdiff format, including a registration
3) workaround the issue by saying that in the absence of a MIME type, 
the gdiff
format is assumed
4) ...?

(Personal preference: #1)

-04 update: I hear that Lisa is trying to get the MIME type registered. 
I don't think this will work as long as the documentation only resides 
in a W3C note. If PATCH is really to become an IETF standards-track 
document at some point of time, normative parts of it (like the GDIFF 
documentation) need to live in an acceptable place; I don't think a 
vendor note to the W3C is suitable; for instance, what do we know about 
IP issues with it?

C-03-08, Section 3.2.2

2) Make this optional for servers that do not care about WebDAV.

3) Stick to RFC3253's terminology (the names identify conditions, not 
errors, thus

s/DAV:delta-format-unsupported/p:delta-format-supported/
s/DAV:delta-format-forbidden-on-resource/p:delta-format-allowed-on-resource/
s/DAV:delta-format-badly-formatted/p:delta-documented-well-formatted/
s/DAV:delta-empty-resource/p:delta-format-applies-to-empty/
S/DAV:patch-result-invalid/p:patch-result-valid/

-05 update: Right now it's unclear whether this is normative. If it is,
the reference to RFC3253 needs to be moved into the Normative References
section.

E-05-01, Abbreviations

...should be expanded the first time they appear in the next.

E-05-02, Security considerations

The one mentioned doesn't seem to be a *Security* consideration at all. 
It's more about correctness/robustness, isn't it?