Subject: [APPS-REVIEW] review of draft-dusseault-http-patch-15

From: Eliot Lear <[email-masked]>
Date: Wed, 28 Oct 2009 19:00:38 +0100

I have been selected as the Applications Area Review Team reviewer for 
this draft (for background on apps-review, please see 
http://www.apps.ietf.org/content/applications-area-review-team).

Please resolve these comments along with any other Last Call comments 
you may receive. Please wait for direction from your document shepherd 
or AD before posting a new version of the draft.

Document: draft-dusseault-http-patch-15
Reviewer: Eliot Lear
Review Date: October 28, 2009

Review Summary:

This draft is almost ready for publication as a Proposed Standard, but 
should address the one major design issue below before proceeding.

Document Summary:

This document specifies an additional HTTP method, PATCH, which operates 
on a URI in a manner specific to document type.

Major Issues:

There is precisely one key design decision in this document that seems 
to me at all controversial:
Is media-type the appropriate key to determine the appropriate PATCH 
method?  Is that level of indirection correct?  This immediately limits 
the number of PATCH methods per media-type to one, and seems to me to 
provide a difficult route to change the PATCH method.  For example, 
suppose patch(1) became the standard for text/plain, and a new more 
expressive diff/patch came along.  How would that be communicated 
between client and server?  Let's assume for the moment that using a new 
media-type is not the best solution.

Minor issues:

No matter how the authors answer the above question, I would suggest 
that an additional IANA registry for PATCH methods be created.  The 
structure of that registry, however, will depend on the answer to the above.

Nits:

Section 2, 3rd para, last sentence:

> i.e., new resources may be
>     created, or existing ones modified, by the application of a PATCH

or removed?

---

From: Julian Reschke <[email-masked]>
Date: Wed, 28 Oct 2009 19:08:50 +0100

Eliot Lear wrote:
> ...
> ...

The media type of the payload of the patch request is completely 
orthogonal to the media type of the thing being patched. Yes, that means 
that we need to define one media type per patch format; but that's by 
design in HTTP, methinks.

> ...

BR, Julian

---

From: Eliot Lear <[email-masked]>
Date: Wed, 28 Oct 2009 19:30:32 +0100

Thanks, Julian, for your response.  Please see below:

> orthogonal to the media type of the thing being patched. Yes, that 
> means that we need to define one media type per patch format; but 
> that's by design in HTTP, methinks.

Ok, I understand.  And so you just re-register a new media-type for the 
different PATCH method (assuming the type  doesn't already exist).  Thus 
the new header.  Okay.  I withdraw my major concern.

---

From: Julian Reschke <[email-masked]>
Date: Wed, 28 Oct 2009 19:33:15 +0100

> the new header.  Okay.  I withdraw my major concern.

Um, which new header? Accept-Patch?

BR, Julian

---

From: Eliot Lear <[email-masked]>
Date: Wed, 28 Oct 2009 19:34:43 +0100

On 10/28/09 7:33 PM, Julian Reschke wrote:
>> Ok, I understand.  And so you just re-register a new media-type for 
>> the different PATCH method (assuming the type  doesn't already 
>> exist).  Thus the new header.  Okay.  I withdraw my major concern.
>

yes.

---

From: Aaron Stone <[email-masked]>
Date: Wed, 28 Oct 2009 11:46:12 -0700

Side note: looks like media types for text/unified-diff, text/context-diff
(or should those be application/vnd.gnu...), vcdiff (which has no media
type registered that I could find), etc., should be registered.

My open question:
What does one do with a patch document that describes actions on multiple
resources? Should we leave it assumed that this just doesn't make sense in
the HTTP context? Or does it make sense, if, say, the resource of the PATCH
method maps to a directory and the patch document applies to several files
within that directory? In that case, does anything need to be said about
the behavior of PATCH for this type of patch?

> media-type is not the best solution.

I found Section 3.2 to adequately describe which patch formats a server is
willing to accept for a given resource. The Accept-Patch header is defined
to allow multiple patch formats. A client might have to issue both HEAD and
OPTIONS to learn the media type of the document and the patch formats that
can be used, and you might have to add ETags to protect against a PUT
changing the resource type out from under you, etc. etc., but I don't think
the issue you raise exists.

3.2.  Example OPTIONS Request and Response

   [request]

   OPTIONS /example/buddies.xml HTTP/1.1
   Host: www.example.com

   [response]

   HTTP/1.1 200 OK
   Allow: GET, PUT, POST, OPTIONS, HEAD, DELETE, PATCH
   Accept-Patch: application/example, text/example

   The examples show a server that supports PATCH generally using two
   hypothetical patch document formats.

---

From: Eliot Lear <[email-masked]>
Date: Wed, 28 Oct 2009 19:48:27 +0100

Aaron,
> the behavior of PATCH for this type of patch?

Here I think the draft is pretty specific:

    The PATCH
    method affects the resource identified by the Request-URI, and also
    MAY have side effects on other resources; i.e., new resources may be
    created, or existing ones modified, by the application of a PATCH.

So to me that says, 'yeah, patch might monkey with groups of files'.  
It's left to the server to decide whether that's allowed.  I would 
imagine this would be based on who is requesting the operation, and 
whether the operation would succeed on the individual files separately.

It's a pretty well written draft.

Eliot

---

From: Aaron Stone <[email-masked]>
Date: Wed, 28 Oct 2009 13:52:02 -0700

> whether the operation would succeed on the individual files separately.

I thought I saw something about multiple resources on first read, but I
missed it the second time around. Thanks for pointing it out. This is the
same place you're suggesting to add 'delete' -- I agree.

If it's worth adding some example text, here's a suggestion:

   For example, the common 'diff' utility can generate a patch document
that
   applies to multiple files in a directory hierarchy. A client could
apply
   this patch to a server using the PATCH method on a resource that maps
to a
   directory on the server containing the files to be patched. The
requirement
   for atomicity of the request holds for all affected files.

> It's a pretty well written draft.

Yep, agreed.

Aaron

---

From: Lisa Dusseault <[email-masked]>
Date: Thu, 29 Oct 2009 15:00:46 -0700

On Wed, Oct 28, 2009 at 1:52 PM, Aaron Stone <[email-masked]> wrote:

>   For example, the common 'diff' utility can generate a patch document that
>   applies to multiple files in a directory hierarchy. A client could apply
>   this patch to a server using the PATCH method on a resource that maps to a
>   directory on the server containing the files to be patched. The requirement

Thanks!  I would be happy to add this to the doc.  I'll file it as
perhaps an RFC Ed. note to save having to do a new version, unless
there are more changes as a result of IETF Last Call and IESG
Evaluation.

Lisa