Allow use of System.Text.Json in Request/Response examples.

[SamHutchings]

Addresses issue #132

Opening this PR mostly for discussion purposes due to one potential showstopping issue.

When in a project using dotnet >= 3.0, this will default to using System.Text.Json. If it finds that the Newtonsoft.Json output formatter has been registered, it will switch to use Newtonsoft.

A couple of caveats:

• The attribute based example will always use Newtonsoft if a contract resolver or jsonConverter is supplied (as these are Newtonsoft types). There is no current option to supply a System.Text.Json converter, though this could be added.
• System.Text.Json currently has no way to not serialize properties based on Obsolete attributes. There's no equivalent to a ContractResolver, and as far as I can tell no plans to implement one soon. See Equivalent of DefaultContractResolver in System.Text.Json dotnet/runtime#31257

The second is what I think could be a showstopper - I'm unsure how vital the obsolete property functionality is. If it could be dropped, then maybe this could be an option.

[mattfrear]

Hi Sam

Thanks for your PR. The obsolete functionality is not a huge showstopper - I only added that functionality relatively recently #98, before that it was omitting Obsolete properties and not many people complained about it.

[mattfrear]

I'd like to see some tests that check the output differs if System.Text.Json is used vs the Newtonsoft serializer, e.g. a specific test which addresses #132 and also a test which addresses #78 (comment)

[mattfrear]

Regarding your implementation. What you've done looks good, however...

In a recent PR, @CumpsD added an MvcOutputFormatter, and I think you should be using that to determine which JSON serializer to use.

See his comment here: #142 (comment)

[SamHutchings]

cheers @mattfrear . good to know about the obsolete props thing. will revisit this week and address the comments.

[TheFireCookie]

Hello @SamHutchings @mattfrear , what's the priority on this?
I've encountered the same bug and having the examples validated by the compilation (if you change/remove a property from a request body/response) seems to be a must-have feature for me.
XML sample providers cannot provide this feature and it's a little bit problematic.

[SamHutchings]

Hey @TheFireCookie. Work got in the way and I totally forgot about this. Will try to get an update in today or over the weekend.

[TheFireCookie]

Hello @SamHutchings will you have some time soon? Otherwise I can be an helping hand if you don't have the time this week? I can retake your PR and try to apply @mattfrear comments.

[SamHutchings]

@TheFireCookie working on this right now, will give you an update tomorrow and we can decide how to move on!

[SamHutchings]

@TheFireCookie so spent a bit of time on this last night. Ran into a bit of a problem where formatter.WriteAsync for System.Text.Json was always returning an empty string - not sure why yet. Will carve out some more time this week to look further into it.

That said if your need is particularly urgent please feel free to grab this and take it over the line. Appreciate that it's annoying being stuck on someone elses schedule. Just let me know if you do and I'll down tools.

[TheFireCookie]

@SamHutchings First of all, thank for your work! I'll take a look if I got some time in the upcoming days. Can you commit your changes so I can pull them?

[SamHutchings]

I've rebased on top of master so this is in a good state for now. There's nothing extra that's worth pushing at this point. My thoughts based on the work I did earlier this week:

I think the ideal scenario as @mattfrear mentioned (and please correct me if I've got the wrong end of the stick) is that we can get rid of most of the work I've done here, and let MvcOutputFormatter do the heavy lifting as we do for XML, by calling it from ExamplesConverter. SerializeExampleJson with application/json as the media type. There are a couple of issues with that approach, though, namely:

• We still need to duplicate the SerializerSettings/SerializerOptions to add the indent setting & deal with obsolete properties in newtonsoft, and that would involve some reflection on what we get from the OutputFormatterSelector.
• The System.Text.Json output formatter doesn't respect the WriterFactory passed in on the OutputFormatterWriteContext - instead it just writes to the HttpContext body. So the current MvcOutputFormatter.Serialize<T> implementation always returns an empty string for System.Text.Json. See
  https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs#L59

vs

https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonOutputFormatter.cs#L120
and
https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerOutputFormatter.cs#L233
(ctrl+f for WriterFactory)

We could switch to reading the memorystream off the HttpContext body maybe. Or there might be a simpler way. Not sure yet. Sorry, this was more of a can of worms than I expected!

If it ends up being too much hassle, what's here now does work. But I can see how it's more fragile than using the OutputFormatterSelector.

[TheFireCookie]

@SamHutchings so for you it's mergeable for now and it adds a little technical debt for later?
@mattfrear do you agree with this? 👍

[SamHutchings]

@TheFireCookie edited my comment after you posted, i don't know at what point you caught it. But broadly I agree with your comment - this would work for now but is less ideal than the other solution. And if @mattfrear is happy to go with this, I can tidy up & add tests today/tomorrow.

[mattfrear]

• The System.Text.Json output formatter doesn't respect the WriterFactory passed in on the OutputFormatterWriteContext - instead it just writes to the HttpContext body. So the current MvcOutputFormatter.Serialize<T> implementation always returns an empty string for System.Text.Json. See
  https://github.com/dotnet/aspnetcore/blob/master/src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs#L59

That's unfortunate. I'm just looking at the code file above, and it looks as though it does use the OutputFormatterWriteContext only if (selectedEncoding.CodePage == Encoding.UTF8.CodePage).

So maybe you need to somehow set that?

Edit: my bad, it doesn't.

[mattfrear]

Thanks again Sam for your work on this and for the investigation as to why the SystemTextJsonOutputFormatter isn't working properly.

My preference would be for someone on the ASP.NET team to fix SystemTextJsonOutputFormatter so that we can use our MvcOutputFormatter. Less code is better.

I've raised an issue in the dotnet/aspnetcore repo. It would be good to know if they can fix it or suggest a workaround. If they won't fix then I guess we'll have to go with this PR.

There seems to be a lot of demand and impatience for this PR, and I don't understand why.
In the meantime... why not use Newtonsoft? What am I missing? Why the rush for System.Text.Json?

[TheFireCookie]

@mattfrear More and more people are migrating to System.Text.Json for their ASP NET CORE apis and there is a lot of good reasons for it. The memory footprint, the performance, the microsoft-backed serializer. Maybe speed & memory are not as important as it is for some peoples but for what's I'm working on it's primordial that we answer fast and System.Text.Json is years ahead of newtonsoft.json on the performance part.

[mattfrear]

I admit the APIs I've been working on lately do not have any demanding performance requirements.

To me it feels like JSON serializer is never going to be the bottleneck though... database or disk access is an order of magnitude or two slower, and worrying about JSON serialization speed smells like a case of premature optimization.

And you know the old rule, never use v1 of anything from Microsoft.

That said, I agree that System.Text.Json is the way forward. I just haven't been in a hurry to adopt it myself.

I also wonder what changes are coming with .NET 5 that could cause me to have to rewrite these filters yet again (like I had to for Swashbuckle 4 -> 5). Now that there's more and more code which I don't even understand in this filter, potentially having to rewrite or port it to something new scares me.

[mattfrear]

OK so we've gotten an answer from the ASP.NET core team about the SystemTextJsonOutputFormatter, and a workaround which I have working locally. Which means we can throw away most of this PR and serialize using the existing MvcOutputFormatter.

I'm having a play and I've gotten it working locally, but it still needs tidying up, and all my tests are broken and probably need to be rewritten.

I was under the impression that if you were using System.Text.Json in your API then the examples weren't working at all. Now that I've had a play, I see that is not true, the examples are working, it's just that they are serialized using Newtonsoft... which surely works fine for 99% of use cases? Unless you happen to be [JsonPropertyName]?

Well whoop-de-doo. All this sweat and tears and investigation, just for that? Or is there other use cases I don't know about?

[SamHutchings]

Ha - yeah that's our use case. We've got some custom json formatters for which we were having to maintain a System.Text.Json version (for the actual api serialization) and a newtonsoft version (for the examples). Which is slightly annoying.

Spent a few hours on the original PR, thought it'd be nice to allow the option for others. Hadn't realised it was going to be quite such a production.

And you know the old rule, never use v1 of anything from Microsoft.

Sadly the adage had slipped my mind.

[TheFireCookie]

Hello @mattfrear , that's exactly what we do in our team. We use the [JsonPropertyName] attribute. I'll add back the Newtonsonft.Json attribute on all my models but if would be easier for us for it to work correctly out of the box. Thanks for the time invested.

[KaptenJon]

@mattfrear The problem for us is that you need to duplicate and maintain all Json annotations (converter, jsonignore, propertyname) and custom converters for all models used. This makes it harder to read and easy to miss out some annotations. I really waiting for this.

[TheFireCookie]

So is a work planned sometimes soon to make this supported or do you need a contributor to help you on this @mattfrear ?

[mattfrear]

@TheFireCookie as you can probably guess, I don't have a lot of motivation or time to fix this. However, I just spent another few hours on it this afternoon, and I've got all my previous tests passing, except for the [Obsolete] property stuff. Reading back the history of this PR, I see that @SamHutchings had the same problem.

Anyway, I'm now using the MvcOutputFormatter which @CumpsD introduced. There's a lot of code in that which I don't understand.

I just want to write some more tests for it which use System.Text.Json and then I think it'll be good enough to release. That'll mean upgrading all my tests to .NET Core 3, which I hope isn't going to be too painful.

I'm going to close this PR as the work here isn't needed.

[mattfrear]

Here's my work in progress PR. #159

[mattfrear]

This should be fixed now @TheFireCookie https://www.nuget.org/packages/Swashbuckle.AspNetCore.Filters/6.0.0

[TheFireCookie]

Nice, thank you @mattfrear , I'll try it when I have a little bit of time.