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

Restores support for instance-based JSON serializer settings in a non-breaking way #4888

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

bkoelman
Copy link
Contributor

This PR adds an instance-based field for serializer settings in generated clients, which is null by default. Consumers can implement the Initialize() partial method and assign the _instanceSettings field from there, which takes precedence over static settings.

Example usage to change serializer settings at runtime, based on configuration:

public sealed class AppOptions
{
    public bool PrettyPrintJson { get; set; }
}

public partial class ExampleApiClient
{
    private readonly IOptionsMonitor<AppOptions> _optionsMonitor;

    private static readonly JsonSerializerSettings JsonIndented = new() { Formatting = Formatting.Indented };
    private static readonly JsonSerializerSettings JsonNone = new() { Formatting = Formatting.None };

    public ExampleApiClient(IOptionsMonitor<AppOptions> optionsMonitor, HttpClient httpClient)
        : this(httpClient)
    {
        _optionsMonitor = optionsMonitor;
    }

    partial void Initialize()
    {
        _instanceSettings = _optionsMonitor.CurrentValue.PrettyPrintJson ? JsonIndented : JsonNone;
    }
}

The real use case is adding stateful JSON converters, such as recommended at https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to?pivots=dotnet-8-0#preserve-references.

@RicoSuter Please consider this PR seriously. I've explained in #4662 why adding this is critical for us to continue supporting NSwag usage.

Fixes #4662, fixes #4701.

@TWhidden
Copy link

Appreciate you putting together a PR for this. I'm on my mobile phone trying to dig through it. Does it also support System.Text.Json? We switched away from Newtonsoft, I imagine the PR would have to support both options. But most definitely something that needs to be done as we have to support changing serialization options at runtime, based on the server version our clients are connected to. Thanks again, and the right idea I feel with a clean option to support both ways.

@bkoelman
Copy link
Contributor Author

@TWhidden Yes, I've tested that it also supports System.Text.Json.

bkoelman added a commit to json-api-dotnet/JsonApiDotNetCore that referenced this pull request May 12, 2024
@ErikPilsits-RJW
Copy link
Contributor

@bkoelman this looks good to me! It's a good combination of safe and flexible.

@RicoSuter

…-breaking way.

Consumers can implement the Initialize() partial method and assign the _instanceSettings field from there, which takes precedence over static settings.
@RicoSuter
Copy link
Owner

So if we merge this now will it break everything again and I have to fix it again?

@bkoelman
Copy link
Contributor Author

bkoelman commented Jul 1, 2024

@RicoSuter I'm not sure what you're referring to, can you clarify? This change is fully backwards compatible. It just adds a field and a partial method to enable overruling the static serializer. The existing behavior is unaffected unless user code activates the added bits.

@RicoSuter
Copy link
Owner

Lgtm, thx.

@RicoSuter RicoSuter merged commit ba5d286 into RicoSuter:master Jul 1, 2024
3 checks passed
@bkoelman bkoelman deleted the fix-issue-4662 branch July 1, 2024 23:54
bkoelman added a commit to json-api-dotnet/JsonApiDotNetCore that referenced this pull request Jul 9, 2024
bkoelman added a commit to json-api-dotnet/JsonApiDotNetCore that referenced this pull request Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants