[Bug]: Example Null-Handling Seems Inverted

[pinkfloydx33]

Describe the bug

I was actually happy to see #3793 as this was a minor gap for us in the .NET 10 upgrade where we had some examples that were explicitly/intentionally written to the API Spec as null.

However moving to the new package 10.1.4 seems to exhibit strange behavior. I am only allowed to write a null example to the document if the data type is JsonSchemaType.String but not JsonSchemaTypeString | JsonSchemaType.Null.

Swashbuckle.AspNetCore/src/Swashbuckle.AspNetCore.SwaggerGen/XmlComments/XmlCommentsExampleHelper.cs

Lines 16 to 23 in 8ad6682

	var isStringType = type is { } value &&
	value.HasFlag(JsonSchemaType.String) &&
	!value.HasFlag(JsonSchemaType.Null);
	if (isStringType)
	{
	return string.Equals(exampleString, "null") ? JsonNullSentinel.JsonNull : JsonValue.Create(exampleString);
	}

This seems inverted! I can set example=null on a non-nullable string, but not on a nullable string?
It also seems a bit funny that it's limited to strings and not any nullable type

Expected behavior

At the very least I would expect to be able to set the example value to null if the property itself supports being null i.e. with JsonSchemaType.Null | JsonSchemaType.String

Though I probably would not limit this to just strings at all! Perhaps something like this could suffice for any type of property:

var type = schema?.ResolveType(schemaRepository);
if (type is { } value && value.HasFlag(JsonSchemaType.Null) && string.Equals(exampleString, "null"))
{
    return JsonNullSentinel.JsonNull;
}

// rest of method

Actual behavior

Cannot write "example": null to document if schema resolves to JsonSchemaTypeString | JsonSchemaType.Null
Cannot write "example": null for numeric or other properties that have the JsonSchemaType.Null flag

Steps to reproduce

Consider this data type:

public sealed class MyType
{
    /// <example>null</example>
    [Required]
    public string Ex0 { get; set; } = null!;  
    /// <example>null</example>
    public string? Ex1 { get; set; }
    /// <example>null</example>
    public Guid Ex2 { get; set; }
    /// <example>null</example>
    public Guid? Ex3 { get; set; }
    /// <example>null</example>
    public int Ex4 { get; set; }
    /// <example>null</example>
    public int? Ex5 { get; set; }
    /// <example>3</example>
    public int? Ex6 { get; set; }
}

The API Specification generated will produce

Field	Expected Example	Actual Example	Correct
Ex0	omitted	"example": null	❌
Ex1	"example": null	omitted	❌
Ex2	omitted	"example": null	❌
Ex3	"example": null	omitted	❌
Ex4	omitted	omitted	✅
Ex5	"example": null	omitted	❌
Ex6	"example": 3	"example": 3	✅

Exception(s) (if any)

No response

Swashbuckle.AspNetCore version

10.1.4

.NET Version

10

Anything else?

As a work-around, you can write the literal value of JsonNullSentinel.NullValue as your example and it will get converted just fine:

public sealed class MyType
{
    /// <example>openapi-json-null-sentinel-value-2BF93600-0FE4-4250-987A-E5DDB203E464</example>  
    public string? Ex1 { get; set; }
}

However that is a bit heavy handed and likely an implementation detail!

[pinkfloydx33]

FYI @jgarciadelanoceda

[jgarciadelanoceda]

Thanks for the detailed test!, I will dig a bit on this

[dldl-cmd]

Changing the JsonModelFactory to the following could also resolve further issues.

When doing this some tests are failing for the DefaultAttribute handling where the same issue exists and the test checked it wrong because the serialized json of null default value should be "null" not a null string.

internal static class JsonModelFactory
{
    public static JsonNode? CreateFromJson(string? json)
    {
        if (json is null)
            return null;

        if (json == "null")
            return JsonNullSentinel.JsonNull;

        return JsonNode.Parse(json);
    }
}

I am right now not sure if this would also be correct in the case for the enum values, where the factory is also used.

[jgarciadelanoceda]

@pinkfloydx33 I will think about compability with the apps that do not have nullable enable when doing the development, the checking for null could cause a regression for those apps

[dldl-cmd]

Additionally changing in the XmlCommentsExampleHelper to the following, which only uses for strings the null value when the string is nullable.

public static JsonNode Create(
    SchemaRepository schemaRepository,
    IOpenApiSchema schema,
    string exampleString)
{
    var type = schema?.ResolveType(schemaRepository);
    if (type is { } schemaType)
    {
        var isStringType = schemaType.HasFlag(JsonSchemaType.String);
        var nullable = schemaType.HasFlag(JsonSchemaType.Null);

        if (isStringType)
        {
            return nullable && string.Equals(exampleString, "null")
                ? JsonNullSentinel.JsonNull
                : JsonValue.Create(exampleString);
        }
    }
    
    // HACK If the value is a string, but we can't detect it as one, then
    // if parsing it fails, assume it is a string that isn't quoted. There's
    // probably a much better way to deal with this scenario.
    try
    {
        return JsonModelFactory.CreateFromJson(exampleString);
    }
    catch (JsonException) when (exampleString?.StartsWith('"') == false)
    {
        return JsonValue.Create(exampleString);
    }
}

Together with the previous change this would lead to the following result:

Field	Expected Example	Old Actual Example	Old Correct	New result	New Correct
`Ex0´	omitted	"example": null	❌	"example": "null"	✅
Ex1	"example": null	omitted	❌	"example": null	✅
Ex2	omitted	"example": null	❌	"example": "null"	❌
Ex3	"example": null	omitted	❌	"example": null	✅
Ex4	omitted	omitted	✅	"example": null	❌
Ex5	"example": null	omitted	❌	"example": null	✅
Ex6	"example": 3	"example": 3	✅	"example": 3	✅

I consider Ex1 now also a correct, as I would have expected that the string null is just used as example string instead of converted to a json null value.

For Ex2 and Ex4 there could be many more scenarios where the example value doesn't fit to the schema. when its not just a basic type. To solve, that the example would need to be check against the schema.

[jgarciadelanoceda]

Indeed the ex0 is also an error I was thinking about playing with SupportNonNullableReferenceTypes, in this case.. if the setting is true then serialize as null when the type is nullable and when the setting is not set always display null (as it was).

[pinkfloydx33]

Yeah I considered the "null" string and was teetering on using it for Ex0 but figured it was too-too niche; my bad there.

I don't think you can rely on just SupportNonNullableReferenceTypes... at least not without looking at the original type. For example a Guid or Guid? is modeled as type: String or String | Null with format: uuid -- so you'd want it processed based on the OpenAPI type, but would need to know about it's original C# type being a (nullable) struct

But, why is looking at Schema.Type not enough? Swagger would've already done "the right thing" to get to this point and it will include | Null where appropriate and would have based it on SupportNonNullableReferenceTypes and any other options.

The funny thing is that I don't actually care about the C# string cases. I did find it odd I couldn't set null for a String|Null which is what got me digging into it... really I need the Guid? case (Ex3) to work for parity with before

[jgarciadelanoceda]

The thing is that at first I thought about checking only if the type is nullable and the example is null .
But then I thought about all the projects that do not have nullable enable. In the versions previous to the 10 all those APIs serialize the example as null. They could have the property as string and it's fine for their use case

It might be the time to enforce it from a library perspective.. not sure what to do

For version 3.1 of OpenApi seems incorrect ( to me), to put the example null when the type does not contain null, for version 3.0 is perfectly fine

I think I will just restablish what Swashbuckle had in versions lower than 10.. that is just to compare if the example s null.. if the example is null it will get deserialized.. is up to the client of Swashbuckle to put the right example for each case( as it was)

[jgarciadelanoceda]

We finally did 2 different solutions.. I placed my pr as draft, because not sure about the treatment of array or objects(frankly to put an example with null on both cases seems weird for me)..

I did think about the 2 worlds, the one with nullable enable( the new one), and the one with nullable not enable ( for my understanding I think is the most used).
I left a message in @dldl-cmd pr to see if he agrees

[martincostello]

@pinkfloydx33 Do the changes in #3803 fit with what you'd expect to be happening?

[pinkfloydx33]

@martincostello I have not been following the PRs (at one point there were two). If it's far enough along I can take a look and review. But it's highly unlikely that I'll get a chance to look at it today... It would be tomorrow at the earliest