fix schema for nullable enums

[ItsVeryWindy]

Pull Request

The issue or feature being addressed

Fixes #3225

Details on the issue fix or feature implementation

This makes changes to the data contract resolvers so that the nullability of enums comes through with the type information.

From there we can ensure that we can set nullable to true and append null as a possible value.

As the values are different there are two different schemas that get built, one which contains the null value, and one that doesn't.

This does mean however that other sections of code now also need to handle nullable types.

Anywhere that does checks if a type is an enum.
Checking for properties that are nullable but have either Required, JsonRequired or JsonProperty with required set.
Checking for required parameters.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.92%. Comparing base (5e2e6fc) to head (537dded).
Report is 30 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3377      +/-   ##
==========================================
+ Coverage   83.47%   84.92%   +1.44%     
==========================================
  Files          84       84              
  Lines        3280     3290      +10     
  Branches      572      578       +6     
==========================================
+ Hits         2738     2794      +56     
+ Misses        542      496      -46     

Flag	Coverage Δ
Linux	65.28% <100.00%> (-18.19%)	⬇️
Windows	83.67% <100.00%> (+0.20%)	⬆️
macOS	83.67% <100.00%> (+0.20%)	⬆️

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.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[martincostello]

Thanks!

[ErikPilsits-RJW]

FYI this PR constitutes a breaking change for generated clients, ie nswag. In our typescript client the enum types we were importing don't exist anymore (our response models only had nullable enum properties). This represents a large effort to fix so we will have to hold off any version upgrades until we can find the capacity to do it. This change should have been reserved for a major version update, not a patch version.

[martincostello]

#3425 (comment)

[KirkMunroSagent]

@martincostello: I don't think this PR was implemented properly. I'm still digging into the issue, but since updating to the latest swashbuckle I'm now getting schema errors. We use a unit test that validates the OpenApi spec of our schema, and that unit test fails with 8.1.2, but it was working fine with 8.1.1.

What seems to be happening now is that nullable enums result in a nullable type being added to the schema like this:

 "System.Nullable`1[[MyApp.V1.RoleTypes, MyApp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]]": {
   "enum": [
      "Consumer",
      "Administrator",
      "Enterprise",
      "SystemAdministrator",
      null
   ],
   "type": "string",
   "nullable": true
 },

That type is now referenced in the OpenApi schema like this:

 {
   "name": "roleType",
   "in": "query",
   "schema": {
     "allOf": [
       {
         "$ref": "#/components/schemas/System.Nullable`1[[MyApp.V1.RoleTypes, MyApp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]]"
       }
     ]
   }
 },

That won't pass OpenApi validation because the schema type name contains invalid characters.

When I looked back at the issue logged behind this PR, I wondered why that was even logged as an issue. Shouldn't nullable enums be defined in the schema like this?

 "MyApp.V1.RoleTypes": {
   "enum": [
      "Consumer",
      "Administrator",
      "Enterprise",
      "SystemAdministrator"
   ],
   "type": "string"
 },

And then, when marking them as nullable, that would be done in the OpenApi schema like this:

 {
   "name": "roleType",
   "in": "query",
   "schema": {
     "allOf": [
       {
         "$ref": "#/components/schemas/MyApp.V1.RoleTypes"
       }
     ],
     "nullable": true
   }
 },

Anyway, regardless of what the intent was here, it broke existing code, and I believe the implementation is either missing something that would help the schema be properly generated or it really wasn't an issue at all to begin with, and it was incorrectly logged as a bug.

If this is unclear or confusing, I apologize, I'm still trying to piece together what broke and what should be fixed before I roll back to the previous version.

Thoughts?

[martincostello]

The broken type names looks the same as this issue that was reported: #3424

I don't have access to a computer for several days, so there will be no new releases until later this week at the earliest.

At this stage it's likely there'll be a 8.1.3 "soon" that reverts these changes until we can resolve the original issues they were intending to fix without causing issues for consumers.

Anyone hitting these problems should just roll back to 8.1.1.

[martincostello]

This change was reverted as part of the 8.1.3 release, which is now available from NuGet.org.