-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Discuss PATCH #2512
Comments
this has always been fine for all my use cases |
I've never thought about deleting data partially. Is this a common use case? |
btw, that should be @steveklabnik Oh, @jashkenas just fixed the typo :P |
It's not just deletion, it's also updating. PATCH needs a diff type, so sending application/json is a violation of the spec. backbone (and I hope Rails) have an early opportunity to educate users on how PATCH works, so we have an opportunity to do it right early. — On Wed, May 1, 2013 at 7:53 PM, Ted Han notifications@github.com wrote:
|
Hrm. We're using it with Tastypie and updating is pretty straightforward with Edit: Just checked out the IETF, and yeah, my look on it is rather naive, since our implementation is so simple. |
For the benefit of everyone else in this discussion: http://tools.ietf.org/html/rfc5789 On Wed, May 1, 2013 at 8:13 PM, Tim Branyen <notifications@github.com="mailto:notifications@github.com">> wrote: — |
For what it's worth a reading of the spec doesn't lead me to believe that it requires a different type. The spec does not explicitly prescribe any particular patch format. Would it not be reasonable for most servers to support Either way, I think it would be quite a stretch for us to send json-patch requests, given that nothing else implements them, we'd have to include json-patch and json-pointer libraries, and most of their implementation isn't relevant to us, as it concerns arbitrary manipulations on arbitrarily nested JSON objects, and we just really care about flat lists of values -- as will any endpoint that's serving from a relational DB. |
@jashkenas right, but Backbone already doesn't manage nested values as attributes. Since Backbone would be generating the JSON-Patch expressions, there's actually nothing stopping us from generating patch operations only for top level attributes (which is all we track anyway). And for more deeply nested objects, folks can set things up to manage them as they will rather than putting the onus on Backbone to figure out the deep diffing. This is predicated on my assumption that the "value" key in each of the operations can take an arbitrary object as a, er, value. Also, regarding object transmission... what about document oriented stores? |
JSON Patch is an RFC now - http://tools.ietf.org/html/rfc6902 There are a few implementations out there, and we have a test suite at: I think what should probably happen is that the current Rails behaviour needs to get assigned its own media type (has anyone talk to the Rails folks about that?); the biggest breakage is that people are assuming "application/json" has PATCH semantics. Then you can support application/json-patch+json as a next step... |
I think the role of And yeah, I don't see any reading of the spec that mandates any particular format. It seems to leave things pretty open-ended. IMO the 80% use case is fixed-shape objects, for which the current format works great, usually. The places where it falls down, and JSON Patch steps in, are from what I can see:
I'm not sure how many of these Backbone could capture without significant user intervention; maybe the first two? |
@jashkenas - see the errata: When we wrote PATCH, we didn't make it explicit because we thought it was obvious (/me rolls eyes); the errata was filed once we saw what Rails had done. |
Eek. That's unfortunate. So if we wanted to be spec-compliant, and we don't want to implement JSON-patch and JSON-pointer ... then we'll have to change the Backbone API from:
... to:
... and have that send a POST to the model's URL, and stop sending PATCH entirely. Correct? |
Probably. There really isn't a benefit of using PATCH over POST if the patch format isn't self-describing. If Rails can accept a different media type for their PATCH format, that's a different thing. |
@steveklabnik -- Is Rails 4.0 going to ship with an intentionally contra-spec implementation of |
How about we create a new media type instead? Rails can do (Edited to use correct syntax for media types with |
I recently implemented Backbone + Rails 3 JSON Patch support - here's a gist of what I had to do: https://gist.github.com/adstage-david/5500352 While it might not be ideal, I definitely don't think it's necessary to create a whole new mimetype for Rails 4 - the pieces are all there for getting proper JSON-patch support going in some capacity. From the Backbone side of things, the main issue was that when I try to call I'm not sure how best to handle this - I'm okay with it not being a core feature in backbone, but seems like there would at very least need to be a way to disable the presave |
For reference - the backbone specific part of the gist: patch: (attrs = {})->
# The default set in save will fail once we build the patch.
@set(attrs)
attrs = @buildPatch(attrs)
# patch makes sure only attrs is sent, rather than full json
# contentType sets json-patch.
#
# we rely on the fact that no error is raised on @set([]) and no side effects occur with that.
@save(attrs, {patch: true, contentType: 'application/json-patch+json'}) |
Question - if you PATCH to Rails today with a media type other than application/json, what does it do? |
In general, I believe what happens in that case is exactly the same thing that happens when you POST/PATCH/PUT anything with any media type - it first tries to turn that media type into a generic hash of parameters, and from there lets the controller do whatever it wants with that hash, which by convention generally is a partial update via I think all that has changed is which HTTP method is used to match to the controller action. With my hacks - it probably would raise an argument error, I'm expecting params in specific format: |
So. I fought endlessly and endlessly over this, and basically what I got out of it was this: http://weblog.rubyonrails.org/2012/2/25/edge-rails-patch-is-the-new-primary-http-method-for-updates/ TL;DR: "So, in Rails 4.0 both PUT and PATCH are routed to update." So we're doing the exact same thing as backbone, really. The default controller scaffold works as @adstage-david mentions; except it's Now, Rails has always had pretty good support for different media types, but I haven't played around at all with how it works with http patch. Tenderlove has an implementation of JSON-patch, but I haven't actually played around with it yet. |
@steveklabnik: curious what you think of the |
I haven't fully thought it through, but here's one bit of weirdness: Rails doesn't send JSON by default, it sends HTML forms. So in a certain sense, Rails can't only support PATCH via a certain type, since you're going to be getting |
Oh, @jashkenas , I had a snarky comment about this last night, but forgot to post it: "Cool URIs don't change" is the Church of Berners-Lee, not the Church of Fielding. ;) Let's return to real discussion now, I don't want to entirely derail this thread. |
@tenderlove hasn't released the RFC compatible JSON patch implementation yet, it's one of the sticking points I lay out in my gist. As for form support, I'm handling this right now by having an |
I've been talking PATCH over a bit with @mnot this afternoon, and have a couple of thoughts and conclusions:
I don't think that Backbone (or Rails) needs to change their current behavior, or defaults. More controversially ... I don't think it makes much sense for Rails to support JSON-Patch at all, as long as you're using ActiveRecord and a relational database -- most of the operations you'd be adding would be errors -- you can't "move" a structure within a value in a column, you can't "delete" an attribute from the schema, only set it to |
I took something quite different away from the conversation... If Rails can still use their own media type to define those semantics, they should; overloading "application/json" to mean "what Rails does when it accepts a JSON Patch" is going to cause interop problems, and isn't very friendly. I'll push that with the Rails folks. It'd be really nice if you could support that. You said that Rails does do the important thing - it dispatches on the media type of the request coming in. If it doesn't, introducing new PATCH formats in the future is going to be harder... |
So, how does Rails behave with PATCH support and nested attributes currently? |
For those who are interested: |
@jashkenas - Wouldn't it still be good to have support for users that want to do PATCH with an actual JSON-Patch? How would you feel about changing https://github.com/documentcloud/backbone/blob/master/backbone.js#L493 to something like // in save
if (method === 'patch') options = this.buildPatchOptions(attrs, options);
// after the save method
buildPatchOptions: function(attrs, options){
options.attrs = attrs;
return options;
} Then it doesn't mean any functionality change for Backbone core, but gives easy hooks to allow backbone plugins to implement PATCH with JSON-Patch for users that want it - override buildPatchOptions to set the right content type and build a patch array. |
@adstage-david I'm fairly sure you can do that already via the |
Yes, I think you're correct @braddunbar - I definitely missed that in trying to hack my implementation of JSON-Patch. I'll give it a shot and see if it works. |
Yeah, that definitely works. Is this behavior documented anywhere? I was looking for how I could do this but missed it. I wouldn't expect that Backbone would prefer the data option to the attributes I provided for syncing. Maybe an update to the documentation of Backbone.sync is in order? |
I thought this fact was documented but I can't seem to find it. I'd be glad to accept a patch that adds it though. :) |
One other option - I think this might be closer to the semantics you want... |
We originally do PATCH by sending a partial representation of the data to the server (a subset of the attributes) ... but this may not be ideal, because (for example), there's no way to mark an attribute for deletion, apart from setting it to null. Discuss. (@steveklabnik @mnot)
Resources:
http://tools.ietf.org/html/rfc5789
http://tools.ietf.org/html/draft-ietf-appsawg-json-patch-05
http://www.mnot.net/blog/2012/09/05/patch
The text was updated successfully, but these errors were encountered: