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

Support for Json Merge Patch - RFC7396 #2436

Open
aspnet-hello opened this issue Jan 1, 2018 · 27 comments

Comments

@aspnet-hello
Copy link

commented Jan 1, 2018

From @adnan-kamili on Wednesday, January 4, 2017 10:22:13 PM

Hi,

Are their any plans to support the Json Merge Patch

https://tools.ietf.org/html/rfc7396

Copied from original issue: aspnet/JsonPatch#52

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @vhe1 on Tuesday, February 21, 2017 3:35:44 AM

Seconded.
Can we have support for it?
It's much easier on the frontend developers and the rfc 6902 patch has a gaping security hole in that any path expression can be added to the list, regardless of whether the property exists on the type given in the generic type parameter.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @gilmishal on Sunday, June 4, 2017 7:40:06 AM

Third.
RFC 6902 is also not very client side friendly.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Thursday, August 24, 2017 8:08:28 AM

This would be great. Seems a lot more appropriate for what I'd imagine the "average" user requires of JSON Patch.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Sunday, August 27, 2017 6:02:01 PM

What does the 1 - Ready label mean?

Has an API for this already been decided? Or is it in discussion -- if so is this public?

I have been playing around a bit and I've got a working PoC which allows for uhhh.. this:

// Normal DTO/model
public class PersonDetailsUpdate
{
    public string GivenName { get; set; }
    public string Surname { get; set; }
    public int Age { get; set; }
    public int? GroupId { get; set; }
}
[HttpPatch("Users/{id:int}")]
public IActionResult Patch([FromRoute]int id, JsonMergeDocument<PersonDetailsUpdate> mergeDocument)
{
    JsonMergeDocumentMember<int?> groupIdMember = mergeDocument.Member(x => x.GroupId);
    if (groupIdMember.IsSet)
    {
        int? newGroupId = groupIdMember.Value;
        if (newGroupId.HasValue) {
            // Throw if group is not valid
            _groupsService.SetUserGroup(id, newGroupId);
        }
        
        _groupsService.UnSetUserGroup(id);
    }

    using (UsersDbContext usersContext = new UsersDbContext())
    {
        // Check if user exists
        UserEntity user = new UserEntity
        {
            Id = id
        };

        EntityEntry entity = dbContext.Users.Attach(dbUser);

        foreach(IEnumerable<JsonMergeDocumentMember> member in mergeDocument.Members.Where(x => x.IsSet))
        {
            PropertyEntry entityProp = entity.Member(member.Name) as PropertyEntry;
            if (entityProp != null)
            {
                 if (entityProp.MetaData.ClrType != member.Type)
                 {
                     // perhaps try convert type and set
                 }
                entityProp.CurrentValue = member.Value;
            }              
        }

        usersContext.SaveChanges();
    }

    return NoContent();
}

Assume the following data is persisted:

UserEntity {
  Id = 10,
  FirstName = "Mardoxx",
  Surname = "",
  Age = 99
}
UserGroup {
    UserId = 10,
    GroupId = 100
}

With group information being stored in a completely isolated service.

That is, a GET Users/10 would return:

{
    "UserId": 10,
    "FirstName": "Mardoxx",
    "Surname": "",
    "Age": 99,
    "GroupId" : 100
}

Sending PATCH Users/10 with a body of

{
    "Age": 20,
    "GroupId" : null
}

Will update the user's age to 20, and remove the user from its group, i.e. GroupId to null.

A GET Users/10 now returns:

{
    "UserId": 10,
    "FirstName": "Mardoxx",
    "Surname": "",
    "Age": 20,
    "GroupId" : null
}

JsonMergeDocumentMember and JsonMergeDocumentMember<T> are DataContractSerializable.

JsonMergePatch<T> is as well.


This is the sort of functionality I would like from an official impl of Json Merge Patch. Validation is a necessary addition to this, so is an T ApplyTo<T>(T to) function. Could also perhaps do with a object ToObject() which emits a new anonymous type with the fields defined -- probably out of scope of this though. Can definitely be improved upon. Also not sure how it copes with complex objects (e.g. Post with an Author field, although I think that is automatically handled by the deserialisation). From the spec it seems that only first-level properties can be patched. You can't do a partial update to a nested field (which works out quite nicely actually)

An official solution to something like this would be really desirable!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @peabnuts123 on Wednesday, August 30, 2017 8:25:07 PM

Just pitching in from a front-end-dev perspective. Granted that JSON Patch is a nice standard but it's pretty widely accepted to do operations in a JSON-Merge kind of way. To a JS developer, it would be pretty weird if their backend developer told them they couldn't support this kind of operation, it would be generally seen as basically an incapable/not very useful technology.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Friday, September 1, 2017 7:57:37 AM

Ok so I've cleaned it up a bit, API is a little different now, Members is a Dictionary<string JsonMergePatchValue>
For the end user all they need to do is change their controller object types to be JsonMergePatchDocument<>, and then use T patchDoc.ApplyTo<TObj>(TObj object) or whatever to update an object for example, or do what I did above.
I've basically designed it in a way that I want it to work. I really want to get the ball rolling with this because I feel it would be very useful!

Shall I write a few tests and do a PR? Or has work on this already begun?

I want this to be DataContract serializable too as it would be great to be able to send this object around in Service Fabric using their remoting libs, for which this is a requirement. Not quite sure how to achieve this though.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @mhosman on Friday, September 1, 2017 8:20:52 AM

+1 For this. Please add support for Json Merge Patch!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @rynowak on Friday, September 1, 2017 10:46:45 AM

@Mardoxx - we'll discuss and get back to you next week. We're still planning the set of features we're going to do for 2.1, and this is under consideration. I'm putting a reminder on my calendar right now.

If you have an example of this somewhere in a repo that we can look at that would help.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @tinchoel10 on Friday, September 1, 2017 11:57:48 AM

+1 Please add json merge patch, this is more client-friendly, easier to understand and to manage in a lot of situations. Is a very important feature.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Friday, September 1, 2017 2:42:08 PM

@rynowak Amazing thanks.

Done here - https://github.com/Mardoxx/RJBM.JsonMergePatch Cleaned it up a lot and simplified it... a lot. Hacked together a handful of tests that should give an idea of behaviour. Not happy about a few things, see here, but happy enough about general API for it (not necessarily the names of things though!). Gives a basic idea of what I want.

Can add end to end example if you like?

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @mhosman on Monday, September 4, 2017 10:27:36 AM

Hey @Mardoxx, this is a working (non-official) example? I know there are several things to work in it (Mardoxx/RJBM.JsonMergePatch#1), but it works?

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Monday, September 4, 2017 10:36:35 AM

@mhosman Yeah it works! Very very basic example here or slight modifications to above should work 😄

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @mhosman on Monday, September 4, 2017 4:57:02 PM

Hey @Mardoxx, thanks for your answer. There's any way to create a NuGet package in order to test this in one of my projects? Thank you!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @rynowak on Tuesday, September 5, 2017 10:20:09 AM

@Mardoxx - have you done any work with your code to support cases like the following:

Original

{
     "title": "Goodbye!",
     "author" : {
       "givenName" : "John",
       "familyName" : "Doe"
     },
     "tags":[ "example", "sample" ],
     "content": "This will be unchanged"
   }

Patch

{
     "title": "Hello!",
     "phoneNumber": "+01-123-456-7890",
     "author": {
       "familyName": null
     },
     "tags": [ "example" ]
}

After

{
     "title": "Hello!",
     "author" : {
       "givenName" : "John"
     },
     "tags": [ "example" ],
     "content": "This will be unchanged",
     "phoneNumber": "+01-123-456-7890"
   }

Notice in this case that author was patched by setting the familyName property to null and the givenName property retains it's original value. This is example 3 from the RFC.

From the .NET point-of-view this seems like the hard part.

Here's the pseudo-code from the spec (emphasis mine)

define MergePatch(Target, Patch):
     if Patch is an Object:
       if Target is not an Object:
         Target = {} # Ignore the contents and set it to an empty Object
       for each Name/Value pair in Patch:
         if Value is null:
           if Name exists in Target:
             remove the Name/Value pair from Target
         else:
           Target[Name] = MergePatch(Target[Name], Value)  // *** Need to apply recursively ***
       return Target
     else:
       return Patch

Some of the other challenges that we've run into implementing patch are around casing and the behavior of dictionary types and dynamic types. JSON-Patch and JSON-Merge (and JSON) in general are case-sensitive.

We banished most of these issues recently by using JsonContract to just do what JSON.NET does with respect to casing and how to 'interpret' your types. Presumably if you are using MVC you are using JSON.NET and already are serializing the types you want to patch elsewhere. That gives you one knob to configure how JSON works.\

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Tuesday, September 5, 2017 10:43:31 AM

Totally overlooked that, will have a look later - and also adding properties that don't exist on the original document (i.e. source is a dynamic object)... phoneNumber in that example. Hmm!

Haven't looked at (didn't occur to me) Json.Net api! Good idea.


Ok, this is very difficult. Does not really fit in with the way I wrote it. Back to the drawing board!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @rynowak on Friday, September 15, 2017 11:55:25 AM

Hi Folks, sorry for the delay.

We don't have an official 2.1 announcement post out yet, but I'm sad to say JSON Merge didn't make the cut. We understand the feedback that this is valuable and we'd like for a good .NET solution to exist, but we're not going to invest in building it right now.


I'd suggest teaming up and fleshing out the implementation that @Mardoxx has started working on. We'd be happy to provide advice and help promote the library if it takes off.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @mhosman on Friday, September 15, 2017 12:17:04 PM

Maybe @Mardoxx could make a library (NuGet Package) at least for the basics.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Friday, September 15, 2017 12:38:11 PM

@rynowak Understandable, it's something that comparatively few will benefit from.

Not at all certain about the current implementation I came up with (besides it only conforming to like 1/3rd of the spec!) -- Not had time to think about it the past few days. Some pointers from someone more experienced than I, or anyone really, would be very welcomed!

@mhosman, seeing as it's only 4 files or so, feel free to just copy them to your own project. I would not feel okay publishing this anywhere too public (e.g. NuGet) until it is in an actual working state!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @mhosman on Monday, September 18, 2017 10:52:52 AM

I just found this.... https://github.com/Morcatko/Morcatko.AspNetCore.JsonMergePatch

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Tuesday, September 19, 2017 2:59:13 AM

@mhosman good find. Feels uneasy to me re-purposing jsonpatch. But hey, if it works.. it works!

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @adnan-kamili on Tuesday, September 19, 2017 3:50:38 AM

Since I logged this issue, I have been using the following to perform a patch:

        public void Patch<TEntity, TViewModel>(TEntity entity, TViewModel updatedModel) where TEntity : class, IEntity
        {
            // copy the value of properties from view model into entity
            PropertyInfo[] entityProperties = entity.GetType().GetProperties();
            foreach (PropertyInfo entityPropertyInfo in entityProperties)
            {
                PropertyInfo updatedModelPropertyInfo = updatedModel.GetType().GetProperty(entityPropertyInfo.Name);
                if (updatedModelPropertyInfo != null)
                {
                    var value = updatedModelPropertyInfo.GetValue(updatedModel, null);
                    if (value != null)
                    {
                        entityPropertyInfo.SetValue(entity, value, null);
                    }
                }
            }
        }
@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @peabnuts123 on Sunday, September 24, 2017 1:09:49 PM

Seems like most people in this thread are missing the subtle difference between omitting a field from the request and sending a null value in the request. Checking against null in the implementation means that frontends sending null back in their payload will be ignored by the backend. This essentially creates a behaviour where, once being set to a value, a field will never be able to be set to null again.

I found this issue because I was trying to solve this problem myself and looking for others talking about the subject. In JavaScript-land this nuance can be modelled with undefined vs. null values but in C# the aspnet core middleware will serialize both an omitted field and a null field to null in the C# model, and therefore (from an information perspective) you literally cannot tell the difference by the time you've made it to a Controller. Wondering if perhaps AspnetCore could set annotations on Omitted fields or similar that could be read out in the Patch method to differentiate fields that were in the payload vs. fields that weren't.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @Mardoxx on Sunday, September 24, 2017 3:57:57 PM

No, both my and that other example account for null vs undefined.

On 24 Sep 2017 21:09, "Jeff" notifications@github.com wrote:

Seems like most people in this thread are missing the subtle difference
between omitting a field from the request and sending a null value in the
request. Checking against null in the implementation means that frontends
sending null back in their payload will be ignored by the backend. This
essentially creates a behaviour where, once being set to a value, a field
will never be able to be set to null again.

I found this issue because I was trying to solve this problem myself and
looking for others talking about the subject. In JavaScript-land this
nuance can be modelled with undefined vs. null values but in C# the
aspnet core middleware will serialize both an omitted field and a null
field to null in the C# model, and therefore (from an information
perspective) you literally cannot tell the difference by the time you've
made it to a Controller. Wondering if perhaps AspnetCore could set
annotations on Omitted fields or similar that could be read out in the
Patch method to differentiate fields that were in the payload vs. fields
that weren't.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
aspnet/JsonPatch#52 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAOoMesFBtV7edaiq48u3STBVGgAJopTks5slrcPgaJpZM4LbYdB
.

@aspnet-hello

This comment has been minimized.

Copy link
Author

commented Jan 1, 2018

From @peabnuts123 on Sunday, September 24, 2017 7:54:49 PM

Alright, that's my mistake. I hadn't trawled the code, and was merely acting on language cues.

@SatishTata

This comment has been minimized.

Copy link

commented May 8, 2018

Is this coming soon as part of asp.net?

@mhosman

This comment has been minimized.

Copy link

commented May 8, 2018

@SatishTata I don't know when this is gonna happend (officially) but meanwhile you can use this third party library: https://github.com/Morcatko/Morcatko.AspNetCore.JsonMergePatch

@Eilon Eilon added area-mvc and removed repo:JsonPatch labels Nov 7, 2018

@alec-lieberman-ef

This comment has been minimized.

Copy link

commented Apr 18, 2019

Is anything regarding this being worked on for .Net Core?

@pranavkm pranavkm added the c label Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.