'Required' remains unpopulated for nullable record properties (dotnet7)

[JonnyWideFoot]

Problem Statment

I have the following response type in a C# dotnet 7.0 project, with enable in the csproj:

public sealed record SomeResponse(string Id, string? Description);

By default, swagger is generating this type as:

      "SomeResponse": {
        "type": "object",
        "properties": {
          "id": {
            "type": "string",
            "nullable": true
          },
          "description": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      },

Which is interpreted by my openAPI generator as:

# Auto-generation for reference:
&docker run --rm -v ${clientLocation}:/local openapitools/openapi-generator-cli generate -i /local/swagger.json -g typescript-axios --additional-properties="supportsES6=true,stringEnums=true" -o /local

export interface SomeResponse {
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  id?: string | null
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  description?: string | null
}

This is not what I want, as nullability does not match the C# type.

If I enable 'SupportNonNullableReferenceTypes':

        services.AddSwaggerGen(options =>
        {
            options.SupportNonNullableReferenceTypes();

Then the nullability is corrected in the swagger json file:

      "SomeResponse": {
        "type": "object",
        "properties": {
          "id": {
            "type": "string"
          },
          "description": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      },

However this generates with "id?", not "id":

export interface SomeResponse {
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  id?: string
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  description?: string | null
}

This is because the property is not listed in the required array for the type, however I think this behaviour is undesirable, as a non-bullable type should be required by definition.

I have made a filter which resolves this for me. See attached code below. I was wondering if you agree that the default behaviour should be that non-nullable types are in the reqired list required by definition?

Usage:

services.AddSwaggerGen(options =>
        {
            options.SupportNonNullableReferenceTypes(); // Insufficient
            options.SchemaFilter<FixNullableAndRequiredInSchemaFilter>(); // My 'fix'

Results in:

      "SomeResponse": {
        "required": ["id"],
        "type": "object",
        "properties": {
          "id": {
            "type": "string"
          },
          "description": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      },

And generates a client thus:

export interface SomeResponse{
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  id: string
  /**
   *
   * @type {string}
   * @memberof SomeResponse
   */
  description?: string | null
}

Bug or intended behaviour?

Sample Code

Filter

using System.Reflection;
using Microsoft.OpenApi.Models;
using Swashbuckle.AspNetCore.SwaggerGen;

namespace Swagger;

public sealed class FixNullableAndRequiredInSchemaFilter : ISchemaFilter
{
    public void Apply(OpenApiSchema schema, SchemaFilterContext context)
    {
        Type type = context.Type;

        if (type.ShouldExcludeAsPrimitive())
        {
            return;
        }

        schema.Required.Clear();

        var properties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);

        foreach (var property in properties)
        {
            string schemaName = property.ToSchemaName();

            // Some types have a variety of reasons to be excluded from serialisation,
            // e.g. [JsonExtensionData] so only change them if they are in the collection.
            if (schema.Properties.ContainsKey(schemaName))
            {
                bool hasNullableAttribute = type.HasNullableAttribute(property);

                schema.Properties[schemaName].Nullable = hasNullableAttribute;

                if (!hasNullableAttribute)
                {
                    schema.Required.Add(schemaName);
                }
            }
        }
    }
}

Helpers

using System.Reflection;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace Swagger;

public static class SwaggerHelperExtensions
{
    // Note: these are compiler internal types.
    private const string NullableContextAttributeName = "NullableContextAttribute";
    private const string NullableAttributeName = "NullableAttribute";

    private enum NullabilityMode
    {
        Unknown,
        NullableByDefault,
        NonNullableByDefault
    }

    public static bool ShouldExcludeAsPrimitive(this Type type)
    {
        if (type == typeof(string) || type.IsPrimitive)
        {
            return true;
        }
        else
        {
            var underlying = Nullable.GetUnderlyingType(type);
            if (underlying != null && (underlying == typeof(string) || underlying.IsPrimitive))
            {
                return true;
            }
        }

        return false;
    }

    public static string ToSchemaName(this PropertyInfo property)
    {
        var name = property.GetCustomAttribute<JsonPropertyNameAttribute>()?.Name ?? property.Name;
        return ToSchemaName(name);
    }

    private static string ToSchemaName(string name)
    {
        var jsonPropertyName = JsonNamingPolicy.CamelCase.ConvertName(name);
        return jsonPropertyName;
    }

    public static bool HasNullableAttribute(this Type type, PropertyInfo propertyInfo)
    {
        if (propertyInfo.PropertyType.IsValueType)
        {
            return Nullable.GetUnderlyingType(propertyInfo.PropertyType) != null;
        }

        var attributes = propertyInfo.GetCustomAttributesData();
        bool hasNullableAttribute = attributes.Any(x => x.AttributeType.Name == NullableAttributeName);

        var mode = type.EstablishNullabilityMode();
        return mode switch
        {
            NullabilityMode.Unknown => hasNullableAttribute,
            NullabilityMode.NullableByDefault => hasNullableAttribute,
            NullabilityMode.NonNullableByDefault => !hasNullableAttribute,
            _ => throw new InvalidOperationException($"Unknown {nameof(NullabilityMode)} mode {mode}")
        };
    }

    // Gnarly. I can't find a better way..
    private static NullabilityMode EstablishNullabilityMode(this Type type)
    {
        var nullableContextAttribute = type
            .GetCustomAttributesData()
            .FirstOrDefault(x => x.AttributeType.Name == NullableContextAttributeName);
        if (nullableContextAttribute != null &&
            nullableContextAttribute.ConstructorArguments.Count == 1)
        {
            object? value = nullableContextAttribute.ConstructorArguments[0].Value;
            if (value is byte b)
            {
                if (b == 1)
                {
                    // If the NullableContextAttribute is 1, the type is considered nullable by default.
                    return NullabilityMode.NullableByDefault;
                }
                else if (b == 2)
                {
                    // If the NullableContextAttribute is 2, the type is| considered non-nullable by default.
                    return NullabilityMode.NonNullableByDefault;
                }
                else
                {
                    throw new UnknownNullabilityModeException($"Unknown nullability mode for type {type.Name}");
                }
            }
        }

        return NullabilityMode.Unknown;
    }

    private sealed class UnknownNullabilityModeException : Exception
    {
        public UnknownNullabilityModeException(string message)
            : base(message)
        {
        }
    }
}

Tests

using FluentAssertions;
using Xunit;

namespace Swagger;

public sealed class SwaggerHelperExtensionsTests
{
    private const string RequiredStringName = @"RequiredString";
    private const string OptionalStringName = @"OptionalString";

    [Theory]
    [InlineData(typeof(TestRecordMixedSwapped), RequiredStringName, false)]
    [InlineData(typeof(TestRecordMixedSwapped), OptionalStringName, true)]
    [InlineData(typeof(TestClass), RequiredStringName, false)]
    [InlineData(typeof(TestClass), OptionalStringName, true)]
    [InlineData(typeof(TestRecord), RequiredStringName, false)]
    [InlineData(typeof(TestRecord), OptionalStringName, true)]
    [InlineData(typeof(TestRecordMixed), RequiredStringName, false)]
    [InlineData(typeof(TestRecordMixed), OptionalStringName, true)]
    [InlineData(typeof(TestRecordInitOptional), RequiredStringName, false)]
    [InlineData(typeof(TestRecordInitOptional), OptionalStringName, true)]
    [InlineData(typeof(TestRecordInitRequired), RequiredStringName, false)]
    [InlineData(typeof(TestRecordInitRequired), OptionalStringName, true)]
    [InlineData(typeof(TestRecordPrimary), RequiredStringName, false)]
    [InlineData(typeof(TestRecordPrimary), OptionalStringName, true)]
    [InlineData(typeof(TestRecordPrimaryInverse), RequiredStringName, false)]
    [InlineData(typeof(TestRecordPrimaryInverse), OptionalStringName, true)]
    [InlineData(typeof(TestValueRecordPrimary), RequiredStringName, false)]
    [InlineData(typeof(TestValueRecordPrimary), OptionalStringName, true)]
    [InlineData(typeof(TestValueRecordPrimaryInverse), RequiredStringName, false)]
    [InlineData(typeof(TestValueRecordPrimaryInverse), OptionalStringName, true)]
    public void TestNullableState(Type type, string propertyName, bool expectedResult)
    {
        var property = type.GetProperty(propertyName)!;
        type.HasNullableAttribute(property).Should().Be(expectedResult);
    }

    private sealed class TestClass
    {
        public TestClass(string requiredString, string optionalString)
        {
            RequiredString = requiredString;
            OptionalString = optionalString;
        }

        public string RequiredString { get; }
        public string? OptionalString { get; }
    }

    private sealed record TestRecordPrimary(string RequiredString, string? OptionalString);
    private sealed record TestRecordPrimaryInverse(string? OptionalString, string RequiredString);

    private sealed record TestValueRecordPrimary(int RequiredString, int? OptionalString);
    private sealed record TestValueRecordPrimaryInverse(int? OptionalString, int RequiredString);

    private sealed record TestRecord
    {
        public TestRecord(string requiredString, string? optionalString)
        {
            RequiredString = requiredString;
            OptionalString = optionalString;
        }

        public string RequiredString { get; }

        public string? OptionalString { get; }
    }

    private sealed record TestRecordInitOptional
    {
        public TestRecordInitOptional(string? optionalString)
        {
            OptionalString = optionalString;
        }

        public string RequiredString { get; set; } = string.Empty;

        public string? OptionalString { get; }
    }

    private sealed record TestRecordInitRequired
    {
        public TestRecordInitRequired(string requiredString)
        {
            RequiredString = requiredString;
        }

        public string RequiredString { get; }

        public string? OptionalString { get; set; }
    }

    private sealed record TestRecordMixed(string RequiredString)
    {
        public string? OptionalString { get; }
    }

    private sealed record TestRecordMixedSwapped(string? OptionalString)
    {
        public string RequiredString { get; set; } = string.Empty;
    }
}

[Havunen]

FYI. This is fixed in DotSwashbuckle 3.0.6+

[martincostello]

To make issue tracking a bit less overwhelming for the new maintainers (see #2778), I've created a new tracking issue to roll-up various nullability issues here: #2793.

We'll refer back to this issue from there and include it as part of resolving that issue, but I'm going to close this one to help prune the backlog.