Subject: Re: PATCH Draft

From: "Mark Baker" <[email-masked]>
Date: Wed, 4 Jul 2007 14:07:52 -0400

Thanks for reviving this, James.  Here are my comments.

"The server MUST NOT create a new resource with the contents of the
request body"

Why not?  I don't see the problem.

"The server SHOULD always apply the entire patch atomically and never
provide (e.g. in response to a GET during this operation) a
partially-patched body"

I understand the desire for atomicity, but I could see that being a
feature as long as the fact that any partial response was tagged as
such (using some future extension).  Suggest removal.

"If the entire delta encoding cannot be successfully applied then the
server MUST fail the entire request, applying none of the changes"

Why is this needed?  It would seem to depend upon the diff format.
Plus I could imagine scenarios where it's undesirable (e.g. a streamed
patch).  Suggest removal.

"Therefore, the client MUST verify that it is applying the delta
encoding to a known entity by first acquiring the strong ETag [...]"

I don't see why this is required.  Of course, if the client wants the
guarantees provided by strong etags, it can get them.  But why does
that matter to a protocol specification?  Suggest removal of that
paragraph.

Sec 2.1.1 - is this section needed?  The MUST requirements there seem
to have similar issues to the MUSTs mentioned above.  Suggest removing
the section.

I suggest the use of 422 instead of 400 in the "malformed delta
encoding" case of sec 2.2.2.

Sec 2.3 seems to over specify a few things.  For example, the Allow
response header isn't required to list "all the allowed methods" so
there's no need for "MUST" there.  I think that all you really need in
this section is the definition of the Accept-Patch header.

The IANA considerations section should register the Accept-Patch
header with the header registry defined in RFC 4229.

>   * Should it be possible to create new resources with PATCH

Sure, why not?  HTTP can indicate that with a 201 response.

>   * Should it be required to use If-Match and If-Unmodified-Since
>     on PATCH.

If that's what the client wants.  But the protocol needn't say
anything about it IMO.

Mark.

---

From: David Morris <[email-masked]>
Date: Wed, 4 Jul 2007 13:12:42 -0700 (PDT)

DO NOT consider relaxing a requirement for atomicity as the default
behavior. It gives me heartburn to even think about programming a client
against a server that doesn't make atomic updates and fail if the patch
can't be applied. I can't imagine a scenario that corrupted data
wouldn't result and thus the added complexity of providing a modifier to
the request seems like overkill.

Furthermore, 'entire request' is a bit nebulous and would allow use of a
patch data format which had a way to accept a request as complete based on
the rules of the format. The protocol depends on the patch engine to
specify the meaning of complete. If the patch engine says the patch is
complete, the http protocol for atomic complete/fail is satisfied.

But applications based on different patch formats will never be blindsided
by corrupted data.

In terms of not serving an inflight update result, the default must be to
NOT deliver a partially updated resource. While I think there is little
value increasing the complexity to allow a client to authorize an
incompletely updated resource, I can accept that approach.

---

From: Lisa Dusseault <[email-masked]>
Date: Wed, 4 Jul 2007 13:43:20 -0700

Hi Mark,

I fail to understand the overall thrust of your proposed changes.  
Several are proposals for removing requirements   with little to gain  
from removing them.  I'd like to see a lot more benefit to removing a  
requirement before opening up what should be a simple feature to more  
variation and complexity.

Specific comments inline,

> Why not?  I don't see the problem.

This is to require that the server not treat the PATCH like a PUT.   
If there's a way to *apply* the patch to an empty body that might be  
fine, but the server should not save the delta algorithm as the  
resource body. If that's what the client wanted, it would do a PUT.

> such (using some future extension).  Suggest removal.

"features" like this cause problems.  If the client wants to  
negotiate partial responses in a separate extension, if that were  
worth while, it could be done.  If not, it makes for more solid  
interoperability to close down this variation.

> Plus I could imagine scenarios where it's undesirable (e.g. a streamed

No.   A streamed patch or a diff format where it's desirable could  
override this requirement.  The possibility of such a format or  
extension should not induce us to leave this variation open.

> that matter to a protocol specification?  Suggest removal of that

Why remove it?  What's the harm in making this requirement that  
clients behave properly?

> encoding" case of sec 2.2.2.

What's 422?

> this section is the definition of the Accept-Patch header.

The benefit of this requirement is so that clients can count on the  
Allow header for at least this case.  What's the harm of this  
requirement?

> header with the header registry defined in RFC 4229.

Agreed.

thanks,
Lisa

---

From: Julian Reschke <[email-masked]>
Date: Wed, 04 Jul 2007 22:58:51 +0200

> variation and complexity.

Well, on the other hand I'd prefer to see a lot more benefit for having 
a requirement in the first place, in particular if it wasn't in RFC2068.

> This is to require that the server not treat the PATCH like a PUT.  If 
> there's a way to *apply* the patch to an empty body that might be fine, 
> but the server should not save the delta algorithm as the resource body. 
> If that's what the client wanted, it would do a PUT.

Yes. So it seems we agree there are use cases to allow PATCH to act on 
"non-existing" resource, so a MUST NOT requirement is clearly a bad 
thing here.

If the concern is that some servers may do the wrong thing when there's 
no existing entity, just add a warning/explanation.

 >> ...
> Why remove it?  What's the harm in making this requirement that clients 
> behave properly?

As previously stated, there are many use cases for PATCH where a client 
doesn't care what the current state is, such as for an append-like 
operation.

>> encoding" case of sec 2.2.2.

Hint: it's defined in RFC4918 :-). (as in RCF2518).

> Allow header for at least this case.  What's the harm of this requirement?

The harm is that the spec walks into a territory where it doesn't need 
to go. Is there any benefit in *practice* from having stricter 
requirements for Allow/PATCH compared to Allow/RFC2616?

 > ...

Best regards, Julian

---

From: James M Snell <[email-masked]>
Date: Wed, 04 Jul 2007 15:04:49 -0700

Lisa Dusseault wrote:
> [snip]

Yes. However, that is different than creating a new resource as the
result of applying a patch.  I think we need to be clear that it is ok
for the application of the patch to result in the creation of a resource
but, as you say, the server should not save the payload of the patch
request as the content of that new resource.

>>[snip]

Well, this requirement is predicated on the idea that the resource being
patched already exists.  If we allow the application of the patch to
result in the creation of a new resource, then it obviously cannot rely
on the use of strong etags.  I would suggest making this a SHOULD rather
than a MUST.

>> encoding" case of sec 2.2.2.

422 Unprocessable Entity from RFC2518.

   The 422 (Unprocessable Entity) status code means the server
   understands the content type of the request entity (hence a
   415(Unsupported Media Type) status code is inappropriate), and the
   syntax of the request entity is correct (thus a 400 (Bad Request)
   status code is inappropriate) but was unable to process the contained
   instructions.  For example, this error condition may occur if an XML
   request body contains well-formed (i.e., syntactically correct), but
   semantically erroneous XML instructions.

The malformed delta encoding response assumes that the syntax of the
delta encoding is incorrect, which would make a 400 response
appropriate.  According to the text of 422, the syntax of the request is
assumed to be correct but still cannot be applied for whatever reason.

> Allow header for at least this case.  What's the harm of this requirement?

I don't see the harm in having it, but I also don't see any harm in
making it a SHOULD.  The presence of an Accept-Patch header in an
OPTIONS, GET, or HEAD response for a given resource will likely be
enough of a clue that the server supports the PATCH method for that
resource.

- James

---

From: "Mark Baker" <[email-masked]>
Date: Wed, 4 Jul 2007 20:23:43 -0400

Hi Lisa,

> variation and complexity.

I'm not quite sure what you mean by "removing a requirement".  Is
there some kind of requirements document that I'm not familiar with,
or perhaps prior discussion where requirements were agreed upon?

> resource body. If that's what the client wanted, it would do a PUT.

Oh, I didn't realize it was meant to address that case.  I interpreted
"create a new resource with the contents of the request body" as
meaning, in effect, that a PATCH shall never result in a 201 response
(one of the unanswered questions in the draft).  I think the wording
could be improved if that's what was intended.

But that brings up a bigger issue.  Even if the wording was fixed to
describe exactly what you describe, it will still be in terms of
server implementation which, IMO, isn't a very good way to specify a
protocol.  Consider the (admittedly edge) case where it just so
happens that the state of the resulting resource was identical to the
patch that was applied, perhaps by trying to patch a patch document.
Do we want to exclude that?

I think an improvement would simply be to describe, as unambiguously
as possible, what PATCH means, and let that definition be what
distinguishes it from PUT.

> extension should not induce us to leave this variation open.

What do you mean by "override"?  It's a MUST.

> Why remove it?  What's the harm in making this requirement that

How can a spec prescribe that clients behave properly?

> > encoding" case of sec 2.2.2.

The 422 response code as specified in RFC 2518/4918.

> Allow header for at least this case.  What's the harm of this

Because that interpretation of Allow isn't licensed by RFC 2616.

I could go along with something like "servers SHOULD use the Allow
response header to indicate their support of PATCH".

Mark.
-- 
Mark Baker.  Ottawa, Ontario, CANADA.         http://www.markbaker.ca
Coactus; Web-inspired integration strategies  http://www.coactus.com

---

From: Lisa Dusseault <[email-masked]>
Date: Thu, 5 Jul 2007 13:49:45 -0700

> What do you mean by "override"?  It's a MUST.

I mean that if I write RFC 6666 saying that "A WG chair MUST wear a  
green hat", and we get IETF consensus on that, then that's the  
requirement.

Later on if you write RFC 7777 saying that "An exception to the  
requirement in RFC6666 that WG chairs wear green hats is when the WG  
chair has no hair and can paint their head green.  The introduction  
of head painting techniques in this document motivates the change in  
requirements."

The first requirement was made before head painting was introduced,  
so it was correct for its day.  The later RFC updates and overrides  
the earlier RFC.

We don't need to write RFCs that allow for any possible future RFC  
wording.  The future RFCs can handle themselves :)

It's "borrowing trouble", as my grandmother would say, to anticipate  
the future too much when there are ways of dealing with the future  
when it arrives.  The case to worry about is when implementations  
might be written that would too strongly prevent the introduction of  
future improvements.  This is not such a case because if in the  
future somebody defines a streamed patch, then servers and clients  
have to implement that streamed patch as well as deal with the  
changed atomicity requirements.

> How can a spec prescribe that clients behave properly?

The same way it prescribes that servers behave properly: it provides  
a social mechanism justifying you in pointing and shaking your finger  
at an implementor.  Not all requirements can or need be verified  
through automated testing.

> Because that interpretation of Allow isn't licensed by RFC 2616.

I disagree with your interpretation of Holy Writ :)

    "The Allow entity-header field lists the set of methods supported
       by the resource identified by the Request-URI."

The use of passive voice instead of requirements language is  
unfortunate, but is more likely to imply MUST than MAY.

Lisa

---

From: Julian Reschke <[email-masked]>
Date: Fri, 06 Jul 2007 08:33:02 +0200

Lisa Dusseault wrote:
> green hat", and we get IETF consensus on that, then that's the requirement.
> 
> chair has no hair and can paint their head green.  The introduction of 
> head painting techniques in this document motivates the change in 
> requirements."
> 
> The first requirement was made before head painting was introduced, so 
> it was correct for its day.  The later RFC updates and overrides the 
> earlier RFC.
> 
> It's "borrowing trouble", as my grandmother would say, to anticipate the 
> future too much when there are ways of dealing with the future when it 
> arrives.  The case to worry about is when implementations might be 
> written that would too strongly prevent the introduction of future 
> improvements.  This is not such a case because if in the future somebody 
> defines a streamed patch, then servers and clients have to implement 
> that streamed patch as well as deal with the changed atomicity 
> requirements.

Understood, but IMHO still a bad idea.

My understanding was that we'll have a single place that defines the 
PATCH method, and many other places that define potential patch formats. 
The latter ones in general will be just description of formats with MIME 
type registrations.

If the PATCH method would be defined in a way so that we *expect* a 
future patch format may to have to obsolete it then we IMHO have done a 
bad design decision.

> The same way it prescribes that servers behave properly: it provides a 
> social mechanism justifying you in pointing and shaking your finger at 
> an implementor.  Not all requirements can or need be verified through 
> automated testing.

-1 on making things a MUST requirement for which we know that they are 
not always useful.

> unfortunate, but is more likely to imply MUST than MAY.

I agree that RFC2616 requires the Allow header to contain PATCH in this 
case. On the other hand, I strongly disagree with the PATCH spec making 
a normative requirement on something that is already defined in RFC2616. 
Just be either silent, or just mention it (for instance in an example), 
but by all means do not have yet another new normative requirement here.

Best regards, Julian