Fix silly bugs in non-nullable reference handling

Fixes #16760
dotnet · Jul 26, 2019 · 34f902a · 34f902a
1 parent a02785c
commit 34f902a
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 8 deletions.
diff --git a/src/EFCore/Metadata/Conventions/NonNullableConventionBase.cs b/src/EFCore/Metadata/Conventions/NonNullableConventionBase.cs
@@ -65,10 +65,15 @@ protected NonNullableConventionBase([NotNull] ProviderConventionSetBuilderDepend
         /// <param name="modelBuilder"> The model builder used to build the model. </param>
         /// <param name="memberInfo"> The member info. </param>
         /// <returns> <c>true</c> if the member type is a non-nullable reference type. </returns>
-        protected virtual bool IsNonNullable(
+        protected virtual bool IsNonNullableRefType(
             [NotNull] IConventionModelBuilder modelBuilder,
             [NotNull] MemberInfo memberInfo)
         {
+            if (memberInfo.GetMemberType().IsValueType)
+            {
+                return false;
+            }
+
             var state = GetOrInitializeState(modelBuilder);
 
             // For C# 8.0 nullable types, the C# currently synthesizes a NullableAttribute that expresses nullability into assemblies
@@ -77,7 +82,7 @@ protected virtual bool IsNonNullable(
             // Note that this may change - if https://github.com/dotnet/corefx/issues/36222 is done we can remove all of this.
 
             // First look for NullableAttribute on the member itself
-            if (Attribute.GetCustomAttributes(memberInfo, true)
+            if (Attribute.GetCustomAttributes(memberInfo)
                     .FirstOrDefault(a => a.GetType().FullName == NullableAttributeFullName) is Attribute attribute)
             {
                 var attributeType = attribute.GetType();
@@ -88,10 +93,9 @@ protected virtual bool IsNonNullable(
                     state.NullableAttrType = attributeType;
                 }
 
-                if (state.NullableFlagsFieldInfo?.GetValue(attribute) is byte[] flags
-                    && flags.FirstOrDefault() == 1)
+                if (state.NullableFlagsFieldInfo?.GetValue(attribute) is byte[] flags)
                 {
-                    return true;
+                    return flags.FirstOrDefault() == 1;
                 }
             }
 

diff --git a/src/EFCore/Metadata/Conventions/NonNullableNavigationConvention.cs b/src/EFCore/Metadata/Conventions/NonNullableNavigationConvention.cs
@@ -86,6 +86,6 @@ public virtual void ProcessNavigationAdded(
         private bool IsNonNullable(IConventionModelBuilder modelBuilder, IConventionNavigation navigation)
             => navigation.DeclaringEntityType.HasClrType()
                && navigation.DeclaringEntityType.GetRuntimeProperties().Find(navigation.Name) is PropertyInfo propertyInfo
-               && IsNonNullable(modelBuilder, propertyInfo);
+               && IsNonNullableRefType(modelBuilder, propertyInfo);
     }
 }
diff --git a/src/EFCore/Metadata/Conventions/NonNullableReferencePropertyConvention.cs b/src/EFCore/Metadata/Conventions/NonNullableReferencePropertyConvention.cs
@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Reflection;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore.Metadata.Builders;
@@ -29,7 +30,7 @@ private void Process(IConventionPropertyBuilder propertyBuilder)
             // If the model is spread across multiple assemblies, it may contain different NullableAttribute types as
             // the compiler synthesizes them for each assembly.
             if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo
-                && IsNonNullable(propertyBuilder.ModelBuilder, memberInfo))
+                && IsNonNullableRefType(propertyBuilder.ModelBuilder, memberInfo))
             {
                 propertyBuilder.IsRequired(true);
             }

diff --git a/test/EFCore.Tests/Metadata/Conventions/NonNullableReferencePropertyConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/NonNullableReferencePropertyConventionTest.cs
@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.ComponentModel.DataAnnotations;
 using Microsoft.EntityFrameworkCore.Metadata.Builders;
 using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
@@ -57,14 +58,27 @@ public void Non_nullability_sets_is_nullable_with_conventional_builder()
         [InlineData(nameof(A.NullObliviousNonNullable), true)]
         [InlineData(nameof(A.NullObliviousNullable), true)]
         [InlineData(nameof(A.RequiredAndNullable), false)]
-        public void Reference_nullability_sets_is_nullable_correctly(string propertyName, bool expectedNullable)
+        public void Reference_nullability_sets_is_nullable_correctly1(string propertyName, bool expectedNullable)
         {
             var modelBuilder = CreateModelBuilder();
             var entityTypeBuilder = modelBuilder.Entity<A>();
 
             Assert.Equal(expectedNullable, entityTypeBuilder.Property(propertyName).Metadata.IsNullable);
         }
 
+        [ConditionalTheory]
+        [InlineData(nameof(B.NonNullableValueType), false)]
+        [InlineData(nameof(B.NullableValueType), true)]
+        [InlineData(nameof(B.NonNullableRefType), false)]
+        [InlineData(nameof(B.NullableRefType), true)]
+        public void Reference_nullability_sets_is_nullable_correctly2(string propertyName, bool expectedNullable)
+        {
+            var modelBuilder = CreateModelBuilder();
+            var entityTypeBuilder = modelBuilder.Entity<B>();
+
+            Assert.Equal(expectedNullable, entityTypeBuilder.Property(propertyName).Metadata.IsNullable);
+        }
+
         private void RunConvention(InternalPropertyBuilder propertyBuilder)
         {
             var context = new ConventionContext<IConventionPropertyBuilder>(
@@ -108,6 +122,16 @@ private class A
 #pragma warning restore CS8632 // The annotation for nullable reference types should only be used in code within a '#nullable' context.
         }
 
+#nullable enable
+        public class B
+        {
+            [Key] public Guid NonNullableValueType { get; set; }
+            public Guid? NullableValueType { get; set; }
+            public string NonNullableRefType { get; set; } = "";
+            public string? NullableRefType { get; set; }
+        }
+#nullable disable
+
         private static ModelBuilder CreateModelBuilder() => InMemoryTestHelpers.Instance.CreateConventionBuilder();
     }
 }