Fully implement element nullability for primitive collections

Closes dotnet#31416

Author: roji

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -219,7 +219,7 @@ when entityQueryRootExpression.GetType() == typeof(EntityQueryRootExpression)
                 Check.DebugAssert(sqlParameterExpression is not null, "sqlParameterExpression is not null");
                 return TranslateCollection(
                         sqlParameterExpression,
-                        elementTypeMapping: null,
+                        property: null,
                         char.ToLowerInvariant(sqlParameterExpression.Name.First(c => c != '_')).ToString())
                     ?? base.VisitExtension(extensionExpression);
 
@@ -265,20 +265,22 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
         if (translated == QueryCompilationContext.NotTranslatedExpression)
         {
             // Attempt to translate access into a primitive collection property (i.e. array column)
-            if (_sqlTranslator.TryTranslatePropertyAccess(methodCallExpression, out var propertyAccessExpression)
-                && propertyAccessExpression is SqlExpression
-                {
-                    TypeMapping.ElementTypeMapping: RelationalTypeMapping elementTypeMapping
-                } collectionPropertyAccessExpression)
+            if (_sqlTranslator.TryTranslatePropertyAccess(methodCallExpression, out var translatedExpression, out var property)
+                && property is IProperty regularProperty
+                && regularProperty.GetElementType() is not null)
             {
-                var tableAlias = collectionPropertyAccessExpression switch
+                Check.DebugAssert(
+                    translatedExpression is SqlExpression { TypeMapping.ElementTypeMapping: not null },
+                    "Translated property access for a primitive collection property isn't a SqlExpression with a type mapping");
+
+                var tableAlias = translatedExpression switch
                 {
                     ColumnExpression c => c.Name[..1].ToLowerInvariant(),
                     JsonScalarExpression { Path: [.., { PropertyName: string propertyName }] } => propertyName[..1].ToLowerInvariant(),
                     _ => "j"
                 };
 
-                if (TranslateCollection(collectionPropertyAccessExpression, elementTypeMapping, tableAlias) is
+                if (TranslateCollection((SqlExpression)translatedExpression, regularProperty, tableAlias) is
                     { } primitiveCollectionTranslation)
                 {
                     return primitiveCollectionTranslation;
@@ -316,17 +318,15 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
     ///     collections.
     /// </remarks>
     /// <param name="sqlExpression">The expression to try to translate as a primitive collection expression.</param>
-    /// <param name="elementTypeMapping">
-    ///     The type mapping of the collection's element, or <see langword="null" /> when it's not known (i.e. for parameters).
+    /// <param name="property">
+    ///     If the primitive collection is a property, contains the <see cref="IProperty" /> for that property. Otherwise, the collection
+    ///     represents a parameter, and this contains <see langword="null" />.
     /// </param>
     /// <param name="tableAlias">
     ///     Provides an alias to be used for the table returned from translation, which will represent the collection.
     /// </param>
     /// <returns>A <see cref="ShapedQueryExpression" /> if the translation was successful, otherwise <see langword="null" />.</returns>
-    protected virtual ShapedQueryExpression? TranslateCollection(
-        SqlExpression sqlExpression,
-        RelationalTypeMapping? elementTypeMapping,
-        string tableAlias)
+    protected virtual ShapedQueryExpression? TranslateCollection(SqlExpression sqlExpression, IProperty? property, string tableAlias)
         => null;
 
     /// <summary>

src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs

@@ -129,7 +129,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
     /// </summary>
     protected override ShapedQueryExpression? TranslateCollection(
         SqlExpression sqlExpression,
-        RelationalTypeMapping? elementTypeMapping,
+        IProperty? property,
         string tableAlias)
     {
         if (_sqlServerCompatibilityLevel < 130)
@@ -145,6 +145,8 @@ protected override Expression VisitExtension(Expression extensionExpression)
         // (i.e. with a columnInfo), which determines the type conversion to apply to the JSON elements coming out.
         // For parameter collections, the element type mapping will only be inferred and applied later (see
         // SqlServerInferredTypeMappingApplier below), at which point the we'll apply it to add the WITH clause.
+        var elementTypeMapping = (RelationalTypeMapping?)sqlExpression.TypeMapping?.ElementTypeMapping;
+
         var openJsonExpression = elementTypeMapping is null
             ? new SqlServerOpenJsonExpression(tableAlias, sqlExpression)
             : new SqlServerOpenJsonExpression(
@@ -159,17 +161,23 @@ protected override Expression VisitExtension(Expression extensionExpression)
                     }
                 });
 
-        // TODO: This is a temporary CLR type-based check; when we have proper metadata to determine if the element is nullable, use it here
         var elementClrType = sqlExpression.Type.GetSequenceType();
-        var isColumnNullable = elementClrType.IsNullableType();
+
+        // If this is a collection property, get the element's nullability out of metadata. Otherwise, this is a parameter property, in
+        // which case we only have the CLR type (note that we cannot produce different SQLs based on the nullability of an *element* in
+        // a parameter collection - our caching mechanism only supports varying by the nullability of the parameter itself (i.e. the
+        // collection).
+        var isElementNullable = property is null
+            ? elementClrType.IsNullableType()
+            : property.GetElementType()!.IsNullable;
 
 #pragma warning disable EF1001 // Internal EF Core API usage.
         var selectExpression = new SelectExpression(
             openJsonExpression,
             columnName: "value",
             columnType: elementClrType,
             columnTypeMapping: elementTypeMapping,
-            isColumnNullable,
+            isElementNullable,
             identifierColumnName: "key",
             identifierColumnType: typeof(string),
             identifierColumnTypeMapping: _typeMappingSource.FindMapping("nvarchar(4000)"));

src/EFCore.SqlServer/Query/Internal/SqlServerQueryableMethodTranslatingExpressionVisitor.cs

@@ -203,7 +203,7 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
     /// </summary>
     protected override ShapedQueryExpression? TranslateCollection(
         SqlExpression sqlExpression,
-        RelationalTypeMapping? elementTypeMapping,
+        IProperty? property,
         string tableAlias)
     {
         // Support for JSON functions (e.g. json_each) was added in Sqlite 3.38.0 (2022-02-22, see https://www.sqlite.org/json1.html).
@@ -215,19 +215,25 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
             return null;
         }
 
+        var elementTypeMapping = (RelationalTypeMapping?)sqlExpression.TypeMapping?.ElementTypeMapping;
         var elementClrType = sqlExpression.Type.GetSequenceType();
         var jsonEachExpression = new JsonEachExpression(tableAlias, sqlExpression);
 
-        // TODO: This is a temporary CLR type-based check; when we have proper metadata to determine if the element is nullable, use it here
-        var isColumnNullable = elementClrType.IsNullableType();
+        // If this is a collection property, get the element's nullability out of metadata. Otherwise, this is a parameter property, in
+        // which case we only have the CLR type (note that we cannot produce different SQLs based on the nullability of an *element* in
+        // a parameter collection - our caching mechanism only supports varying by the nullability of the parameter itself (i.e. the
+        // collection).
+        var isElementNullable = property is null
+            ? elementClrType.IsNullableType()
+            : property.GetElementType()!.IsNullable;
 
 #pragma warning disable EF1001 // Internal EF Core API usage.
         var selectExpression = new SelectExpression(
             jsonEachExpression,
             columnName: "value",
             columnType: elementClrType,
             columnTypeMapping: elementTypeMapping,
-            isColumnNullable,
+            isElementNullable,
             identifierColumnName: "key",
             identifierColumnType: typeof(int),
             identifierColumnTypeMapping: _typeMappingSource.FindMapping(typeof(int)));

src/EFCore.Sqlite.Core/Query/Internal/SqliteQueryableMethodTranslatingExpressionVisitor.cs

@@ -1,6 +1,8 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Diagnostics.CodeAnalysis;
+
 namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
 
 /// <summary>
@@ -33,13 +35,19 @@ protected NonNullableConventionBase(ProviderConventionSetBuilderDependencies dep
     /// </summary>
     /// <param name="modelBuilder">The model builder used to build the model.</param>
     /// <param name="memberInfo">The member info.</param>
+    /// <param name="nullabilityInfo">
+    /// The nullability info for the <paramref name="memberInfo" />, or <see langword="null" /> if it does not represent a valid reference
+    /// type.
+    /// </param>
     /// <returns><see langword="true" /> if the member type is a non-nullable reference type.</returns>
-    protected virtual bool IsNonNullableReferenceType(
+    protected virtual bool TryGetNullabilityInfo(
         IConventionModelBuilder modelBuilder,
-        MemberInfo memberInfo)
+        MemberInfo memberInfo,
+        [NotNullWhen(true)] out NullabilityInfo? nullabilityInfo)
     {
         if (memberInfo.GetMemberType().IsValueType)
         {
+            nullabilityInfo = null;
             return false;
         }
 
@@ -49,14 +57,14 @@ protected virtual bool IsNonNullableReferenceType(
 
         var nullabilityInfoContext = (NullabilityInfoContext)annotation.Value!;
 
-        var nullabilityInfo = memberInfo switch
+        nullabilityInfo = memberInfo switch
         {
             PropertyInfo propertyInfo => nullabilityInfoContext.Create(propertyInfo),
             FieldInfo fieldInfo => nullabilityInfoContext.Create(fieldInfo),
             _ => null
         };
 
-        return nullabilityInfo?.ReadState == NullabilityState.NotNull;
+        return nullabilityInfo is not null;
     }
 
     /// <inheritdoc />

src/EFCore/Metadata/Conventions/NonNullableConventionBase.cs

@@ -77,5 +77,6 @@ private void ProcessNavigation(IConventionNavigationBuilder navigationBuilder)
 
     private bool IsNonNullable(IConventionModelBuilder modelBuilder, IConventionNavigation navigation)
         => navigation.DeclaringEntityType.GetRuntimeProperties().Find(navigation.Name) is PropertyInfo propertyInfo
-            && IsNonNullableReferenceType(modelBuilder, propertyInfo);
+            && TryGetNullabilityInfo(modelBuilder, propertyInfo, out var nullabilityInfo)
+            && nullabilityInfo.ReadState == NullabilityState.NotNull;
 }

src/EFCore/Metadata/Conventions/NonNullableNavigationConvention.cs

@@ -29,16 +29,28 @@ public NonNullableReferencePropertyConvention(ProviderConventionSetBuilderDepend
     private void Process(IConventionPropertyBuilder propertyBuilder)
     {
         if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo
-            && IsNonNullableReferenceType(propertyBuilder.ModelBuilder, memberInfo))
+            && TryGetNullabilityInfo(propertyBuilder.ModelBuilder, memberInfo, out var nullabilityInfo))
         {
-            propertyBuilder.IsRequired(true);
+            if (nullabilityInfo.ReadState == NullabilityState.NotNull)
+            {
+                propertyBuilder.IsRequired(true);
+            }
+
+            // If there's an element type, this is a primitive collection; check and apply the element's nullability as well.
+            if (propertyBuilder.Metadata.GetElementType() is IConventionElementType elementType
+                && (nullabilityInfo.ElementType is { ReadState: NullabilityState.NotNull }
+                    || nullabilityInfo.GenericTypeArguments is [{ ReadState: NullabilityState.NotNull }]))
+            {
+                elementType.SetIsNullable(false);
+            }
         }
     }
 
     private void Process(IConventionComplexPropertyBuilder propertyBuilder)
     {
         if (propertyBuilder.Metadata.GetIdentifyingMemberInfo() is MemberInfo memberInfo
-            && IsNonNullableReferenceType(propertyBuilder.ModelBuilder, memberInfo))
+            && TryGetNullabilityInfo(propertyBuilder.ModelBuilder, memberInfo, out var nullabilityInfo)
+            && nullabilityInfo.ReadState == NullabilityState.NotNull)
         {
             propertyBuilder.IsRequired(true);
         }