Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Support Json Patch as a top level feature #1976

Closed
yishaigalatzer opened this issue Feb 9, 2015 · 6 comments
Closed

Support Json Patch as a top level feature #1976

yishaigalatzer opened this issue Feb 9, 2015 · 6 comments

Comments

@yishaigalatzer
Copy link
Contributor

http://jsonpatch.com/
https://tools.ietf.org/html/rfc6902

Here are a couple projects out there that add partial support for JSON Patch within Web API, but they're both kind of "alpha" at this stage (neither implement the spec fully):

myquay/JsonPatch
Blog Post: http://michael-mckenna.com/Blog/how-to-add-json-patch-support-to-asp-net-web-api
Github: https://github.com/myquay/JsonPatch
NuGet: https://www.nuget.org/packages/JsonPatch/1.0.0

KevinDockx/JsonPatch
GitHub: https://github.com/KevinDockx/JsonPatch
NuGet: https://www.nuget.org/packages/Marvin.JsonPatch/0.3.0

Split from a comment on Delta issue by @SergKr #650

@yishaigalatzer yishaigalatzer changed the title Consider Supporting Json Patch as a top level formatter Consider Supporting Json Patch as a top level feature Feb 9, 2015
@yishaigalatzer yishaigalatzer added this to the 6.0.0-rc1 milestone Feb 11, 2015
@KevinDockx
Copy link
Contributor

+1. I'm the author of Marvin.JsonPatch, and aim to fully implement the spec by the time I reach v1, but I'd prefer this to be baked in. Of course: fully according to spec (eg: not like the OData Delta implementation). A request from any type of client should be able to be accepted by an HttpPatch action at Web API level as long as its according to spec. Same for the other way around: if helper classes for creating such a list of operations are included at MVC (-server side) level, a request created at that side with the list of operations in its body should be according to spec, so any API created in any tech that supports the standard can accept that request.

I'd be glad to help/provide input if needed.

@yishaigalatzer
Copy link
Contributor Author

Kevin would you be interested in working with us on the implementation (perhaps as a series of pull requests?

@KevinDockx
Copy link
Contributor

Hi Yishai,

of course, I'd be happy to!

@lukas-shawford
Copy link

Couple questions to consider.

First, is there a possibility we would (eventually) want to support the notion of patch documents in XML as well, using something like RFC 5261? Even if it's not going to be done right now, it's worth bring it up as I imagine it might potentially affect the implementation (i.e., we might want to have a generic PatchDocument that isn't tightly coupled to either JSON Patch or XML Patch, and can be deserialized from both JSON and XML).

Second, it's common to have separate classes for domain objects ("entities") versus what you send/receive over the wire ("DTOs"). This is especially true if your entities are Entity Framework model classes (which may have circular references to establish foreign key constraints / navigation properties).

The issue is the client is likely going to be sending a patch document against a DTO, but generally we want to apply the patch against the actual entity. Ideally, we would want to be able to do something like this:

public IHttpActionResult Patch(int id, [FromBody]JsonPatchDocument<DTO.Expense> patch)
{
      // get the expense from the repository
      Entities.Expense expense = _repository.GetExpense(id);

      // apply the patch document - can't actually be done if the types don't match
      patch.ApplyTo(expense);

      // changes have been applied.  Save the entity...
}

The above example wouldn't actually work since we're trying to apply a patch document of type DTO.Expense against an entity of type Entities.Expense. I think being able to support this use case is kind of important, as otherwise we're limiting being able to use Patch in all but the simplest of APIs.

Now, when an API separates the domain objects from the DTOs, it would generally also have a mechanism for converting back and forth between the two (maybe just using AutoMapper, or something like the ModelFactory concept). So, the API author could load the entity from the database, convert it to a DTO using their preferred mechanism of choice, apply the patch against the DTO, then convert the patched DTO back to an entity, then finally save the entity:

public IHttpActionResult Patch(int id, [FromBody]JsonPatchDocument<DTO.Expense> patch)
{
      // get the expense from the repository
      Entities.Expense expense = _repository.GetExpense(id);

      // Convert to a DTO
      DTO.Expense expenseDto = ConvertToDTO(expense);

      // apply the patch document against the DTO
      patch.ApplyTo(expenseDto);

      // convert back to an entity
      expense = ConvertToEntity(expenseDto);

      // changes have been applied.  Save the entity...
}

The issue I see with this (besides the number of steps involved) is it's basically replacing the entire expense entity (and if that entity references other entities, it may potentially be replacing an entire object graph) even if the patch was for a single property on the root object. That's fine for PUT/POST, but it almost seems to defeat the entire purpose of PATCH to be doing this...

Kevin's implementation does have the notion of "adapters", which I think can be used to accommodate this use case:

If you want to provide your own adapter (responsible for applying the operations to your objects), create a new class that implements the IObjectAdapter interface, and pass in an instance of that class in the ApplyTo method.

I like this idea, but the issue I see with this is implementing an IObjectAdapter is not a simple matter. If you look at how the default SimpleObjectAdapter is implemented, for instance, it's basically asking the user to implement all the operations in the JSON patch spec (though granted, there are helpers they can use for things like checking if the types are compatible, etc).

Here are my own thoughts on this. I like the idea of having "adapters", but I'd like them to be simpler for the user to implement. For instance, if the only thing that's different between the entity and the DTO is the name of a property, that's something they should be able to express (ideally in a single line of code), and otherwise be able to rely on the default implementations of all the operations such as Add, Replace, Remove and so on. But at the same time, they should be able to accommodate complex things, too (for example, the DTO might have a "Type" property which, if changed, should result in instantiating a different type for the entity - might make sense if your entities form an OOP hierarchy, such as a base Question type, and inherited FillInQuestion and MultipleChoiceQuestion derived classes).

Furthermore, I don't think the user's primary concern is going to be changing how the operations themselves (Add, Remove, Replace, etc) are implemented - they probably want those to remain as close to the spec as possible. They likely still want a copy to do what a copy normally does. The actual concern, I think, is likely going to be the fact that path "/Foo/Bar" in the DTO might actually correspond to property Foo.Qux in the entity; or, maybe the property does not exist in the entity at all, and must be retrieved from elsewhere. Or maybe they want to prevent being able to set a particular path. In other words, it is likely that the thing they want to be able to override is what to do when we attempt to retrieve a value at a given path (regardless of whether the read is occurring as a result of move, copy, or test), or set a value at a given path (regardless of whether the write is occurring as a result of add, remove, replace, etc).

All the operations in a JSON Patch document can be decomposed to a list of get/set operations on either properties or collection indexes, as well as Insert/RemoveAt operations on collection indexes. Here are a couple examples of JSON patch documents, and the operations that they decompose to:

  • { "op": "replace", "path": "/a/2", "value": "foo" }
    • entity.a[2] = "foo"
  • { "op": "add", "path": "/a/2", "value": "foo" }
    • entity.a.InsertAt(2, "foo")
  • { "op": "move", "from": "/a/2", "path": "/b/3" }
    • temp = entity.a[2]
    • entity.a.RemoveAt(2)
    • entity.b.InsertAt(3, temp)
  • { "op": "copy", "from": "/a/2", "path": "/b/3" }
    • temp = entity.a[2]
    • entity.b.InsertAt(3, temp)

So, what if in the adapter, rather than having to override how Add, Move, Replace, etc. are implemented, you instead get the ability to override how these decomposed operations are applied for a given path? For instance, if "/Foo/Bar" should actually be Foo.Qux, maybe the user could write something like this:

var adapter = new JsonPatchAdapter<Expense.DTO, Entities.Expense>();
adapter.Get("/Foo/Bar", entity => entity.Foo.Qux);
adapter.Set("/Foo/Bar", (entity, value) => { entity.Foo.Qux = value; });
patch.ApplyTo(expenseDto, adapter);

I haven't though this all the way through yet. I know there are probably going to be some challenges here, especially with anything that refers to collection indexes, but I wanted to get some thoughts on the idea.

@yishaigalatzer yishaigalatzer changed the title Consider Supporting Json Patch as a top level feature Support Json Patch as a top level feature Feb 20, 2015
@kirthik
Copy link
Contributor

kirthik commented Mar 9, 2015

First check-in went into jsonpatch branch. I am closing this one. I will open new issues for the items we discussed.

@abpatel
Copy link

abpatel commented Feb 6, 2017

Will there be support for Deltafor JsonMediaTypeFormatter for the full .NETFX WebAPI?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants