Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There is a bug about Nullable enum with UseReferencedDefinitionsForEnums. #861

Closed
PMExtra opened this issue Aug 24, 2018 · 26 comments
Closed

Comments

@PMExtra
Copy link

PMExtra commented Aug 24, 2018

class Dto1 {
    public SomeEnum Some {get; set;}
}
class Dto2 {
    public SomeEnum? Some {get; set;}
}

It will generate redundant enums.

@domaindrivendev domaindrivendev added the p1 High priority label Aug 24, 2018
@PMExtra
Copy link
Author

PMExtra commented Aug 24, 2018

src/Swashbuckle.AspNetCore.SwaggerGen/Generator/SchemaRegistry.cs#L70

- (type.GetTypeInfo().IsEnum && _options.UseReferencedDefinitionsForEnums));
+ (_options.UseReferencedDefinitionsForEnums && (type.GetTypeInfo().IsEnum || Nullable.GetUnderlyingType(type)?.GetTypeInfo().IsEnum == true)));

private Schema CreateSchema(Type type, Queue<Type> referencedTypes)
{
// If Option<T> (F#), use the type argument
if (type.IsFSharpOption())
type = type.GetGenericArguments()[0];
var jsonContract = _jsonContractResolver.ResolveContract(type);
var createReference = !_options.CustomTypeMappings.ContainsKey(type)
&& type != typeof(object)
&& (// Type describes an object
jsonContract is JsonObjectContract ||
// Type is self-referencing
jsonContract.IsSelfReferencingArrayOrDictionary() ||
// Type is enum and opt-in flag set
(type.GetTypeInfo().IsEnum && _options.UseReferencedDefinitionsForEnums));
return createReference
? CreateReferenceSchema(type, referencedTypes)
: CreateInlineSchema(type, referencedTypes);
}

@PMExtra
Copy link
Author

PMExtra commented Aug 24, 2018

@domaindrivendev
I sincerely hope that this problem can be fixed and released as soon as possible, which has troubled my front-end development. (with NSwag)

PMExtra added a commit to PMExtra/Swashbuckle.AspNetCore that referenced this issue Aug 24, 2018
PMExtra added a commit to PMExtra/Swashbuckle.AspNetCore that referenced this issue Aug 24, 2018
@PMExtra
Copy link
Author

PMExtra commented Aug 24, 2018

It's not fix completely.
Just make another Nullable reference.
I'm wondering if we can unpack Nullable generic at any time such as:
type = Nullable.GetUnderlyingType(type) ?? type;.

@PMExtra
Copy link
Author

PMExtra commented Aug 25, 2018

@onionhammer Could you help me to review that? #864

@kwinkel
Copy link

kwinkel commented Jan 10, 2019

@PMExtra, any news on this (and #888)? Currently, I am unable to generate nullable enum models when I include referenced definitions with SwaggerGenOptions.UseReferencedDefinitionsForEnums.

@PMExtra
Copy link
Author

PMExtra commented Jan 11, 2019

@kwinkel I think I've fixed it in the PR #888 . But still no one to accepts it.

@domaindrivendev
Copy link
Owner

Could you add a test please?

@Rassilion
Copy link

Any news on this?

@PMExtra
Copy link
Author

PMExtra commented Apr 3, 2019

@Rassilion My patched version works fine. But I'm too busy to make an unittest.

@Rassilion
Copy link

I updated to 5.0.0-rc2 and looks like bug fixed, I was on 5.0.0-beta.

@rsibanez89
Copy link

rsibanez89 commented May 1, 2019

@Rassilion that version has a different issue

	[Flags]
	public enum TargetGender : byte
	{
		None = 0,
		Male = 1,
		Female = 2,
		Both = Male | Female
	}

will get translated as:

"TargetGender": {
        "enum": [
          null,
          null,
          null,
          null
        ],
        "type": "integer",
        "format": "int32"
      },

@spanevin
Copy link

There is no release version of the package with this fix yet. When do we expect v5 release to be issued?
Can we backport the fix to v4?
Is there a workaround which I can use in swagger initialization code to "fix" the issue with 4.0.1 ?

@domaindrivendev domaindrivendev added this to the v5.0.0 milestone Aug 9, 2019
@domaindrivendev
Copy link
Owner

domaindrivendev commented Aug 9, 2019

@Rassilion if you've discovered a separate issue, please create a separate issue for it - it's too difficult to track issues within issues.

@PMExtra - after further review, I'm not convinced that the generation of two separate enums should be considered a bug ...

In OpenApi 3 (OA3), the schema definition has a Nullable property to indiciate if a null value can be submitted for a given property. In your example, this is true for one of the properties but not the other. So, using OA3, there has to be two separate schema definitions to represent the API contract accurately.

@domaindrivendev
Copy link
Owner

Due to this unavoidable constraint, I don't believe this is an issue with SB. If it's causing issues in downstream tooling (e.g. Nswag) then I think the issue actually lies there.

In closing, I can offer an alternative approach that may or may not work for you. I assume your goal with the non-nullable vs non-nullable properties is to make the value "required" in one case and not in the other. In OA3, this should be represented using the Schema.Required property rather than the Nullable property (for a JSON API, there's actually a subtle different between optional and nullable). So to do this, you could try the following:

class Dto1 {
    [Required]
    public SomeEnum Some {get; set;}
}
class Dto2 {
    public SomeEnum Some {get; set;}
}

Of course the downside here is that if no value is submitted for the optional (non-required) property, then server side the value will default to the first value in your enum definition. So, you would need to introduce a value of Unspecified and have your code handle that in the same way you would have handled a null value. Hope that makese sense.

@domaindrivendev domaindrivendev added waiting for response and removed p1 High priority labels Aug 9, 2019
@domaindrivendev domaindrivendev removed this from the v5.0.0 milestone Aug 9, 2019
@TimothyByrd
Copy link

TimothyByrd commented Sep 25, 2019

@domaindrivendev I would like UseReferencedDefinitionsForEnums to work with nullable enums.

Maybe I'm missing something, but I thought the semantics of C# worked like this:

  1. Reference types (e.g., class instances) are nullable.
  2. Value types (e.g. integers and enums) are not nullable unless explicitly made so using the "?" qualifier.

So having to put a [Required] on an enum seems backward to the language.

class MyClass
{
    public MyEnum RequiredEnum  { get; set; }
    public MyEnum? OptionalEnum  { get; set; }
}

It seems to me that the above should generate two references to MyEnum, one required one not.

    "MyClass": {
      "required": [
        "requiredEnum"
      ],
      "type": "object",
      "properties": {
        "requiredEnum": {
          "$ref": "#/definitions/MyEnum",
        },
        "optionalEnum": {
          "$ref": "#/definitions/MyEnum",
        }
      }
    }

@domaindrivendev
Copy link
Owner

domaindrivendev commented Mar 11, 2020

In Swagger/OpenAPI, required and nullable are mutually exclusive. @TimothyByrd - I think you may be conflating the two. The former means that the property MUST be provided in the request, whereas the latter indicates the property MAY have a value of null. Given, your example:

class MyClass
{
    public MyEnum RequiredEnum  { get; set; }
    public MyEnum? OptionalEnum  { get; set; }
}

There's nothing in the code here that definitively indicates that OptionalEnum MUST be provided with the request. If JSON was submitted without the OptionalEnum property then the property value would be defaulted to null but the request wouldn't fail. To definitively state that a value MUST be provided, Swashbuckle needs more information - i.e. the RequiredAttribute.

@spanevin
Copy link

I installed 5.0.0 release and haven't found UseReferencedDefinitionsForEnums in it, what did I miss?

5.0.0 still shows optional (nullable, not marked as [Required]) enums as required.

@TimothyByrd
Copy link

TimothyByrd commented Mar 13, 2020

@domaindrivendev the issue I have is that OptionalEnum isn't a 'MyEnum'. Given this code:

[Newtonsoft.Json.JsonConverter(typeof(Newtonsoft.Json.Converters.StringEnumConverter))]
public enum MyEnum
{
    v1,
    v2
};

public class MyClass1
{
    public MyEnum RequiredEnum { get; set; }
    public MyEnum? OptionalEnum { get; set; }
}

The swagger I get is for it is:

"MyClass1": {
  "required": [
    "requiredEnum"
  ],
  "type": "object",
  "properties": {
    "requiredEnum": {
      "$ref": "#/definitions/MyEnum"
    },
    "optionalEnum": {
      "enum": [
        "v1",
        "v2"
      ],
      "type": "string"
    }
  }
},
"MyEnum": {
  "enum": [
    "v1",
    "v2"
  ],
  "type": "string"
},

Since OptionalEnum is not a reference to "#/definitions/MyEnum", when I use Nswag to generate code, they come out as two different types.

So parts of my interface use my "MyEnum" enum and parts use things with names like "MyClass1OptionalEnum". And then consumers of my SDK have to do weird casts.

@domaindrivendev
Copy link
Owner

Which version of SB are you using? Using v5.1.0, I see the following Swagger being generated for your example class MyClass:

"MyClass1": {
  "type": "object",
  "properties": {
    "requiredEnum": {
      "$ref": "#/components/schemas/MyEnum"
    },
    "optionalEnum": {
      "$ref": "#/components/schemas/MyEnum"
    }
  }
}

@TimothyByrd
Copy link

I'm using 4.0.1 which was the latest stable release when I wrote that comment last September.

We are in the process of moving from .Net core 2.1 to 3.1.

It looks like the problem I see got fixed.
I'll try updating Swashbuckle independently.

@TimothyByrd
Copy link

I'm still slogging through upgrading 4.0.1 => 5.1.0.
So now I'm asking why optionalEnum isn't marked as nullable like this?

"MyClass1": {
  "type": "object",
  "properties": {
    "requiredEnum": {
      "$ref": "#/components/schemas/MyEnum"
    },
    "optionalEnum": {
      "$ref": "#/components/schemas/MyEnum",
      "nullable": true
    }
  }
}

@TimothyByrd
Copy link

TimothyByrd commented Mar 20, 2020

I've worked through most of the issues moving to 5.1.0, but this is a blocker for me. I have a shared code project to test against both my original code and the SDK and this causes the generated SDK code to be incompatible.

As it is, to build my C# SDK, I have to hand-edit the generated swagger, adding the "nullable": true line before calling NSwag to generate the SDK.

I added a schema filter to set Nullable to true but it doesn't affect the generated swagger. Am I doing something wrong here?

(Note: This and some other filters are here: https://github.com/TimothyByrd/swashbuckle_stuff )

        public void Apply(OpenApiSchema model, SchemaFilterContext context)
        {
            if (model == null) throw new ArgumentNullException(nameof(model));
            if (context == null) throw new ArgumentNullException(nameof(context));

            if (model.Properties == null) return;

            foreach (var property in context.Type.GetProperties())
            {
                if (property.PropertyType.Name.StartsWith("Nullable`", StringComparison.OrdinalIgnoreCase) && property.PropertyType.GenericTypeArguments[0].IsEnum)
                {
                    string schemaPropertyName = PropertyName(property); // camel case if necessary
                    if (model.Properties.TryGetValue(schemaPropertyName, out var modelProperty))
                    {
                        modelProperty.Nullable = true;
                    }
                }
            }
        }

@domaindrivendev
Copy link
Owner

@TimothyByrd the JSON that you're expecting is actually invalid Swagger/OpenAPI because a "reference" schema MUST NOT have any properties present other than the $ref property. This presents a bit of a dilemna. On the one hand you probably want enum schema's to be referenced as opposed to being redefined inline with every usage. But, if they're referenced then you can't really apply something "contextual" like nullable to the definition schema because it can be shared across different contexts. In fact, there's an issue in the Swagger/OpenAPI repo addressing this exact problem.

Swashbuckle gives you two options here which may or may not suit your needs. You can apply the suggested workaround from that issue with the UseAllOfToExtendReferenceSchemas() setting: This would generate the following JSON for your example:

      "MyClass": {
        "type": "object",
        "properties": {
          "requiredEnum": {
            "allOf": [
              {
                "$ref": "#/components/schemas/MyEnum"
              }
            ]
          },
          "optionalEnum": {
            "allOf": [
              {
                "$ref": "#/components/schemas/MyEnum"
              }
            ],
            "nullable": true
          }
        }
      }

Or, you can force all enum schema's to be defined inline by setting UseInlineDefinitionsForEnums(). This would generate the following JSON for your example:

      "MyClass": {
        "type": "object",
        "properties": {
          "requiredEnum": {
            "enum": [
              "Foo",
              "Bar"
            ],
            "type": "string"
          },
          "optionalEnum": {
            "enum": [
              "Foo",
              "Bar"
            ],
            "type": "string",
            "nullable": true
          }
        }
      }

@TimothyByrd
Copy link

TimothyByrd commented Mar 23, 2020

@domaindrivendev Thanks - after trying them out, I think UseAllOfToExtendReferenceSchemas will work. (As you'd expect UseInlineDefinitionsForEnums, gives me all individual types.)

Now to do regression testing...

One thing I noticed - using UseAllOfToExtendReferenceSchemas causes my references to also include the XML documentation in the generated swagger. So that's better.

I suppose that's because a $ref doesn't allow any other properties, as you said above.

class MyClass
{
    /// <summary>This description is lost on a plain $ref</summary>
    public MyEnum SomeEnum  { get; set; }
}

@domaindrivendev
Copy link
Owner

domaindrivendev commented Mar 24, 2020

Yes exactly - the AllOf approach allows any "contextual" metadata (including property comments) be assigned to the schema so it has a ton of advantages. In fact, I initially made it the default behavior, but this caused a lot of issues for downstream code generators (technically their issue not SB's but we're all in this together 😉) and so I ended up making it opt-in for now. In the next major version, I may switch to opt-out.

@maurei
Copy link
Contributor

maurei commented Jan 6, 2023

When enabling this option, what is the reason the AllOf approach is only applied for parameters and members, not types?

Specifically, I mean in this code path:

return GenerateSchemaForType(modelType, schemaRepository);

I would have expected it to return a wrapped in AllOf schema here:

? GenerateReferencedSchema(dataContract, schemaRepository, schemaFactory)

Is this by design?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants