Use the configured ValueComparer in ModificationCommandComparer

[AndriySvyryd]

No description provided.

[ajcvickers]

The KeyComparer of the property is the appropriate one to use in this case.

[ajcvickers]

The KeyComparer should be used so that keys of any type that do not compare appropriately with the default Equals (e.g. IGeometry) will be sorted correctly.

[ajcvickers]

While investigating this it became apparent that there are currently implicit requirements for types used as key properties. Testing indicates:

1. Class key types must override reference equality (or ensure singleton instances)
2. Querying requires that struct key types have equality operator overloads. (Exception below.)
3. The update pipeline currently requires key types to be IComparable. (Exception below.)

Proposal:

• For 1, this seems like a reasonable requirement; just document it.
• For 2, this seems like a bug we should fix.
• For 3, things are more tricky. Ordering comparisons are different from equality comparisons because every object has some form of equality that can be used. Types that don't implement any forms of IComparable cannot be ordered. Some possibilities:
  • Require that key property types implement some form of IComparable. Throw in model validation if this isn't the case. This is a breaking change for code that didn't use the update pipeline in a way that would hit this, which I suspect is not uncommon. On the other hand, it is clear, clean, and reasonable.
  • Fallback in the update pipeline to stop ordering if not possible, with a warning. Finding out that you're doing something wrong will be a lot harder.
  • Convert key values to the provider value in the update pipeline and order using this. All the normal primitive types support IComparable. However, it's not impossible that some provider-specific type could be used as a key without being comparable. This could be a breaking change, and would be hard to fix without the provider type being changed, which in turn may not owned by the provider or be easy.

Exception when key structs don't implement equality operator:

System.InvalidOperationException : The binary operator Equal is not defined for the types 'System.Nullable`1[Microsoft.EntityFrameworkCore.BuiltInDataTypesTestBase`1+KeyStruct[Microsoft.EntityFrameworkCore.BuiltInDataTypesSqlServerTest+BuiltInDataTypesSqlServerFixture]]' and 'System.Nullable`1[Microsoft.EntityFrameworkCore.BuiltInDataTypesTestBase`1+KeyStruct[Microsoft.EntityFrameworkCore.BuiltInDataTypesSqlServerTest+BuiltInDataTypesSqlServerFixture]]'.
   at System.Linq.Expressions.Expression.GetEqualityComparisonOperator(ExpressionType binaryType, String opName, Expression left, Expression right, Boolean liftToNull)
   at System.Linq.Expressions.Expression.Equal(Expression left, Expression right, Boolean liftToNull, MethodInfo method)
   at System.Linq.Expressions.Expression.Equal(Expression left, Expression right)
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.ExpandingExpressionVisitor.ExpandNavigation(Expression root, EntityReference entityReference, INavigation navigation, Boolean derivedTypeConversion) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 231
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.IncludeExpandingExpressionVisitor.ExpandIncludesHelper(Expression root, EntityReference entityReference) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 486
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.IncludeExpandingExpressionVisitor.ExpandInclude(Expression root, EntityReference entityReference) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 448
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.IncludeExpandingExpressionVisitor.VisitExtension(Expression extensionExpression) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 325
   at System.Linq.Expressions.Expression.Accept(ExpressionVisitor visitor)
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.ExpandingExpressionVisitor.Expand(Expression expression, Boolean applyIncludes) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 36
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.PendingSelectorExpandingExpressionVisitor.Visit(Expression expression) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.ExpressionVisitors.cs:line 523
   at Microsoft.EntityFrameworkCore.Query.Internal.NavigationExpandingExpressionVisitor.Expand(Expression query) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\NavigationExpandingExpressionVisitor.cs:line 75
   at Microsoft.EntityFrameworkCore.Query.QueryTranslationPreprocessor.Process(Expression query) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\QueryTranslationPreprocessor.cs:line 40
   at Microsoft.EntityFrameworkCore.Query.QueryCompilationContext.CreateQueryExecutor[TResult](Expression query) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\QueryCompilationContext.cs:line 74
   at Microsoft.EntityFrameworkCore.Storage.Database.CompileQuery[TResult](Expression query, Boolean async) in C:\aspnet\EntityFrameworkCore\src\EFCore\Storage\Database.cs:line 71
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.CompileQueryCore[TResult](IDatabase database, Expression query, IModel model, Boolean async) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\QueryCompiler.cs:line 112
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.<>c__DisplayClass9_0`1.<Execute>b__0() in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\QueryCompiler.cs:line 96
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQueryCore[TFunc](Object cacheKey, Func`1 compiler) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\CompiledQueryCache.cs:line 84
   at Microsoft.EntityFrameworkCore.Query.Internal.CompiledQueryCache.GetOrAddQuery[TResult](Object cacheKey, Func`1 compiler) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\CompiledQueryCache.cs:line 59
   at Microsoft.EntityFrameworkCore.Query.Internal.QueryCompiler.Execute[TResult](Expression query) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\QueryCompiler.cs:line 92
   at Microsoft.EntityFrameworkCore.Query.Internal.EntityQueryProvider.Execute[TResult](Expression expression) in C:\aspnet\EntityFrameworkCore\src\EFCore\Query\Internal\EntityQueryProvider.cs:line 79
   at System.Linq.Queryable.Single[TSource](IQueryable`1 source, Expression`1 predicate)
   at Microsoft.EntityFrameworkCore.BuiltInDataTypesTestBase`1.<Can_insert_and_read_back_with_struct_key>g__QueryByStructKey|23_0(DbContext context, KeyStruct id) in C:\aspnet\EntityFrameworkCore\test\EFCore.Specification.Tests\BuiltInDataTypesTestBase.cs:line 1520
   at Microsoft.EntityFrameworkCore.BuiltInDataTypesTestBase`1.Can_insert_and_read_back_with_struct_key() in C:\aspnet\EntityFrameworkCore\test\EFCore.Specification.Tests\BuiltInDataTypesTestBase.cs:line 1527

Exception when key property is not IComparable

System.InvalidOperationException : Failed to compare two elements in the array.
---- System.ArgumentException : At least one object must implement IComparable.
   at System.Collections.Generic.ArraySortHelper`1.Sort(T[] keys, Int32 index, Int32 length, IComparer`1 comparer)
   at System.Array.Sort[T](T[] array, Int32 index, Int32 length, IComparer`1 comparer)
   at System.Collections.Generic.List`1.Sort(Int32 index, Int32 count, IComparer`1 comparer)
   at System.Collections.Generic.List`1.Sort(IComparer`1 comparer)
   at Microsoft.EntityFrameworkCore.Update.Internal.CommandBatchPreparer.BatchCommands(IList`1 entries, IUpdateAdapter updateAdapter)+MoveNext() in C:\aspnet\EntityFrameworkCore\src\EFCore.Relational\Update\Internal\CommandBatchPreparer.cs:line 88
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.Execute(IEnumerable`1 commandBatches, IRelationalConnection connection) in C:\aspnet\EntityFrameworkCore\src\EFCore.Relational\Update\Internal\BatchExecutor.cs:line 76
   at Microsoft.EntityFrameworkCore.Storage.RelationalDatabase.SaveChanges(IList`1 entries) in C:\aspnet\EntityFrameworkCore\src\EFCore.Relational\Storage\RelationalDatabase.cs:line 61
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(IList`1 entriesToSave) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\StateManager.cs:line 1057
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(DbContext _, Boolean acceptAllChangesOnSuccess) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\StateManager.cs:line 1104
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteImplementation[TState,TResult](Func`3 operation, Func`3 verifySucceeded, TState state) in C:\aspnet\EntityFrameworkCore\src\EFCore\Storage\ExecutionStrategy.cs:line 173
   at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 verifySucceeded) in C:\aspnet\EntityFrameworkCore\src\EFCore\Storage\ExecutionStrategy.cs:line 159
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChanges(Boolean acceptAllChangesOnSuccess) in C:\aspnet\EntityFrameworkCore\src\EFCore\ChangeTracking\Internal\StateManager.cs:line 1084
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges(Boolean acceptAllChangesOnSuccess) in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 454
   at Microsoft.EntityFrameworkCore.DbContext.SaveChanges() in C:\aspnet\EntityFrameworkCore\src\EFCore\DbContext.cs:line 417
   at Microsoft.EntityFrameworkCore.BuiltInDataTypesTestBase`1.Can_insert_and_read_back_with_class_key() in C:\aspnet\EntityFrameworkCore\test\EFCore.Specification.Tests\BuiltInDataTypesTestBase.cs:line 1628
----- Inner Stack Trace -----
   at System.Collections.Comparer.Compare(Object a, Object b)
   at System.Collections.Generic.ObjectComparer`1.Compare(T x, T y)
   at Microsoft.EntityFrameworkCore.Update.Internal.ModificationCommandComparer.CompareValue[T](T x, T y) in C:\aspnet\EntityFrameworkCore\src\EFCore.Relational\Update\Internal\ModificationCommandComparer.cs:line 149
   at Microsoft.EntityFrameworkCore.Update.Internal.ModificationCommandComparer.Compare(ModificationCommand x, ModificationCommand y) in C:\aspnet\EntityFrameworkCore\src\EFCore.Relational\Update\Internal\ModificationCommandComparer.cs:line 106
   at System.Collections.Generic.ArraySortHelper`1.InsertionSort(T[] keys, Int32 lo, Int32 hi, Comparison`1 comparer)
   at System.Collections.Generic.ArraySortHelper`1.IntroSort(T[] keys, Int32 lo, Int32 hi, Int32 depthLimit, Comparison`1 comparer)
   at System.Collections.Generic.ArraySortHelper`1.IntrospectiveSort(T[] keys, Int32 left, Int32 length, Comparison`1 comparer)
   at System.Collections.Generic.ArraySortHelper`1.Sort(T[] keys, Int32 index, Int32 length, IComparer`1 comparer)

[smitpatel]

@ajcvickers - Can you share the query you used?

[ajcvickers]

Branch: https://github.com/aspnet/EntityFrameworkCore/tree/BuiltInDataTypesTests1225

[smitpatel]

Filed #19407 for query bug.

[ajcvickers]

Notes from team discussion:

• If the key type implements some form of IComparable, then use it
• If not, then convert to store type and use IComparable on that
• Check in model validation that either the model type or the store type implements some form of IComparable