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

[5.0.0] Respect Nullable context #1487

Closed
mrmartan opened this issue Jan 16, 2020 · 4 comments
Closed

[5.0.0] Respect Nullable context #1487

mrmartan opened this issue Jan 16, 2020 · 4 comments
Labels
p2 Medium priority

Comments

@mrmartan
Copy link

When C# 8 nullable context is enabled and ASP.NET Core 3 MvcOptions.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes == false (the default) the generated API spec does not match C# semantics (e.g. string model field is marked as nullable whci is not true with C# nullable context enabled) and ASP.NET model validation behavior.

<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
  <TargetFramework>netcoreapp3.0</TargetFramework>
  <Nullable>enable</Nullable>
</PropertyGroup>
services
    .AddControllers(options =>
    {
        options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = false;
    });
namespace TestApp
{
    [ApiController]
    public class HomeController : ControllerBase
    {
        [HttpPost]
        [Route("welcome")]
        [ProducesResponseType(StatusCodes.Status200OK, Type = typeof(TestModel))]
        public async Task<ActionResult<TestModel>> WelcomeJsonModel(TestModel model)
        {
            return model;
        }
    }

    public class TestModel
    {
        public string TestString { get; set; } = null!;

        [Required]
        public int TestInt {  get; set; }

        public AnotherTestModel? NullableSubmodel { get; set; }
    }

    public class AnotherTestModel
    {
        public int TestInt2 { get; set; }
    }
}

image

image

@domaindrivendev domaindrivendev added this to the Backlog milestone Mar 5, 2020
@ketovdk
Copy link

ketovdk commented Mar 17, 2020

Not sure it is about required. It just does not detect that these types are not nullable. I tried to do something like
public class RequiredSchemaFilter: ISchemaFilter { public void Apply(OpenApiSchema schema, SchemaFilterContext context) { schema.Required ??= new HashSet<string>(); schema.Required.AddRange(schema.Properties.Where(x => !x.Value.Nullable).Select(x => x.Key));
But it detects not nullable types as nullable

@foriequal0
Copy link

foriequal0 commented May 15, 2020

I use this with new options.SupportNonNullableReferenceTypes(); option

    internal class NonNullableAsRequiredSchemaFilter : ISchemaFilter
    {
        private readonly MvcOptions mvcOptions;
        private readonly ISerializerDataContractResolver resolver;

        public NonNullableAsRequiredSchemaFilter(IOptions<MvcOptions> mvcOptions, ISerializerDataContractResolver resolver)
        {
            this.mvcOptions = mvcOptions.Value;
            this.resolver = resolver;
        }

        public void Apply(OpenApiSchema schema, SchemaFilterContext context)
        {
            if (mvcOptions.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes)
            {
                return;
            }

            if (context.MemberInfo != null || context.ParameterInfo != null)
            {
                return;
            }

            var objectProperties = resolver.GetDataContractForType(context.Type).ObjectProperties;
            foreach (var property in objectProperties)
            {
                var attributes = CollectContextAttributes(property.MemberInfo.DeclaringType)
                    .Concat(property.MemberInfo.CustomAttributes)
                    .ToList();
                var nullable = TryGetNullable(property.MemberType, attributes);
                if (nullable == false)
                {
                    schema.Required.Add(property.Name);
                }
            }
        }

        private bool? TryGetNullable(Type type, IEnumerable<CustomAttributeData> attributes)
        {
            var valueNullable =
                type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>);
            if (valueNullable)
            {
                return true;
            }

            if (type.IsValueType)
            {
                return false;
            }

            var nullableFlag = GetNullableFlagByAttribute(attributes);
            if (nullableFlag != 0)
            {
                return nullableFlag == 2;
            }

            // Don't touch if the compiler doesn't generate NullableAttribute.
            return null;
        }

        private IEnumerable<CustomAttributeData> CollectContextAttributes(Type? declaringType)
        {
            if (declaringType == null)
            {
                return Enumerable.Empty<CustomAttributeData>();
            }

            var attributes = declaringType
                .CustomAttributes
                .Where(x => x.AttributeType.FullName == "System.Runtime.CompilerServices.NullableContextAttribute");
            return attributes.Concat(CollectContextAttributes(declaringType.DeclaringType));
        }

        /// <remarks>
        /// https://github.com/dotnet/roslyn/blob/7bc44488c661fd6bbb6c53f39512a6fe0cc5ef84/docs/features/nullable-metadata.md
        /// </remarks>
        private static int GetNullableFlagByAttribute(List<CustomAttributeData> attributes)
        {
            for (var i = attributes.Count - 1; i >= 0; i--)
            {
                var attribute = attributes[i];
                var fullName = attribute.AttributeType.FullName;
                if (fullName == null)
                {
                    continue;
                }

                if (fullName.Contains(
                    "System.Runtime.CompilerServices.NullableAttribute",
                    StringComparison.InvariantCulture))
                {
                    var arg = attribute.ConstructorArguments[0];
                    if (arg.ArgumentType == typeof(byte))
                    {
                        return (byte)arg.Value!;
                    }
                    else
                    {
                        return ((byte[])arg.Value!)[0];
                    }
                }

                if (fullName.Contains(
                    "System.Runtime.CompilerServices.NullableContextAttribute",
                    StringComparison.InvariantCulture))
                {
                    var nullableFlag = (byte)attribute.ConstructorArguments[0].Value!;

                    return nullableFlag;
                }
            }

            return 0;
        }
    }

edit: Now it is works with objects. Thank you @gaboe to point it out.

@gaboe
Copy link

gaboe commented Oct 1, 2020

@foriequal0 Thank you for your answer, your code works well for primitive types, but for objects not so well.

For example, code:

public class DocumentDto
{
    public int DocumentID { get; set; }

    public string? ExternalID { get; set; }

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

    public NestedClass? NestedProperty { get; set; }

    public string? Extension { get; set; }
}

public class NestedClass
{
    public int PrimaryID { get; set; }
}

will generate this:

image

nestedProperty is required here, but it should not.

@foriequal0 Have you encountered the same problem?

@martincostello
Copy link
Collaborator

#2056 claims it likely fixed this issue. Please open a new issue if you can still reproduce this issue in the latest release.

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
Development

No branches or pull requests

6 participants