Skip to content
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

Open
deepak opened this issue Jun 20, 2013 · 27 comments
Open

does not know about the PATCH http method #109

deepak opened this issue Jun 20, 2013 · 27 comments
Labels

Comments

@deepak
Copy link

deepak commented Jun 20, 2013

known_methods in callbacks is
['GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'TRACE', 'CONNECT', 'OPTIONS']

@bernd bernd mentioned this issue Jul 18, 2013
@bethesque
Copy link
Contributor

Is there any plan/schedule for PATCH to be implemented?

@seancribbs
Copy link
Member

There are currently no plans, but I am open to proposals for how to re-adjust the FSM to handle PATCH

@tarcieri
Copy link
Contributor

@seancribbs are there considerations at play that make PATCH significantly different from POST?

@seancribbs
Copy link
Member

@tarcieri I suppose the conneg-dance would have to be done, but honestly I haven't looked too deeply into it.

@bethesque
Copy link
Contributor

I understand PATCH to be more similar to PUT than POST. As a stop gap hack for my current use case, I have overridden request.put? to return true when PATCH is used, and the behaviour seems to proceed as I expect it for my use case. However, I have not yet looked deeply into the HTTP spec for PATCH to see where it differs from PUT.

@tarcieri
Copy link
Contributor

Oof, my bad. Thinking about it more PATCH should be idempotent which definitely makes it different from POST

@seancribbs
Copy link
Member

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.

@bethesque
Copy link
Contributor

"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!

@bethesque
Copy link
Contributor

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.

http://s886.photobucket.com/user/bethesk/media/http-headers-status-v3-with-patch_zps845e3d42.png.html

@bethesque
Copy link
Contributor

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.)

@ghost
Copy link

ghost commented Nov 20, 2013

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:

   The difference between the PUT and PATCH requests is reflected in the
   way the server processes the enclosed entity to modify the resource
   identified by the Request-URI.  In a PUT request, the enclosed entity
   is considered to be a modified version of the resource stored on the
   origin server, and the client is requesting that the stored version
   be replaced.  With PATCH, however, the enclosed entity contains a set
   of instructions describing how a resource currently residing on the
   origin server should be modified to produce a new version.

Regarding PATCH is not idempotent: it can be effectively idempotent, depending on the patch instructions. Clients just shouldn't ever blindly consider it idempotent without taking the media-type into account.

@bethesque
Copy link
Contributor

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.

@ghost
Copy link

ghost commented Nov 20, 2013

you should not accept the request if the resource doesn't exist

@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

@bethesque
Copy link
Contributor

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.

@bethesque
Copy link
Contributor

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?

bethesque@18e9d4d

@seancribbs
Copy link
Member

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:

  1. "PATCH is neither safe nor idempotent as defined by [RFC2616], Section 9.1." Yet, conditional requests are allowed! So, sort of the bastard child of PUT and POST.
  2. The set of media types accepted for PUT will likely be disjoint from PATCH. This implies either a different callback or forcing the developer to detect the method in content_types_accepted.
  3. OPTIONS should return the acceptable patch types in the Accept-Patch header, implying again a different content_types_accepted callback or the internal switching.
  4. The Content-Location header (why not Location?) must be specified and equal to the Request URI when there is a body returned, in order for caching to work properly.
  5. PATCH may be applied to resources that don't exist, meaning something similar to allow_missing_post? is needed.

This is fraught with edge-cases.

@bethesque
Copy link
Contributor

  1. Yes, the media type should be different according to what I've read. It should not be "application/json"' it should be "application/json+patch" (or -patch, I've also seen), so the accept_helper method should handle it. You'd have to do something hacky if you wanted to use application/json, for both put and patch, but then, you shouldn't be doing that anyway.
  2. I knew I'd forgotten something, you're right, we'd need to add the Accept-Patch header
  3. You can put to a missing resource too though, in what do you think the flow would differ for patch?

@seancribbs
Copy link
Member

@bethesque Regarding the last item, the resource should be able to deny PATCH if missing, replying with 404, i.e. the same semantics as allow_missing_post?.

@seancribbs
Copy link
Member

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.

@bethesque
Copy link
Contributor

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.

@bethesque
Copy link
Contributor

Ok, @seancribbs, had another go at the diagram, thoughts?

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

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?

@andreineculau
Copy link

@seancribbs

"PATCH is neither safe nor idempotent as defined by [RFC2616], Section 9.1." Yet, conditional requests are allowed! So, sort of the bastard child of PUT and POST.

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.

The Content-Location header (why not Location?)

Location is a GOTO statement (201, 303, etc), so the client should do a GET {LocationURI}, while Content-Location is "I did the GET for you and returned the response". The latter is the PATCH scenario where the PATCH response would include an updated representation of the resource.


Although I never thought about it, this would be a good illustration of the 2 headers:

POST /images
< 201 Created
< Location: /images/1
< Content-Location: /images/1
< Content-Type: image/png
...uploaded PNG...

201 Created demands a Location response header, but in this case the server not only tell the client "I created /images/1" but also "If you want to do a GET /images/1, I can already tell you what you'll find".

If the server includes a representation without the Content-Location header, the semantics of the representation become more like ...some image saying that the other image got uploaded.... See (Figure 21: Non-CRUD HTTP POST interaction](http://www.infoq.com/articles/designing-restful-http-apps-roth)

@bethesque
Copy link
Contributor

Haven't forgotten about this, but I've had too many things on and nobody else seemed to want it!

@bethesque
Copy link
Contributor

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 support

https://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.
The different content type (e.g. application/json-patch+json) will differentiate it from a PUT.
The options method can be overridden manually to add the Accept-Patch header should the developer wish.
By the way, it would be nice to provide a helper method to automatically generate the OPTIONS response, the way the Method Not Allowed response does.

Option 2 - Add patch_content_types_accepted method

https://github.com/bethesque/webmachine-ruby/tree/patch-option-2

def content_types_accepted
  [["application/xml", :from_xml]]
end

def patch_content_types_accepted
  [["application/json-patch+json", :patch_json]]
end

Option 3 - Add a PATCH qualifier to content_types_accepted

https://github.com/bethesque/webmachine-ruby/tree/patch-option-3

def content_types_accepted
  [["application/xml", :from_xml],
  ["application/json-patch+json", :patch_json, "PATCH"]]
end

Still outstanding/queries:

  • Location-Content header. I cannot get my head around when it is required. Maybe @andreineculau can give me a hand?
  • In o7, the Location header is only set if the response body is set, because if a body is not set, I believe a 204 should be returned. Does this sound right?
  • flow_spec.rb, search for "# Beth - don't really understand this”. For ["PATCH", false], o7 automatically creates a Location header, so it will always be a 201. Not sure if this is the right thing to do or not.
  • Generally, do you think am I heading in the right direction here?!?! To get this far has been HARD, patch is a biatch of a spec to implement!
  • Do you have any nicer ideas on how to implement the Accept-Patch header?

My preference

I 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.

@andreineculau
Copy link

Sorry but I don't have time to look over the whole options now, but re:
Content-Location - it's not required at any time.

Responding to a PATCH request with a body means this is the response to the
request.
Adding a Location header, doesn't make sense - it tells the client "if
there's a body, ignore it. just go do a GET request there"
Adding a Content-Location header instead, means this the body is the new
state of the resource (i.e. if you do a GET request, this is what you'll
see). This would allow the body to be cached, and remove the need to do a
GET.

Generally, do you think am I heading in the right direction here?!?! To
get this far has been HARD, patch is a biatch of a spec to implement!

By no means, do I want to sound negativist or bring your efforts down, but
it's not PATCH that is the biatch. It's the constraints of this version
of the diagram and webmachine's callback implementation. You're trying to
add PATCH with as little effort/change as possible. biased opinion

@bethesque
Copy link
Contributor

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.

@bethesque
Copy link
Contributor

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.

bethesque@0c0e231

bestpractical-mirror pushed a commit to bestpractical/rt-extension-rest2 that referenced this issue Dec 13, 2016
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants