Warn when using a value converter on a collection/enumeration type wi…

…thout a comparer

Fixes #18600

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
         ///     </para>
         /// </summary>
         public static readonly EventId ContextDisposed = MakeInfraId(Id.ContextDisposed);
+
+        /// <summary>
+        ///     <para>
+        ///         A property has a collection or enumeration type with a value converter but with no value comparer.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="PropertyEventData" /> payload when used with a <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId CollectionWithoutComparer = MakeModelValidationId(Id.CollectionWithoutComparer);
     }
 }

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());
         }
 
+        /// <summary>
+        ///     Logs for the <see cref="CoreEventId.CollectionWithoutComparer" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="property"> The property. </param>
+        public static void CollectionWithoutComparer(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> 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<string, string>)definition;
+            var p = (PropertyEventData)payload;
+            return d.GenerateMessage(p.Property.Name, p.Property.DeclaringEntityType.DisplayName());
+        }
+
         /// <summary>
         ///     Logs for the <see cref="CoreEventId.RedundantIndexRemoved" /> event.
         /// </summary>

src/EFCore/Diagnostics/LoggingDefinitions.cs

@@ -394,6 +394,15 @@ public abstract class LoggingDefinitions
         [EntityFrameworkInternal]
         public EventDefinitionBase LogShadowPropertyCreated;
 
+        /// <summary>
+        ///     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.
+        /// </summary>
+        [EntityFrameworkInternal]
+        public EventDefinitionBase LogCollectionWithoutComparer;
+
         /// <summary>
         ///     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

src/EFCore/Infrastructure/ModelValidator.cs

@@ -74,6 +74,7 @@ public virtual void Validate(IModel model, IDiagnosticsLogger<DbLoggerCategory.M
             ValidateQueryFilters(model, logger);
             ValidateDefiningQuery(model, logger);
             ValidateData(model, logger);
+            ValidateTypeMappings(model, logger);
             LogShadowProperties(model, logger);
         }
 
@@ -808,6 +809,37 @@ protected virtual void ValidateFieldMapping(
             }
         }
 
+        /// <summary>
+        ///     Validates the type mapping of properties the model.
+        /// </summary>
+        /// <param name="model"> The model to validate. </param>
+        /// <param name="logger"> The logger to use. </param>
+        protected virtual void ValidateTypeMappings(
+            [NotNull] IModel model, [NotNull] IDiagnosticsLogger<DbLoggerCategory.Model.Validation> 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);
+                        }
+                    }
+                }
+            }
+        }
+
         /// <summary>
         ///     Validates the mapping/configuration of entity types without keys in the model.
         /// </summary>

src/EFCore/Properties/CoreStrings.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!-- 
-    Microsoft ResX Schema 
-    
+  <!--
+    Microsoft ResX Schema
+
     Version 2.0
-    
-    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes 
+
+    The primary goals of this format is to allow a simple XML format
+    that is mostly human readable. The generation and parsing of the
+    various data types are done through the TypeConverter classes
     associated with the data types.
-    
+
     Example:
-    
+
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-                
-    There are any number of "resheader" rows that contain simple 
+
+    There are any number of "resheader" rows that contain simple
     name/value pairs.
-    
-    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the 
+
+    Each data row contains a name, and value. The row also contains a
+    type or mimetype. Type corresponds to a .NET class that support
+    text/value conversion through the TypeConverter architecture.
+    Classes that don't support this are serialized and stored with the
     mimetype set.
-    
-    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not 
+
+    The mimetype is used for serialized objects, and tells the
+    ResXResourceReader how to depersist the object. This is currently not
     extensible. For a given mimetype the value must be set accordingly:
-    
-    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can 
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format
+    that the ResXResourceWriter will generate, however the reader can
     read any of the formats listed below.
-    
+
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-    
+
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with 
+    value   : The object must be serialized with
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array 
+    value   : The object must be serialized into a byte array
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -979,6 +979,10 @@
     <value>The property '{property}' on entity type '{entityType}' was created in shadow state because there are no eligible CLR members with a matching name.</value>
     <comment>Debug CoreEventId.ShadowPropertyCreated string string</comment>
   </data>
+  <data name="LogCollectionWithoutComparer" xml:space="preserve">
+    <value>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.</value>
+    <comment>Warning CoreEventId.CollectionWithoutComparer string string</comment>
+  </data>
   <data name="LogExecutionStrategyRetrying" xml:space="preserve">
     <value>A transient exception has been encountered during execution and the operation will be retried after {delay}ms.{newline}{error}</value>
     <comment>Information CoreEventId.ExecutionStrategyRetrying int string Exception</comment>

test/EFCore.Specification.Tests/CustomConvertersTestBase.cs

@@ -828,13 +828,26 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                             .HasConversion(
                                 BytesToStringConverter.DefaultInfo.Create())
                             .HasMaxLength(LongStringLength * 2);
+
+                        var bytesComparer = new ValueComparer<byte[]>(
+                            (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<StringListDataType>(
                     b =>
                     {
                         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<IList<string>>(
+                            (v1, v2) => v1.SequenceEqual(v2),
+                            v => v.GetHashCode());
+
+                        b.Property(e => e.Strings).Metadata.SetValueComparer(comparer);
                     });
 
                 modelBuilder.Entity<Order>(
@@ -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<IDictionary<string, string>>(
+                            (v1, v2) => v1.SequenceEqual(v2),
+                            v => v.GetHashCode());
+
+                        b.Property(e => e.Discriminator).Metadata.SetValueComparer(comparer);
                     });
 
                 var urlConverter = new UrlSchemeRemover();

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<TestLoggingDefinitions>()).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<string[]>(
+                    (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<string[], string>(
+                    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<byte[], byte[]>(v => v, v => v));
+
+            var stringProperty = entityType.AddProperty(nameof(WithStringAndBinaryKey.AString), typeof(string));
+            stringProperty.SetValueConverter(
+                new ValueConverter<string, string>(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()
         {