Add customized document serialization support

[remcolam]

Added support to inject a ISwaggerDocumentSerializer implementation, which can be called to manually serialize an OpenAPIDocument object.

• Resolves How to put extension at the top of the document: Is there a way to change the order in which components are serialized? #2668
• Resolves Invalid JSON output #2700
• Resolves Set culture of Json Serializer to InvariantCulture #2105

[martincostello]

Thanks for contributing - if you'd like to continue with this pull request, please rebase against the default branch to pick up our new CI.

Tests will also be required.

[remcolam]

I will have to look into the tests still, I always struggle with creating those, maybe I'll ask for some help from some colleagues

[martincostello]

Looks OK to me so far, modulo a few style comments.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.48148% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 92.06%. Comparing base (4baef30) to head (04c3c32).
Report is 2 commits behind head on master.

Files	Patch %	Lines
...washbuckle.AspNetCore.Swagger/SwaggerMiddleware.cs	78.57%	3 Missing ⚠️
...er/DependencyInjection/SwaggerOptionsExtensions.cs	75.00%	1 Missing ⚠️
...SwaggerGen/DependencyInjection/DocumentProvider.cs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2677      +/-   ##
==========================================
- Coverage   92.13%   92.06%   -0.08%     
==========================================
  Files          91       93       +2     
  Lines        3088     3111      +23     
  Branches      529      536       +7     
==========================================
+ Hits         2845     2864      +19     
- Misses        243      247       +4     

Flag	Coverage Δ
Linux	92.06% <81.48%> (-0.08%)	⬇️
Windows	92.06% <81.48%> (-0.08%)	⬇️
macOS	91.43% <81.48%> (-0.70%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[remcolam]

@martincostello As I am adding a test, I have the CLI test failing for me. Swashbuckle.AspNetCore.Cli.Test.ToolTests.Can_Generate_Swagger_Json() fails, because my local culture is serializing the OpenApiDouble(14.37) price example with a comma, not a period.

It seems the CLI is not correctly serializing different cultures. Should I create an issue for this?

(FYI, I have actually run into this myself when adding examples, and solved it by creating a SwaggerHostFactory and specifically setting the invariant culture in there)

[martincostello]

Should I create an issue for this?

I believe that's #2105 and #2700, which there's a proposed fix for in #2726.

The author of that PR hasn't rebased, so I can't merge it. If you pull that change into your PR and it fixes it, then we can include that fix here too.

[remcolam]

I had trouble finding good tests, I hope, the 6 I added are satisfactory.

I added the fix for the culture in CLI, copied the change from the other PR.

I marked the original constructors that I overloaded obsolete, I think they should be removed in the next major release, there shouldn't be multiple injection constructors, and changing the order of them breaks the DI also. (Which my test found out)

[martincostello]

Just a few minor comments.

[martincostello]

Thanks for adding the tests too!

[martincostello]

Thanks again for your work on this - I'll merge this once/when the next planned release is intended to be 6.6.0.

[remcolam]

@martincostello I was looking at what I am trying to achieve again. I am wondering if I shouldn't change the place I am hooking in.

Instead of using DI, and registering the serializer in the service provider, it could be added to SwaggerGeneratorOptions. It will prevent changing the constructor of DocumentProvider and SwaggerMiddleware. And since it's an optional serializer, it shouldn't really go via DI.

But if you think, never mind, this is good enough also for other future use, that's fine

[martincostello]

Maybe have a quick look at doing that locally in your branch and see if that makes things simpler or not? My only thought is that it might make it more difficult for people to assign a custom ISwaggerDocumentSerializer in the case that they need to create it from other services registered in DI. If that's still possible with adding it to SwaggerOptions and the isn't a snowball effect of needing to update other things then that seems reasonable.

[remcolam]

Who is responsible for solving the conflicts?

I noticed this in the conflicts...

During my testing I noticed, DI picks the top constructor. my new constructor and the added one need to be merged into a single one, with the new parameter encapsulated by #if !NETSTANDARD

[martincostello]

The short answer is: you 😄

Regarding the constructor, that can be solved by things such as [ActivatorUtilitiesConstructor] to give a hint to the DI system, or changing the DI registration to explicitly pick a specific constructor.

[remcolam]

Fixed merge conflicts. @martincostello Take a close look at the SwaggerMiddleware.cs, where I merged the new changes from main

[remcolam]

@martincostello I abondened my idea of putting it into any of the options for obvious reasons, I fixed the call in SwaggerBuilderExtensions.cs, so I think now it should all be good