Discuss PATCH - Githubissues

jashkenas commented 11 years ago

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

philfreo commented 11 years ago

no way to mark an attribute for deletion, apart from setting it to null

this has always been fine for all my use cases

tbranyen commented 11 years ago

I've never thought about deleting data partially. Is this a common use case?

knowtheory commented 11 years ago

btw, that should be @steveklabnik Oh, @jashkenas just fixed the typo :P

steveklabnik commented 11 years ago

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.

— Sent from Mailbox for iPhone

On Wed, May 1, 2013 at 7:53 PM, Ted Han notifications@github.com wrote:

btw, that should be @steveklabnik

Reply to this email directly or view it on GitHub: https://github.com/documentcloud/backbone/issues/2512#issuecomment-17316853

tbranyen commented 11 years ago

Hrm. We're using it with Tastypie and updating is pretty straightforward with resource_uris. I didn't know there was a spec for this.

Edit: Just checked out the IETF, and yeah, my look on it is rather naive, since our implementation is so simple.

steveklabnik commented 11 years ago

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: Hrm. We're using it with Tastypie and updating is pretty straightforward with resource_uris. I didn't know there was a spec for this.

— Reply to this email directly or view it on GitHub.

jashkenas commented 11 years ago

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 application/json PATCH requests with the usual merge semantics ... and application/json-patch for fancier use cases?

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.

knowtheory commented 11 years ago

@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?

mnot commented 11 years ago

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: https://github.com/json-patch/json-patch-tests

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

domenic commented 11 years ago

I think the role of Accept-Patch in this should be considered. I wouldn't try to send JSON Patch to a server that doesn't indicate it accepts it.

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:

deletion of properties (changing the object shape)
modifying arrays
more advanced semantics like move, copy
conditional updates whose semantics can't be captured by ETags (via JSON Patch's test)

I'm not sure how many of these Backbone could capture without significant user intervention; maybe the first two?

mnot commented 11 years ago

@jashkenas - see the errata: http://www.rfc-editor.org/errata_search.php?rfc=5789

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.

jashkenas commented 11 years ago

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:

model.save(attributes, {patch: true})

... to:

model.save(attributes, {partial: true})

... and have that send a POST to the model's URL, and stop sending PATCH entirely. Correct?

mnot commented 11 years ago

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.

jashkenas commented 11 years ago

@steveklabnik -- Is Rails 4.0 going to ship with an intentionally contra-spec implementation of application/json PATCH? Once that happens, isn't it set in stone? Cool HTTP APIs don't change ;)

domenic commented 11 years ago

How about we create a new media type instead? application/json-overwrite+json. Its semantics are that you send fields to overwrite the current ones, just like the current Backbone and Rails implementations.

Rails can do Accept-Patch: application/json-overwrite+json in Rails 4.0, and in 4.1 add support for application/json-patch+json. It can then switch on the incoming media type and do either the overwrite processing or the JSON patch processing.

(Edited to use correct syntax for media types with +s.)

adstage-david commented 11 years ago

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 save([...]), backbone can't do the pre-AJAX set() (it doesn't expect an array), so I had to set the attributes manually before building my patch array.

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 set (or the post-success _.extend(attrs, serverAttrs), if you use wait), so that we don't try to call set([]) (which does nothing as near as I can tell, but seems like it could have crazy unexpected behavior).

adstage-david commented 11 years ago

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'})

mnot commented 11 years ago

Question - if you PATCH to Rails today with a media type other than application/json, what does it do?

adstage-david commented 11 years ago

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 Model#update_attributes.

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: {patch: [...]}. Think I'll need to add a check for that and return a proper 415 code.

steveklabnik commented 11 years ago

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 update and not update_attributes now.

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.

domenic commented 11 years ago

@steveklabnik: curious what you think of the appplication/json-overwrite+json idea, so as to provide something easy for the 80% use case, like Rails and Backbone already do, but also allow future extensibility for full application/json-patch+json support and not violate any spec errata. Am I barking up the wrong tree?

steveklabnik commented 11 years ago

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 application/x-www-url-form-encoded, generally speaking. But it could/should support the 'right thing' as well as the override, just like how it supports sending actual HTTP requests with the right verb or POST requests with _method.

steveklabnik commented 11 years ago

Cool HTTP APIs don't change ;)

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.

adstage-david commented 11 years ago

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.

@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 if request.patch?; else block so the standard updates and patch versions can work side by side, but this is kind of ugly and wouldn't work if Rails was trying to pretend the form was a PATCH. This is definitely a wrinkle in the process and I have no idea how it could be properly handled. I think to support HTML form updates, Rails would need the standard update method that takes a POST and a patch method for actual PATCH requests (sent from Backbone hopefully), so they could be treated separately.

jashkenas commented 11 years ago

I've been talking PATCH over a bit with @mnot this afternoon, and have a couple of thoughts and conclusions:

The full semantics of JSON-Patch and JSON-Pointer go beyond what Backbone and Rails need, or any API that's backed by a relational database instead of a JSON store.
As soon as Rails (and Node via Express support) start accepting application/json PATCH by default, the cat's out of the bag. Major REST APIs beginning to use PATCH in this fashion will ensure that it becomes a de-facto standard, for better or for worse.
I don't see any real fundamental problem with saying that the semantics for sending an application/json PATCH are defined by the particular API you're talking to. Most will interpret the list of attributes as properties to be set on the top level of the resource ... and perhaps a few will go further to perform deep merges of the JSON. The fact that the same PATCH might be interpreted slightly differently by two different applications doesn't feel like a dealbreaker -- especially when the same is already true in effect for POST and PUT, and given how unlikely it will be.
Being able to GET application/json, POST it, PUT it, and PATCH it -- all with the same format -- is an aesthetically pleasing interface for a web application to expose.
If you need fancier operations, you can always use JSON-Patch.

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 null. Which is another way of saying that it would be nice to have in theory, but I can't imagine anyone who isn't using a document store wanting to turn it on.

mnot commented 11 years ago

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

knowtheory commented 11 years ago

So, how does Rails behave with PATCH support and nested attributes currently?

mnot commented 11 years ago

For those who are interested: https://github.com/rails/rails/issues/10439

adstage-david commented 11 years ago

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

braddunbar commented 11 years ago

@adstage-david I'm fairly sure you can do that already via the data option, right?

adstage-david commented 11 years ago

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.

adstage-david commented 11 years ago

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?

braddunbar commented 11 years ago

Backbone.sync (and by proxy, fetch/save/destroy/create) passes any options you give it on to Backbone.ajax ($.ajax by default). Using those options for custom behavior is encouraged and idiomatic.

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

mnot commented 11 years ago

One other option - http://tools.ietf.org/html/draft-snell-merge-patch-07

I think this might be closer to the semantics you want...

jashkenas / backbone

Discuss PATCH #2512

btw, that should be @steveklabnik