aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.61k stars 2.14k forks source link

Support Json Patch as a top level feature #1976

Closed yishaigalatzer closed 9 years ago

yishaigalatzer commented 9 years ago

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

KevinDockx commented 9 years ago

+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 commented 9 years ago

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

KevinDockx commented 9 years ago

Hi Yishai,

of course, I'd be happy to!

lukas-shawford commented 9 years ago

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:

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.

kirthik commented 9 years ago

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

abpatel commented 7 years ago

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