Model validation for primitive collections

src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

@@ -266,14 +266,9 @@ protected override Expression VisitMethodCall(MethodCallExpression methodCallExp
         {
             // Attempt to translate access into a primitive collection property (i.e. array column)
 
-            // 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
-                } sqlExpression)
+                && property is IProperty { IsPrimitiveCollection: true } regularProperty
+                && translatedExpression is SqlExpression sqlExpression)
             {
                 var tableAlias = sqlExpression switch
                 {
@@ -2309,10 +2304,8 @@ static TableExpressionBase FindRootTableExpressionForColumn(ColumnExpression col
                 }
             }
 
-            // TODO: Check that the property is a primitive collection property directly once we have that in metadata, rather than
-            // looking at the type mapping.
             var property = type.FindProperty(memberName);
-            if (property?.GetRelationalTypeMapping().ElementTypeMapping is null)
+            if (property?.IsPrimitiveCollection != true)
             {
                 return null;
             }

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

@@ -2003,7 +2003,7 @@ protected override Expression VisitBinary(BinaryExpression node)
                     if (node.Right is MethodCallExpression methodCallExpression
                         && IsPropertyAssignment(methodCallExpression, out var property, out var parameter))
                     {
-                        if (property!.GetTypeMapping().ElementTypeMapping != null
+                        if (property!.IsPrimitiveCollection
                             && !property.ClrType.IsArray)
                         {
                             var currentVariable = Variable(parameter!.Type);

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

@@ -167,10 +167,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
         // 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;
+        var isElementNullable = property?.GetElementType()!.IsNullable;
 
 #pragma warning disable EF1001 // Internal EF Core API usage.
         var selectExpression = new SelectExpression(

src/EFCore.SqlServer/Update/Internal/SqlServerUpdateSqlGenerator.cs

@@ -152,8 +152,7 @@ protected override void AppendUpdateColumnValue(
             stringBuilder.Append(columnModification.JsonPath);
             stringBuilder.Append("', ");
 
-            if (columnModification.Property != null
-                && columnModification.Property.GetTypeMapping().ElementTypeMapping == null)
+            if (columnModification.Property is { IsPrimitiveCollection: false })
             {
                 base.AppendUpdateColumnValue(updateSqlGeneratorHelper, columnModification, stringBuilder, name, schema);
             }

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

@@ -223,10 +223,7 @@ protected override QueryableMethodTranslatingExpressionVisitor CreateSubqueryVis
         // 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;
+        var isElementNullable = property?.GetElementType()!.IsNullable;
 
 #pragma warning disable EF1001 // Internal EF Core API usage.
         var selectExpression = new SelectExpression(

src/EFCore.Sqlite.Core/Update/Internal/SqliteUpdateSqlGenerator.cs

@@ -159,8 +159,7 @@ protected override void AppendUpdateColumnValue(
             stringBuilder.Append(columnModification.JsonPath);
             stringBuilder.Append("', ");
 
-            if (columnModification.Property != null
-                && columnModification.Property.GetTypeMapping().ElementTypeMapping == null)
+            if (columnModification.Property is { IsPrimitiveCollection: false })
             {
                 var providerClrType = (columnModification.Property.GetTypeMapping().Converter?.ProviderClrType
                     ?? columnModification.Property.ClrType).UnwrapNullableType();

src/EFCore/ChangeTracking/ListComparer.cs

@@ -92,7 +92,6 @@ public static bool Compare(IEnumerable<TElement>? a, IEnumerable<TElement>? b, V
         throw new InvalidOperationException(
             CoreStrings.BadListType(
                 (a is IList<TElement?> ? b : a).GetType().ShortDisplayName(),
-                typeof(ListComparer<TElement?>).ShortDisplayName(),
                 typeof(IList<>).MakeGenericType(elementComparer.Type).ShortDisplayName()));
     }
 
@@ -129,7 +128,6 @@ public static IList<TElement> Snapshot(IEnumerable<TElement> source, ValueCompar
             throw new InvalidOperationException(
                 CoreStrings.BadListType(
                     source.GetType().ShortDisplayName(),
-                    typeof(ListComparer<TElement?>).ShortDisplayName(),
                     typeof(IList<>).MakeGenericType(elementComparer.Type).ShortDisplayName()));
         }
 

src/EFCore/ChangeTracking/NullableValueTypeListComparer.cs

@@ -93,7 +93,6 @@ public static bool Compare(IEnumerable<TElement?>? a, IEnumerable<TElement?>? b,
         throw new InvalidOperationException(
             CoreStrings.BadListType(
                 (a is IList<TElement?> ? b : a).GetType().ShortDisplayName(),
-                typeof(NullableValueTypeListComparer<TElement>).ShortDisplayName(),
                 typeof(IList<>).MakeGenericType(elementComparer.Type.MakeNullable()).ShortDisplayName()));
     }
 
@@ -130,7 +129,6 @@ public static int GetHashCode(IEnumerable<TElement?> source, ValueComparer<TElem
             throw new InvalidOperationException(
                 CoreStrings.BadListType(
                     source.GetType().ShortDisplayName(),
-                    typeof(NullableValueTypeListComparer<TElement>).ShortDisplayName(),
                     typeof(IList<>).MakeGenericType(elementComparer.Type.MakeNullable()).ShortDisplayName()));
         }
 

src/EFCore/ChangeTracking/ObjectListComparer.cs

@@ -91,7 +91,6 @@ public static bool Compare(IEnumerable<TElement>? a, IEnumerable<TElement>? b, V
         throw new InvalidOperationException(
             CoreStrings.BadListType(
                 (a is IList<TElement?> ? b : a).GetType().ShortDisplayName(),
-                typeof(ListComparer<TElement?>).ShortDisplayName(),
                 typeof(IList<>).MakeGenericType(elementComparer.Type).ShortDisplayName()));
     }
 
@@ -128,7 +127,6 @@ public static IList<TElement> Snapshot(IEnumerable<TElement> source, ValueCompar
             throw new InvalidOperationException(
                 CoreStrings.BadListType(
                     source.GetType().ShortDisplayName(),
-                    typeof(ListComparer<TElement?>).ShortDisplayName(),
                     typeof(IList<>).MakeGenericType(elementComparer.Type).ShortDisplayName()));
         }
 

src/EFCore/Infrastructure/ModelValidator.cs

@@ -60,6 +60,7 @@ public virtual void Validate(IModel model, IDiagnosticsLogger<DbLoggerCategory.M
         ValidateQueryFilters(model, logger);
         ValidateData(model, logger);
         ValidateTypeMappings(model, logger);
+        ValidatePrimitiveCollections(model, logger);
         ValidateTriggers(model, logger);
         LogShadowProperties(model, logger);
     }
@@ -1003,6 +1004,43 @@ static void Validate(ITypeBase typeBase, IDiagnosticsLogger<DbLoggerCategory.Mod
         }
     }
 
+    /// <summary>
+    ///     Validates the mapping of primitive collection properties the model.
+    /// </summary>
+    /// <param name="model">The model to validate.</param>
+    /// <param name="logger">The logger to use.</param>
+    protected virtual void ValidatePrimitiveCollections(
+        IModel model,
+        IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
+    {
+        foreach (var entityType in model.GetEntityTypes())
+        {
+            Validate(entityType, logger);
+        }
+
+        static void Validate(ITypeBase typeBase, IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
+        {
+            foreach (var property in typeBase.GetDeclaredProperties())
+            {
+                var elementClrType = property.GetElementType()?.ClrType;
+                if (property is { IsPrimitiveCollection: true, ClrType.IsArray: false, ClrType.IsSealed: true }
+                    && elementClrType is { IsSealed: true }
+                    && elementClrType.TryGetElementType(typeof(IList<>)) == null)
+                {
+                    throw new InvalidOperationException(
+                        CoreStrings.BadListType(
+                            property.ClrType.ShortDisplayName(),
+                            typeof(IList<>).MakeGenericType(elementClrType).ShortDisplayName()));
+                }
+            }
+
+            foreach (var complexProperty in typeBase.GetDeclaredComplexProperties())
+            {
+                Validate(complexProperty.ComplexType, logger);
+            }
+        }
+    }
+
     /// <summary>
     ///     Validates the mapping/configuration of query filters in the model.
     /// </summary>

src/EFCore/Metadata/Conventions/ElementMappingConvention.cs

@@ -0,0 +1,55 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
+
+/// <summary>
+///     A convention that ensures property mappings have any ElementMapping discovered by the type mapper.
+/// </summary>
+/// <remarks>
+///     <para>
+///         See <see href="https://aka.ms/efcore-docs-conventions">Model building conventions</see> for more information and examples.
+///     </para>
+/// </remarks>
+public class ElementMappingConvention : IModelFinalizingConvention
+{
+    /// <summary>
+    ///     Creates a new instance of <see cref="ElementMappingConvention" />.
+    /// </summary>
+    /// <param name="dependencies">Parameter object containing dependencies for this convention.</param>
+    public ElementMappingConvention(ProviderConventionSetBuilderDependencies dependencies)
+    {
+        Dependencies = dependencies;
+    }
+
+    /// <summary>
+    ///     Dependencies for this service.
+    /// </summary>
+    protected virtual ProviderConventionSetBuilderDependencies Dependencies { get; }
+
+    /// <inheritdoc />
+    public void ProcessModelFinalizing(IConventionModelBuilder modelBuilder, IConventionContext<IConventionModelBuilder> context)
+    {
+        foreach (var entityType in modelBuilder.Metadata.GetEntityTypes())
+        {
+            Validate(entityType);
+        }
+
+        void Validate(IConventionTypeBase typeBase)
+        {
+            foreach (var property in typeBase.GetDeclaredProperties())
+            {
+                var typeMapping = Dependencies.TypeMappingSource.FindMapping((IProperty)property);
+                if (typeMapping is { ElementTypeMapping: not null })
+                {
+                    property.SetElementType(property.ClrType.TryGetElementType(typeof(IEnumerable<>)));
+                }
+            }
+
+            foreach (var complexProperty in typeBase.GetDeclaredComplexProperties())
+            {
+                Validate(complexProperty.ComplexType);
+            }
+        }
+    }
+}

src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs

@@ -100,6 +100,7 @@ public virtual ConventionSet CreateConventionSet()
         conventionSet.Add(new BackingFieldConvention(Dependencies));
         conventionSet.Add(new QueryFilterRewritingConvention(Dependencies));
         conventionSet.Add(new RuntimeModelConvention(Dependencies));
+        conventionSet.Add(new ElementMappingConvention(Dependencies));
 
         return conventionSet;
     }

src/EFCore/Metadata/Conventions/RuntimeModelConvention.cs

@@ -62,7 +62,7 @@ protected virtual RuntimeModel Create(IModel model)
                 var elementType = property.GetElementType();
                 if (elementType != null)
                 {
-                    var runtimeElementType = Create(runtimeProperty, elementType);
+                    var runtimeElementType = Create(runtimeProperty, elementType, property.IsPrimitiveCollection);
                     CreateAnnotations(
                         elementType, runtimeElementType, static (convention, annotations, source, target, runtime) =>
                             convention.ProcessElementTypeAnnotations(annotations, source, target, runtime));
@@ -398,7 +398,7 @@ private static RuntimeProperty Create(IProperty property, RuntimeTypeBase runtim
                 property.GetJsonValueReaderWriter(),
                 property.GetTypeMapping());
 
-    private static RuntimeElementType Create(RuntimeProperty runtimeProperty, IElementType element)
+    private static RuntimeElementType Create(RuntimeProperty runtimeProperty, IElementType element, bool primitiveCollection)
         => runtimeProperty.SetElementType(
                 element.ClrType,
                 element.IsNullable,
@@ -410,7 +410,8 @@ private static RuntimeElementType Create(RuntimeProperty runtimeProperty, IEleme
                 element.GetValueConverter(),
                 element.GetValueComparer(),
                 element.GetJsonValueReaderWriter(),
-                element.GetTypeMapping());
+                element.GetTypeMapping(),
+                primitiveCollection);
 
     /// <summary>
     ///     Updates the property annotations that will be set on the read-only object.
@@ -524,7 +525,7 @@ private RuntimeComplexProperty Create(IComplexProperty complexProperty, RuntimeE
             var elementType = property.GetElementType();
             if (elementType != null)
             {
-                var runtimeElementType = Create(runtimeProperty, elementType);
+                var runtimeElementType = Create(runtimeProperty, elementType, property.IsPrimitiveCollection);
                 CreateAnnotations(
                     elementType, runtimeElementType, static (convention, annotations, source, target, runtime) =>
                         convention.ProcessElementTypeAnnotations(annotations, source, target, runtime));
@@ -571,7 +572,7 @@ private RuntimeComplexProperty Create(IComplexProperty complexProperty, RuntimeC
             var elementType = property.GetElementType();
             if (elementType != null)
             {
-                var runtimeElementType = Create(runtimeProperty, elementType);
+                var runtimeElementType = Create(runtimeProperty, elementType, property.IsPrimitiveCollection);
                 CreateAnnotations(
                     elementType, runtimeElementType, static (convention, annotations, source, target, runtime) =>
                         convention.ProcessElementTypeAnnotations(annotations, source, target, runtime));

src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs

@@ -496,6 +496,7 @@ public virtual bool CanSetValueGeneratorFactory(
     {
         if (CanSetConversion(converter, configurationSource))
         {
+            Metadata.SetElementType(null, configurationSource);
             Metadata.SetProviderClrType(null, configurationSource);
             Metadata.SetValueConverter(converter, configurationSource);
 
@@ -531,6 +532,7 @@ public virtual bool CanSetConversion(
     {
         if (CanSetConversion(providerClrType, configurationSource))
         {
+            Metadata.SetElementType(null, configurationSource);
             Metadata.SetValueConverter((ValueConverter?)null, configurationSource);
             Metadata.SetProviderClrType(providerClrType, configurationSource);
 

src/EFCore/Metadata/Internal/Property.cs

@@ -1220,7 +1220,8 @@ public virtual bool IsPrimitiveCollection
         {
             var elementType = GetElementType();
             return elementType != null
-                && ClrType.TryGetElementType(typeof(IEnumerable<>))?.IsAssignableFrom(elementType!.ClrType) == true;
+                && ClrType.TryGetElementType(typeof(IEnumerable<>))?.UnwrapNullableType()
+                    .IsAssignableFrom(elementType.ClrType.UnwrapNullableType()) == true;
         }
     }
 

src/EFCore/Metadata/RuntimeProperty.cs

@@ -125,6 +125,7 @@ public RuntimeProperty(
     /// <param name="valueComparer">The <see cref="ValueComparer" /> for this property.</param>
     /// <param name="jsonValueReaderWriter">The <see cref="JsonValueReaderWriter" /> for this property.</param>
     /// <param name="typeMapping">The <see cref="CoreTypeMapping" /> for this property.</param>
+    /// <param name="primitiveCollection">A value indicating whether this property represents a primitive collection.</param>
     /// <returns>The newly created property.</returns>
     public virtual RuntimeElementType SetElementType(
         Type clrType,
@@ -137,7 +138,8 @@ public virtual RuntimeElementType SetElementType(
         ValueConverter? valueConverter = null,
         ValueComparer? valueComparer = null,
         JsonValueReaderWriter? jsonValueReaderWriter = null,
-        CoreTypeMapping? typeMapping = null)
+        CoreTypeMapping? typeMapping = null,
+        bool primitiveCollection = false)
     {
         var elementType = new RuntimeElementType(
             clrType,
@@ -155,7 +157,7 @@ public virtual RuntimeElementType SetElementType(
 
         SetAnnotation(CoreAnnotationNames.ElementType, elementType);
 
-        IsPrimitiveCollection = ClrType.TryGetElementType(typeof(IEnumerable<>))?.IsAssignableFrom(clrType) == true;
+        IsPrimitiveCollection = primitiveCollection;
 
         return elementType;
     }

src/EFCore/Properties/CoreStrings.resx

@@ -184,7 +184,7 @@
     <value>The type '{givenType}' cannot be used as a 'JsonValueReaderWriter' because it does not inherit from the generic 'JsonValueReaderWriter&lt;TValue&gt;'. Make sure to inherit json reader/writers from 'JsonValueReaderWriter&lt;TValue&gt;'.</value>
   </data>
   <data name="BadListType" xml:space="preserve">
-    <value>The type '{givenType}' cannot be used with '{comparerType}' because it does not implement '{listType}'. Collections of primitive types must be ordered lists.</value>
+    <value>The type '{givenType}' cannot be used as a primitive collection because it is not an array and does not implement '{listType}'. Collections of primitive types must be arrays or ordered lists.</value>
   </data>
   <data name="BadValueComparerType" xml:space="preserve">
     <value>The type '{givenType}' cannot be used as a value comparer because it does not inherit from '{expectedType}'. Make sure to inherit value comparers from '{expectedType}'.</value>