From 88edb550b926ecc7aa47e632499764fa8fe940f5 Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Mon, 11 Sep 2023 19:38:07 +0100 Subject: [PATCH] Separate element type from primitive collection (#31677) --- .../Builders/IConventionPropertyBuilder.cs | 4 +-- .../PropertyDiscoveryConvention.cs | 6 +++- src/EFCore/Metadata/IConventionProperty.cs | 2 +- src/EFCore/Metadata/IMutableProperty.cs | 2 +- src/EFCore/Metadata/IReadOnlyProperty.cs | 6 ++++ .../Internal/InternalPropertyBuilder.cs | 14 ++++---- .../Internal/InternalTypeBaseBuilder.cs | 11 +++++- src/EFCore/Metadata/Internal/Property.cs | 36 ++++++++++++------- src/EFCore/Metadata/RuntimeProperty.cs | 10 ++++++ .../Conventions/ConventionDispatcherTest.cs | 20 +++++------ .../Internal/ClrPropertyGetterFactoryTest.cs | 2 ++ .../Internal/ClrPropertySetterFactoryTest.cs | 2 ++ .../Metadata/Internal/PropertyTest.cs | 36 +++++++++++++++++++ 13 files changed, 116 insertions(+), 35 deletions(-) diff --git a/src/EFCore/Metadata/Builders/IConventionPropertyBuilder.cs b/src/EFCore/Metadata/Builders/IConventionPropertyBuilder.cs index cc8e81b2a92..ae3e3d683d4 100644 --- a/src/EFCore/Metadata/Builders/IConventionPropertyBuilder.cs +++ b/src/EFCore/Metadata/Builders/IConventionPropertyBuilder.cs @@ -545,7 +545,7 @@ bool CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IConventionElementTypeBuilder? SetElementType(bool elementType, bool fromDataAnnotation = false); + IConventionElementTypeBuilder? SetElementType(Type? elementType, bool fromDataAnnotation = false); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -553,5 +553,5 @@ bool CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - bool CanSetElementType(bool elementType, bool fromDataAnnotation = false); + bool CanSetElementType(Type? elementType, bool fromDataAnnotation = false); } diff --git a/src/EFCore/Metadata/Conventions/PropertyDiscoveryConvention.cs b/src/EFCore/Metadata/Conventions/PropertyDiscoveryConvention.cs index 311a5d6fdfe..fc3cd2dd6aa 100644 --- a/src/EFCore/Metadata/Conventions/PropertyDiscoveryConvention.cs +++ b/src/EFCore/Metadata/Conventions/PropertyDiscoveryConvention.cs @@ -94,7 +94,11 @@ private void Process(IConventionEntityTypeBuilder entityTypeBuilder) var propertyBuilder = entityTypeBuilder.Property(propertyInfo); if (mapping?.ElementTypeMapping != null) { - propertyBuilder?.SetElementType(true); + var elementType = propertyInfo.PropertyType.TryGetElementType(typeof(IEnumerable<>)); + if (elementType != null) + { + propertyBuilder?.SetElementType(elementType); + } } } } diff --git a/src/EFCore/Metadata/IConventionProperty.cs b/src/EFCore/Metadata/IConventionProperty.cs index 86384ce8566..39b407f9120 100644 --- a/src/EFCore/Metadata/IConventionProperty.cs +++ b/src/EFCore/Metadata/IConventionProperty.cs @@ -472,7 +472,7 @@ bool IsImplicitlyCreated() /// If , then the type mapping has an element type, otherwise it is removed. /// Indicates whether the configuration was specified using a data annotation. /// The configuration for the elements. - IConventionElementType? SetElementType(bool elementType, bool fromDataAnnotation = false); + IConventionElementType? SetElementType(Type? elementType, bool fromDataAnnotation = false); /// /// Returns the configuration source for . diff --git a/src/EFCore/Metadata/IMutableProperty.cs b/src/EFCore/Metadata/IMutableProperty.cs index 39f3b203e3c..c8e04a37669 100644 --- a/src/EFCore/Metadata/IMutableProperty.cs +++ b/src/EFCore/Metadata/IMutableProperty.cs @@ -283,7 +283,7 @@ void SetProviderValueComparer( /// Sets the configuration for elements of the primitive collection represented by this property. /// /// If , then this is a collection of primitive elements. - void SetElementType(bool elementType); + void SetElementType(Type? elementType); /// bool IReadOnlyProperty.IsNullable diff --git a/src/EFCore/Metadata/IReadOnlyProperty.cs b/src/EFCore/Metadata/IReadOnlyProperty.cs index 182f0cccbe0..533e771bf01 100644 --- a/src/EFCore/Metadata/IReadOnlyProperty.cs +++ b/src/EFCore/Metadata/IReadOnlyProperty.cs @@ -179,6 +179,12 @@ CoreTypeMapping GetTypeMapping() /// The configuration for the elements. IReadOnlyElementType? GetElementType(); + /// + /// A property is a primitive collection if it has an element type that matches the element type of the CLR type. + /// + /// if the property represents a primitive collection. + bool IsPrimitiveCollection { get; } + /// /// Finds the first principal property that the given property is constrained by /// if the given property is part of a foreign key. diff --git a/src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs b/src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs index d20f3be7fec..93dc41b9d25 100644 --- a/src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalPropertyBuilder.cs @@ -564,7 +564,7 @@ public virtual bool CanSetConversion(Type? providerClrType, ConfigurationSource? { if (CanSetConverter(converterType, configurationSource)) { - Metadata.SetElementType(false, configurationSource); + Metadata.SetElementType(null, configurationSource); Metadata.SetProviderClrType(null, configurationSource); Metadata.SetValueConverter(converterType, configurationSource); @@ -776,13 +776,13 @@ public virtual bool CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual InternalElementTypeBuilder? SetElementType(bool elementType, ConfigurationSource configurationSource) + public virtual InternalElementTypeBuilder? SetElementType(Type? elementType, ConfigurationSource configurationSource) { if (CanSetElementType(elementType, configurationSource)) { Metadata.SetElementType(elementType, configurationSource); Metadata.SetValueConverter((Type?)null, configurationSource); - return new InternalElementTypeBuilder((ElementType)Metadata.GetElementType()!, ModelBuilder); + return new InternalElementTypeBuilder(Metadata.GetElementType()!, ModelBuilder); } return null; @@ -794,9 +794,9 @@ public virtual bool CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - public virtual bool CanSetElementType(bool elementType, ConfigurationSource? configurationSource) + public virtual bool CanSetElementType(Type? elementType, ConfigurationSource? configurationSource) => configurationSource.Overrides(Metadata.GetElementTypeConfigurationSource()) - && (elementType != (Metadata.GetElementType() != null)); + && (elementType != Metadata.GetElementType()?.ClrType); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -1523,7 +1523,7 @@ bool IConventionPropertyBuilder.CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - IConventionElementTypeBuilder? IConventionPropertyBuilder.SetElementType(bool elementType, bool fromDataAnnotation) + IConventionElementTypeBuilder? IConventionPropertyBuilder.SetElementType(Type? elementType, bool fromDataAnnotation) => SetElementType(elementType, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); /// @@ -1532,6 +1532,6 @@ bool IConventionPropertyBuilder.CanSetProviderValueComparer( /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - bool IConventionPropertyBuilder.CanSetElementType(bool elementType, bool fromDataAnnotation) + bool IConventionPropertyBuilder.CanSetElementType(Type? elementType, bool fromDataAnnotation) => CanSetElementType(elementType, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); } diff --git a/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs b/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs index 9e13e940d68..6f19dd76331 100644 --- a/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalTypeBaseBuilder.cs @@ -333,7 +333,16 @@ public static bool IsCompatible(MemberInfo? newMemberInfo, PropertyBase existing { var builder = Property(propertyType, propertyName, memberInfo, typeConfigurationSource, configurationSource); - builder?.SetElementType(true, configurationSource!.Value); + if (builder != null) + { + var elementClrType = builder.Metadata.ClrType.TryGetElementType(typeof(IEnumerable<>)); + if (elementClrType == null) + { + throw new InvalidOperationException(CoreStrings.NotCollection(builder.Metadata.ClrType.ShortDisplayName(), propertyName)); + } + + builder.SetElementType(elementClrType, configurationSource!.Value); + } return builder; } diff --git a/src/EFCore/Metadata/Internal/Property.cs b/src/EFCore/Metadata/Internal/Property.cs index 73829eff60d..686c11091af 100644 --- a/src/EFCore/Metadata/Internal/Property.cs +++ b/src/EFCore/Metadata/Internal/Property.cs @@ -1208,6 +1208,22 @@ public virtual CoreTypeMapping? TypeMapping public virtual ElementType? GetElementType() => (ElementType?)this[CoreAnnotationNames.ElementType]; + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual bool IsPrimitiveCollection + { + get + { + var elementType = GetElementType(); + return elementType != null + && ClrType.TryGetElementType(typeof(IEnumerable<>))?.IsAssignableFrom(elementType!.ClrType) == true; + } + } + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in @@ -1215,25 +1231,21 @@ public virtual CoreTypeMapping? TypeMapping /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual ElementType? SetElementType( - bool elementType, + Type? elementType, ConfigurationSource configurationSource) { var existingElementType = GetElementType(); - if (existingElementType == null - && elementType) + if (elementType != null + && elementType != existingElementType?.ClrType) { - var elementClrType = ClrType.TryGetElementType(typeof(IEnumerable<>)); - if (elementClrType == null) - { - throw new InvalidOperationException(CoreStrings.NotCollection(ClrType.ShortDisplayName(), Name)); - } - var newElementType = new ElementType(elementClrType, this, configurationSource); + var newElementType = new ElementType(elementType, this, configurationSource); SetAnnotation(CoreAnnotationNames.ElementType, newElementType, configurationSource); OnElementTypeSet(newElementType, null); return newElementType; } - if (existingElementType != null && !elementType) + if (elementType == null + && existingElementType != null) { existingElementType.SetRemovedFromModel(); RemoveAnnotation(CoreAnnotationNames.ElementType); @@ -2017,7 +2029,7 @@ void IMutableProperty.SetJsonValueReaderWriterType(Type? readerWriterType) /// doing so can result in application failures when updating to a new Entity Framework Core release. /// [DebuggerStepThrough] - IConventionElementType? IConventionProperty.SetElementType(bool elementType, bool fromDataAnnotation) + IConventionElementType? IConventionProperty.SetElementType(Type? elementType, bool fromDataAnnotation) => SetElementType( elementType, fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); @@ -2029,7 +2041,7 @@ void IMutableProperty.SetJsonValueReaderWriterType(Type? readerWriterType) /// doing so can result in application failures when updating to a new Entity Framework Core release. /// [DebuggerStepThrough] - void IMutableProperty.SetElementType(bool elementType) + void IMutableProperty.SetElementType(Type? elementType) => SetElementType(elementType, ConfigurationSource.Explicit); /// diff --git a/src/EFCore/Metadata/RuntimeProperty.cs b/src/EFCore/Metadata/RuntimeProperty.cs index f2ec1db2445..e156b0f0455 100644 --- a/src/EFCore/Metadata/RuntimeProperty.cs +++ b/src/EFCore/Metadata/RuntimeProperty.cs @@ -155,6 +155,8 @@ public virtual RuntimeElementType SetElementType( SetAnnotation(CoreAnnotationNames.ElementType, elementType); + IsPrimitiveCollection = ClrType.TryGetElementType(typeof(IEnumerable<>))?.IsAssignableFrom(clrType) == true; + return elementType; } @@ -305,6 +307,14 @@ public override object? Sentinel public virtual IElementType? GetElementType() => (IElementType?)this[CoreAnnotationNames.ElementType]; + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + public virtual bool IsPrimitiveCollection { get; private set; } + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs b/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs index b2364a55d22..19a3ef87f04 100644 --- a/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs @@ -3689,12 +3689,12 @@ public void OnPropertyElementTypeChanged_calls_conventions_in_order(bool useBuil if (useBuilder) { - Assert.NotNull(propertyBuilder.SetElementType(true, ConfigurationSource.Convention)); - elementType = (ElementType)propertyBuilder.Metadata.GetElementType()!; + Assert.NotNull(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention)); + elementType = propertyBuilder.Metadata.GetElementType()!; } else { - elementType = (ElementType)propertyBuilder.Metadata.SetElementType(true, ConfigurationSource.Convention); + elementType = propertyBuilder.Metadata.SetElementType(typeof(int), ConfigurationSource.Convention); } if (useScope) @@ -3710,12 +3710,12 @@ public void OnPropertyElementTypeChanged_calls_conventions_in_order(bool useBuil if (useBuilder) { - Assert.Null(propertyBuilder.SetElementType(true, ConfigurationSource.Convention)); - elementType = (ElementType)propertyBuilder.Metadata.GetElementType()!; + Assert.Null(propertyBuilder.SetElementType(typeof(int), ConfigurationSource.Convention)); + elementType = propertyBuilder.Metadata.GetElementType()!; } else { - elementType = (ElementType)propertyBuilder.Metadata.SetElementType(true, ConfigurationSource.Convention); + elementType = propertyBuilder.Metadata.SetElementType(typeof(int), ConfigurationSource.Convention); } Assert.Equal(new (object, object)[] { (null, elementType) }, convention1.Calls); @@ -3724,11 +3724,11 @@ public void OnPropertyElementTypeChanged_calls_conventions_in_order(bool useBuil if (useBuilder) { - Assert.NotNull(propertyBuilder.SetElementType(false, ConfigurationSource.Convention)); + Assert.NotNull(propertyBuilder.SetElementType(null, ConfigurationSource.Convention)); } else { - Assert.Null(propertyBuilder.Metadata.SetElementType(false, ConfigurationSource.Convention)); + Assert.Null(propertyBuilder.Metadata.SetElementType(null, ConfigurationSource.Convention)); } Assert.Equal(new (object, object)[] { (null, elementType), (elementType, null) }, convention1.Calls); @@ -5069,7 +5069,7 @@ public void OnElementTypeAnnotationChanged_calls_conventions_in_order(bool useBu var builder = new InternalModelBuilder(new Model(conventions)); var elementTypeBuilder = builder.Entity(typeof(SpecialOrder), ConfigurationSource.Convention)! .Property(nameof(SpecialOrder.OrderIds), ConfigurationSource.Convention)! - .SetElementType(true, ConfigurationSource.Convention)!; + .SetElementType(typeof(int), ConfigurationSource.Convention)!; var scope = useScope ? builder.Metadata.ConventionDispatcher.DelayConventions() : null; @@ -5174,7 +5174,7 @@ public void OnElementTypeNullabilityChanged_calls_conventions_in_order(bool useB var builder = new InternalModelBuilder(model); var elementTypeBuilder = builder.Entity(typeof(SpecialOrder), ConfigurationSource.Convention)! .Property(nameof(SpecialOrder.Notes), ConfigurationSource.Convention)! - .SetElementType(true, ConfigurationSource.Convention)!; + .SetElementType(typeof(string), ConfigurationSource.Convention)!; if (useBuilder) { diff --git a/test/EFCore.Tests/Metadata/Internal/ClrPropertyGetterFactoryTest.cs b/test/EFCore.Tests/Metadata/Internal/ClrPropertyGetterFactoryTest.cs index 666b80f7438..69125e024cb 100644 --- a/test/EFCore.Tests/Metadata/Internal/ClrPropertyGetterFactoryTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/ClrPropertyGetterFactoryTest.cs @@ -91,6 +91,8 @@ public JsonValueReaderWriter GetJsonValueReaderWriter() IReadOnlyElementType IReadOnlyProperty.GetElementType() => GetElementType(); + public bool IsPrimitiveCollection { get; } + public IElementType GetElementType() => throw new NotImplementedException(); diff --git a/test/EFCore.Tests/Metadata/Internal/ClrPropertySetterFactoryTest.cs b/test/EFCore.Tests/Metadata/Internal/ClrPropertySetterFactoryTest.cs index b09e3299b77..ea2439a7ab8 100644 --- a/test/EFCore.Tests/Metadata/Internal/ClrPropertySetterFactoryTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/ClrPropertySetterFactoryTest.cs @@ -103,6 +103,8 @@ public JsonValueReaderWriter GetJsonValueReaderWriter() IReadOnlyElementType IReadOnlyProperty.GetElementType() => GetElementType(); + public bool IsPrimitiveCollection { get; } + public IElementType GetElementType() => throw new NotImplementedException(); diff --git a/test/EFCore.Tests/Metadata/Internal/PropertyTest.cs b/test/EFCore.Tests/Metadata/Internal/PropertyTest.cs index 0f1c2fc2ebc..4c3cb76eeee 100644 --- a/test/EFCore.Tests/Metadata/Internal/PropertyTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/PropertyTest.cs @@ -531,6 +531,42 @@ public void Throws_when_JsonValueReaderWriter_instance_cannot_be_created(Type ty () => property.GetJsonValueReaderWriter()).Message); } + [ConditionalFact] + public void Can_set_element_type_for_primitive_collection() + { + var model = CreateModel(); + var entityType = model.AddEntityType(typeof(object)); + var property = entityType.AddProperty("Random", typeof(IList)); + property.SetElementType(typeof(int)); + + Assert.Equal(typeof(int), property.GetElementType()!.ClrType); + Assert.True(property.IsPrimitiveCollection); + } + + [ConditionalFact] + public void Can_set_derived_element_type_for_primitive_collection() + { + var model = CreateModel(); + var entityType = model.AddEntityType(typeof(object)); + var property = entityType.AddProperty("Random", typeof(IList)); + property.SetElementType(typeof(int)); + + Assert.Equal(typeof(int), property.GetElementType()!.ClrType); + Assert.True(property.IsPrimitiveCollection); + } + + [ConditionalFact] + public void Can_set_element_type_for_non_primitive_collection() + { + var model = CreateModel(); + var entityType = model.AddEntityType(typeof(object)); + var property = entityType.AddProperty("Random", typeof(Random)); + property.SetElementType(typeof(int)); + + Assert.Equal(typeof(int), property.GetElementType()!.ClrType); + Assert.False(property.IsPrimitiveCollection); + } + private class SimpleJasonValueReaderWriter : JsonValueReaderWriter { public override string FromJsonTyped(ref Utf8JsonReaderManager manager, object existingObject = null)