Skip to content

Commit 00b152e

Browse files
rynowakrynowak
committed
Fix nullable detection
Fixes: #11828 and #11813
1 parent 76408c9 commit 00b152e

File tree

4 files changed

+112
-30
lines changed

4 files changed

+112
-30
lines changed

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

Lines changed: 98 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@
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.ComTypes;
1013
using Microsoft.AspNetCore.Mvc.ModelBinding;
1114
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
1215
using Microsoft.Extensions.Localization;
@@ -27,6 +30,9 @@ internal class DataAnnotationsMetadataProvider :
2730
private const string NullableAttributeFullTypeName = "System.Runtime.CompilerServices.NullableAttribute";
2831
private const string NullableFlagsFieldName = "NullableFlags";
2932

33+
private const string NullableContextAttributeFullName = "System.Runtime.CompilerServices.NullableContextAttribute";
34+
private const string NullableContextFlagsFieldName = "Flag";
35+
3036
private readonly IStringLocalizerFactory _stringLocalizerFactory;
3137
private readonly MvcOptions _options;
3238
private readonly MvcDataAnnotationsLocalizationOptions _localizationOptions;
@@ -350,20 +356,44 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
350356
if (!_options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes &&
351357
requiredAttribute == null &&
352358
!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>()))
359+
context.Key.MetadataKind != ModelMetadataKind.Type)
357360
{
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()
361+
var addInferredRequiredAttribute = false;
362+
if (context.Key.MetadataKind == ModelMetadataKind.Type)
363+
{
364+
// Do nothing.
365+
}
366+
else if (context.Key.MetadataKind == ModelMetadataKind.Property)
367+
{
368+
addInferredRequiredAttribute = IsNullableReferenceType(
369+
context.Key.ContainerType,
370+
member: null,
371+
context.PropertyAttributes);
372+
}
373+
else if (context.Key.MetadataKind == ModelMetadataKind.Parameter)
374+
{
375+
addInferredRequiredAttribute = IsNullableReferenceType(
376+
context.Key.ParameterInfo?.Member.ReflectedType,
377+
context.Key.ParameterInfo.Member,
378+
context.ParameterAttributes);
379+
}
380+
else
363381
{
364-
AllowEmptyStrings = true,
365-
};
366-
attributes.Add(requiredAttribute);
382+
throw new InvalidOperationException("Unsupported ModelMetadataKind: " + context.Key.MetadataKind);
383+
}
384+
385+
if (addInferredRequiredAttribute)
386+
{
387+
// Since this behavior specifically relates to non-null-ness, we will use the non-default
388+
// option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in
389+
// a validation error by default because we convert empty/whitespace strings to null
390+
// unless you say otherwise.
391+
requiredAttribute = new RequiredAttribute()
392+
{
393+
AllowEmptyStrings = true,
394+
};
395+
attributes.Add(requiredAttribute);
396+
}
367397
}
368398

369399
if (requiredAttribute != null)
@@ -419,16 +449,27 @@ private static string GetDisplayGroup(FieldInfo field)
419449
return string.Empty;
420450
}
421451

452+
internal static bool IsNullableReferenceType(Type containingType, MemberInfo member, IEnumerable<object> attributes)
453+
{
454+
if (HasNullableAttribute(attributes, out var result))
455+
{
456+
return result;
457+
}
458+
459+
return IsNullableBasedOnContext(containingType, member);
460+
}
461+
422462
// Internal for testing
423-
internal static bool IsNonNullable(IEnumerable<object> attributes)
463+
internal static bool HasNullableAttribute(IEnumerable<object> attributes, out bool isNullable)
424464
{
425465
// [Nullable] is compiler synthesized, comparing by name.
426466
var nullableAttribute = attributes
427467
.Where(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal))
428468
.FirstOrDefault();
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,51 @@ 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+
var attributes = member?.GetCustomAttributes(inherit: true) ?? Array.Empty<object>();
498+
var isNullable = AttributesHasNullableContext(attributes);
499+
if (isNullable != null)
500+
{
501+
return isNullable.Value;
502+
}
503+
504+
attributes = containingType.GetCustomAttributes(inherit: false);
505+
isNullable = AttributesHasNullableContext(attributes);
506+
if (isNullable != null)
507+
{
508+
return isNullable.Value;
509+
}
510+
511+
// If we don't find the attribute on the declaring type then repeat at the module level
512+
attributes = containingType.Module.GetCustomAttributes(inherit: false);
513+
isNullable = AttributesHasNullableContext(attributes);
514+
return isNullable ?? false;
515+
516+
bool? AttributesHasNullableContext(object[] attributes)
517+
{
518+
var nullableContextAttribute = attributes
519+
.Where(a => string.Equals(a.GetType().FullName, NullableContextAttributeFullName, StringComparison.Ordinal))
520+
.FirstOrDefault();
521+
if (nullableContextAttribute != null)
522+
{
523+
if (nullableContextAttribute.GetType().GetField(NullableContextFlagsFieldName) is FieldInfo field &&
524+
field.GetValue(nullableContextAttribute) is byte @byte)
525+
{
526+
return @byte == 1; // [NullableContext] found
527+
}
528+
}
529+
530+
return null;
531+
}
450532
}
451533
}
452534
}

‎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

0 commit comments

Comments
 (0)