Required.AllowNull does not mark property as required

[basilfx]

Describe the issue
According to the documentation, only Required.Always and Required.DisallowNull in a JsonProperty attribute will mark a property as required in the schema.

Required.AllowNull should also mark it as required. The documentation states: "The property must be defined in JSON but can be a null value." It must be present in the JSON, therefore it must be marked as required.

Required.DisallowNull should not mark it as required. The documentation states: "The property is not required but it cannot be a null value." It may be present in the JSON, therefore it should not be marked as required.

To Reproduce
See this demonstration project: https://github.com/basilfx/azure-functions-openapi-function-timespan (yes, same project as #401).

Note how MyInstance.MyNullableTimeSpan has Required = Required.AllowNull, yet it is not marked as required in the schema. As a work-around, add the Required attribute.

Expected behavior
The property is marked as required in the inferred schema.

Screenshots

Environment (please complete the following information, if applicable):

• OS: macOS
• Azure Functions v4
• .NET SDK 6.0.102
• OpenAPI Extension 1.2.0

[justinyoo]

@basilfx Thanks for the issue. I intepreted both .AllowNull and .DisallowNull as completely opposite. I rather focused on the later part of those statements:

• .AllowNull: ... can be a null value ➡️ not "required"
• .DisallowNull: ... cannot be a null value ➡️ "required"

[basilfx]

@justinyoo Any update on this issue? Anything I can do to help?

[William-Froelich]

I'm also seeing this issue. Our API relies heavily on DisallowNull.

[William-Froelich]

The documentation for azure-functions-openapi-extensions states that .DisallowNull indicates a field is always required. but from the Newtonsoft Documentation, this statement is wrong:

From Src/Newtonsoft.Json/Required.cs

//
// Summary:
//     Indicating whether a property is required.
public enum Required
{
    //
    // Summary:
    //     The property is not required. The default state.
    Default = 0,
    //
    // Summary:
    //     The property must be defined in JSON but can be a null value.
    AllowNull = 1,
    //
    // Summary:
    //     The property must be defined in JSON and cannot be a null value.
    Always = 2,
    //
    // Summary:
    //     The property is not required but it cannot be a null value.
    DisallowNull = 3
}

It looks like the change should be pretty trivial in both src/Microsoft.Azure.WebJobs.Extensions.OpenApi.Core/Visitors/ObjectTypeVisitor.cs and src/Microsoft.Azure.WebJobs.Extensions.OpenApi.Core/Visitors/RecursiveObjectTypeVisitor.cs. .DisallowNull would be replaced with .AllowNull to match Newtonsoft's design.

I'd be happy to make the updates if you agree it should be changed.

[William-Froelich]

#336 relates to this issue.

[timosnel]

@justinyoo any chance you can take a look at the PR of @William-Froelich?

For us this really prevents production-ready usage of this extension, because our generated client marks everything as optional even though the JsonProperty is correct. Changing it to Required.Always would break serialization.

[justinyoo]

I'm still trying to understand whether my interpretation of those properties AllowNull and DisallowNull is correct or not.

From the Newtonsoft.Json point of view, your change makes sense, but the OpenAPI spec perspective, I'm still thinking my interpretation is more appropriate.

• required from the OpenAPI spec means the property MUST have a value (null is not a value).
• AllowNull from Newtonsoft.Json means it MUST be shown while serialising and allows null.

[William-Froelich]

@justinyoo I just looked over OpenAPI spec for parameters and I disagree with that assessment. There's a required property and as of OpenAPI version 3 a nullable property. required only indicates if something must be present in your JSON request and nullable only indicates if the value null is allowed.

From OpenAPI:
Required:

Nullable:

These two parameters are not mutually-exclusive. A field can be marked required but also marked nullable.

I also found an interesting discussion from 2017 on Stack Overflow about the concepts and I think some confusion may come from null not being allowed because it failed type coercion, not because of the required flag. (in other words null did not match a given type such as string so was not accepted by the validator)

I don't think it makes sense to add any other meaning to required beyond enforcing presence in the JSON. I also think for the four values from Netwonsoft I shared the documentation for previously, the property mappings would be:

Default: 
  Nullable: False ? // This one I wasn't sure on. Newtonsoft documentation only states "The property is not required. The default state."
  Required: False
AllowNull
  Nullable: True
  Required: True
Always
  Nullable: False
  Required: True
DisallowNull
  Nullable: False
  Required: False

My PR only addresses the Required state on this. Maybe we need to include Nullable for Api Spec version 3+?

[William-Froelich]

@justinyoo have you had any time to look into this further and look over what I posted? How can I help get this resolved faster?

[justinyoo]

@William-Froelich Now I understand your assessment, and I should update my interpretation.

Then, we should also take care of both Required and Nullable attributes. Would you please be able to include both? It will be then included in the next release.