-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
does not know about the PATCH http method #109
Comments
Is there any plan/schedule for PATCH to be implemented? |
There are currently no plans, but I am open to proposals for how to re-adjust the FSM to handle PATCH |
@seancribbs are there considerations at play that make PATCH significantly different from POST? |
@tarcieri I suppose the conneg-dance would have to be done, but honestly I haven't looked too deeply into it. |
I understand PATCH to be more similar to PUT than POST. As a stop gap hack for my current use case, I have overridden |
Oof, my bad. Thinking about it more PATCH should be idempotent which definitely makes it different from POST |
Right, my intuition is that you'd want conditional request semantics with PATCH, but also you should not accept the request if the resource doesn't exist, so it'll be mostly like PUT, but with a few changes. |
"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." http://tools.ietf.org/html/rfc5789 This makes sense to me - in my use case, I wanted to set one attribute on a resource, and if that resource didn't exist, I wanted it to be created. It would be annoying to have to do a GET to determine if it existed or not, and then do a PATCH - that would defeat part of the purpose of a PATCH. And actually, reading the spec further, PATCH is not idempotent (I'd initially thought it would be). For example, you might have a PATCH request to add an item to a collection - running this twice should result in two items being added to the collection. It seems like it's something of a cross between POST and PUT. Should be interesting to implement! |
I've had a go at adding patch to the FSM diagram, based on my understanding of the HTTP spec - not at all confident it's right or complete, but have to start somewhere! Excuse my terrible image modification skills. |
Bump! Sorry to hassle you @seancribbs, but we're hoping to use webmachine as the default framework for all new microservices in our organisation, and the lack of patch is a bit of a problem for us. Happy to do the work, but would appreciate your thoughts on the state machine diagram above, as you're the expert on how it currently works. (You'll need to dig into the photobucket menu to get to the original image as it's obviously illegible at the preview resolution.) |
With the changes in your diagram, the possible previous existance of the resource is taken into account, and implicit creation can only happen if the resource didn't previously exist. We could as well keep it simpler and branch behind I7, going the same way up north if PATCH?. That's also what you already do in O7. That'd also make explaining the feature easy: PATCH behaves exactly like PUT, but instead of a new version of the resource, the request entity contains instructions on how to modify the existing resource on the server. And it's up to your resource implementation how to process these instructions. RFC5789 suggests something very similar:
Regarding |
Thanks for the response! Yes, I agree about PATCH being similar to put - the fact that my hack works (return true to request.put? when the method is PATCH) kind of supports that. I'll have another go at the diagram. |
@seancribbs I think it's simpler to leave that to the resource, just as with PUT. def content_types_accepted
[['application/x-patch', :from_patch]]
end
def from_patch
if resource_exists?
apply_patch(request.body.to_s)
response.code = 204
else
response.code = 404
end
end |
Agreed. The HTTP spec states "the server MAY create a new resource". For my use case, I definitely wanted it to create the resource for me if it didn't exist, then apply the changes from the patch. Defeats half the purpose of using patch otherwise. |
Had a go at the code (my image editing skills are so bad I thought it would be quicker to modify the code than update the diagram!). Thoughts? |
Looking at this more closely, I don't think those minor modifications will cut it, here's the few things I've discovered that make me think that:
This is fraught with edge-cases. |
|
@bethesque Regarding the last item, the resource should be able to deny PATCH if missing, replying with 404, i.e. the same semantics as |
General note, it might be worthwhile to diagram/list the decisions that PATCH needs, apart the existing decisions, and then overlay atop the diagram. Also, we could look at https://github.com/for-GET/http-decision-diagram for inspiration. |
Ah yes, using a "server permits patch to missing resource" must have been in my head when I did the original diagram, that's why the decision was on the 'false' branch of L7, rather than just following the put flow to I4. You make a good suggestion about doing a separate PATCH diagram first, I'll have a go at it. |
Ok, @seancribbs, had another go at the diagram, thoughts? This puts the responsibility of returning a 409 on the FSM, though I can see a case for keeping it simple and letting the resource implementation design handle it, as suggested by @lgierth. I guess it's kind of up to you which way you'd prefer us to go with this - keeping the FSM simple, and following the PUT paths, or having the extra smarts/complication of handling the "resource missing" in the same way POST does. If we went the post-like way for missing resource, I'd assume we'd need something like allow_missing_patch? and create_path_from_patch? |
Not sure I follow. Conditional headers are method agnostic so to speak. I can use them both in a PUT and POST and PATCH request.
Although I never thought about it, this would be a good illustration of the 2 headers:
If the server includes a representation without the |
Haven't forgotten about this, but I've had too many things on and nobody else seemed to want it! |
OK, so we need PATCH at my work, and it's time to stop using a hacked version! I've made 3 different implementations. The flow part is the same for each, but they vary in their treatment of the Accept-Patch header. Here is an updated FSM diagram http://s886.photobucket.com/user/bethesk/media/P4203348_zpsf61e7645.jpg.html For each of these options, the classes in examples/app/lib/example/api/resources and their corresponding specs are probably the easiest way to visualise the changes. I do not imagine that any of these options is complete or correct, but you have to start somewhere! Option 1 - No inbuilt Accept-Patch header supporthttps://github.com/bethesque/webmachine-ruby/tree/patch-option-1 According to the spec, the Accept-Patch header is optional. It is enough to return PATCH in the Allow header. Option 2 - Add patch_content_types_accepted methodhttps://github.com/bethesque/webmachine-ruby/tree/patch-option-2
Option 3 - Add a PATCH qualifier to content_types_acceptedhttps://github.com/bethesque/webmachine-ruby/tree/patch-option-3
Still outstanding/queries:
My preferenceI prefer option 1, as it messes the least with the elegance of webmachine, and still allows a developer to implement their own Accept-Patch header using the options override if they wish. |
Sorry but I don't have time to look over the whole options now, but re: Responding to a PATCH request with a body means this is the response to the
By no means, do I want to sound negativist or bring your efforts down, but |
Thanks @andreineculau. You're right, I'm trying to modify webmachine as little as possible, and that does make the job harder than it could be otherwise. I'll stop blaming the patch spec :P How have you been going with v4 of the HTTP diagram? Does patch fit in better there? @seancribbs, based on Andrei's comment, I believe I have implemented o7 incorrectly by automatically setting the Location header. I did it so that p11 would return a 201, but I think my understanding purpose of that method is incorrect - I assumed setting the Location header was a way of indicating a that a new resource had been created for both POST and PUT, but Andrei's explanation indicates that I was wrong. What is the correct way to indicate that a resource has been created rather than modified for PUT? PATCH should do it the same way I guess. |
Ok, I've created branch patch-option1.1 from patch-option-1, and modified the o7 logic, which helps fix the issue I had with the flow_spec. The 201 logic for UserAddress seems a bit of a hack, but it's the only way I've found to do it from my searching though the various issues on the webmachine repos and documentation. Very happy to be corrected if there's proper way to do it. |
This reverts commit c8581fb. Web::Machine doesn't support PATCH yet, and it seems like no other implementation in another language does either. webmachine/webmachine-ruby#109 for-GET/http-decision-diagram#35
known_methods in callbacks is
['GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'TRACE', 'CONNECT', 'OPTIONS']
The text was updated successfully, but these errors were encountered: