Closed bethesque closed 9 years ago
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? :)
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 https://github.com/seancribbs/webmachine-ruby/issues/109#issuecomment-28892771
Will take a look and get back to you in ~9 hours
Hmm, both your annotated diagram and the https://github.com/seancribbs/webmachine-ruby/issues/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:
PATCH
follow 1) the PUT
flow when resource missing 2) the POST
flow otherwisecontent_types_accepted
dependent on the request methodprocess_post
dependent on the request method + if PATCH
and it fail, then force a 409 Conflict
response status codeAccept-Patch
on OPTIONS
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.
side-comments:
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.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.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!
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.
My webmachine-ruby branch is called PATCH-patch... it amused me.
@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
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
atis_method_create
andis_method_process
, however there is no flow after that that can take us to a 409 like there is for PUT.