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,24 @@ 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
+
+            // TODO: We should be detecting primitive collections by looking at GetElementType() of the property and not at its type
+            // mapping; but #31469 is blocking that for shadow properties.
+            if (_sqlTranslator.TryTranslatePropertyAccess(methodCallExpression, out var translatedExpression, out var property)
+                && property is IProperty regularProperty
+                && translatedExpression is SqlExpression
                 {
-                    TypeMapping.ElementTypeMapping: RelationalTypeMapping elementTypeMapping
-                } collectionPropertyAccessExpression)
+                    TypeMapping.ElementTypeMapping: RelationalTypeMapping
+                } sqlExpression)
             {
-                var tableAlias = collectionPropertyAccessExpression switch
+                var tableAlias = sqlExpression 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, regularProperty, tableAlias) is
                     { } primitiveCollectionTranslation)
                 {
                     return primitiveCollectionTranslation;
@@ -316,17 +320,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,24 @@ 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).
+        // TODO: if property is non-null, GetElementType() should never be null, but we have #31469 for shadow properties
+        var isElementNullable = property?.GetElementType() 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,26 @@ 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).
+        // TODO: if property is non-null, GetElementType() should never be null, but we have #31469 for shadow properties
+        var isElementNullable = property?.GetElementType() 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,29 @@ 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 is
+                    { ElementType.ReadState: NullabilityState.NotNull } or
+                    { GenericTypeArguments: [{ 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);
         }

src/EFCore/Metadata/Conventions/NonNullableReferencePropertyConvention.cs

@@ -3,10 +3,27 @@
 
 #nullable enable
 
+using Xunit.Sdk;
+
 namespace Microsoft.EntityFrameworkCore.Cosmos;
 
 public class JsonTypesCosmosTest : JsonTypesTestBase
 {
+    // #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
+    // that has no ElementType; that causes the assertion on the element nullability to fail.
+    public override void Can_read_write_collection_of_string_JSON_values()
+        => Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_string_JSON_values());
+
+    // #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
+    // that has no ElementType; that causes the assertion on the element nullability to fail.
+    public override void Can_read_write_collection_of_binary_JSON_values()
+        => Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_binary_JSON_values());
+
+    // #25765 - the Cosmos type mapping source doesn't support primitive collections, so we end up with a Property
+    // that has no ElementType; that causes the assertion on the element nullability to fail.
+    public override void Can_read_write_collection_of_nullable_string_JSON_values()
+        => Assert.Throws<EqualException>(() => base.Can_read_write_collection_of_nullable_string_JSON_values());
+
     public override void Can_read_write_point()
         // No built-in JSON support for spatial types in the Cosmos provider
         => Assert.Throws<InvalidOperationException>(() => base.Can_read_write_point());

test/EFCore.Cosmos.FunctionalTests/JsonTypesCosmosTest.cs

@@ -3443,9 +3443,29 @@ protected virtual void Can_read_and_write_JSON_value<TEntity, TModel>(
 
             Assert.Equal(typeof(TModel).GetSequenceType(), element.ClrType);
             Assert.Same(property, element.CollectionProperty);
-            Assert.Equal(json.Contains("null", StringComparison.Ordinal) || !element.ClrType.IsValueType, element.IsNullable);
             Assert.Null(element.FindTypeMapping()!.ElementTypeMapping);
 
+            bool elementNullable;
+            if (element.ClrType.IsValueType)
+            {
+                elementNullable = element.ClrType.IsNullableType();
+            }
+            else
+            {
+                var nullabilityInfo = property switch
+                {
+                    { PropertyInfo: PropertyInfo p } => _nullabilityInfoContext.Create(p),
+                    { FieldInfo: FieldInfo f } => _nullabilityInfoContext.Create(f),
+                    _ => throw new UnreachableException()
+                };
+
+                elementNullable = nullabilityInfo is not
+                    { ElementType.ReadState: NullabilityState.NotNull } and not
+                    { GenericTypeArguments: [{ ReadState: NullabilityState.NotNull }] };
+            }
+
+            Assert.Equal(elementNullable, element.IsNullable);
+
             var comparer = element.GetValueComparer()!;
             var elementReaderWriter = element.GetJsonValueReaderWriter()!;
             foreach (var item in (IEnumerable)value!)
@@ -3741,4 +3761,6 @@ protected virtual void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
                 CoreEventId.MappedEntityTypeIgnoredWarning,
                 CoreEventId.MappedPropertyIgnoredWarning,
                 CoreEventId.MappedNavigationIgnoredWarning));
+
+    private readonly NullabilityInfoContext _nullabilityInfoContext = new();
 }