for-GET/http-decision-diagram

Question regarding PATCH support

Closed this issue · 9 comments

bethesque commented

I'm working with @seancribbs to add PATCH support to webmachine, and he suggested I look at this project for a guide. If someone could answer a question for me, I'd greatly appreciate it.

Is PATCH fully supported by the current design? I can see Accept-Patch headers specified, which suggests it is at least partially supported, but reading the HTTP spec for PATCH tells me that I should be able to get a 409 response when using PATCH, and I cannot see in the current flow how that could happen - I'm assuming that we would follow true at is_method_create and is_method_process, however there is no flow after that that can take us to a 409 like there is for PUT.

andreineculau commented

First, a clarification - PATCH on the flow missing:true should end up on is_method_create:false and 404 Not Found. I have to double-check, but there is no definition of PATCH on a non-existing resource. Am I mistaken?

And then about the diagram. Indeed, PATCH here is treated as a miscellaneous "process" method which doesn't yield a 409 automagically. The intention for the process callback is to act as a catch-all and use trigger another FSM or just simply have a switch statement based on the request's method.

In the long run, I might extract the process (red) block in another FSM, and have the http-decision-diagram acting as a high-level FSM. I will definitely not be very keen on extending this FSM over and over again to cover more of the HTTP methods that are in the IANA registry http://tools.ietf.org/html/draft-ietf-httpbis-method-registrations-14 . Not only does it overload an already fluffy FSM, but it will also not be comprehensive:

From http://tools.ietf.org/html/rfc5789

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

Wdyt?

Digressing + a bit of context: we (at Klarna) have a PATCH webmachine-patch, and initially we were blinded by the "need" for 409, and made PATCH to follow PUT's flow, but that's not the case. If considering webmachine diagram's, and the RFC in general - PATCH is POST with some extended needs (e.g. Accept-Patch header) and semantics (e.g. update resource with a patch document).

ping @dmitriid - it's not much, but maybe you cherry-pick our PATCH commits, and do a pull-request? :)

bethesque commented

Actually, it seems that you are allowed to PATCH to a missing resource (it is exactly that use case that got me involved in trying to add PATCH to webmachine-ruby in the first place) "If the Request-URI does not point to an existing resource, the server MAY create a new resource, depending on the patch document type (whether it can logically modify a null resource) and permissions, etc."

One of the intents of a PATCH is to avoid having to do a full GET, modify it, then a full PUT, so if a PATCH to a non existing resource always returned a 404, it would defeat one of the main uses of PATCH, imo.

"Conflicting state: Can be specified with a 409 (Conflict) status
code when the request cannot be applied given the state of the
resource. For example, if the client attempted to apply a
structural modification and the structures assumed to exist did
not exist (with XML, a patch might specify changing element 'foo'
to element 'bar' but element 'foo' might not exist)."

http://tools.ietf.org/html/rfc5789

409 seems even more likely with a patch than a put, as you're specifying changes to apply to an existing resource, making an assumption of existing state, which may be incorrect. I think, given that PUT already has a 409 path, that it would be consistent and helpful to ensure PATCH had one too.

I've had a go at adding PATCH to seancribbs webmachine diagram, but I'm not at all confident it's right (especially the last orange arrow to N11), I'd appreciate an experienced eye on it if you have a spare moment. I too tried to implement PATCH as a variant of PUT, but seancribbs correctly pointed out the multiple ways in which it differs. It really is a bit of a bastard child of a method.

http://s886.photobucket.com/user/bethesk/media/http-headers-status-v3_zps878d2b44.png.html

Here's our thread if you're interested in the context webmachine/webmachine-ruby#109 (comment)

andreineculau commented

Will take a look and get back to you in ~9 hours

andreineculau commented

Hmm, both your annotated diagram and the webmachine/webmachine-ruby#109 thread reminded me why I started http-decision-diagram -- v3 is more of a spec snapshot of RFC2616 and possible outcomes, while my ongoing v4 is more of a sane implementation of HTTP-related specs. That's a reason of you see also 500 Internal Server Error in http-decision-diagram.

I know this is not what you want to hear, but I have been where you are and it doesn't end up nicely on paper, not to mention in code. If your org needs to support PATCH requests, just keep it simple for now:

  • do NOT modify the FSM, and let PATCH follow 1) the PUT flow when resource missing 2) the POST flow otherwise
  • make the content_types_accepted dependent on the request method
  • make the process_post dependent on the request method + if PATCH and it fail, then force a 409 Conflict response status code
  • return Accept-Patch on OPTIONS
  • return Content-Location both for create/process flow (see here)

That's more or less what my team did, and knowing the guts of the diagram&webmachine it was the only thing to do org-wise, while I personally ventured into this alternative effort.

Disclaimer: I am talking about webmachine from memory, and I'm talking about erlang webmachine under the presumption that webmachine-ruby is an exact match.

andreineculau commented

side-comments:

  1. re:PATCH "MAY create a new resource" -- verified. It's actually easy to understand the rationale, but I still wish that I could find an IETF mailing list thread related to this topic.
  2. My diagram actually isn't restrictive about which methods match is_method_create, just defaults to PUT|POST only. After this issue, it will surely default to include PATCH as well. And I will think more about adding a special case for PATCH returning 409 "on the diagram", like I do for PUT, and the alternatives.
  3. Just to "give to Caesar what is Caesar's", the diagram that webmachine is built on is @adean's - https://code.google.com/p/http-headers-status/ .
  4. Irrelevant but I wouldn't say that 409 is more likely with PATCH than with PUT. "Existing state" is not confined to the Request-URI resource, but to server state. Assumption is the mother of all fuckups - and that's why you have conditional requests :) or application/json-patch+json "test" operation.
bethesque commented

Really appreciate your feedback Andrei, thanks for taking the time to look at this. I can see why it makes sense to follow the PUT path for non-existant resource (though I can also see the logic for following the POST path!), but I'm not sure why you've recommended following POST rather than PUT for the already existing resource?

Also finding it a little bit comforting that it's not just me who has found this tricky!

andreineculau commented

I forgot about the Content-Location response header. I just updated the above comment for future readers.

Follow POST instead of PUT - it is because of webmachine's flow, at least the Erlang version. I wish I had a better memory and tell you specifically what was the issue of us switching to POST, after running a patched (pun intended) webmachine with PATCH following PUT in both scenarios.

bethesque commented

My webmachine-ruby branch is called PATCH-patch... it amused me.

andreineculau commented

@bethesque I'm going to do the same with this issue - close it. I do think that the current version is PATCH friendly.

Feedback extremely appreciated. Thanks