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

[martincostello]

Minus feedback, do you think you're done now @remcolam? I was poised to merge this when the last set of changes came in 😄

[remcolam]

@martincostello If you are happy, then I am happy. I prefer going the route of the swagger options, it feels cleaner. I think I addressed all your last comments

[martincostello]

Just a few last nit-picks then this should be good to merge 👍

[remcolam]

Accepted all your changes

[remcolam]

I enjoyed discussing this with you and working on it with you. Cheers!

[martincostello]

Missed this - needs putting back as it breaks the build.

[remcolam]

Shouldn't it be in the compiler switch? Because VS is saying it is not used

[martincostello]

If only some TFMs use it, then you'll need to wrap it in an #if to stop the suggestion to remove it.

[remcolam]

Yeah, it is needed, still strange that visual studio isn't recognizing it as being used

[remcolam]

restored it

[martincostello]

It's probably because you have a TFM that isn't the one that needs it selected as the current "view" (I don't know what the terminology for it is) you have of the code open in the document well.

[remcolam]

BTW, should there be documentation in the readme.md about how to use this?

[martincostello]

BTW, should there be documentation in the readme.md about how to use this?

If you can open a new PR (to save more churn) once this is merged that adds a small note about it somewhere in the README that would be great 👍

[remcolam]

BTW, should there be documentation in the readme.md about how to use this?

If you can open a new PR (to save more churn) once this is merged that adds a small note about it somewhere in the README that would be great 👍

Will do

[martincostello]

Thanks again for the work you've put into this change!