RicoSuter / NJsonSchema

JSON Schema reader, generator and validator for .NET
http://NJsonSchema.org
MIT License
1.37k stars 529 forks source link

INPC: Skip change check (to support partial patch) #1717

Open bkoelman opened 1 month ago

bkoelman commented 1 month ago

When using <NSwagClassStyle>INPC</NSwagClassStyle> to generate INotifyPropertyChanged, the generated C# code only calls RaisePropertyChanged when the property value differs from the original value.

I'd like to support partial patch (part of the JSON:API spec), which states that sending a property with its default value means something else than omitting it from the request body. For example, sending "firstName": null writes null in the database column, whereas not sending it leaves it unchanged.

I have a prototype using a stateful JSON converter to implement this. A client developer can call my helper to track property assignments. Example usage:

var updatePersonRequest = new UpdatePersonRequestDocument
{
    Data = new DataInUpdatePersonRequest
    {
        Id = "1",
        Attributes = new TrackChangesFor<AttributesInUpdatePersonRequest>(_apiClient)
        {
            Initializer =
            {
                FirstName = null, // <-- this gets tracked
                LastName = "last"
            }
        }.Initializer
    }
};

var response = await _apiClient.PatchPersonAsync(updatePersonRequest.Data.Id, updatePersonRequest);

_apiClient.Clear(); // optional: clears the tracked assignments for _apiClient reuse.

For this to work, I need RaisePropertyChanged to always be invoked when the property is assigned. Conceptually, this dirty tracking is similar to the IBackingStore in kiota, which is activated using the --backing-store switch.

My initial ask is to add a command-line switch to control this. It could be /skipChangeCheck (false by default), or an additional class style INPS (where the "s" means Set instead of Changed).

However, I imagine there's a barrier to adding new configuration switches, from a maintenance perspective. The next best thing would be to ship an adapted liquid template from our NuGet package, which gets activated in the target project from an embedded .props file in the package. But this means we need to include a customized version of the full Class.liquid, which contains many conditionals. While we only need to hide the line at https://github.com/RicoSuter/NJsonSchema/blob/1569e52401628d009af8044d1886d5a26770d05c/src/NJsonSchema.CodeGeneration.CSharp/Templates/Class.liquid#L107C13-L107C51:

if ({{ property.FieldName }} != value)

So the next best thing to ask for, is splitting up this template. This would reduce the risk of our copy getting out of sync with the built-in version. Similar to the existing Class.Inpc.liquid, I'd like to propose introducing Class.PropertySetter.Inpc.liquid with the following contents (extracted from Class.liquid):

{{PropertySetterAccessModifier}}set
{
    if ({{ property.FieldName }} != value)
    {
        {{ property.FieldName }} = value;
        RaisePropertyChanged();
    }
}

Whether it's going to be a new switch or an extracted template, I'd be happy to do all the work to implement/test this.

Please let me know what you think. Would one of these be an acceptable design, or would you prefer something else? Are there additional concerns I should be aware of?

bkoelman commented 1 month ago

@RicoSuter Would you mind indicating your preferred solution here? I want to get some input to determine future directions for my project.

And can you please tag the core contributors, so they can also express their opinions?