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

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

Open
bkoelman opened this issue Jul 14, 2024 · 1 comment
Open

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

bkoelman opened this issue Jul 14, 2024 · 1 comment

Comments

@bkoelman
Copy link

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
Copy link
Author

@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?

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

No branches or pull requests

1 participant