Make `SerializerSetings` properties get-only and `protected`

[javiercn]

JsonOutputFormatter "allows" to change the settings at runtime, this is different from what JsonInputFormatter does, so It leaves me wondering, is there really an scenario for changing the settings after startup, or is this just the case that we wrote the code but didn't think the scenarios through?

public JsonSerializerSettings SerializerSettings
{
    get
    {
        return _serializerSettings;
    }
    set
    {
        if (value == null)
        {
            throw new ArgumentNullException(nameof(value));
        }

        _serializerSettings = value;

        // If the settings change, then invalidate the cached serializer.
        _serializer = null;
    }
}

I don't think we should support this scenario as it's inconsistent with the scenarios in JsonInputFormatter and I can't see an advantage of doing this over replacing the formatter in the list of formatters in the options. I believe Options has support for this built in https://github.com/aspnet/Options/blob/dev/test/Microsoft.Extensions.Options.Test/OptionsMonitorTest.cs

[Eilon]

Yeah this seems a bit weird. I don't think you can effectively change any of these settings after startup. You'd cause crazy race conditions and the world would likely end.

@rynowak @dougbu any sense whether there was some scenario here at some point? I suspect we can delete some of this "guard" code, no?

[dougbu]

@Eilon this code may exist only for unit testing scenarios. I don't recall a production scenario.

Then again, output formatters are more likely to be created from user code than input formatters e.g. within a custom IActionResult. I could imagine a user looking to customize an output formatter on, say, a per-action or per-response Type basis.

[Eilon]

You might be able to customize formatters on a per-request basis, but definitely not by modifying the instance that's already in the OutputFormatters collection. Isn't that instance shared among all requests?

[rynowak]

Yeah, we might want to just make it clearer what's supported and not by taking in the settings in the constructor.

Having the settings in a mutable property is a holdover from WebAPI2 and it might be a distraction from the intended pattern for configuring the formatter.

[Eilon]

Yeah let's clean up the ctor's and make the properties get-only.

[kichalla]

I am a bit unclear. So what is the final change that needs to be made here?

• Remove the SerializerSettings property (or make it protected in case of JsonInputFormatter as JsonPathInputFormatter uses it). If we remove or make the property protected, wondering how can an end user start off from the default settings that we provide as it has some important details and which the user should probably build upon (https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Formatters.Json/Internal/SerializerSettingsProvider.cs ...this is internal)
• If we still keep the property and make it get-only then the settings are still mutable right?

[javiercn]

What @rynowak said, take the settings on the constructor and make the property read only. We can have a parameter-less constructor that initializes it with the default settings and that the user can further tweak.

The goal here is not to be correct as you can't be 100% without hiding the property or having it be immutable, the goal is to declare intention by not having extra code that implies this is an scenario we care to support. IMO any formater should be fully configured before it processes its first request/response (basically fully configure them during startup in options, or before passing them to ObjectResultExecutor).

If you are doing something really advanced and require different settings, then have different formatter instances.

The point is that assuming the formatter is only configured before its first use simplifies the code and makes clearer what the intended supported scenarios are.

I wouldn't complain if you had a static property or something like that on the formatter that is DefaultSettings if @rynowak is ok with it, in any case, although "internal" the code you mention is totally accessible to customers, and the configuration done there is not something they can't do by themselves.

[dougbu]

1. Don't reason based on the existence of parameterless (at least shorter) test-only constructors. I'll be removing most of them soon (see Remove extra constructors in JsonInputFormatter and JsonPatchInputFormatter #4339). I probably should clean up the extra constructors in JsonOutputFormatter too.
2. No need for statics here. The correct serializer settings are available everywhere the MvcOptions MvcJsonOptions is available.
3. I'm not sure why we have the SerializerSettings properties at all. @javiercn why not make them protected or remove them entirely (subclasses can also store values in fields since they're passed into the constructors)?

[javiercn]

1. Fair enough.
2. Good point.
3. Protected is ok for me I think, but I don't see a problem making them public get only.

[kichalla]

No need for statics here. The correct serializer settings are available everywhere the MvcOptions is available.

@dougbu you meant MvcJsonOptions here? This gives the same instance of serializer settings which users should not mutate?

[dougbu]

@kichalla yes to both questions.