Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure non-nullable types as required #2036

Closed
coder925 opened this issue Feb 26, 2021 · 23 comments · Fixed by #2803
Closed

Configure non-nullable types as required #2036

coder925 opened this issue Feb 26, 2021 · 23 comments · Fixed by #2803
Labels
p2 Medium priority
Milestone

Comments

@coder925
Copy link

In my project we do not distinguish between required and nullable: false. If a type is non-nullable, it is also required. This is regardless if it is a value type or reference type.

It would be great if one could optionally configure Swashbuckle to set all non-nullable properties to required. E.g. like: setupAction.MarkNonNullableTypesAsRequired(); This way, I could remove all my [Required] annotations on the non-nullable C# properties.

This is a subsequent request following #1686. As originally posted by @felschr in #1686 (comment) , treating non-nullable as required is the default for nullable contexts in .NET validation.

@flibustier7seas
Copy link

flibustier7seas commented Mar 23, 2021

Temporary solution:

serviceCollection.AddSwaggerGen(builder =>
{
    builder.SupportNonNullableReferenceTypes();
    builder.SchemaFilter<RequiredNotNullableSchemaFilter>();

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

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

        foreach (var property in notNullableProperties)
        {
            schema.Required.Add(property.Key);
        }
    }
}

UPD: This solution is not completely correct, see example:

public class User
{
    public Guid Id { get; set; }
    public Guid? OrganizationId { get; set; }
    public string FirstName { get; set; } = default!;
    public string? MiddleName { get; set; }
    public string LastName { get; set; } = default!;

    public UserEmail Email { get; set; } = default!;
    public UserPhone? Phone { get; set; }
}

public class UserEmail
{
    public string Address { get; set; } = default!;
}

public class UserPhone
{
    public string Number { get; set; } = default!;
}
{
      "User": {
        "required": [
          "email",
          "firstName",
          "id",
          "lastName",
          "phone" // Incorrect
        ],
        "type": "object",
        "properties": {
          "id": {
            "type": "string",
            "format": "uuid"
          },
          "organizationId": {
            "type": "string",
            "format": "uuid",
            "nullable": true
          },
          "firstName": {
            "type": "string"
          },
          "middleName": {
            "type": "string",
            "nullable": true
          },
          "lastName": {
            "type": "string"
          },
          "email": {
            "$ref": "#/components/schemas/UserEmail"
          },
          "phone": {
            "$ref": "#/components/schemas/UserPhone"
          }
        },
        "additionalProperties": false
      }

@JosNun
Copy link

JosNun commented Jul 22, 2021

@flibustier7seas you mention that your solution isn't completely correct, and I see you mention the phone field being marked as required, even though it's not. Do you know what in particular causes this case, or if there's any workarounds?

@flibustier7seas
Copy link

flibustier7seas commented Aug 6, 2021

@JosNun I'm using the following workaround:

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

        FixNullableProperties(schema, context);

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

        foreach (var property in notNullableProperties)
        {
            schema.Required.Add(property.Key);
        }
    }

    /// <summary>
    /// Option "SupportNonNullableReferenceTypes" not working with complex types ({ "type": "object" }), 
    /// so they always have "Nullable = false",
    /// see method "SchemaGenerator.GenerateSchemaForMember"
    /// </summary>
    private static void FixNullableProperties(OpenApiSchema schema, SchemaFilterContext context)
    {
        foreach (var property in schema.Properties)
        {
            if (property.Value.Reference != null)
            {
                var field = context.Type
                    .GetMembers(BindingFlags.Public | BindingFlags.Instance)
                    .FirstOrDefault(x =>
                        string.Equals(x.Name, property.Key, StringComparison.InvariantCultureIgnoreCase));

                if (field != null)
                {
                    var fieldType = field switch
                    {
                        FieldInfo fieldInfo => fieldInfo.FieldType,
                        PropertyInfo propertyInfo => propertyInfo.PropertyType,
                        _ => throw new NotSupportedException(),
                    };

                    property.Value.Nullable = fieldType.IsValueType
                        ? Nullable.GetUnderlyingType(fieldType) != null
                        : !field.IsNonNullableReferenceType();
                }
            }
        }
    }    
}

Result:

{
  "User": {
    "required": ["email", "firstName", "id", "lastName"],
    "type": "object",
    "properties": {
      "id": {
        "type": "string",
        "format": "uuid"
      },
      "organizationId": {
        "type": "string",
        "format": "uuid",
        "nullable": true
      },
      "firstName": {
        "type": "string"
      },
      "middleName": {
        "type": "string",
        "nullable": true
      },
      "lastName": {
        "type": "string"
      },
      "email": {
        "$ref": "#/components/schemas/UserEmail"
      },
      "phone": {
        "$ref": "#/components/schemas/UserPhone"
      }
    },
    "additionalProperties": false
  }
}

@ahmednfwela
Copy link

small modification on @flibustier7seas 's answer

var notNullableProperties = schema
      .Properties
      .Where(x => !x.Value.Nullable && x.Value.Default == default && !schema.Required.Contains(x.Key))
      .ToList();

@brather1ng
Copy link

brather1ng commented Jan 28, 2022

If you use the SwaggerGenOptions.UseAllOfToExtendReferenceSchemas() extension method, the FixNullableProperties workaround of @flibustier7seas is not necessary. With that, references are wrapped in an allOf and SupportNonNullableReferenceTypes also applies to them.

@niemyjski
Copy link

I would love to see this work out of the box

@sebasptsch
Copy link

+1 For this feature

@mladenkn
Copy link

Thx @flibustier7seas, that works for me!
But I had to remove if (property.Value.Reference != null).

@Rookian
Copy link

Rookian commented Sep 19, 2022

If you use the SwaggerGenOptions.UseAllOfToExtendReferenceSchemas() extension method, the FixNullableProperties workaround of @flibustier7seas is not necessary. With that, references are wrapped in an allOf and SupportNonNullableReferenceTypes also applies to them.

This won't work for string type or e.g. DateTimeOffset.

@josundt
Copy link

josundt commented Oct 27, 2022

With C# 11.0/.NET 7.0, the required keyword is being introduced for properties.
Properties with this keyword needs to be initialized through the object initializer when the class/struct/record is initialized from code.

But required properties also affects the behavior of System.Text.Json 7.x deserialization and thereby aspnetcore model validation.

The default behavior for System.Text.Json deserialization / aspnetcore model validation in .NET 7.0 is as follows.

Nullable/not required properties:

  • Property CAN be null, and property CAN be omitted from JSON payload.

Nullable/required properties:

  • Property CAN be null, but property CAN NOT be omitted from JSON payload.

Not nullable/not required properties:

  • Property CAN NOT be null, but property CAN be omitted from JSON payload (since it implies null).

Not nullable/required properties:

  • Property CAN NOT be null, and property CAN NOT be omitted from JSON payload.

Since nullablility and required/not are different things where one does not necessarily imply the other, I think SwasBuckle should use the C# property nullability for swagger property nullable, and C# property required/not for the swagger property required.

If the C# property is annotated with RequiredAttribute, I think this should override the above, and set both swagger nullable/required to true.

References:

@JohnGalt1717
Copy link

With C# 11.0/.NET 7.0, the required keyword is being introduced for properties. Properties with this keyword needs to be initialized through the object initializer when the class/struct/record is initialized from code.

But required properties also affects the behavior of System.Text.Json 7.x deserialization and thereby aspnetcore model validation.

The default behavior for System.Text.Json deserialization / aspnetcore model validation in .NET 7.0 is as follows.

Nullable/not required properties:

  • Property CAN be null, and property CAN be omitted from JSON payload.

Nullable/required properties:

  • Property CAN be null, but property CAN NOT be omitted from JSON payload.

Not nullable/not required properties:

  • Property CAN NOT be null, but property CAN NOT be omitted from JSON payload (since it implies null).

Not nullable/required properties:

  • Property CAN NOT be null, and property CAN NOT be omitted from JSON payload.

Since nullablility and required/not are different things where one does not necessarily imply the other, I think SwasBuckle should use the C# property nullability for swagger property nullable, and C# property required/not for the swagger property required.

If the C# property is annotated with RequiredAttribute, I think this should override the above, and set both swagger nullable/required to true.

References:

... And if C# 11's required modifier is specified it should also set required regardless of if it's a string or not.

@piskov
Copy link

piskov commented Oct 8, 2023

One more case to consider: avoiding inferred required attribute when default value is defined.

public string Prop { get; init; } = "default value";

It’s non-nullable, yet it shoudn’t be required as it has a default value.

See how ASP validation is working as a result of this issue

@JohnGalt1717
Copy link

Keep in mind that the required keyword rules everything now.

@piskov
Copy link

piskov commented Oct 8, 2023

@JohnGalt1717

Keep in mind that the required keyword rules everything now.

Absolutely not. When you use required, caller must provide the value. For example this code will not compile even with default value:

var p = new Person(); 
// ↑ compile error: Required member 'Person.Name' must be set in the object initializer



public class Person

{

    public required string Name { get; init; } = "Will not work";

}

There’re a lot of use-cases for non-nullable but also not required properties. For example, some options with default values. You don’t want them to be nullable in your code (as they will never be with default value), yet you don’t want to force consumers to supply them, hence the default value:



public class Project
{

    public SomeSettings Prop { get; init; } = new();
}

↑ // non-nullable, yet not required. This is exactly how ASP checker works

tldr; required non-nullable means you must provide the value; non-nullable with default value means, it’s ok to ommit as some sensible value will be used.

@JohnGalt1717
Copy link

We’re in agreement. No required means same as before.

Required being there means always must be filled in.

@piskov
Copy link

piskov commented Oct 8, 2023

Yet some PRs and this sentence ↓ seem to ignore the default logic.

Not nullable/not required properties:
Property CAN NOT be null, but property CAN NOT be omitted from JSON payload (since it implies null).

It obviosly CAN be ommited from json IF default value is set (this way it DOES not imply null).

@josundt
Copy link

josundt commented Oct 11, 2023

Yet some PRs and this sentence ↓ seem to ignore the default logic.

Not nullable/not required properties:
Property CAN NOT be null, but property CAN NOT be omitted from JSON payload (since it implies null).

It obviosly CAN be ommited from json IF default value is set (this way it DOES not imply null).

You're right, for this bullet there was a unintentional NOT in my original comment.
I updated it now.

@cremor
Copy link
Contributor

cremor commented Oct 11, 2023

@josundt About your updated comment:

Not nullable/not required properties:

* Property CAN NOT be null, but property CAN be omitted from JSON payload (since it implies null).

Is this really true for System.Text.Json deserialization / aspnetcore model validation? This doesn't make any sense to me. If it is implicitly null when it is omitted, then why can it be omitted if it can't be null?
It would make more sense if it could only be omitted if a non-null default value is set (like the last comment from @piskov mentions).

@josundt
Copy link

josundt commented Oct 16, 2023

@cremor
I guess not nullable properties in most cases should also be required.

But I also see that it sometimes may be useful to make not nullable properties optional (a.k.a. not required) with default values, and allow them to be omitted in JSON payloads or in object initializers.

I agree that the combination not nullable/not required could be a bit illogical, and arguably in many cases a bad design decision. F.ex. in a WebAPI context when a JSON payload is submitted by a client, the client may not expect properties omitted in the payload to magically have default values assigned on the server-side. The client is unaware of this hidden logic.

@piskov
Copy link

piskov commented Oct 16, 2023

@josundtit it will be “bad design decision” only if the default values are surprising to the user. Suppose you have aProfileDto and a VisibilityOptionsDto Settings(which consists of 20 bool flags) inside it.

You have 3 options:

  1. Make VisibilityOptionsDto? Settings nullable in C# and thus be forced to always check it for null in code.
  2. Make it required non-nullable required VisibilityOptionsDto Settings and force both API client and your own c# code to provide default value even if it’s just an object with 20 false bools.
  3. Make it non-nullable with simple default value VisibilityOptionsDto Settings {get; set; } = new(). This is the best of all worlds: neither you, nor client is needed to supply an object with 20 false bools, and you are free of useless nullability checks in C# code.

So as long as default value is clear, it’s completely reasonable to support this. Imagine if you were forced to explicitly supply every property of the class every time you were to create a new object in C#.

@martincostello
Copy link
Collaborator

#2710 (comment)

@blouflashdb
Copy link
Contributor

i am no longer able to work on it.

@martincostello martincostello added the help-wanted A change up for grabs for contributions from the community label Apr 14, 2024
@martincostello
Copy link
Collaborator

If anyone else would like to pick up the above fix and take it forward that would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 Medium priority
Projects
None yet