API review updates (#34241)

Part of #33220

-  Replace `IUpdateEntry.GetOriginalOrCurrentValue` with `HasOriginalValue`
- Review `MigrationsSqlGenerator.KeyWithOptions` - no longer exists
- Rename `TConcreteCollection` to `TConcreteList` for ListOf*Comparer
- Consider sealing JsonCollection*ReaderWriter classes - considered, but decided against it.
- Obsolete the old CosmosQueryableExtensions.WithPartitionKey overload - Overload resolution picks the obsolete method, so better not to make it obsolete otherwise it is a pain (explicit cast needed) for people attempting to try to use the new method instead of the obsolete one.
- Ensure PartitionKey is appropriate for diagnostics
-  Remove EntityMaterializerSource from QueryContext and QueryContextDependencies

Author: ajcvickers

src/EFCore.Relational/Update/ColumnModification.cs

@@ -184,7 +184,7 @@ public virtual object? Value
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public static object? GetOriginalValue(IUpdateEntry entry, IProperty property)
-        => entry.GetOriginalOrCurrentValue(property);
+        => entry.CanHaveOriginalValue(property) ? entry.GetOriginalValue(property) : entry.GetCurrentValue(property);
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/ChangeTracking/Internal/CompositePrincipalKeyValueFactory.cs

@@ -99,7 +99,7 @@ public virtual IProperty FindNullPropertyInKeyValues(IReadOnlyList<object?> keyV
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual IReadOnlyList<object?> CreateFromOriginalValues(IUpdateEntry entry)
-        => CreateFromEntry(entry, (e, p) => e.GetOriginalOrCurrentValue(p));
+        => CreateFromEntry(entry, (e, p) => e.CanHaveOriginalValue(p) ? e.GetOriginalValue(p) : e.GetCurrentValue(p));
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/ChangeTracking/Internal/CompositeValueFactory.cs

@@ -91,7 +91,7 @@ public virtual bool TryCreateFromPreStoreGeneratedCurrentValues(IUpdateEntry ent
     ///     doing so can result in application failures when updating to a new Entity Framework Core release.
     /// </summary>
     public virtual bool TryCreateFromOriginalValues(IUpdateEntry entry, [NotNullWhen(true)] out IReadOnlyList<object?>? key)
-        => TryCreateFromEntry(entry, (e, p) => e.GetOriginalOrCurrentValue(p), out key);
+        => TryCreateFromEntry(entry, (e, p) => e.CanHaveOriginalValue(p) ? e.GetOriginalValue(p) : e.GetCurrentValue(p), out key);
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs

@@ -1141,10 +1141,8 @@ public bool RemoveFromCollection(INavigationBase navigationBase, object value)
     ///     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 object? GetOriginalOrCurrentValue(IPropertyBase propertyBase)
-        => propertyBase.GetOriginalValueIndex() >= 0
-            ? _originalValues.GetValue(this, (IProperty)propertyBase)
-            : GetCurrentValue(propertyBase);
+    public bool CanHaveOriginalValue(IPropertyBase propertyBase)
+        => propertyBase.GetOriginalValueIndex() >= 0;
 
     /// <summary>
     ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to

src/EFCore/ChangeTracking/ListOfNullableValueTypesComparer.cs

@@ -11,31 +11,31 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking;
 /// </summary>
 /// <remarks>
 ///     <para>
-///         This comparer should be used for nullable value types. Use <see cref="ListOfNullableValueTypesComparer{TConcreteCollection,TElement}" /> for reference
+///         This comparer should be used for nullable value types. Use <see cref="ListOfNullableValueTypesComparer{TConcreteList,TElement}" /> for reference
 ///         types and non-nullable value types.
 ///     </para>
 ///     <para>
 ///         See <see href="https://aka.ms/efcore-docs-value-comparers">EF Core value comparers</see> for more information and examples.
 ///     </para>
 /// </remarks>
-/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
+/// <typeparam name="TConcreteList">The collection type to create an index of, if needed.</typeparam>
 /// <typeparam name="TElement">The element type.</typeparam>
-public sealed class ListOfNullableValueTypesComparer<TConcreteCollection, TElement> : ValueComparer<IEnumerable<TElement?>>, IInfrastructure<ValueComparer>
+public sealed class ListOfNullableValueTypesComparer<TConcreteList, TElement> : ValueComparer<IEnumerable<TElement?>>, IInfrastructure<ValueComparer>
     where TElement : struct
 {
-    private static readonly bool IsArray = typeof(TConcreteCollection).IsArray;
+    private static readonly bool IsArray = typeof(TConcreteList).IsArray;
 
     private static readonly bool IsReadOnly = IsArray
-        || (typeof(TConcreteCollection).IsGenericType
-            && typeof(TConcreteCollection).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
+        || (typeof(TConcreteList).IsGenericType
+            && typeof(TConcreteList).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
 
-    private static readonly MethodInfo CompareMethod = typeof(ListOfNullableValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo CompareMethod = typeof(ListOfNullableValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Compare), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement?>), typeof(IEnumerable<TElement?>), typeof(ValueComparer<TElement?>)])!;
 
-    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfNullableValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfNullableValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(GetHashCode), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement?>), typeof(ValueComparer<TElement?>)])!;
 
-    private static readonly MethodInfo SnapshotMethod = typeof(ListOfNullableValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo SnapshotMethod = typeof(ListOfNullableValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Snapshot), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement?>), typeof(ValueComparer<TElement?>)])!;
 
     /// <summary>
@@ -198,14 +198,14 @@ private static int GetHashCode(IEnumerable<TElement?> source, ValueComparer<TEle
         }
         else
         {
-            var snapshot = IsReadOnly ? new List<TElement?>() : (IList<TElement?>)Activator.CreateInstance<TConcreteCollection>()!;
+            var snapshot = IsReadOnly ? new List<TElement?>() : (IList<TElement?>)Activator.CreateInstance<TConcreteList>()!;
             foreach (var e in sourceList)
             {
                 snapshot.Add(e == null ? null : elementComparer.Snapshot(e));
             }
 
             return IsReadOnly
-                ? (IList<TElement?>)Activator.CreateInstance(typeof(TConcreteCollection), [snapshot])!
+                ? (IList<TElement?>)Activator.CreateInstance(typeof(TConcreteList), [snapshot])!
                 : snapshot;
         }
     }

src/EFCore/ChangeTracking/ListOfReferenceTypesComparer.cs

@@ -18,24 +18,24 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking;
 ///         See <see href="https://aka.ms/efcore-docs-value-comparers">EF Core value comparers</see> for more information and examples.
 ///     </para>
 /// </remarks>
-/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
+/// <typeparam name="TConcreteList">The collection type to create an index of, if needed.</typeparam>
 /// <typeparam name="TElement">The element type.</typeparam>
-public sealed class ListOfReferenceTypesComparer<TConcreteCollection, TElement> : ValueComparer<object>, IInfrastructure<ValueComparer>
+public sealed class ListOfReferenceTypesComparer<TConcreteList, TElement> : ValueComparer<object>, IInfrastructure<ValueComparer>
     where TElement : class
 {
-    private static readonly bool IsArray = typeof(TConcreteCollection).IsArray;
+    private static readonly bool IsArray = typeof(TConcreteList).IsArray;
 
     private static readonly bool IsReadOnly = IsArray
-        || (typeof(TConcreteCollection).IsGenericType
-            && typeof(TConcreteCollection).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
+        || (typeof(TConcreteList).IsGenericType
+            && typeof(TConcreteList).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
 
-    private static readonly MethodInfo CompareMethod = typeof(ListOfReferenceTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo CompareMethod = typeof(ListOfReferenceTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Compare), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(object), typeof(ValueComparer)])!;
 
-    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfReferenceTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfReferenceTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(GetHashCode), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable), typeof(ValueComparer)])!;
 
-    private static readonly MethodInfo SnapshotMethod = typeof(ListOfReferenceTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo SnapshotMethod = typeof(ListOfReferenceTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Snapshot), BindingFlags.Static | BindingFlags.NonPublic, [typeof(object), typeof(ValueComparer)])!;
 
     /// <summary>
@@ -194,14 +194,14 @@ private static int GetHashCode(IEnumerable source, ValueComparer elementComparer
         }
         else
         {
-            var snapshot = IsReadOnly ? new List<TElement?>() : (IList<TElement?>)Activator.CreateInstance<TConcreteCollection>()!;
+            var snapshot = IsReadOnly ? new List<TElement?>() : (IList<TElement?>)Activator.CreateInstance<TConcreteList>()!;
             foreach (var e in sourceList)
             {
                 snapshot.Add(e == null ? null : (TElement?)elementComparer.Snapshot(e));
             }
 
             return IsReadOnly
-                ? (IList<TElement?>)Activator.CreateInstance(typeof(TConcreteCollection), [snapshot])!
+                ? (IList<TElement?>)Activator.CreateInstance(typeof(TConcreteList), [snapshot])!
                 : snapshot;
         }
     }

src/EFCore/ChangeTracking/ListOfValueTypesComparer.cs

@@ -12,30 +12,30 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking;
 /// <remarks>
 ///     <para>
 ///         This comparer should be used for reference types and non-nullable value types. Use
-///         <see cref="ListOfNullableValueTypesComparer{TConcreteCollection,TElement}" /> for nullable value types.
+///         <see cref="ListOfNullableValueTypesComparer{TConcreteList,TElement}" /> for nullable value types.
 ///     </para>
 ///     <para>
 ///         See <see href="https://aka.ms/efcore-docs-value-comparers">EF Core value comparers</see> for more information and examples.
 ///     </para>
 /// </remarks>
-/// <typeparam name="TConcreteCollection">The collection type to create an index of, if needed.</typeparam>
+/// <typeparam name="TConcreteList">The collection type to create an index of, if needed.</typeparam>
 /// <typeparam name="TElement">The element type.</typeparam>
-public sealed class ListOfValueTypesComparer<TConcreteCollection, TElement> : ValueComparer<IEnumerable<TElement>>, IInfrastructure<ValueComparer>
+public sealed class ListOfValueTypesComparer<TConcreteList, TElement> : ValueComparer<IEnumerable<TElement>>, IInfrastructure<ValueComparer>
     where TElement : struct
 {
-    private static readonly bool IsArray = typeof(TConcreteCollection).IsArray;
+    private static readonly bool IsArray = typeof(TConcreteList).IsArray;
 
     private static readonly bool IsReadOnly = IsArray
-        || (typeof(TConcreteCollection).IsGenericType
-            && typeof(TConcreteCollection).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
+        || (typeof(TConcreteList).IsGenericType
+            && typeof(TConcreteList).GetGenericTypeDefinition() == typeof(ReadOnlyCollection<>));
 
-    private static readonly MethodInfo CompareMethod = typeof(ListOfValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo CompareMethod = typeof(ListOfValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Compare), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement>), typeof(IEnumerable<TElement>), typeof(ValueComparer<TElement>)])!;
 
-    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo GetHashCodeMethod = typeof(ListOfValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(GetHashCode), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement>), typeof(ValueComparer<TElement>)])!;
 
-    private static readonly MethodInfo SnapshotMethod = typeof(ListOfValueTypesComparer<TConcreteCollection, TElement>).GetMethod(
+    private static readonly MethodInfo SnapshotMethod = typeof(ListOfValueTypesComparer<TConcreteList, TElement>).GetMethod(
         nameof(Snapshot), BindingFlags.Static | BindingFlags.NonPublic, [typeof(IEnumerable<TElement>), typeof(ValueComparer<TElement>)])!;
 
     /// <summary>
@@ -183,14 +183,14 @@ private static IList<TElement> Snapshot(IEnumerable<TElement> source, ValueCompa
         }
         else
         {
-            var snapshot = IsReadOnly ? new List<TElement>() : (IList<TElement>)Activator.CreateInstance<TConcreteCollection>()!;
+            var snapshot = IsReadOnly ? new List<TElement>() : (IList<TElement>)Activator.CreateInstance<TConcreteList>()!;
             foreach (var e in sourceList)
             {
                 snapshot.Add(elementComparer.Snapshot(e));
             }
 
             return IsReadOnly
-                ? (IList<TElement>)Activator.CreateInstance(typeof(TConcreteCollection), [snapshot])!
+                ? (IList<TElement>)Activator.CreateInstance(typeof(TConcreteList), [snapshot])!
                 : snapshot;
         }
     }

src/EFCore/Query/QueryContext.cs

@@ -1,7 +1,6 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Diagnostics.CodeAnalysis;
 using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
 using Microsoft.EntityFrameworkCore.Query.Internal;
 
@@ -51,12 +50,6 @@ protected QueryContext(QueryContextDependencies dependencies)
     /// </summary>
     protected virtual QueryContextDependencies Dependencies { get; }
 
-    /// <summary>
-    ///    The <see cref="EntityMaterializerSource"/>, which can be used to create stand-alone entity instances.
-    /// </summary>
-    public virtual IEntityMaterializerSource EntityMaterializerSource
-        => Dependencies.EntityMaterializerSource;
-
     /// <summary>
     ///     Sets the navigation for given entity as loaded.
     /// </summary>

src/EFCore/Query/QueryContextDependencies.cs

@@ -53,15 +53,13 @@ public QueryContextDependencies(
         IExecutionStrategy executionStrategy,
         IConcurrencyDetector concurrencyDetector,
         IExceptionDetector exceptionDetector,
-        IEntityMaterializerSource entityMaterializerSource,
         IDiagnosticsLogger<DbLoggerCategory.Database.Command> commandLogger,
         IDiagnosticsLogger<DbLoggerCategory.Query> queryLogger)
     {
         CurrentContext = currentContext;
         ExecutionStrategy = executionStrategy;
         ConcurrencyDetector = concurrencyDetector;
         ExceptionDetector = exceptionDetector;
-        EntityMaterializerSource = entityMaterializerSource;
         CommandLogger = commandLogger;
         QueryLogger = queryLogger;
     }
@@ -96,11 +94,6 @@ public IStateManager StateManager
     /// </summary>
     public IExceptionDetector ExceptionDetector { get; init; }
 
-    /// <summary>
-    ///    The <see cref="EntityMaterializerSource"/>, which can be used to create stand-alone entity instances.
-    /// </summary>
-    public IEntityMaterializerSource EntityMaterializerSource { get; }
-
     /// <summary>
     ///     The command logger.
     /// </summary>

src/EFCore/Update/IUpdateEntry.cs

@@ -95,12 +95,11 @@ public interface IUpdateEntry
     object? GetOriginalValue(IPropertyBase propertyBase);
 
     /// <summary>
-    ///     Gets the value assigned to the property when it was retrieved from the database, or
-    ///     the current value if the original value is not being stored.
+    /// Returns <see langword="true"/> only if the property has storage for an original value.
     /// </summary>
-    /// <param name="propertyBase">The property to get the value for.</param>
-    /// <returns>The value for the property.</returns>
-    object? GetOriginalOrCurrentValue(IPropertyBase propertyBase);
+    /// <param name="propertyBase">The property.</param>
+    /// <returns><see langword="true"/> if the property may have an original value; <see langword="false"/> if it never can.</returns>
+    bool CanHaveOriginalValue(IPropertyBase propertyBase);
 
     /// <summary>
     ///     Gets the value assigned to the property.

src/EFCore/Update/UpdateEntryExtensions.cs

@@ -49,7 +49,10 @@ public static class UpdateEntryExtensions
     /// <returns>The value for the property.</returns>
     public static object? GetOriginalProviderValue(this IUpdateEntry updateEntry, IProperty property)
     {
-        var value = updateEntry.GetOriginalOrCurrentValue(property);
+        var value = updateEntry.CanHaveOriginalValue(property)
+            ? updateEntry.GetOriginalValue(property)
+            : updateEntry.GetCurrentValue(property);
+
         var typeMapping = property.GetTypeMapping();
         value = value?.GetType().IsInteger() == true && typeMapping.ClrType.UnwrapNullableType().IsEnum
             ? Enum.ToObject(typeMapping.ClrType.UnwrapNullableType(), value)

test/EFCore.Cosmos.FunctionalTests/TestUtilities/CosmosTestStore.cs

@@ -531,7 +531,7 @@ public IUpdateEntry SharedIdentityEntry
         public object GetCurrentValue(IPropertyBase propertyBase)
             => throw new NotImplementedException();
 
-        public object GetOriginalOrCurrentValue(IPropertyBase propertyBase)
+        public bool CanHaveOriginalValue(IPropertyBase propertyBase)
             => throw new NotImplementedException();
 
         public TProperty GetCurrentValue<TProperty>(IPropertyBase propertyBase)

test/EFCore.Tests/ExceptionTest.cs

@@ -130,7 +130,7 @@ public object GetCurrentValue(IPropertyBase propertyBase)
         public object GetOriginalValue(IPropertyBase propertyBase)
             => throw new NotImplementedException();
 
-        public object GetOriginalOrCurrentValue(IPropertyBase propertyBase)
+        public bool CanHaveOriginalValue(IPropertyBase propertyBase)
             => throw new NotImplementedException();
 
         public TProperty GetCurrentValue<TProperty>(IPropertyBase propertyBase)