Swashbuckle.aspnetcore: nullable is set to true when the body parameter is annotated as required in 5.0.0-rc4

[erickguan]

The binding model is:
```c#
public class UserRolesBindingModel
{
[Required]
public IEnumerable Roles { get; set; }
}

The action method:

[HttpPut("{username}/roles")]
public async Task<ActionResult<XAPIModel>> EditRoles(
  [FromBody] UserRolesBindingModel binding, [FromRoute] string username)
{
  return Ok(new XAPIModel());
}

```

Generated swagger file lists Roles in UserRolesBindingModel as nullable.

Swashbuckle.AspNetCore 5.0.0-rc4
ASP.NET Core 3.0
In the project setting, the nullable is enabled and the language version is set to 8.

[Mordag]

@fantasticfears I had the same issue and was able to temp fix it by using [JsonProperty(Required = Required.DisallowNull)]. In your example it would be:

public class UserRolesBindingModel
  {
    [Required]
    [JsonProperty(Required = Required.DisallowNull)]
    public IEnumerable<string> Roles { get; set; }
  }

[davidhenley]

Is there a filter that we can use to automatically add this property to all non-nullable strings until we get a fix for 8.0 nullable enabled?

[cremor]

I agree that it is weird that a property can be both required and nullable at the same time. I recently noticed the same problem and created this simple filter for my project:

   public class RequiredNotNullableSchemaFilter : ISchemaFilter {
      public void Apply(OpenApiSchema schema, SchemaFilterContext context) {
         if (schema.Properties == null) {
            return;
         }

         var requiredButNullableProperties = schema
            .Properties
            .Where(x => x.Value.Nullable && schema.Required.Contains(x.Key))
            .ToList();

         foreach (var property in requiredButNullableProperties) {
            property.Value.Nullable = false;
         }
      }
   }

[domaindrivendev]

From a Swagger/OpenAPI (or more specifically from a JSON perspective), "nullable" DOES NOT mean "optional". A JSON property can be "nullable" but "required". That is, you _must_ provide the property in the JSON body but it _can_ have a value of null

[cremor]

Yes, but the use for a required and nullable JSON property is very limited, isn't it? Maybe it would make sense if I want to enforce a very strict JSON structure, but otherwise I see no use case for it.

Wouldn't it make sense to mark required properties as non-nullable by default when the OpenAPI document is created?

MVC model validation also works that way:

On the server, a required value is considered missing if the property is null.

Source: https://docs.microsoft.com/en-us/aspnet/core/mvc/models/validation?view=aspnetcore-3.0#required-attribute

[erickguan]

@domaindrivendev You are right about nullable and required are different. But from API perspective as swashbuckle is used, when a value comes in, I would only want to know if it has a value. @cremor has a good point that a required and nullable value is rare. Changing default might go well.

[domaindrivendev]

@fantasticfears @cremor - as part of #1411, I updated the generator to honor the configured serializer behavior for handling null values - IgnoreNullValues if you're using STJ and NullValueHandling if you're using Newtonsoft. If you configure your serializer to "ignore null values", then Swashbuckle will never set the Nullable flag, otherwise it will set it according to the "nullability" of the C# type.

Rather than making assumptions, I will always favor code inspection (including serializer config), because Swashbuckle is fundamentally based on a "code first" approach to Swagger. If as an API author, you have no use case for a property to be "required but allowing null", then you should really be configuring your serializer to ignore null values. With the latest change, if you do this, then I think you'll get the behavior you're looking for. Let me know if this makes sense?

[cremor]

I'm not sure if I understand your change correctly. But if I do, it doesn't really help me.

I like that the serializer writes null values for null properties, so I'll leave the NullValueHandling serializer configuration on the default value. That's the "outgoing" side of things. There I also don't mark any properties as required.

But for the "incoming" side I do mark properties that my API needs as required. And those then should also never be null. But since those are only ever deserialized, not serialized, the NullValueHandling serialization setting doesn't apply to them.

Anyway, I'm happy with the simple schema filter that I've posted earlier. So if you think that the current (and upcoming) behavior is correct, that's fine.

[domaindrivendev]

@cremor on reflection after your comments I think it's still debatable as to whether or not the new behavior I'm proposing is correct. So, I'm going to hold back on introducing it for now.

With that said, I _have_ come round to the thinking that properties annotated with the RequiredAttribute should always have Nullable = false. The piece that was missing for me was understanding the "true" meaning of the RequiredAttribute. It literally means that a value of null is invalid. I was getting hung up on the meaning of the word "required" in the context of JSON schema, which is slightly different.

So, I've pushed another change (#1416) that reverts the new behavior I introduced but also sets Nullable = false for property schema's that are annotated with the RequiredAttribute

[domaindrivendev]

If you get a moment please pull it down and let me know if it gives the behavior you need out-of-the-box?

[cremor]

With that said, I _have_ come round to the thinking that properties annotated with the RequiredAttribute should always have Nullable = false.

Nice, thanks!

If you get a moment please pull it down and let me know if it gives the behavior you need out-of-the-box?

Will do in the next few days.

[danzel]

This looks close to us, [Required] works as we'd expect.

Now however the classes that controller methods return are marked as nullable, so when generating a client (using NSwag) all of the controller methods return MyType | null, which is not desired (they should just return MyType).

I've put up a minimal example here:
https://github.com/danzel/SwashbuckleTtest20191213/blob/master/TestApp/Controllers/TestController.cs#L20

Controller get method returns MyType (Same happens with ActionResult<MyType>).
The generated schema for MyType says that it is "nullable": true
https://gist.github.com/danzel/ba3d7f905916772a96f9d0845085e071#file-gistfile1-txt-L64

And this causes NSwag to generate a return result of MyType | null.

With rc4 this nullable was not set.

Thanks!

[cremor]

If you get a moment please pull it down and let me know if it gives the behavior you need out-of-the-box?

I've tested rc5 and can confirm that the required properties are not marked as nullable any more. Thanks!

But I've found a few differences in my OpenAPI schema between rc4 and rc5. See my new issue for details: #1420 Many of those are regarding nullablility, so I wanted to mention it here.

[domaindrivendev]

Fixed as part of #1416. Previews available on myget.org