diff --git a/src/EFCore/Diagnostics/CoreEventId.cs b/src/EFCore/Diagnostics/CoreEventId.cs index 93dc34fb6dc..f99ba5f3eb3 100644 --- a/src/EFCore/Diagnostics/CoreEventId.cs +++ b/src/EFCore/Diagnostics/CoreEventId.cs @@ -101,6 +101,7 @@ private enum Id NonNullableReferenceOnDependent, RequiredAttributeInverted, RequiredAttributeOnCollection, + CollectionWithoutComparer, // ChangeTracking events DetectChangesStarting = CoreBaseId + 800, @@ -884,5 +885,18 @@ public static readonly EventId PossibleUnintendedReferenceComparisonWarning /// /// public static readonly EventId ContextDisposed = MakeInfraId(Id.ContextDisposed); + + /// + /// + /// A property has a collection or enumeration type with a value converter but with no value comparer. + /// + /// + /// This event is in the category. + /// + /// + /// This event uses the payload when used with a . + /// + /// + public static readonly EventId CollectionWithoutComparer = MakeModelValidationId(Id.CollectionWithoutComparer); } } diff --git a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs index 190089b4ae1..2a678c82669 100644 --- a/src/EFCore/Diagnostics/CoreLoggerExtensions.cs +++ b/src/EFCore/Diagnostics/CoreLoggerExtensions.cs @@ -982,6 +982,44 @@ private static string ShadowPropertyCreated(EventDefinitionBase definition, Even return d.GenerateMessage(p.Property.Name, p.Property.DeclaringEntityType.DisplayName()); } + /// + /// Logs for the event. + /// + /// The diagnostics logger to use. + /// The property. + public static void CollectionWithoutComparer( + [NotNull] this IDiagnosticsLogger diagnostics, + [NotNull] IProperty property) + { + var definition = CoreResources.LogCollectionWithoutComparer(diagnostics); + + var warningBehavior = definition.GetLogBehavior(diagnostics); + if (warningBehavior != WarningBehavior.Ignore) + { + definition.Log( + diagnostics, + warningBehavior, + property.Name, property.DeclaringEntityType.DisplayName()); + } + + if (diagnostics.DiagnosticSource.IsEnabled(definition.EventId.Name)) + { + diagnostics.DiagnosticSource.Write( + definition.EventId.Name, + new PropertyEventData( + definition, + CollectionWithoutComparer, + property)); + } + } + + private static string CollectionWithoutComparer(EventDefinitionBase definition, EventData payload) + { + var d = (EventDefinition)definition; + var p = (PropertyEventData)payload; + return d.GenerateMessage(p.Property.Name, p.Property.DeclaringEntityType.DisplayName()); + } + /// /// Logs for the event. /// diff --git a/src/EFCore/Diagnostics/LoggingDefinitions.cs b/src/EFCore/Diagnostics/LoggingDefinitions.cs index 7447ffdf3f5..f0a22c0d983 100644 --- a/src/EFCore/Diagnostics/LoggingDefinitions.cs +++ b/src/EFCore/Diagnostics/LoggingDefinitions.cs @@ -394,6 +394,15 @@ public abstract class LoggingDefinitions [EntityFrameworkInternal] public EventDefinitionBase LogShadowPropertyCreated; + /// + /// 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. + /// + [EntityFrameworkInternal] + public EventDefinitionBase LogCollectionWithoutComparer; + /// /// 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/src/EFCore/Infrastructure/ModelValidator.cs b/src/EFCore/Infrastructure/ModelValidator.cs index 517bfe77eb4..e1f87296c31 100644 --- a/src/EFCore/Infrastructure/ModelValidator.cs +++ b/src/EFCore/Infrastructure/ModelValidator.cs @@ -74,6 +74,7 @@ public virtual void Validate(IModel model, IDiagnosticsLogger + /// Validates the type mapping of properties the model. + /// + /// The model to validate. + /// The logger to use. + protected virtual void ValidateTypeMappings( + [NotNull] IModel model, [NotNull] IDiagnosticsLogger logger) + { + Check.NotNull(model, nameof(model)); + Check.NotNull(logger, nameof(logger)); + + foreach (var entityType in model.GetEntityTypes()) + { + foreach (var property in entityType.GetDeclaredProperties()) + { + var converter = property.GetValueConverter(); + if (converter != null + && property.GetValueComparer() == null) + { + var type = converter.ModelClrType; + if (type != typeof(string) + && !(type == typeof(byte[]) && property.IsKey()) // Already special-cased elsewhere + && type.TryGetSequenceType() != null) + { + logger.CollectionWithoutComparer(property); + } + } + } + } + } + /// /// Validates the mapping/configuration of entity types without keys in the model. /// diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index c1c3c350391..4b58b9987f3 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -3294,6 +3294,30 @@ public static EventDefinition LogShadowPropertyCreated([NotNull] return (EventDefinition)definition; } + /// + /// The property '{property}' on entity type '{entityType}' is a collection or enumeration type with a value converter but with no value comparer. Set a value comparer to ensure the collection/enumeration elements are compared correctly. + /// + public static EventDefinition LogCollectionWithoutComparer([NotNull] IDiagnosticsLogger logger) + { + var definition = ((LoggingDefinitions)logger.Definitions).LogCollectionWithoutComparer; + if (definition == null) + { + definition = LazyInitializer.EnsureInitialized( + ref ((LoggingDefinitions)logger.Definitions).LogCollectionWithoutComparer, + () => new EventDefinition( + logger.Options, + CoreEventId.CollectionWithoutComparer, + LogLevel.Warning, + "CoreEventId.CollectionWithoutComparer", + level => LoggerMessage.Define( + level, + CoreEventId.CollectionWithoutComparer, + _resourceManager.GetString("LogCollectionWithoutComparer")))); + } + + return (EventDefinition)definition; + } + /// /// A transient exception has been encountered during execution and the operation will be retried after {delay}ms.{newline}{error} /// diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index 1ecba898b67..01ca87b509e 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1,17 +1,17 @@  - @@ -979,6 +979,10 @@ The property '{property}' on entity type '{entityType}' was created in shadow state because there are no eligible CLR members with a matching name. Debug CoreEventId.ShadowPropertyCreated string string + + The property '{property}' on entity type '{entityType}' is a collection or enumeration type with a value converter but with no value comparer. Set a value comparer to ensure the collection/enumeration elements are compared correctly. + Warning CoreEventId.CollectionWithoutComparer string string + A transient exception has been encountered during execution and the operation will be retried after {delay}ms.{newline}{error} Information CoreEventId.ExecutionStrategyRetrying int string Exception diff --git a/test/EFCore.Specification.Tests/CustomConvertersTestBase.cs b/test/EFCore.Specification.Tests/CustomConvertersTestBase.cs index 5977b003258..e991d777cb9 100644 --- a/test/EFCore.Specification.Tests/CustomConvertersTestBase.cs +++ b/test/EFCore.Specification.Tests/CustomConvertersTestBase.cs @@ -828,6 +828,13 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con .HasConversion( BytesToStringConverter.DefaultInfo.Create()) .HasMaxLength(LongStringLength * 2); + + var bytesComparer = new ValueComparer( + (v1, v2) => v1.SequenceEqual(v2), + v => v.GetHashCode()); + + b.Property(e => e.ByteArray5).Metadata.SetValueComparer(bytesComparer); + b.Property(e => e.ByteArray9000).Metadata.SetValueComparer(bytesComparer); }); modelBuilder.Entity( @@ -835,6 +842,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con { b.Property(e => e.Strings).HasConversion(v => string.Join(",", v), v => v.Split(new[] { ',' }).ToList()); b.Property(e => e.Id).ValueGeneratedNever(); + + var comparer = new ValueComparer>( + (v1, v2) => v1.SequenceEqual(v2), + v => v.GetHashCode()); + + b.Property(e => e.Strings).Metadata.SetValueComparer(comparer); }); modelBuilder.Entity( @@ -852,6 +865,12 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con b.Property(c => c.Discriminator).HasConversion( d => StringToDictionarySerializer.Serialize(d), json => StringToDictionarySerializer.Deserialize(json)); + + var comparer = new ValueComparer>( + (v1, v2) => v1.SequenceEqual(v2), + v => v.GetHashCode()); + + b.Property(e => e.Discriminator).Metadata.SetValueComparer(comparer); }); var urlConverter = new UrlSchemeRemover(); diff --git a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs index 6041aa977e1..6d61910a606 100644 --- a/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs +++ b/test/EFCore.Tests/Infrastructure/ModelValidatorTest.cs @@ -6,9 +6,11 @@ using System.ComponentModel; using System.Linq; using System.Linq.Expressions; +using Microsoft.EntityFrameworkCore.ChangeTracking; using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Diagnostics.Internal; using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Storage.ValueConversion; using Microsoft.EntityFrameworkCore.TestUtilities; using Microsoft.Extensions.Logging; using Xunit; @@ -19,6 +21,84 @@ namespace Microsoft.EntityFrameworkCore.Infrastructure { public class ModelValidatorTest : ModelValidatorTestBase { + [ConditionalFact] + public virtual void Detects_custom_converter_for_collection_type_without_comparer() + { + var convertedProperty = CreateConvertedCollectionProperty(); + + VerifyWarning( + CoreResources.LogCollectionWithoutComparer( + new TestLogger()).GenerateMessage("SomeStrings", "WithCollectionConversion"), + convertedProperty.DeclaringEntityType.Model); + } + + [ConditionalFact] + public virtual void Ignores_custom_converter_for_collection_type_with_comparer() + { + var convertedProperty = CreateConvertedCollectionProperty(); + + convertedProperty.SetValueComparer( + new ValueComparer( + (v1, v2) => v1.SequenceEqual(v2), + v => v.GetHashCode())); + + Validate(convertedProperty.DeclaringEntityType.Model); + + Assert.Empty(LoggerFactory.Log.Where(l => l.Level == LogLevel.Warning)); + } + + private IMutableProperty CreateConvertedCollectionProperty() + { + var model = CreateConventionlessModelBuilder().Model; + + var entityType = model.AddEntityType(typeof(WithCollectionConversion)); + entityType.SetPrimaryKey(entityType.AddProperty(nameof(WithCollectionConversion.Id), typeof(int))); + + var convertedProperty = entityType.AddProperty( + nameof(WithCollectionConversion.SomeStrings), typeof(string[])); + + convertedProperty.SetValueConverter( + new ValueConverter( + v => string.Join(',', v), + v => v.Split(',', StringSplitOptions.None))); + + return convertedProperty; + } + + private class WithCollectionConversion + { + public int Id { get; set; } + public string[] SomeStrings { get; set; } + } + + [ConditionalFact] + public virtual void Ignores_binary_keys_and_strings_without_custom__comparer() + { + var model = CreateConventionlessModelBuilder().Model; + + var entityType = model.AddEntityType(typeof(WithStringAndBinaryKey)); + + var keyProperty = entityType.AddProperty(nameof(WithStringAndBinaryKey.Id), typeof(byte[])); + keyProperty.IsNullable = false; + entityType.SetPrimaryKey(keyProperty); + keyProperty.SetValueConverter( + new ValueConverter(v => v, v => v)); + + var stringProperty = entityType.AddProperty(nameof(WithStringAndBinaryKey.AString), typeof(string)); + stringProperty.SetValueConverter( + new ValueConverter(v => v, v => v)); + + Validate(model); + + Assert.Empty(LoggerFactory.Log.Where(l => l.Level == LogLevel.Warning)); + } + + private class WithStringAndBinaryKey + { + public byte[] Id { get; set; } + public string AString { get; set; } + } + [ConditionalFact] public virtual void Detects_filter_on_derived_type() { diff --git a/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs b/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs index a86136793cd..24549e9aa2a 100644 --- a/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs +++ b/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs @@ -814,8 +814,17 @@ public virtual void IEnumerable_properties_with_value_converter_set_are_not_disc var model = modelBuilder.Model; modelBuilder.Entity( - b => b.Property(e => e.ExpandoObject).HasConversion( - v => (string)((IDictionary)v)["Value"], v => DeserializeExpandoObject(v))); + b => + { + b.Property(e => e.ExpandoObject).HasConversion( + v => (string)((IDictionary)v)["Value"], v => DeserializeExpandoObject(v)); + + var comparer = new ValueComparer( + (v1, v2) => v1.SequenceEqual(v2), + v => v.GetHashCode()); + + b.Property(e => e.ExpandoObject).Metadata.SetValueComparer(comparer); + }); modelBuilder.FinalizeModel();