Skip to content

Fix nullable detection #12202

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

Merged
merged 2 commits into from
Jul 17, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 110 additions & 18 deletions src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.Diagnostics.Contracts;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.ComTypes;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.Extensions.Localization;
Expand All @@ -27,6 +31,9 @@ internal class DataAnnotationsMetadataProvider :
private const string NullableAttributeFullTypeName = "System.Runtime.CompilerServices.NullableAttribute";
private const string NullableFlagsFieldName = "NullableFlags";

private const string NullableContextAttributeFullName = "System.Runtime.CompilerServices.NullableContextAttribute";
private const string NullableContextFlagsFieldName = "Flag";

private readonly IStringLocalizerFactory _stringLocalizerFactory;
private readonly MvcOptions _options;
private readonly MvcDataAnnotationsLocalizationOptions _localizationOptions;
Expand Down Expand Up @@ -350,20 +357,44 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
if (!_options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes &&
requiredAttribute == null &&
!context.Key.ModelType.IsValueType &&

// Look specifically at attributes on the property/parameter. [Nullable] on
// the type has a different meaning.
IsNonNullable(context.ParameterAttributes ?? context.PropertyAttributes ?? Array.Empty<object>()))
context.Key.MetadataKind != ModelMetadataKind.Type)
{
// Since this behavior specifically relates to non-null-ness, we will use the non-default
// option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in
// a validation error by default because we convert empty/whitespace strings to null
// unless you say otherwise.
requiredAttribute = new RequiredAttribute()
var addInferredRequiredAttribute = false;
if (context.Key.MetadataKind == ModelMetadataKind.Type)
{
// Do nothing.
}
else if (context.Key.MetadataKind == ModelMetadataKind.Property)
{
addInferredRequiredAttribute = IsNullableReferenceType(
context.Key.ContainerType,
member: null,
context.PropertyAttributes);
}
else if (context.Key.MetadataKind == ModelMetadataKind.Parameter)
{
AllowEmptyStrings = true,
};
attributes.Add(requiredAttribute);
addInferredRequiredAttribute = IsNullableReferenceType(
context.Key.ParameterInfo?.Member.ReflectedType,
context.Key.ParameterInfo.Member,
context.ParameterAttributes);
}
else
{
throw new InvalidOperationException("Unsupported ModelMetadataKind: " + context.Key.MetadataKind);
}

if (addInferredRequiredAttribute)
{
// Since this behavior specifically relates to non-null-ness, we will use the non-default
// option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in
// a validation error by default because we convert empty/whitespace strings to null
// unless you say otherwise.
requiredAttribute = new RequiredAttribute()
{
AllowEmptyStrings = true,
};
attributes.Add(requiredAttribute);
}
}

if (requiredAttribute != null)
Expand Down Expand Up @@ -419,16 +450,26 @@ private static string GetDisplayGroup(FieldInfo field)
return string.Empty;
}

internal static bool IsNullableReferenceType(Type containingType, MemberInfo member, IEnumerable<object> attributes)
{
if (HasNullableAttribute(attributes, out var result))
{
return result;
}

return IsNullableBasedOnContext(containingType, member);
}

// Internal for testing
internal static bool IsNonNullable(IEnumerable<object> attributes)
internal static bool HasNullableAttribute(IEnumerable<object> attributes, out bool isNullable)
{
// [Nullable] is compiler synthesized, comparing by name.
var nullableAttribute = attributes
.Where(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal))
.FirstOrDefault();
.FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal));
if (nullableAttribute == null)
{
return false;
isNullable = false;
return false; // [Nullable] not found
}

// We don't handle cases where generics and NNRT are used. This runs into a
Expand All @@ -443,10 +484,61 @@ internal static bool IsNonNullable(IEnumerable<object> attributes)
flags.Length >= 0 &&
flags[0] == 1) // First element is the property/parameter type.
{
return true;
isNullable = true;
return true; // [Nullable] found and type is an NNRT
}

return false;
isNullable = false;
return true; // [Nullable] found but type is not an NNRT
}

internal static bool IsNullableBasedOnContext(Type containingType, MemberInfo member)
{
// The [Nullable] and [NullableContext] attributes are not inherited.
//
// The [NullableContext] attribute can appear on a method or on the module.
var attributes = member?.GetCustomAttributes(inherit: false) ?? Array.Empty<object>();
var isNullable = AttributesHasNullableContext(attributes);
if (isNullable != null)
{
return isNullable.Value;
}

// Check on the containing type
var type = containingType;
do
{
attributes = type.GetCustomAttributes(inherit: false);
isNullable = AttributesHasNullableContext(attributes);
if (isNullable != null)
{
return isNullable.Value;
}

type = type.DeclaringType;
}
while (type != null);

// If we don't find the attribute on the declaring type then repeat at the module level
attributes = containingType.Module.GetCustomAttributes(inherit: false);
isNullable = AttributesHasNullableContext(attributes);
return isNullable ?? false;

bool? AttributesHasNullableContext(object[] attributes)
{
var nullableContextAttribute = attributes
.FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableContextAttributeFullName, StringComparison.Ordinal));
if (nullableContextAttribute != null)
{
if (nullableContextAttribute.GetType().GetField(NullableContextFlagsFieldName) is FieldInfo field &&
field.GetValue(nullableContextAttribute) is byte @byte)
{
return @byte == 1; // [NullableContext] found
}
}

return null;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -1142,7 +1142,7 @@ public void CreateValidationMetadata_NoRequiredAttribute_IsRequiredLeftAlone(boo
Assert.Equal(initialValue, context.ValidationMetadata.IsRequired);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProperty()
{
// Arrange
Expand All @@ -1152,7 +1152,7 @@ public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProper
typeof(NullableReferenceTypes),
typeof(NullableReferenceTypes).GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType)));
var key = ModelMetadataIdentity.ForProperty(
typeof(NullableReferenceTypes),
typeof(NullableReferenceTypes),
nameof(NullableReferenceTypes.NonNullableReferenceType), typeof(string));
var context = new ValidationMetadataProviderContext(key, attributes);

Expand Down Expand Up @@ -1325,15 +1325,15 @@ public void CreateValidationDetails_ValidatableObject_AlreadyInContext_Ignores()
Assert.Same(attribute, validatorMetadata);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public void IsNonNullable_FindsNonNullableProperty()
{
// Arrange
var type = typeof(NullableReferenceTypes);
var property = type.GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType));

// Act
var result = DataAnnotationsMetadataProvider.IsNonNullable(property.GetCustomAttributes(inherit: true));
var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true));

// Assert
Assert.True(result);
Expand All @@ -1347,13 +1347,13 @@ public void IsNonNullable_FindsNullableProperty()
var property = type.GetProperty(nameof(NullableReferenceTypes.NullableReferenceType));

// Act
var result = DataAnnotationsMetadataProvider.IsNonNullable(property.GetCustomAttributes(inherit: true));
var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true));

// Assert
Assert.False(result);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public void IsNonNullable_FindsNonNullableParameter()
{
// Arrange
Expand All @@ -1362,7 +1362,7 @@ public void IsNonNullable_FindsNonNullableParameter()
var parameter = method.GetParameters().Where(p => p.Name == "nonNullableParameter").Single();

// Act
var result = DataAnnotationsMetadataProvider.IsNonNullable(parameter.GetCustomAttributes(inherit: true));
var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, method, parameter.GetCustomAttributes(inherit: true));

// Assert
Assert.True(result);
Expand All @@ -1377,7 +1377,7 @@ public void IsNonNullable_FindsNullableParameter()
var parameter = method.GetParameters().Where(p => p.Name == "nullableParameter").Single();

// Act
var result = DataAnnotationsMetadataProvider.IsNonNullable(parameter.GetCustomAttributes(inherit: true));
var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, method, parameter.GetCustomAttributes(inherit: true));

// Assert
Assert.False(result);
Expand Down Expand Up @@ -1429,12 +1429,12 @@ private DataAnnotationsMetadataProvider CreateIStringLocalizerProvider(bool useS
return CreateProvider(options: null, localizationOptions, useStringLocalizer ? stringLocalizerFactory.Object : null);
}

private ModelAttributes GetModelAttributes(IEnumerable<object> typeAttributes)
private ModelAttributes GetModelAttributes(IEnumerable<object> typeAttributes)
=> new ModelAttributes(typeAttributes, Array.Empty<object>(), Array.Empty<object>());

private ModelAttributes GetModelAttributes(
IEnumerable<object> typeAttributes,
IEnumerable<object> propertyAttributes)
IEnumerable<object> propertyAttributes)
=> new ModelAttributes(typeAttributes, propertyAttributes, Array.Empty<object>());

private class KVPEnumGroupAndNameComparer : IEqualityComparer<KeyValuePair<EnumGroupAndName, string>>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public NonNullableReferenceTypesTest(MvcTestFixture<BasicWebSite.StartupWithoutE

private HttpClient Client { get; set; }

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public async Task CanUseNonNullableReferenceType_WithController_OmitData_ValidationErrors()
{
// Arrange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private class Person1
}
#nullable restore

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError()
{
// Arrange
Expand Down Expand Up @@ -112,7 +112,7 @@ private class Person3
}
#nullable restore

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError_CustomMessage()
{
// Arrange
Expand Down Expand Up @@ -159,7 +159,7 @@ private void NonNullableParameter(string param1)
}
#nullable restore

[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
[Fact]
public async Task BindParameter_WithNonNullableReferenceType_NoData_ValidationError()
{
// Arrange
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

#nullable enable
using BasicWebSite.Models;
using Microsoft.AspNetCore.Mvc;

namespace BasicWebSite.Controllers
Expand Down