Remove extra constructors in JsonInputFormatter and JsonPatchInputFormatter

[dougbu]

The JsonInputFormatter and JsonPatchInputFormatter classes previously had a parameterless constructor for ease of use. This was lost when a required ILogger parameter was added. We have also since decided there's no important user scenario involving construction of these types. But JsonInputFormatter still has 3 constructor overloads while JsonPatchInputFormatter has 2. The extra overloads are used only in test code and are somewhat complicated i.e. don't just pass null to the longest overload.

Should remove all but the longest constructor overload in each class. Change the related tests to use the one remaining constructor for each class.

[Eilon]

Is there any reasonably common scenario for new'ing up these types? (Aside from tests, which is mostly our own internal concern.)

[rynowak]

I'm not really sure. There was in WebAPI - but that was often a workaround for other things being obtuse....

Only case I can think of is if you have two different sets of JSON settings.

[rynowak]

There's still an action item to take here if we don't want to make them constructable, we should clean up the other ctors.

[Eilon]

If there's nothing "common" then I'll put in Backlog. If there's something more urgent for v1, what is it that we should do?

[dougbu]

Suggest that if there's no common scenario, we were wrong to add the extra constructors. They are a bit complicated and do odd things like new up a DefaultObjectPoolProvider. As @rynowak said, the action item would be to remove them. Could even change the bug title 😺

[Eilon]

Well, given what I've understood here, this would be backlog. So if you could distill this down to exactly what new proposed action we should take sooner, let's take a look at that.

[dougbu]

Sure. I've updated the title and description. (In retrospect, should have posed this issue as a question on which way to go.)

[rynowak]

Suggest that if there's no common scenario, we were wrong to add the extra constructors

• Constructable
• DI
• Simple

Choose any two.

[Eilon]

Should remove all but the longest constructor overload in each class. Change the related tests to use the one remaining constructor for each class.

@rynowak are you onboard with that?

[rynowak]

Yes

[Eilon]

Ok, we'll do in RTM.

[kwaclaw]

As long as issue #4270 is not fixed, the workaround requires to be able to derive from JsonInputFormatter and have access to the base constructor.

[dougbu]

d8d2e54

[AlexCuse]

Sorry for the necropost but I'm working on upgrading to 1.0 at the moment. We were overriding input/output formatters like so, mostly to globally configure converters.

            services
                .AddMvc(mvco =>
                {
                    mvco.InputFormatters.Add(new JsonInputFormatter(serializerSettings));
                    mvco.OutputFormatters.Add(new JsonOutputFormatter(serializerSettings));

                    //snip...
                })

I did not see any announcements on this. Is there a different way I should be overriding serializer settings in 1.0? If not, what values should I use the logger, charPool and objectPoolProvider parameters?

[rynowak]

You can modify the "shared" JSON.NET settings by doing:

services.AddMvc(...).AddJsonOptions(options =>
{
    // Do things here
});

[AlexCuse]

Thanks for the quick response - I just found that AddJsonOptions when looking at the next json-related issue we're having and was hoping it would do the trick. Good to know it will!

[rynowak]

Hi, it looks like you are posting on a closed issue/PR/commit!

We're very likely to lose track of your bug/feedback/question unless you:

1. Open a new issue
2. Explain very clearly what you need help with
3. If you think you have found a bug, include detailed repro steps so that we can investigate the problem

Thanks!