Please revert static UpdateJsonSerializerSettings

[bkoelman]

When trying to update to v14-preview, I found that our code no longer compiles because the generated UpdateJsonSerializerSettings partial method has been changed to become static. This doesn't work for us, because we provide a NuGet package that contains a base class to hook into the serializer settings, in order to support JSON:API partial patch.

For example:

// In our NuGet package
public abstract class JsonApiClient : IJsonApiClient
{
   protected void SetSerializerSettingsForJsonApi(JsonSerializerSettings settings)
   {
        // implementation relies on instance-based state
   }
}

// Consumer of our NuGet package
public partial class ExampleApiClient : JsonApiClient
{
    partial void UpdateJsonSerializerSettings(JsonSerializerSettings settings)
    {
        SetSerializerSettingsForJsonApi(settings);
    }
}

Would you please consider reverting this change? I can't think of a way to work around it.

[RicoSuter]

Ref: #4256

I'd say you should already implement it "correctly" in the nuget's static method via partial - i.e. the consumer should not need to know about the internal serialization behavior of the server, no?

/cc @ErikPilsits-RJW

BTW: You can always override the templates and implement the old behavior this way.

[ErikPilsits-RJW]

I agree it should be implemented correctly in the NuGet package. There is no alternative for STJ here.

Your case is a bit unique because you have a NuGet pkg dependency on the version of an external application in some way. But if you have clients using your code and STJ, then I would HIGHLY recommend you and they take the time to make the changes instead of staying on the previous generated client code.

[bkoelman]

I'm very well aware of the performance implications when recreating JsonSerializerOptions each time. But how is that relevant here? As far as I know, NSwag uses Newtonsoft, which doesn't have this problem.

One blocker of the static partial is that it's no longer possible to build a singleton options instance that is lazily initialized based on IConfiguration. While it's easy to return a singleton from an instance method, the reverse is impossible.

Note I'm developing a library, which is part of a bigger open-source framework. Therefore overriding the templates is not an option: we'd need to instruct all of our users to tweak templates, which defeats the purpose of providing a library to make our scenario work.

The reason we're intentionally recreating options is the following. Our framework implements the JSON:API REST protocol, which supports partial patch. Setting a property to null means to overwrite the stored value with null, whereas omitting the property means to leave the stored value unmodified. Because there's no way in C# to make that distinction, our library provides an extension method with a base class to make it work. Based on that, we modify the JSON contract temporarily for the next request. When the feature is unused, we use a cached default options instance. So the behavior is bound to a client instance. The switch to static makes it impossible to work in multi-threaded scenarios where multiple client instances exist concurrently.

See bullet 7 at https://github.com/json-api-dotnet/JsonApiDotNetCore/blob/openapi-includes/docs/usage/openapi-client.md#visual-studio for example usage.

To overcome the STJ performance impact, I can imagine we could provide a JsonConverter that performs similar tweaks. We'd have a singleton JsonSerializerOptions instance that contains the converter, which behaves differently each time. But we'd still need the partial to be non-static to hook things up.

There's really no way I can think of to make this work (in a user-friendly way) as long as the partial method remains static. The whole point of our library is that the consumer doesn't need to mess with serializer settings or customize NSwag itself.

[bkoelman]

@RicoSuter @ErikPilsits-RJW Please keep in mind that HttpClient and the serializer are the ultimate extensibility points to control anything from general-purpose code that doesn't rely on an OpenAPI-specific shape.

Would you take a PR that adds /GenerateStaticSerializerMethod:false to revert to the old behavior?

[bkoelman]

For context, our project is the most popular .NET server implementation of the JSON:API protocol, for which 200+ libraries exist in various languages. Our project is used by thousands of APIs (800k NuGet downloads).

I'd like it to work with the latest version of NSwag, but I'm running out of options.

[efanovroman]

+1, we need non static -> partial void UpdateJsonSerializerSettings(JsonSerializerSettings settings).
For example, I use inside

settings.Error = (sender, args) =>
{
    errorHandler?.Invoke($"{args.ErrorContext.Error.Message} - {System.Environment.StackTrace}");
    args.ErrorContext.Handled = true;
};

where errorHandler private field and now since the method is static I cannot use client class fields in it

[ErikPilsits-RJW]

But again, that's a fundamental misuse of a static settings instance, which is the correct way to use the settings. You can't keep re-adding an instance handler to a static settings object.

I've looked at solutions for the other issue brought up here, changing serialization per-request, and also given up because the serialization frameworks don't have a proper way to support it.

For both issues, I might suggest if you need some other kind of per-request data in your handler/converter that isn't part of the http request/response, that you look into a static AsyncLocal<> property that acts as a context per async context.

[bkoelman]

@ErikPilsits-RJW Thanks for responding.

I understand your intent to guide unfamiliar developers from falling into the trap of recreating unneeded instances. However, the way it was implemented in NSwag goes against the STJ design by forcing upon users that there can only ever be a single instance. And worse, that it can't rely on startup state for initialization. That's very different from what Microsoft recommends at https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/configure-options?pivots=dotnet-8-0#reuse-jsonserializeroptions-instances:

If you use JsonSerializerOptions repeatedly with the same options, don't create a new JsonSerializerOptions instance each time you use it. Reuse the same instance for every call. This guidance applies to code you write for custom converters and when you call JsonSerializer.Serialize or JsonSerializer.Deserialize.

Did you spot the difference? The essence is in the condition: "If you use JsonSerializerOptions repeatedly with the same options...". So this guidance doesn't apply to our use cases, where we require varying options, depending on context. Sure, we should still be smart about caching the few of them, yet the NSwag static design makes that impossible.

Microsoft could easily have enforced a single instance by making JsonSerializerOptions internally track a static instance, and then throwing whenever a second instance is created. They intentionally didn't do that, because there are legitimate use cases for having multiple.

The first that comes to mind is storing JsonSerializerOptions in IConfiguration, combined with IOptionsMonitor<>. The client would reference optionsMonitor.CurrentValue, so that a new JsonSerializerOptions instance is created whenever appsettings.json changes on disk (or any other external config provider, it is quite common in enterprises to use externally-stored central configuration management). This way, the client picks up the changed options without an app restart.

Another use case is explained at https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to?pivots=dotnet-8-0#preserve-references, which shows how converters can share state. Obviously, this is only possible when JsonSerializerOptions is non-static. Creating the options instance defines at which scope the sharing of state occurs, which is basically what my project needs.

Did you even consider the implications when NativeAOT (JSON source generator) is used?

Making everything static is like using global variables, which is an anti-pattern. Using AsyncLocal<> is just a bandage for compensating a flawed design by relying on low-level threading semantics. We need to be able to define the scope from unit tests, ie recreate a client, reuse a client, use multiple clients at the same time, await multiple operations in parallel, run tests concurrently, etc. It should be agnostic of the execution context (hosting model), ie WinForms/WPF handle async/await thread marshaling differently than ASP.NET does, so AsyncLocal<> would not work on desktop apps.

I cannot come to any other conclusion than that making the partial method static was a mistake. You should have aligned with how the .NET ecosystem handles this, which is recommending (not enforcing) to not reuse option instances when unneeded. While the internal caching pain was somewhat reduced in .NET 8, the official guidance remains the same.

Because NSwag has already shipped, the best next thing is to provide a switch to revert to the old behavior. And internally make everything non-static unconditionally. Then at the top of the call stack, fallback to JsonSerializerOptions.Default when creation is not overridden by the developer. Put a code comment advising to use a shared instance on the generated partial method, so users are aware.

Please share your technical challenges if you could use some help. My day job is building libraries that deeply integrate into ASP.NET at a tech company. I'm sure we can figure it out.

If you're unwilling to unblock us, we have no other choice than to abandon NSwag entirely and focus our efforts on supporting an alternative OAS client generator. This likely affects tens of thousands of APIs running in production.

[ErikPilsits-RJW]

It was not my intention to cause so much friction, rather to create the least breaking change which would accomplish the goal of fixing a serious issue with the generated client when using STJ. As it was, there was no way to prevent a new settings instance from being created with every client instance. I can appreciate clever library uses, but the existing process badly degraded our enterprise API layers.

I have an idea to hopefully accommodate everyone, but it would require a larger breaking change in how the partials work. The generated client would need to accept an options instance from the client without creating its own, and only create one as a fallback if one is not provided by the client. Then the client could determine if they want to use a static instance or not.

Maybe something like this?

public partial class TmsApiClient : ApiClientBase, ITmsApiClient
{
    private System.Net.Http.HttpClient _httpClient;
    private System.Lazy<System.Text.Json.JsonSerializerOptions> _settings;

    public TmsApiClient(ILogger<TmsApiClient> configuration, System.Net.Http.HttpClient httpClient) : base(configuration)
    {
        _httpClient = httpClient;
        _settings = new System.Lazy<System.Text.Json.JsonSerializerOptions>(InternalCreateSerializerSettings, true);
    }

    private System.Text.Json.JsonSerializerOptions InternalCreateSerializerSettings()
    {
        System.Text.Json.JsonSerializerOptions settings = null;
        CreateJsonSerializerSettings(ref settings);
        settings ??= new System.Text.Json.JsonSerializerOptions();
        UpdateJsonSerializerSettings(settings);
        return settings;
    }

    protected System.Text.Json.JsonSerializerOptions JsonSerializerSettings { get { return _settings.Value; } }

    partial void CreateJsonSerializerSettings(ref System.Text.Json.JsonSerializerOptions settings);
    partial void UpdateJsonSerializerSettings(System.Text.Json.JsonSerializerOptions settings);
}

Then in the partial

private static readonly JsonSerializerOptions Options = new(JsonSerializerDefaults.Web)
{
    DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
    PropertyNameCaseInsensitive = true,
    NumberHandling = JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.AllowNamedFloatingPointLiterals,
    Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
    Converters = { new DateOnlyJsonConverter(), new TimeOnlyJsonConverter() }
};

partial void CreateJsonSerializerSettings(ref JsonSerializerOptions options)
{
    options = Options;
}

ref is used as opposed to out or a return value, because using out or a return value requires an implementation of the partial.

Thoughts? @bkoelman @efanovroman @RicoSuter

EDIT:
Updated to include the original UpdateJsonSerializerSettings partial to maintain some kind of backward compatibility.

[ErikPilsits-RJW]

I could have a template PR today for the above changes if desired.

[bkoelman]

Thanks for the proposal. I'd prefer something simpler, how about this?

[GeneratedCode("NSwag", "...")]
public partial class ApiClient
{
    private static readonly JsonSerializerOptions DefaultSerializerSettings = JsonSerializerOptions.Default;

    private HttpClient _httpClient;
    private JsonSerializerOptions _settings;

    public ApiClient(HttpClient httpClient)
    {
        _httpClient = httpClient;
        
        CreateSerializerSettings();
        _settings ??= DefaultSerializerSettings;
    }

    // If you use JsonSerializerOptions repeatedly with the same options, don't create a new JsonSerializerOptions
    // instance each time you use it. Reuse the same instance for every call. 
    partial void CreateSerializerSettings();

    // Example OAS-specific generated method to demonstrate usage of JSON settings.
    public virtual async Task PatchResourceAsync(object body, CancellationToken cancellationToken)
    {
        _ = JsonSerializer.Serialize(body, _settings);
        await Task.Yield();
    }
}

// Optional: Manually added by developer
public partial class ApiClient
{
    partial void CreateSerializerSettings()
    {
        // Using customized shared options.
        _settings = MyAppWideSettings.SerializerOptions;
    }
}

internal static class MyAppWideSettings
{
    public static JsonSerializerOptions SerializerOptions = new(JsonSerializerDefaults.Web)
    {
        DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
        PropertyNameCaseInsensitive = true,
        NumberHandling = JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.AllowNamedFloatingPointLiterals,
        Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping
    };
}

This would cover the needs for my project as well, because it also enables the following:

// Manually added by developer, from instructions provided for using our NuGet package.
public partial class ApiClient : JsonApiClient
{
    partial void CreateSerializerSettings()
    {
        // Intentionally going against the advice to use a shared instance, because our NuGet needs this.
        _settings = new JsonSerializerOptions();
        SetSerializerSettingsForJsonApi(_settings);
    }
}

// In external NuGet package
public abstract class JsonApiClient
{
    protected void SetSerializerSettingsForJsonApi(JsonSerializerOptions settings)
    {
        // Adds a stateful JsonConverter.
    }

    // Provides other methods that act on the internally-tracked JsonConverter, left out for brevity.
}

How this differs from what v13 produces:

1. _settings is no longer lazy. Lazy looks more complicated to inexperienced developers, while the benefits are neglectable. Because a shared instance is used by default, there's no point in deferring creation. It only matters in the rare case when CreateJsonSerializerSettings is provided and the client instance is created without ever performing an API request.
2. UpdateJsonSerializerSettings has been removed. Because initialization is no longer lazy, there's no point in having it.
3. Protected JsonSerializerSettings get-only property has been removed. Because it misleadingly implies that serializer settings can be changed multiple times. While this works for Newtonsoft, STJ throws an exception after the first serialization has taken place. The proper way of changing serialization settings over time is using a stateful converter. Alternatively, if a user desperately wants to change them, they can easily modify _settings whenever that seems appropriate. And even provide their own protected method to do so. This is a little more work, which is a good thing. Trying to do the (most likely) wrong thing should not be made easy.
4. This also doesn't include the bool isRequest parameter that I saw in the PR that made the partial static. I don't see the point of having it, but perhaps I'm missing something.

[bkoelman]

@ErikPilsits-RJW Any progress?

[ErikPilsits-RJW]

We need feedback from @RicoSuter which approach he thinks is more appropriate, and how he feels about another breaking change. I think only your point 1 would be acceptable though. The rest are either required or would be kept for better backwards compatibility.

I could go either way on 1. It simplifies the partial method (no ref), but it's a bit odd to require a client partial to access a private field (I think?).

[bkoelman]

Makes sense. My proposal is just one way of addressing this. Reducing that to maximize backward compatibility sounds like a good next step. To clarify, the bare minimum we need is:

1. The private field becomes non-static and remains non-readonly (it could remain lazy).
2. A non-static partial method is available that executes from the constructor before the private field gets assigned with a shared instance (this could be named UpdateJsonSerializerSettings).
3. The constructor only assigns the private field when it isn't null.

For what it's worth, I know what it's like to be an open-source maintainer. Sometimes it can be challenging to stay motivated, with so many people asking for things without ever giving back. I've seen people get upset when they didn't get what they needed, as if it's their right to freely profit from all the hard work and thousands of hours spent by total strangers. I realize that @RicoSuter or any team member has no obligations to me whatsoever. I'd just like to point out how crucial this fix is for my team and our users. If there's a way to kindly nudge him to prioritize this, I would be very grateful. I'm not in a position to donate money, but please let me know if there's anything else I can do to help.

[patrickklaeren]

This is why there's a major version change. If everything, everywhere, were backward compatible we would never make progress.

I disagree with the sentiment here to revert back.

I think the "I cannot donate time or money, but please nudge this as a priority the people benefitting off my software indirectly benefitting from you." is not helpful nor productive.

I think you should fix your software, not the other way around.