Robust ordering on key properties

Fixes #13507
Fixes #19638

See also #19641

Basic logic is:
* If model type implements some sort of IComparable, then use it
* Otherwise, if provider type implement some sore of IComparable, then use it
* Otherwise fail in model validation

Also added new test suite specifically for different types of keys with and without conversions.

src/EFCore.Cosmos/ValueGeneration/Internal/IdValueGenerator.cs

@@ -54,7 +54,17 @@ protected override object NextValue(EntityEntry entry)
                     continue;
                 }
 
-                AppendString(builder, entry.Property(property.Name).CurrentValue);
+                var converter = property.GetValueConverter()
+                    ?? property.GetTypeMapping().Converter;
+
+                var value = entry.Property(property.Name).CurrentValue;
+                if (converter != null)
+                {
+                    value = converter.ConvertToProvider(value);
+                }
+
+                AppendString(builder, value);
+
                 builder.Append("|");
             }
 
@@ -79,7 +89,7 @@ private void AppendString(StringBuilder builder, object propertyValue)
 
                     return;
                 default:
-                    builder.Append(propertyValue.ToString().Replace("|", "/|"));
+                    builder.Append(propertyValue == null ? "null" : propertyValue.ToString().Replace("|", "/|"));
                     return;
             }
         }

src/EFCore.InMemory/Query/Internal/InMemoryExpressionTranslatingExpressionVisitor.cs

@@ -167,8 +167,7 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
 
             if (property != null)
             {
-                var comparer = property.GetValueComparer()
-                    ?? property.FindTypeMapping()?.Comparer;
+                var comparer = property.GetValueComparer();
 
                 if (comparer != null
                     && comparer.Type.IsAssignableFrom(newLeft.Type)

src/EFCore.InMemory/Storage/Internal/InMemoryTable.cs

@@ -65,7 +65,7 @@ public InMemoryTable([NotNull] IEntityType entityType, bool sensitiveLoggingEnab
                     _valueConverters.Add((property.GetIndex(), converter));
                 }
 
-                var comparer = GetStructuralComparer(property);
+                var comparer = property.GetStructuralValueComparer();
                 if (!comparer.HasDefaultBehavior)
                 {
                     if (_valueComparers == null)
@@ -147,13 +147,7 @@ public virtual IReadOnlyList<object[]> SnapshotRows()
         }
 
         private static List<ValueComparer> GetStructuralComparers(IEnumerable<IProperty> properties)
-            => properties.Select(GetStructuralComparer).ToList();
-
-        private static ValueComparer GetStructuralComparer(IProperty p)
-            => p.GetStructuralValueComparer()
-                ?? p.GetKeyValueComparer()
-                ?? p.GetValueComparer()
-                ?? p.FindTypeMapping()?.StructuralComparer;
+            => properties.Select(p => p.GetStructuralValueComparer()).ToList();
 
         /// <summary>
         ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
@@ -164,7 +158,7 @@ private static ValueComparer GetStructuralComparer(IProperty p)
         public virtual void Create(IUpdateEntry entry)
         {
             var row = entry.EntityType.GetProperties()
-                .Select(p => SnapshotValue(p, GetStructuralComparer(p), entry))
+                .Select(p => SnapshotValue(p, p.GetStructuralValueComparer(), entry))
                 .ToArray();
 
             _rows.Add(CreateKey(entry), row);
@@ -211,14 +205,18 @@ private static bool IsConcurrencyConflict(
             object rowValue,
             Dictionary<IProperty, object> concurrencyConflicts)
         {
-            if (property.IsConcurrencyToken
-                && !StructuralComparisons.StructuralEqualityComparer.Equals(
-                    rowValue,
-                    entry.GetOriginalValue(property)))
+            if (property.IsConcurrencyToken)
             {
-                concurrencyConflicts.Add(property, rowValue);
+                var comparer = property.GetStructuralValueComparer();
+                var originalValue = entry.GetOriginalValue(property);
+
+                if ((comparer != null && !comparer.Equals(rowValue, originalValue))
+                    || (comparer == null && !StructuralComparisons.StructuralEqualityComparer.Equals(rowValue, originalValue)))
+                {
+                    concurrencyConflicts.Add(property, rowValue);
 
-                return true;
+                    return true;
+                }
             }
 
             return false;

src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs

@@ -1803,8 +1803,8 @@ protected virtual void DiffData(
 
                             var sourceValue = sourceEntry.GetCurrentValue(sourceProperty);
                             var targetValue = entry.GetCurrentValue(targetProperty);
-                            var comparer = targetProperty.GetValueComparer()
-                                ?? sourceProperty.GetValueComparer()
+                            var comparer = targetProperty.GetValueComparer(fallback: false)
+                                ?? sourceProperty.GetValueComparer(fallback: false)
                                 ?? targetProperty.FindTypeMapping()?.Comparer ?? sourceProperty.FindTypeMapping()?.Comparer;
 
                             var modelValuesChanged

src/EFCore.Relational/Update/Internal/ModificationCommandComparer.cs

@@ -2,12 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Collections;
-using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Linq.Expressions;
-using System.Reflection;
-using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.Extensions.DependencyInjection;
 
 namespace Microsoft.EntityFrameworkCore.Update.Internal
@@ -101,9 +97,8 @@ public virtual int Compare(ModificationCommand x, ModificationCommand y)
                 for (var i = 0; i < xKey.Properties.Count; i++)
                 {
                     var xKeyProperty = xKey.Properties[i];
-                    var compare = GetComparer(xKeyProperty.ClrType);
 
-                    result = compare(xEntry.GetCurrentValue(xKeyProperty), yEntry.GetCurrentValue(yKey.Properties[i]));
+                    result = xKeyProperty.GetCurrentValueComparer().Compare(xEntry, yEntry);
                     if (0 != result)
                     {
                         return result;
@@ -113,54 +108,5 @@ public virtual int Compare(ModificationCommand x, ModificationCommand y)
 
             return result;
         }
-
-        private readonly ConcurrentDictionary<Type, Func<object, object, int>> _comparers =
-            new ConcurrentDictionary<Type, Func<object, object, int>>();
-
-        /// <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>
-        protected virtual Func<object, object, int> GetComparer([NotNull] Type type)
-            => _comparers.GetOrAdd(
-                type, t =>
-                {
-                    var xParameter = Expression.Parameter(typeof(object), name: "x");
-                    var yParameter = Expression.Parameter(typeof(object), name: "y");
-                    return Expression.Lambda<Func<object, object, int>>(
-                            Expression.Call(
-                                null,
-                                (typeof(IStructuralComparable).IsAssignableFrom(type)
-                                    ? _structuralCompareMethod
-                                    : _compareMethod)
-                                .MakeGenericMethod(t),
-                                Expression.Convert(xParameter, t),
-                                Expression.Convert(yParameter, t)),
-                            xParameter,
-                            yParameter)
-                        .Compile();
-                });
-
-        private static readonly MethodInfo _compareMethod
-            = typeof(ModificationCommandComparer).GetTypeInfo().GetDeclaredMethod(nameof(CompareValue));
-
-        private static int CompareValue<T>(T x, T y) => Comparer<T>.Default.Compare(x, y);
-
-        private static readonly MethodInfo _structuralCompareMethod
-            = typeof(ModificationCommandComparer).GetTypeInfo().GetDeclaredMethod(nameof(CompareStructureValue));
-
-        private static int CompareStructureValue<T>(T x, T y)
-        {
-            if (x is Array array1
-                && y is Array array2
-                && array1.Length != array2.Length)
-            {
-                return array1.Length - array2.Length;
-            }
-
-            return StructuralComparisons.StructuralComparer.Compare(x, y);
-        }
     }
 }

src/EFCore.Relational/Update/ModificationCommand.cs

@@ -377,7 +377,7 @@ public void RecordValue(IProperty property, IUpdateEntry entry)
                     case EntityState.Added:
                         _currentValue = entry.GetCurrentValue(property);
 
-                        var comparer = property.GetValueComparer() ?? property.FindTypeMapping()?.Comparer;
+                        var comparer = property.GetValueComparer();
                         if (comparer == null)
                         {
                             _write = !Equals(_originalValue, _currentValue);

src/EFCore/ChangeTracking/Internal/ChangeDetector.cs

@@ -197,7 +197,7 @@ private void LocalDetectChanges(InternalEntityEntry entry)
                     var current = entry[property];
                     var original = entry.GetOriginalValue(property);
 
-                    var comparer = property.GetValueComparer() ?? property.FindTypeMapping()?.Comparer;
+                    var comparer = property.GetValueComparer();
 
                     if (comparer == null)
                     {
@@ -251,9 +251,7 @@ private void DetectKeyChange(InternalEntityEntry entry, IProperty property)
                 var snapshotValue = entry.GetRelationshipSnapshotValue(property);
                 var currentValue = entry[property];
 
-                var comparer = property.GetKeyValueComparer()
-                    ?? property.GetValueComparer()
-                    ?? property.FindTypeMapping()?.KeyComparer;
+                var comparer = property.GetKeyValueComparer();
 
                 // Note that mutation of a byte[] key is not supported or detected, but two different instances
                 // of byte[] with the same content must be detected as equal.

src/EFCore/ChangeTracking/Internal/CompositeValueFactory.cs

@@ -132,9 +132,7 @@ protected virtual bool TryCreateFromEntry(
         /// </summary>
         protected static IEqualityComparer<object[]> CreateEqualityComparer([NotNull] IReadOnlyList<IProperty> properties)
         {
-            var comparers = properties.Select(p => p.GetKeyValueComparer()
-                ?? p.GetValueComparer()
-                ?? p.FindTypeMapping()?.KeyComparer).ToList();
+            var comparers = properties.Select(p => p.GetKeyValueComparer()).ToList();
 
             return comparers.All(c => c != null)
                 ? new CompositeCustomComparer(comparers)

src/EFCore/ChangeTracking/Internal/CurrentProviderValueComparer.cs

@@ -0,0 +1,44 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
+using Microsoft.EntityFrameworkCore.Update;
+
+namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
+{
+    /// <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>
+    public class CurrentProviderValueComparer : CurrentValueComparer
+    {
+        private readonly ValueConverter _converter;
+
+        /// <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>
+        public CurrentProviderValueComparer(
+            [NotNull] IPropertyBase property,
+            [NotNull] ValueConverter converter)
+            : base(property)
+        {
+            _converter = converter;
+        }
+
+        /// <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>
+        public override object GetCurrentValue(IUpdateEntry entry)
+            => _converter.ConvertToProvider(base.GetCurrentValue(entry));
+    }
+}

src/EFCore/ChangeTracking/Internal/CurrentProviderValueComparer`.cs

@@ -0,0 +1,51 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System;
+using System.Collections.Generic;
+using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
+using Microsoft.EntityFrameworkCore.Update;
+
+namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
+{
+    /// <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>
+    public class CurrentProviderValueComparer<TModel, TProvider> : IComparer<IUpdateEntry>
+    {
+        private readonly IPropertyBase _property;
+        private readonly IComparer<TProvider> _underlyingComparer;
+        private readonly Func<TModel, TProvider> _converter;
+
+        /// <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>
+        public CurrentProviderValueComparer(
+            [NotNull] IPropertyBase property,
+            [NotNull] ValueConverter<TModel, TProvider> converter)
+        {
+            _property = property;
+            _converter = converter.ConvertToProviderExpression.Compile();
+            _underlyingComparer = Comparer<TProvider>.Default;
+        }
+
+        /// <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>
+        public virtual int Compare(IUpdateEntry x, IUpdateEntry y)
+            => _underlyingComparer.Compare(
+                _converter(x.GetCurrentValue<TModel>(_property)),
+                _converter(y.GetCurrentValue<TModel>(_property)));
+    }
+}