aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.61k stars 2.14k forks source link

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

Closed javiercn closed 8 years ago

javiercn commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

kichalla commented 8 years ago

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

javiercn commented 8 years ago

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 commented 8 years ago
  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 #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 commented 8 years ago
  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 commented 8 years ago

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 commented 8 years ago

@kichalla yes to both questions.

dougbu commented 8 years ago

d8d2e54

ghost commented 8 years ago

Why SerializerSettings settings are now protected? How can I change default settings?

for example

private void ConfgureFormatters(MvcOptions options)
{
    JsonInputFormatter input = (JsonInputFormatter)options.InputFormatters.First(item => item is JsonInputFormatter);
    JsonOutputFormatter output = (JsonOutputFormatter)options.OutputFormatters.First(item => item is JsonOutputFormatter);

    input.SerializerSettings.ContractResolver = new PascalCasePropertyNamesContractResolver();
    output.SerializerSettings.ContractResolver = new CamelCasePropertyNamesContractResolver();
    output.SerializerSettings.Formatting = Formatting.Indented;
}

It looks like overkill to drop all formatters and create new ones.

And for now for some reason you do not have default settings for input and output formatters. But even in case of solving #4562 (which solves above code) it will still be bad design... because we cannot now setup specific formatter individually. We will have to remove affected formatter and recreate it again... So logic which initially creates formaters and logic of default serialilzer settings already suck.

As well as solution of @javiercn... it is very nice from point of view of tricking but absolutely bad for real development... 50 lines of code.. additional class... just to set 2 properties in some formatters.

dougbu commented 8 years ago

@maxim-boligatov I answered your questions in https://github.com/aspnet/Mvc/commit/d8d2e54506c7b1973156316f1dd82c6226de2c95#commitcomment-18275275