Subject: Re: Last Call: draft-dusseault-http-patch (PATCH Method for HTTP) to Proposed Standard

From: IETF Member Dave Aronson <[email-masked]>
Date: Tue, 27 Oct 2009 12:50:20 -0400

The IESG <[email-masked]> wrote:

> The IESG has received a request from an individual submitter to consider
> the following document:
>
> - 'PATCH Method for HTTP '
>   <draft-dusseault-http-patch-15.txt> as a Proposed Standard

This could be sort of a nitpick, but....

It seems to me that this could be used for both simple sequences of
bytes, and for much more complex entities.  The latter case could
include, say, a relational database table (where the patch might be to
insert an entry, change a cell, remove an attribute, etc.), or even a
database consisting of several tables (where the patch might include
inserting or modifying several entries).  Maybe I missed it, but I
didn't see anything that clarifies this.

If we don't want to just take that for granted, I suggest the
following text (or something similar) be inserted after the first
sentence in Section 2: "The nature of the resource may vary, from a
simple sequence of literal stored bytes (i.e., a file), to any kind of
much more complex entity.  Therefore, the nature of the patch may
vary, from simple addition, removal, or modification of a series of
bytes, to any type of modifying operation that may be specified on any
type of resource, such as deletion of objects within a database,
modification of permissions on a file, or starting or stopping a
service."

Or am I misinterpreting, overanalyzing, overexplaining, etc?

-Dave

-- 
Dave Aronson - Have Pun, Will Babble | Work: davearonson.com | /\ ASCII
-------------------------------------+ Play: davearonson.net | \/ Ribbon
"Specialization is for insects."     | Life: dare2xl.com     | /\ Campaign
-Robert A. Heinlein                  | Wife: nasjleti.net    | Email<>Web

---

From: Julian Reschke <[email-masked]>
Date: Tue, 27 Oct 2009 18:05:25 +0100

The IESG wrote:
> The IESG plans to make a decision in the next few weeks, and solicits
> final comments on this action.  Please send substantive comments to the
> [email-masked] mailing lists by 2009-11-24. Exceptionally, 
> comments may be sent to [email-masked] instead. In either case, please 
> retain the beginning of the Subject line to allow automated sorting.
> ...

Looks good to me, except for one thing I mentioned before (see, for
instance,
<http://lists.w3.org/Archives/Public/ietf-http-wg/2009JanMar/0174.html> 
and again in 
<http://lists.w3.org/Archives/Public/ietf-http-wg/2009OctDec/0112.html>):

 > Collisions from multiple PATCH requests are more dangerous than PUT
 > collisions, because a patch document that is not operating from a
 > known base point may corrupt the resource.  Clients wishing to apply
 > a patch document to a known entity can first acquire the strong ETag
 > [RFC2616] of the resource to be modified, and use that Etag in the
 > If-Match header on the PATCH request to verify that the resource is
 > still unchanged.  If a strong ETag is not available for a given
 > resource, the client can use If-Unmodified-Since as a less-reliable
 > safeguard.

Recommending the use of timestamps instead a potentially available weak
etag simple does not make sense. After all, the server is minting the
etags, and it also controls what it accepts in conditional PATCH
requests. Let it decide what's right.

(This issue was pointed out previously; note that HTTPbis already allows 
weak etags in write operations, and WebDAV has allowed weak etags in the 
"If" header since its inception).

BR, Julian

---

From: Julian Reschke <[email-masked]>
Date: Thu, 29 Oct 2009 06:19:41 +0100

Nikunj R. Mehta wrote:
> 
> This draft places unreasonable restriction on servers about processing 
> requests. Specifically, in §2.2,
> 
> [[
> Concurrent modification: When a server receives multiple concurrent 
> requests to modify a resource, those requests SHOULD be queued and 
> processed in the order in which they are received. If a server is 
> incapable of queuing concurrent requests, all subsequent requests SHOULD 
> be rejected with a 409 (Conflict) until the first modification request 
> is complete.
> ]]
> 
> RFC2616 describes the above status code (409) but not in the context of 
> a particular type of HTTP request. I fail to see why this draft has 
> mandated specific error codes and specific server behavior in response 
> to certain requests. It curtails server behavior without a good reason.
> ...

Indeed.

I think this is one of these requirements that would get ignored in 
practice when it makes sense (for instance, if the patch request has the 
semantics of "append a line of text to a log resource").

It would be better to rewrite this things in terms of advice what a 
server might do, not as a normative requirement (or, not to mention it 
at all).

Nikunj: you really should include the mailing list.

BR, Julian

---

From: "Nikunj R. Mehta" <[email-masked]>
Date: Wed, 28 Oct 2009 11:03:44 -0700

[[
Concurrent modification: When a server receives multiple concurrent  
requests to modify a resource, those requests SHOULD be queued and  
processed in the order in which they are received. If a server is  
incapable of queuing concurrent requests, all subsequent requests  
SHOULD be rejected with a 409 (Conflict) until the first modification  
request is complete.
]]

RFC2616 describes the above status code (409) but not in the context  
of a particular type of HTTP request. I fail to see why this draft has  
mandated specific error codes and specific server behavior in response  
to certain requests. It curtails server behavior without a good reason.

Nikunj

On Oct 27, 2009, at 10:10 AM, Julian Reschke wrote:

>
> (FYI)
>
> The IESG wrote:
>> The IESG has received a request from an individual submitter to  
>> consider the following document:
>> [email-masked] mailing lists by 2009-11-24. Exceptionally, comments  
>> may be sent to [email-masked] instead. In either case, please retain  
>> the beginning of the Subject line to allow automated sorting.
>> The file can be obtained via
>> http://www.ietf.org/internet-drafts/draft-dusseault-http-patch-15.txt
>> IESG discussion can be tracked via
>> https://datatracker.ietf.org/public/pidtracker.cgi?command=view_id&dTag=11143&rfc_flag=0
>> _______________________________________________
>> IETF-Announce mailing list
>> [email-masked]
>> https://www.ietf.org/mailman/listinfo/ietf-announce
>

---

From: SM <[email-masked]>
Date: Thu, 29 Oct 2009 11:29:09 -0700

>[email-masked] mailing lists by 2009-11-24. Exceptionally,

I am only writing some quick comments as I missed the deadline.

In Section 2:

   "If the entire patch document cannot be successfully applied
    then the server MUST fail the entire request, applying none
    of the changes."

I suggest rewriting the sentence:

    If the entire patch document cannot be successfully applied
    then the server MUST NOT apply any of the changes and it
    SHOULD return the appropriate HTTP status code as defined
    in Section 2.2.

I used a "SHOULD" as the draft has:

  "Other HTTP status codes can also be used under the appropriate
   circumstances."

In Section 2.2:

   "There are several known conditions under which a PATCH request can
      fail.

    Malformed patch document:  Can be specified using a 400 (Bad Request)
       when the server finds that the patch document provided by the
       client was not properly formatted.  The definition of badly
       formatted depends on the patch document chosen, but generally if
       the server finds it cannot handle the patch due to the
       serialization of the patch document, this response ought to be
       appropriate."

The wording is clear and I will probably follow the intent of the 
authors of this draft.  In terms of implementation, the "can be 
specified using" and the "ought to be appropriate" does not clearly 
spell out what status code to expect for the error condition.  The 
comment is applicable to the known conditions in the "Error handling" 
section.  I suggest being more specific about what the response 
should be.  For example:

    Malformed patch document:  When the server determines that the patch
       document provided by the client is not properly formatted, it SHOULD
       return a 400 (Bad Request) response.  The definition of badly
       formatted depends on the patch document chosen.  If the server
       finds it cannot handle the patch due to the  serialization of the
       patch document, this response is appropriate."

As an additional comment about "Conflicting modification", 
"Precondition Failed" is discussed in Section 10.4.13 of RFC 2616.  I 
see this more of a precondition evaluated as false instead of a 
conflicting modification.

As an additional comment about "Concurrent modification", this looks 
more appropriate as a 503 (service unavailable).

Regards,
-sm

---

From: Lisa Dusseault <[email-masked]>
Date: Fri, 30 Oct 2009 11:01:55 -0700

Hi,

On Wed, Oct 28, 2009 at 11:03 AM, Nikunj R. Mehta
<[email-masked]> wrote:
> Concurrent modification: When a server receives multiple concurrent requests
> to modify a resource, those requests SHOULD be queued and processed in the
> order in which they are received. If a server is incapable of queuing
> concurrent requests, all subsequent requests SHOULD be rejected with a 409
> (Conflict) until the first modification request is complete.
> ]]
>
> RFC2616 describes the above status code (409) but not in the context of a
> particular type of HTTP request. I fail to see why this draft has mandated
> specific error codes and specific server behavior in response to certain
> requests. It curtails server behavior without a good reason.

The draft doesn't describe the reason for mandating specific error
handling, but the reason is to let clients handle some error cases
automatically, or at least help users make sensible choices rather
than just show "FAIL".  Also,  I'll note that another review of this
document posted just yesterday makes the opposite request, asking that
the error handling responses be more specific and constrained (SM's
review).

This is clearly a tradeoff between server flexibility and client's
being able to know what to do, so let's look at whether a client can
actually do something with the information.  In the case of a
concurrent patch, if the client knows that something else is going on
with the resource right now, it can wait a few seconds and see if the
resource has changed, then try to recalculate the patch based on the
new resource if that's appropriate.  It may be possible for the client
to resubmit the patch automatically depending on the patch format and
use case.  Even if the patch can't be resubmitted, it's possible for
the client to explain to the user what happened.

Since the requirement is a SHOULD, there is an escape clause for the
server to retain flexibility if it needs to, at the cost of having
behavior that is less predictable and manageable for the client.

Can you live with that?

Lisa

---

From: Lisa Dusseault <[email-masked]>
Date: Fri, 30 Oct 2009 11:07:32 -0700

Hi,

>   SHOULD return the appropriate HTTP status code as defined

I am fine with this change.

> The wording is clear and I will probably follow the intent of the authors of
> this draft.  In terms of implementation, the "can be specified using" and
> the "ought to be appropriate" does not clearly spell out what status code to
> expect for the error condition.  The comment is applicable to the known
> conditions in the "Error handling" section.  I suggest being more specific
> about what the response should be.  For example:
>

I am fine with this change too.

>
> As an additional comment about "Conflicting modification", "Precondition
> Failed" is discussed in Section 10.4.13 of RFC 2616.  I see this more of a
> precondition evaluated as false instead of a conflicting modification.

In HTTP, "Precondition Failed" means something very specific, that
there was a precondition header supplied by the client, and evaluating
the client's precondition failed.  It would confuse a client to send a
precondition header that would succeed but this response was returned
anyway, or for a client to send a request with no precondition headers
that failed with this response.

>
> As an additional comment about "Concurrent modification", this looks more
> appropriate as a 503 (service unavailable).
>

To me, using 503 implies a longer-term or less-well-understood issue.
There are lots of reasons a server could return 503 and the right
response for the client would not normally be to retrieve an updated
resource.  For both uses of 409 Conflict in this specification (both
conflicting and concurrent modification scenarios), one sensible
client action would be to tell the user that the resource may have or
has changed, to download the changed rseources, and possibly to try to
re-apply the user's requested changes to the modified resource.  For
503, that reaction would not be indicated.

Does that make sense?

Lisa