Skip to content

Commit 4ba66f0

Browse files
rynowakrynowak
authored
Fix nullable detection (#12202)
* Fix nullable detection Fixes: #11828 and #11813
1 parent 892ea7e commit 4ba66f0

File tree

5 files changed

+124
-33
lines changed

5 files changed

+124
-33
lines changed

‎src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs

Lines changed: 110 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,15 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
5+
using System.Collections;
56
using System.Collections.Generic;
67
using System.ComponentModel;
78
using System.ComponentModel.DataAnnotations;
9+
using System.Diagnostics.Contracts;
810
using System.Linq;
911
using System.Reflection;
12+
using System.Runtime.InteropServices;
13+
using System.Runtime.InteropServices.ComTypes;
1014
using Microsoft.AspNetCore.Mvc.ModelBinding;
1115
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
1216
using Microsoft.Extensions.Localization;
@@ -27,6 +31,9 @@ internal class DataAnnotationsMetadataProvider :
2731
private const string NullableAttributeFullTypeName = "System.Runtime.CompilerServices.NullableAttribute";
2832
private const string NullableFlagsFieldName = "NullableFlags";
2933

34+
private const string NullableContextAttributeFullName = "System.Runtime.CompilerServices.NullableContextAttribute";
35+
private const string NullableContextFlagsFieldName = "Flag";
36+
3037
private readonly IStringLocalizerFactory _stringLocalizerFactory;
3138
private readonly MvcOptions _options;
3239
private readonly MvcDataAnnotationsLocalizationOptions _localizationOptions;
@@ -350,20 +357,44 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
350357
if (!_options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes &&
351358
requiredAttribute == null &&
352359
!context.Key.ModelType.IsValueType &&
353-
354-
// Look specifically at attributes on the property/parameter. [Nullable] on
355-
// the type has a different meaning.
356-
IsNonNullable(context.ParameterAttributes ?? context.PropertyAttributes ?? Array.Empty<object>()))
360+
context.Key.MetadataKind != ModelMetadataKind.Type)
357361
{
358-
// Since this behavior specifically relates to non-null-ness, we will use the non-default
359-
// option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in
360-
// a validation error by default because we convert empty/whitespace strings to null
361-
// unless you say otherwise.
362-
requiredAttribute = new RequiredAttribute()
362+
var addInferredRequiredAttribute = false;
363+
if (context.Key.MetadataKind == ModelMetadataKind.Type)
364+
{
365+
// Do nothing.
366+
}
367+
else if (context.Key.MetadataKind == ModelMetadataKind.Property)
368+
{
369+
addInferredRequiredAttribute = IsNullableReferenceType(
370+
context.Key.ContainerType,
371+
member: null,
372+
context.PropertyAttributes);
373+
}
374+
else if (context.Key.MetadataKind == ModelMetadataKind.Parameter)
363375
{
364-
AllowEmptyStrings = true,
365-
};
366-
attributes.Add(requiredAttribute);
376+
addInferredRequiredAttribute = IsNullableReferenceType(
377+
context.Key.ParameterInfo?.Member.ReflectedType,
378+
context.Key.ParameterInfo.Member,
379+
context.ParameterAttributes);
380+
}
381+
else
382+
{
383+
throw new InvalidOperationException("Unsupported ModelMetadataKind: " + context.Key.MetadataKind);
384+
}
385+
386+
if (addInferredRequiredAttribute)
387+
{
388+
// Since this behavior specifically relates to non-null-ness, we will use the non-default
389+
// option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in
390+
// a validation error by default because we convert empty/whitespace strings to null
391+
// unless you say otherwise.
392+
requiredAttribute = new RequiredAttribute()
393+
{
394+
AllowEmptyStrings = true,
395+
};
396+
attributes.Add(requiredAttribute);
397+
}
367398
}
368399

369400
if (requiredAttribute != null)
@@ -419,16 +450,26 @@ private static string GetDisplayGroup(FieldInfo field)
419450
return string.Empty;
420451
}
421452

453+
internal static bool IsNullableReferenceType(Type containingType, MemberInfo member, IEnumerable<object> attributes)
454+
{
455+
if (HasNullableAttribute(attributes, out var result))
456+
{
457+
return result;
458+
}
459+
460+
return IsNullableBasedOnContext(containingType, member);
461+
}
462+
422463
// Internal for testing
423-
internal static bool IsNonNullable(IEnumerable<object> attributes)
464+
internal static bool HasNullableAttribute(IEnumerable<object> attributes, out bool isNullable)
424465
{
425466
// [Nullable] is compiler synthesized, comparing by name.
426467
var nullableAttribute = attributes
427-
.Where(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal))
428-
.FirstOrDefault();
468+
.FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal));
429469
if (nullableAttribute == null)
430470
{
431-
return false;
471+
isNullable = false;
472+
return false; // [Nullable] not found
432473
}
433474

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

449-
return false;
491+
isNullable = false;
492+
return true; // [Nullable] found but type is not an NNRT
493+
}
494+
495+
internal static bool IsNullableBasedOnContext(Type containingType, MemberInfo member)
496+
{
497+
// The [Nullable] and [NullableContext] attributes are not inherited.
498+
//
499+
// The [NullableContext] attribute can appear on a method or on the module.
500+
var attributes = member?.GetCustomAttributes(inherit: false) ?? Array.Empty<object>();
501+
var isNullable = AttributesHasNullableContext(attributes);
502+
if (isNullable != null)
503+
{
504+
return isNullable.Value;
505+
}
506+
507+
// Check on the containing type
508+
var type = containingType;
509+
do
510+
{
511+
attributes = type.GetCustomAttributes(inherit: false);
512+
isNullable = AttributesHasNullableContext(attributes);
513+
if (isNullable != null)
514+
{
515+
return isNullable.Value;
516+
}
517+
518+
type = type.DeclaringType;
519+
}
520+
while (type != null);
521+
522+
// If we don't find the attribute on the declaring type then repeat at the module level
523+
attributes = containingType.Module.GetCustomAttributes(inherit: false);
524+
isNullable = AttributesHasNullableContext(attributes);
525+
return isNullable ?? false;
526+
527+
bool? AttributesHasNullableContext(object[] attributes)
528+
{
529+
var nullableContextAttribute = attributes
530+
.FirstOrDefault(a => string.Equals(a.GetType().FullName, NullableContextAttributeFullName, StringComparison.Ordinal));
531+
if (nullableContextAttribute != null)
532+
{
533+
if (nullableContextAttribute.GetType().GetField(NullableContextFlagsFieldName) is FieldInfo field &&
534+
field.GetValue(nullableContextAttribute) is byte @byte)
535+
{
536+
return @byte == 1; // [NullableContext] found
537+
}
538+
}
539+
540+
return null;
541+
}
450542
}
451543
}
452544
}

‎src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1142,7 +1142,7 @@ public void CreateValidationMetadata_NoRequiredAttribute_IsRequiredLeftAlone(boo
11421142
Assert.Equal(initialValue, context.ValidationMetadata.IsRequired);
11431143
}
11441144

1145-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
1145+
[Fact]
11461146
public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProperty()
11471147
{
11481148
// Arrange
@@ -1152,7 +1152,7 @@ public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProper
11521152
typeof(NullableReferenceTypes),
11531153
typeof(NullableReferenceTypes).GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType)));
11541154
var key = ModelMetadataIdentity.ForProperty(
1155-
typeof(NullableReferenceTypes),
1155+
typeof(NullableReferenceTypes),
11561156
nameof(NullableReferenceTypes.NonNullableReferenceType), typeof(string));
11571157
var context = new ValidationMetadataProviderContext(key, attributes);
11581158

@@ -1325,15 +1325,15 @@ public void CreateValidationDetails_ValidatableObject_AlreadyInContext_Ignores()
13251325
Assert.Same(attribute, validatorMetadata);
13261326
}
13271327

1328-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
1328+
[Fact]
13291329
public void IsNonNullable_FindsNonNullableProperty()
13301330
{
13311331
// Arrange
13321332
var type = typeof(NullableReferenceTypes);
13331333
var property = type.GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType));
13341334

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

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

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

13521352
// Assert
13531353
Assert.False(result);
13541354
}
13551355

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

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

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

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

13821382
// Assert
13831383
Assert.False(result);
@@ -1429,12 +1429,12 @@ private DataAnnotationsMetadataProvider CreateIStringLocalizerProvider(bool useS
14291429
return CreateProvider(options: null, localizationOptions, useStringLocalizer ? stringLocalizerFactory.Object : null);
14301430
}
14311431

1432-
private ModelAttributes GetModelAttributes(IEnumerable<object> typeAttributes)
1432+
private ModelAttributes GetModelAttributes(IEnumerable<object> typeAttributes)
14331433
=> new ModelAttributes(typeAttributes, Array.Empty<object>(), Array.Empty<object>());
14341434

14351435
private ModelAttributes GetModelAttributes(
14361436
IEnumerable<object> typeAttributes,
1437-
IEnumerable<object> propertyAttributes)
1437+
IEnumerable<object> propertyAttributes)
14381438
=> new ModelAttributes(typeAttributes, propertyAttributes, Array.Empty<object>());
14391439

14401440
private class KVPEnumGroupAndNameComparer : IEqualityComparer<KeyValuePair<EnumGroupAndName, string>>

‎src/Mvc/test/Mvc.FunctionalTests/NonNullableReferenceTypesTest.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ public NonNullableReferenceTypesTest(MvcTestFixture<BasicWebSite.StartupWithoutE
2626

2727
private HttpClient Client { get; set; }
2828

29-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
29+
[Fact]
3030
public async Task CanUseNonNullableReferenceType_WithController_OmitData_ValidationErrors()
3131
{
3232
// Arrange

‎src/Mvc/test/Mvc.IntegrationTests/NullableReferenceTypeIntegrationTest.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ private class Person1
2525
}
2626
#nullable restore
2727

28-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
28+
[Fact]
2929
public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError()
3030
{
3131
// Arrange
@@ -112,7 +112,7 @@ private class Person3
112112
}
113113
#nullable restore
114114

115-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
115+
[Fact]
116116
public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError_CustomMessage()
117117
{
118118
// Arrange
@@ -159,7 +159,7 @@ private void NonNullableParameter(string param1)
159159
}
160160
#nullable restore
161161

162-
[Fact(Skip = "https://github.com/aspnet/AspNetCore/issues/11828")]
162+
[Fact]
163163
public async Task BindParameter_WithNonNullableReferenceType_NoData_ValidationError()
164164
{
165165
// Arrange

‎src/Mvc/test/WebSites/BasicWebSite/Controllers/NonNullableController.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
#nullable enable
5-
using BasicWebSite.Models;
65
using Microsoft.AspNetCore.Mvc;
76

87
namespace BasicWebSite.Controllers

0 commit comments

Comments
 (0)