Skip to content

Commit 7ee1aa2

Browse files
AndriySvyrydAndriySvyryd
authored
Check value converter configuration source when setting element type. (#32612)
Fixes #32430
1 parent 2f9a85f commit 7ee1aa2

File tree

6 files changed

+77
-39
lines changed

6 files changed

+77
-39
lines changed

‎src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,9 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Internal;
1414
public class InternalPropertyBuilder
1515
: InternalPropertyBaseBuilder<IConventionPropertyBuilder, Property>, IConventionPropertyBuilder
1616
{
17+
internal static readonly bool UseOldBehavior32430 =
18+
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue32430", out var enabled32430) && enabled32430;
19+
1720
/// <summary>
1821
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
1922
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
@@ -496,7 +499,10 @@ public virtual bool CanSetValueGeneratorFactory(
496499
{
497500
if (CanSetConversion(converter, configurationSource))
498501
{
499-
Metadata.SetElementType(null, configurationSource);
502+
if (converter != null || UseOldBehavior32430)
503+
{
504+
Metadata.SetElementType(null, configurationSource);
505+
}
500506
Metadata.SetProviderClrType(null, configurationSource);
501507
Metadata.SetValueConverter(converter, configurationSource);
502508

@@ -520,7 +526,8 @@ public virtual bool CanSetConversion(
520526
&& Metadata.CheckValueConverter(converter) == null)
521527
|| (Metadata[CoreAnnotationNames.ValueConverterType] == null
522528
&& (ValueConverter?)Metadata[CoreAnnotationNames.ValueConverter] == converter))
523-
&& configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource());
529+
&& configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource())
530+
&& (converter == null || CanSetElementType(null, configurationSource) || UseOldBehavior32430);
524531

525532
/// <summary>
526533
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -532,7 +539,10 @@ public virtual bool CanSetConversion(
532539
{
533540
if (CanSetConversion(providerClrType, configurationSource))
534541
{
535-
Metadata.SetElementType(null, configurationSource);
542+
if (providerClrType != null || UseOldBehavior32430)
543+
{
544+
Metadata.SetElementType(null, configurationSource);
545+
}
536546
Metadata.SetValueConverter((ValueConverter?)null, configurationSource);
537547
Metadata.SetProviderClrType(providerClrType, configurationSource);
538548

@@ -551,7 +561,8 @@ public virtual bool CanSetConversion(
551561
public virtual bool CanSetConversion(Type? providerClrType, ConfigurationSource? configurationSource)
552562
=> (configurationSource.Overrides(Metadata.GetProviderClrTypeConfigurationSource())
553563
|| Metadata.GetProviderClrType() == providerClrType)
554-
&& configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource());
564+
&& configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
565+
&& (providerClrType == null || CanSetElementType(null, configurationSource) || UseOldBehavior32430);
555566

556567
/// <summary>
557568
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -566,7 +577,10 @@ public virtual bool CanSetConversion(Type? providerClrType, ConfigurationSource?
566577
{
567578
if (CanSetConverter(converterType, configurationSource))
568579
{
569-
Metadata.SetElementType(null, configurationSource);
580+
if (converterType != null || UseOldBehavior32430)
581+
{
582+
Metadata.SetElementType(null, configurationSource);
583+
}
570584
Metadata.SetProviderClrType(null, configurationSource);
571585
Metadata.SetValueConverter(converterType, configurationSource);
572586

@@ -586,9 +600,10 @@ public virtual bool CanSetConverter(
586600
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
587601
Type? converterType,
588602
ConfigurationSource? configurationSource)
589-
=> configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
603+
=> (configurationSource.Overrides(Metadata.GetValueConverterConfigurationSource())
590604
|| (Metadata[CoreAnnotationNames.ValueConverter] == null
591-
&& (Type?)Metadata[CoreAnnotationNames.ValueConverterType] == converterType);
605+
&& (Type?)Metadata[CoreAnnotationNames.ValueConverterType] == converterType))
606+
&& (converterType == null || CanSetElementType(null, configurationSource) || UseOldBehavior32430);
592607

593608
/// <summary>
594609
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -783,7 +798,10 @@ public virtual bool CanSetProviderValueComparer(
783798
if (CanSetElementType(elementType, configurationSource))
784799
{
785800
Metadata.SetElementType(elementType, configurationSource);
786-
Metadata.SetValueConverter((Type?)null, configurationSource);
801+
if (elementType != null || UseOldBehavior32430)
802+
{
803+
Metadata.SetValueConverter((Type?)null, configurationSource);
804+
}
787805
return new InternalElementTypeBuilder(Metadata.GetElementType()!, ModelBuilder);
788806
}
789807

@@ -797,8 +815,9 @@ public virtual bool CanSetProviderValueComparer(
797815
/// doing so can result in application failures when updating to a new Entity Framework Core release.
798816
/// </summary>
799817
public virtual bool CanSetElementType(Type? elementType, ConfigurationSource? configurationSource)
800-
=> configurationSource.Overrides(Metadata.GetElementTypeConfigurationSource())
801-
&& (elementType != Metadata.GetElementType()?.ClrType);
818+
=> (configurationSource.Overrides(Metadata.GetElementTypeConfigurationSource())
819+
&& (elementType == null || CanSetConversion((Type?)null, configurationSource) || UseOldBehavior32430))
820+
|| elementType == Metadata.GetElementType()?.ClrType;
802821

803822
/// <summary>
804823
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to

‎test/EFCore.Relational.Specification.Tests/Query/JsonQueryAdHocTestBase.cs‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ public virtual async Task Project_json_array_of_primitives_on_reference(bool asy
306306
}
307307
}
308308

309-
[ConditionalTheory]
309+
[ConditionalTheory(Skip = "Issue #32611")]
310310
[MemberData(nameof(IsAsyncData))]
311311
public virtual async Task Project_json_array_of_primitives_on_collection(bool async)
312312
{
@@ -426,33 +426,10 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
426426
{
427427
modelBuilder.Entity<MyEntityArrayOfPrimitives>().Property(x => x.Id).ValueGeneratedNever();
428428
modelBuilder.Entity<MyEntityArrayOfPrimitives>().OwnsOne(
429-
x => x.Reference, b =>
430-
{
431-
b.ToJson();
432-
b.Property(x => x.IntArray).HasConversion(
433-
x => string.Join(" ", x),
434-
x => x.Split(" ", StringSplitOptions.None).Select(v => int.Parse(v)).ToArray(),
435-
new ValueComparer<int[]>(true));
436-
437-
b.Property(x => x.ListOfString).HasConversion(
438-
x => string.Join(" ", x),
439-
x => x.Split(" ", StringSplitOptions.None).ToList(),
440-
new ValueComparer<List<string>>(true));
441-
});
429+
x => x.Reference, b => b.ToJson());
442430

443431
modelBuilder.Entity<MyEntityArrayOfPrimitives>().OwnsMany(
444-
x => x.Collection, b =>
445-
{
446-
b.ToJson();
447-
b.Property(x => x.IntArray).HasConversion(
448-
x => string.Join(" ", x),
449-
x => x.Split(" ", StringSplitOptions.None).Select(v => int.Parse(v)).ToArray(),
450-
new ValueComparer<int[]>(true));
451-
b.Property(x => x.ListOfString).HasConversion(
452-
x => string.Join(" ", x),
453-
x => x.Split(" ", StringSplitOptions.None).ToList(),
454-
new ValueComparer<List<string>>(true));
455-
});
432+
x => x.Collection, b => b.ToJson());
456433
}
457434
}
458435

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
88

99
public class ConventionDispatcherTest
1010
{
11-
// TODO: Use public API to add conventions, issue #214
12-
1311
[ConditionalFact]
1412
public void Infinite_recursion_throws()
1513
{
@@ -3710,7 +3708,7 @@ public void OnPropertyElementTypeChanged_calls_conventions_in_order(bool useBuil
37103708

37113709
if (useBuilder)
37123710
{
3713-
Assert.Null(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention));
3711+
Assert.NotNull(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention));
37143712
elementType = propertyBuilder.Metadata.GetElementType()!;
37153713
}
37163714
else

‎test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3255,6 +3255,45 @@ public virtual void Element_types_can_have_unicode_set()
32553255
Assert.False(entityType.FindProperty("Stranger")!.GetElementType()!.IsUnicode());
32563256
}
32573257

3258+
[ConditionalFact]
3259+
public virtual void Conversion_on_base_property_prevents_primitive_collection()
3260+
{
3261+
var modelBuilder = CreateModelBuilder();
3262+
modelBuilder.Entity<DerivedCollectionQuarks>();
3263+
modelBuilder.Entity<CollectionQuarks>(b =>
3264+
{
3265+
b.Property(c => c.Down).HasConversion(gs => string.Join(',', gs!),
3266+
s => new ObservableCollection<string>(s.Split(',', StringSplitOptions.RemoveEmptyEntries)));
3267+
});
3268+
3269+
var model = modelBuilder.FinalizeModel();
3270+
3271+
var property = model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
3272+
Assert.False(property.IsPrimitiveCollection);
3273+
Assert.NotNull(property.GetValueConverter());
3274+
}
3275+
3276+
[ConditionalFact]
3277+
public virtual void Conversion_on_base_property_prevents_primitive_collection_when_base_first()
3278+
{
3279+
var modelBuilder = CreateModelBuilder();
3280+
modelBuilder.Entity<CollectionQuarks>(b =>
3281+
{
3282+
b.Property(c => c.Down).HasConversion(gs => string.Join(',', gs!),
3283+
s => new ObservableCollection<string>(s.Split(',', StringSplitOptions.RemoveEmptyEntries)));
3284+
});
3285+
3286+
var property = (IProperty)modelBuilder.Model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
3287+
Assert.False(property.IsPrimitiveCollection);
3288+
3289+
modelBuilder.Entity<DerivedCollectionQuarks>();
3290+
3291+
var model = modelBuilder.FinalizeModel();
3292+
property = model.FindEntityType(typeof(CollectionQuarks))!.FindProperty(nameof(CollectionQuarks.Down))!;
3293+
Assert.False(property.IsPrimitiveCollection);
3294+
Assert.NotNull(property.GetValueConverter());
3295+
}
3296+
32583297
[ConditionalFact]
32593298
public virtual void Element_types_can_have_provider_type_set()
32603299
{

‎test/EFCore.Tests/ModelBuilding/OwnedTypesTestBase.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1652,6 +1652,7 @@ public virtual void Can_configure_owned_entity_and_property_of_same_type()
16521652

16531653
var departmentIdProperty = departmentType.FindProperty(nameof(Department.Id));
16541654
Assert.NotNull(departmentIdProperty);
1655+
Assert.NotNull(departmentIdProperty.GetValueConverter());
16551656
Assert.NotNull(departmentNestedType);
16561657
Assert.NotNull(officeNestedType);
16571658

‎test/EFCore.Tests/ModelBuilding/TestModel.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,10 @@ public ObservableCollection<string>? Down
340340
#pragma warning restore 67
341341
}
342342

343+
protected class DerivedCollectionQuarks : CollectionQuarks
344+
{
345+
}
346+
343347
protected class Hob
344348
{
345349
public string? Id1 { get; set; }

0 commit comments

Comments
 (0)