Check value converter configuration source when setting element type (#…

…32600)

Fixes #32430

src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System;
 using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.Reflection;
@@ -522,7 +523,10 @@ public virtual bool CanSetValueGeneratorFactory(
     {
         if (CanSetConversion(converter, configurationSource))
         {
-            Metadata.SetElementType(null, configurationSource);
+            if (converter != null)
+            {
+                Metadata.SetElementType(null, configurationSource);
+            }
             Metadata.SetProviderClrType(null, configurationSource);
             Metadata.SetValueConverter(converter, configurationSource);
 
@@ -546,7 +550,8 @@ public virtual bool CanSetConversion(
                     && Metadata.CheckValueConverter(converter) == null)
                 || (Metadata[CoreAnnotationNames.ValueConverterType] == null
                     && (ValueConverter?)Metadata[CoreAnnotationNames.ValueConverter] == converter))
-            && configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource());
+            && configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource())
+            && (converter == null || CanSetElementType(null, configurationSource));
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -558,7 +563,10 @@ public virtual bool CanSetConversion(
     {
         if (CanSetConversion(providerClrType, configurationSource))
         {
-            Metadata.SetElementType(null, configurationSource);
+            if (providerClrType != null)
+            {
+                Metadata.SetElementType(null, configurationSource);
+            }
             Metadata.SetValueConverter((ValueConverter?)null, configurationSource);
             Metadata.SetProviderClrType(providerClrType, configurationSource);
 
@@ -577,7 +585,8 @@ public virtual bool CanSetConversion(
     public virtual bool CanSetConversion(Type? providerClrType, ConfigurationSource? configurationSource)
         => (configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource())
                 || Metadata.GetProviderClrType() == providerClrType)
-            && configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource());
+            && configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
+            && (providerClrType == null || CanSetElementType(null, configurationSource));
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -592,7 +601,10 @@ public virtual bool CanSetConversion(Type? providerClrType, ConfigurationSource?
     {
         if (CanSetConverter(converterType, configurationSource))
         {
-            Metadata.SetElementType(null, configurationSource);
+            if (converterType != null)
+            {
+                Metadata.SetElementType(null, configurationSource);
+            }
             Metadata.SetProviderClrType(null, configurationSource);
             Metadata.SetValueConverter(converterType, configurationSource);
 
@@ -612,9 +624,10 @@ public virtual bool CanSetConverter(
         [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
         Type? converterType,
         ConfigurationSource? configurationSource)
-        => configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
+        => (configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
             || (Metadata[CoreAnnotationNames.ValueConverter] == null
-                && (Type?)Metadata[CoreAnnotationNames.ValueConverterType] == converterType);
+                && (Type?)Metadata[CoreAnnotationNames.ValueConverterType] == converterType))
+            && (converterType == null || CanSetElementType(null, configurationSource));
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -809,7 +822,10 @@ public virtual bool CanSetProviderValueComparer(
         if (CanSetElementType(elementType, configurationSource))
         {
             Metadata.SetElementType(elementType, configurationSource);
-            Metadata.SetValueConverter((Type?)null, configurationSource);
+            if (elementType != null)
+            {
+                Metadata.SetValueConverter((Type?)null, configurationSource);
+            }
             return new InternalElementTypeBuilder(Metadata.GetElementType()!, ModelBuilder);
         }
 
@@ -823,8 +839,9 @@ public virtual bool CanSetProviderValueComparer(
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual bool CanSetElementType(Type? elementType, ConfigurationSource? configurationSource)
-        => configurationSource.Overrides(Metadata.GetElementTypeConfigurationSource())
-            && (elementType != Metadata.GetElementType()?.ClrType);
+        => (configurationSource.Overrides(Metadata.GetElementTypeConfigurationSource())
+            && (elementType == null || CanSetConversion((Type?)null, configurationSource)))
+            || elementType == Metadata.GetElementType()?.ClrType;
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

test/EFCore.Relational.Specification.Tests/Query/AdHocJsonQueryTestBase.cs

@@ -306,7 +306,7 @@ public virtual async Task Project_json_array_of_primitives_on_reference(bool asy
         }
     }
 
-    [ConditionalTheory]
+    [ConditionalTheory(Skip = "Issue #32611")]
     [MemberData(nameof(IsAsyncData))]
     public virtual async Task Project_json_array_of_primitives_on_collection(bool async)
     {
@@ -426,33 +426,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
         {
             modelBuilder.Entity<MyEntityArrayOfPrimitives>().Property(x => x.Id).ValueGeneratedNever();
             modelBuilder.Entity<MyEntityArrayOfPrimitives>().OwnsOne(
-                x => x.Reference, b =>
-                {
-                    b.ToJson();
-                    b.Property(x => x.IntArray).HasConversion(
-                        x => string.Join(" ", x),
-                        x => x.Split(" ", StringSplitOptions.None).Select(v => int.Parse(v)).ToArray(),
-                        new ValueComparer<int[]>(true));
-
-                    b.Property(x => x.ListOfString).HasConversion(
-                        x => string.Join(" ", x),
-                        x => x.Split(" ", StringSplitOptions.None).ToList(),
-                        new ValueComparer<List<string>>(true));
-                });
+                x => x.Reference, b => b.ToJson());
 
             modelBuilder.Entity<MyEntityArrayOfPrimitives>().OwnsMany(
-                x => x.Collection, b =>
-                {
-                    b.ToJson();
-                    b.Property(x => x.IntArray).HasConversion(
-                        x => string.Join(" ", x),
-                        x => x.Split(" ", StringSplitOptions.None).Select(v => int.Parse(v)).ToArray(),
-                        new ValueComparer<int[]>(true));
-                    b.Property(x => x.ListOfString).HasConversion(
-                        x => string.Join(" ", x),
-                        x => x.Split(" ", StringSplitOptions.None).ToList(),
-                        new ValueComparer<List<string>>(true));
-                });
+                x => x.Collection, b => b.ToJson());
         }
     }
 

test/EFCore.Specification.Tests/ModelBuilding/ModelBuilderTest.NonRelationship.cs

@@ -3284,6 +3284,45 @@ public virtual void Element_types_can_have_unicode_set()
             Assert.False(entityType.FindProperty("Stranger")!.GetElementType()!.IsUnicode());
         }
 
+        [ConditionalFact]
+        public virtual void Conversion_on_base_property_prevents_primitive_collection()
+        {
+            var modelBuilder = CreateModelBuilder();
+            modelBuilder.Entity<DerivedCollectionQuarks>();
+            modelBuilder.Entity<CollectionQuarks>(b =>
+            {
+                b.Property(c => c.Down).HasConversion(gs => string.Join(',', gs!),
+                    s => new ObservableCollection<string>(s.Split(',', StringSplitOptions.RemoveEmptyEntries)));
+            });
+
+            var model = modelBuilder.FinalizeModel();
+
+            var property = model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
+            Assert.False(property.IsPrimitiveCollection);
+            Assert.NotNull(property.GetValueConverter());
+        }
+
+        [ConditionalFact]
+        public virtual void Conversion_on_base_property_prevents_primitive_collection_when_base_first()
+        {
+            var modelBuilder = CreateModelBuilder();
+            modelBuilder.Entity<CollectionQuarks>(b =>
+            {
+                b.Property(c => c.Down).HasConversion(gs => string.Join(',', gs!),
+                    s => new ObservableCollection<string>(s.Split(',', StringSplitOptions.RemoveEmptyEntries)));
+            });
+
+            var property = (IProperty)modelBuilder.Model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
+            Assert.False(property.IsPrimitiveCollection);
+
+            modelBuilder.Entity<DerivedCollectionQuarks>();
+
+            var model = modelBuilder.FinalizeModel();
+            property = model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
+            Assert.False(property.IsPrimitiveCollection);
+            Assert.NotNull(property.GetValueConverter());
+        }
+
         [ConditionalFact]
         public virtual void Element_types_can_have_provider_type_set()
         {

test/EFCore.Specification.Tests/ModelBuilding/ModelBuilderTest.OwnedTypes.cs

@@ -1659,6 +1659,7 @@ public virtual void Can_configure_owned_entity_and_property_of_same_type()
 
             var departmentIdProperty = departmentType.FindProperty(nameof(Department.Id));
             Assert.NotNull(departmentIdProperty);
+            Assert.NotNull(departmentIdProperty.GetValueConverter());
             Assert.NotNull(departmentNestedType);
             Assert.NotNull(officeNestedType);
 

test/EFCore.Specification.Tests/ModelBuilding/ModelBuilderTest.TestModel.cs

@@ -340,6 +340,10 @@ public ObservableCollection<string>? Down
 #pragma warning restore 67
     }
 
+    protected class DerivedCollectionQuarks : CollectionQuarks
+    {
+    }
+
     protected class Hob
     {
         public string? Id1 { get; set; }

test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs

@@ -8,8 +8,6 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
 
 public class ConventionDispatcherTest
 {
-    // TODO: Use public API to add conventions, issue #214
-
     [ConditionalFact]
     public void Infinite_recursion_throws()
     {
@@ -3930,7 +3928,7 @@ public void OnPropertyElementTypeChanged_calls_conventions_in_order(bool useBuil
 
         if (useBuilder)
         {
-            Assert.Null(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention));
+            Assert.NotNull(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention));
             elementType = propertyBuilder.Metadata.GetElementType()!;
         }
         else