Fix handling of nullable enums for 3.0#2920
Conversation
21bcff8 to
6ee0df1
Compare
| inferredType = inferredAnyOf ?? inferredType; | ||
|
|
||
| hasOneOfNullAndSingleEnumWith3_0 = nullInOneOf && effectiveOneOf is { Count: 1 } && | ||
| effectiveOneOf[0].Enum is { Count: > 0 }; |
There was a problem hiding this comment.
should this be count > 1 because enum values will have null and at least one enum value? Maybe I am wrong
There was a problem hiding this comment.
It's valid (and more correct) to not have null in the enum array. We are dealing with something like:
"oneOf": [
{
"type": "null"
},
{
"enum": [
// ...
]
}
]The enum array could have a single element, and in all reasonable practical use cases, it must not have null, because then null becomes invalid as it matches both child schemas.
Note that the above shape is draft 2020-12, which isn't what we emit for OpenAPI 3.0. However, the OpenApiSchema model is designed more towards the 2020-12 draft.
That's why we can't emit the thing as-is and we have to do lots of special casing for versions. So, we emit the exact shape above that I mentioned for 3.1 and 3.2 which use 2020-12. But the same OpenApiSchema model serializes differently for 3.0 because it's not really using 2020-12 draft and the above shape isn't valid for OpenAPI 3.0.
That's at least how I'm understanding things currently.
There was a problem hiding this comment.
valid yes. More correct, not really. Each keyword is evaluated independently, the instance needs to validate one of the keywords. And enums are effectively oneOf consts. We could argue it's easier to ready though.
There was a problem hiding this comment.
What I mean is something like:
"oneOf": [
{
"type": "null"
},
{
"enum": [
"A",
"B",
null
]
}
]
In this case, null matches both child schemas. And so, oneOf fails and null won't be considered valid.
See https://www.jsonschemavalidator.net/s/bdAKMMTl
So my point is that a schema like the above is pointless.
| foreach (var enumValue in schema.Enum) | ||
| { | ||
| if (enumValue is not null) | ||
| { | ||
| var currentType = enumValue.GetValueKind() switch | ||
| { | ||
| JsonValueKind.Array => JsonSchemaType.Array, | ||
| JsonValueKind.String => JsonSchemaType.String, | ||
| JsonValueKind.Number => JsonSchemaType.Number, | ||
| JsonValueKind.True or JsonValueKind.False => JsonSchemaType.Boolean, | ||
| JsonValueKind.Null => (JsonSchemaType)0, | ||
| _ => JsonSchemaType.Object, | ||
| }; | ||
|
|
||
| commonType |= currentType; | ||
| } | ||
| } |
baywet
left a comment
There was a problem hiding this comment.
Thank you for making the changes!
|
@baywet Is there anything remaining to get this merged? Would we have a new version released before .NET 11 Preview 7 so that we try to get that in aspnetcore repo and possibly build some fixes on our side on top of this? |
Previously, this scenario wasn't producing a
typeand wasn't producing a null value in the enum array.This fixes it so that they are produced.
In the future, we could also avoid adding a
oneOfwith a single element in this scenario.