Fix null examples being wrongly converted

[dldl-cmd]

Fixes #3801

Changes the JsonModelFactory, which is used in more cases than the XML comment examples to convert a json string to JsonNode with special handling for the null json string.

Moreover, adding a special rule for the null example on string which results in either the string null ("example": "null") or the json value null ("example": null)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.93%. Comparing base (8ad6682) to head (cb6efd6).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3802   +/-   ##
=======================================
  Coverage   94.93%   94.93%           
=======================================
  Files         111      111           
  Lines        3886     3892    +6     
  Branches      784      785    +1     
=======================================
+ Hits         3689     3695    +6     
  Misses        197      197           

Flag	Coverage Δ
Linux	94.93% <100.00%> (+<0.01%)	⬆️
Windows	94.93% <100.00%> (+<0.01%)	⬆️
macOS	94.93% <100.00%> (+<0.01%)	⬆️

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.

[jgarciadelanoceda]

@dldl-cmd , let me know what you think about this other one #3803.. I marked my PR as draft to see if you agree to proceed

[jgarciadelanoceda]

For applications that do not have nullable enable this sounds weird

[dldl-cmd]

Why is the weird to applications which do not have nullable enabled? When the generated schema does indicate, that null isn't allowed having null in the example is wrong. For strings to use than just the value the developer has put there in the example would be a single a good understandable solution.

[jgarciadelanoceda]

But an application which has nullable enabled the desired behaviour I think it'll be to present that value as null(There is no possibility to use string?).. Indeed in the SwashBuckle versions prior to 10 that was the behaviour.. What I tried to do in #3803 is to put the null when the type is null or when is a string that is not DateTime/DateOnly/TimeOnly/Timespan (Because those had the possibility always to have the DateTime?/DateOnly?...)

Also think about the possibility that the client is using OpenApi 3.0.. To put the example as null seems fine.. Also I think that if the client put null we must never put the nullstring.. Because the client mean null not "null"(Which was the original issue #3784)

[jgarciadelanoceda]

If you want you can merge my changes (There are a couple of test more with DateTime/DateOnly).. And get to a consensus

[dldl-cmd]

The library already handles the nullable type indication correctly. So when nullable is enabled

public string Prop { get; set; };

{
  "type": "string"
}

If nullable annotations are not enabled:

public string Prop { get; set; };

{
  "type": "string",
  "nullable": true
}

Also the difference between OpenApi 3.0 and 3.1 doesn't make any difference here as the OpenApi.NET library abstracts the difference away:

OpenApi v3.0

{
  "type": "string",
  "nullable": true
}

OpenApi v3.1

{
   "type": ["null","string"],
}

[dldl-cmd]

@dldl-cmd , let me know what you think about this other one #3803.. I marked my PR as draft to see if you agree to proceed

1. Your PR doesn't fix the issue for the default value, where the same issue can occur.

2. For the differentiation between arrays and objects in the null handling I don't see a good reason to do this.

3. Your check on a string type for a format fixes just some cases, where null is the value. In general the whole example handling doesn't care about whether the value of the example or also default value matches the schema. Therefore adding here some special cases feels to me like mixing concerns.

   Alone with the string formats (list could be extended at any time) there are other cases like IP addresses where a string with the text null doesn't make sense. More over Pattern, MinLength, MaxLength are not considered. But then maybe also enum values need to be considered. On numbers minimum and maximum. On array minItems and maxItems and so on.

4. Regardless of our changes you can do the following, which shows how completely wrong an example can be:

   /// <example>{ "property": "value", "complex": { "propA": 1, "propB": null} }</example>
   public int IntExample { get; set; }

   Results in:

   {
     "type": "integer",
     "format": "int32",
     "example": {
       "property": "value",
       "complex": {
         "propA": 1,
         "propB": null
       }
     }
   }

[jgarciadelanoceda]

@dldl-cmd , let me know what you think about this other one #3803.. I marked my PR as draft to see if you agree to proceed

1. Your PR doesn't fix the issue for the default value, where the same issue can occur.

2. For the differentiation between arrays and objects in the null handling I don't see a good reason to do this.

3. Your check on a string type for a format fixes just some cases, where null is the value. In general the whole example handling doesn't care about whether the value of the example or also default value matches the schema. Therefore adding here some special cases feels to me like mixing concerns.
   Alone with the string formats (list could be extended at any time) there are other cases like IP addresses where a string with the text null doesn't make sense. More over Pattern, MinLength, MaxLength are not considered. But then maybe also enum values need to be considered. On numbers minimum and maximum. On array minItems and maxItems and so on.

4. Regardless of our changes you can do the following, which shows how completely wrong an example can be:

   /// <example>{ "property": "value", "complex": { "propA": 1, "propB": null} }</example>
   public int IntExample { get; set; }

   Results in:

   {
     "type": "integer",
     "format": "int32",
     "example": {
       "property": "value",
       "complex": {
         "propA": 1,
         "propB": null
       }
     }
   }

Could you talk me about the first point?, I don't get to see it
About the second/third point I just checked if the object is nullable.. If you see the issue #3801, most of the test cases are fine with the implementation.

I do not like to put "null" as an example because if the example was set as null we shouldn´t create the example as "null", if they want to set the example as "null", they could put "null"

[dldl-cmd]

For the first point the JsonModelFactory is also used to convert a json string for default values.

Why I think putting "null" as an example instead of skipping the example or setting the example to null:

• When the schema doesn't allow null, the example shouldn't be "example": null because this would generate an invalid OpenApi document.
• Therefore two options remain. Skipping the example or having "example": "null":
  • As an example was added in code we can expect, that as a result in the generated document is expected
  • As the schema indicates, the type is just a string. It makes sense to take what ever is inside of it.

Furthermore considering these two cases:

/// <example>null</example>
public required string NullExampleOnNotNullable { get; set; }

/// <example>null2</example>
public required string Null2ExampleOnNotNullable { get; set; }

My expectation would be that I see:

{
    "NullExampleOnNotNullable": {
        "type": "string",
        "example": "null"
    },
    "Null2ExampleOnNotNullable": {
        "type": "string",
        "example": "null2"
    }
}

And for the nullable case:

/// <example>null</example>
public string? NullExampleOnNullable { get; set; }

/// <example>null2</example>
public string? Null2ExampleOnNullable { get; set; }

{
    "NullExampleOnNullable": {
        "type": "string",
        "nullable": true,
        "example": null
    },
    "Null2ExampleOnNullable": {
        "type": "string",
        "nullable": true,
        "example": "null2"
    }
}

[jgarciadelanoceda]

With your branch for simple int values with example set to null it will get serialized to null which is incorrect right?.
If I just use the example on the SwashBuckle, it's a wrong example.
Also the change you did on JsonModelFactory isn´t going to be hitted because on all the calls made to XmlCommentsExampleHelper.Create there is a check to do not pass null(I mean the if (json is null))

From the table sent on the issue my branch does not fail at any case

[jgarciadelanoceda]

@dldl-cmd should I remove the draft from my PR? if so you can do the comments there ;)