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

implement json patch RFC #69

Closed
oli-obk opened this issue May 13, 2016 · 5 comments
Closed

implement json patch RFC #69

oli-obk opened this issue May 13, 2016 · 5 comments

Comments

@oli-obk
Copy link
Member

oli-obk commented May 13, 2016

Should RFC6902 JSON Patch be implemented or is this something for a separate crate? (Patch is based on Pointer)

#41 (comment)

@dtolnay
Copy link
Member

dtolnay commented Dec 7, 2016

Copying this from IRC so it doesn't get lost:

<Cryptarchy> I have a library design quandary, if anyone would like to offer an opinion. I'm attempting to implement JSON Patch into serde_json.
<Cryptarchy> And I've got the general functionality just about done. But now I've run into the interesting circular problem that JSON Patches are JSON objects. So I'd need to convert them into something else in order to operate on them.
<Cryptarchy> I could just use from_str to get serde_json::Value structs to work with, and just use the values out of the map directly, but that seems kind of a hack.
<Cryptarchy> On the other hand, deserializing JSON into an object would require higher level bits of serde, which aren't available in serde_json. Or are they? That's what I'm unsure of: how to treat the string JSON object.

From serde_json's perspective I don't think we want to treat JSON patches as strings (i.e. dealing with from_str). If we just expose a type like serde_json::Patch and implement Serialize and Deserialize for Patch, the user's crate can handle serialization in the way that makes the most sense for them. For example they may want to embed a JSON patch into a larger struct, or deserialize from bytes instead of str, or use other generic deserialization machinery in their crate, or etc.

We can expose Patch and we can expose a function or method to apply a Patch to a serde_json::Value. I don't know JSON patch well enough to know what makes the most sense semantically but something like serde_json::Value::apply_patch(Patch) or serde_json::Patch::apply_to(Value) or serde_json::apply_patch(Patch, Value).

@CryptArchy does that approach address your concern about needing "higher level bits of serde"?

@CryptArchy
Copy link
Contributor

Yes, I think that all makes sense. I currently have a Command enum with the different ops, which I may rename as Patch or maybe Patch will be a type alias for Vec<Command>. And there's a apply_patch method on Values (currently in a Patcher trait) that mutates the Value according to the op(s).

I'll look into using the general (de)serialization traits and how to implement them for the final Patch type, that sounds like the way to go.


The trickiest part so far has been meeting the demands of atomicity required for HTTP Patch semantics. The RFC isn't totally clear if that's required by the implementation itself or if the library user should respect those semantics. I opted to implement it so the library is easier to use. If any part of the patch fails, the whole thing has no effect.

I could think of two obvious ways to do it:

  1. copy the Value first and if the patch fails, replace the original with the copy
  2. generate the inverse command and build an undo stack that can be executed to rollback

I went with the second option, though more complicated, it seems like it should be more efficient (needs testing). As a nice side benefit, you can also return the undo stuck so every application of a patch gives you the commands to revert it yourself if there's a need from a higher level.

It also creates a pleasing symmetry and flow in the execution. For example, Removeing a value gives ownership of that value, which is then consumed by creating an Add command to the same path which would put the value back. Adding a value can replace an existing value, so sometimes the inverse of an Add is a Replace with the old value in it. Errors return the faulty value. Essentially, no data is ever lost, just shuffled around and it's all enforced by the ownership system. It's a pretty uniquely Rustic version of JSON Patch! I'll post the code up soon to start review, though I have a lot of cleanup to do yet, so a PR is still a little ways off.

@CryptArchy
Copy link
Contributor

Alright, I cleaned up things a little and pushed some code up to my fork.

https://github.com/serde-rs/json/compare/master...CryptArchy:patch?expand=1

It's very rough as I'm still iterating on the concepts, but all the ops are in and working.

I added one non-standard op _Bump to act as the inversion of the Move op. Because of the annoying semantics where Add can either add or replace a value, any ops that rely on Add-like behavior get complicated. Move is the trickiest, as it always removes one value and then may create or replace a value. So reverting it requires either an inverted Move (if a new value was added) or a Move and an Add (if an existing value was replaced).

Bump is equivalent (and implemented as) a Replace then an Add. I could return a Vec or tuple of Commands from patch_move but then you need special handling for apply_patch and everything else to flatten it.

Currently, I'm thinking that the system could just convert _Bump into the equivalent commands before returning to the user, so that it's just an internal convenience. Since variants can't be private, I think I'd need a second Command enum that doesn't have it or something like that. I'm not sure yet, tricky stuff!

@idubrov
Copy link

idubrov commented Feb 1, 2018

I wrote my own implementation of both JSON Patch and JSON Merge Patch a while ago: https://github.com/idubrov/json-patch

It tries to be optimal and modify Value in-place and revert changes if one of the operations fails.

@dtolnay
Copy link
Member

dtolnay commented Mar 28, 2018

The json-patch crate looks great! I am happy pointing people to the implementation there -- I don't think we need this to be provided by serde_json. Thanks all!

@dtolnay dtolnay closed this as completed Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants