Improve exception messages for invalid identity and identity conflicts

Issue #7001

Includes:
- Null key and conflicting key cases
- Property names
- Help for null alternate key case
- Conflicting key values included when sensitive data logging is enabled
- Updated messages that better reflect current behavior

src/Microsoft.EntityFrameworkCore.Specification.Tests/AsNoTrackingTestBase.cs

@@ -48,8 +48,9 @@ private class StateManagerProxy : StateManager
                 IModel model,
                 IDatabase database,
                 IConcurrencyDetector concurrencyDetector,
-                ICurrentDbContext currentContext)
-                : base(factory, subscriber, notifier, valueGeneration, model, database, concurrencyDetector, currentContext)
+                ICurrentDbContext currentContext,
+                IDbContextOptions contextOptions)
+                : base(factory, subscriber, notifier, valueGeneration, model, database, concurrencyDetector, currentContext, contextOptions)
             {
                 IsInitialized = true;
             }

src/Microsoft.EntityFrameworkCore.Specification.Tests/ThrowingMonsterStateManager.cs

@@ -6,6 +6,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.EntityFrameworkCore.ChangeTracking.Internal;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Storage;
@@ -22,8 +23,9 @@ public class ThrowingMonsterStateManager : StateManager
             IModel model,
             IDatabase database,
             IConcurrencyDetector concurrencyDetector,
-            ICurrentDbContext currentContext)
-            : base(factory, subscriber, notifier, valueGeneration, model, database, concurrencyDetector, currentContext)
+            ICurrentDbContext currentContext,
+            IDbContextOptions contextOptions)
+            : base(factory, subscriber, notifier, valueGeneration, model, database, concurrencyDetector, currentContext, contextOptions)
         {
         }
 

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/CompositePrincipalKeyValueFactory.cs

@@ -63,13 +63,34 @@ public virtual object CreateFromBuffer(ValueBuffer valueBuffer)
             return values;
         }
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual IProperty FindNullPropertyInValueBuffer(ValueBuffer valueBuffer) 
+            => _properties.FirstOrDefault(p => valueBuffer[p.GetIndex()] == null);
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
         public virtual object[] CreateFromCurrentValues(InternalEntityEntry entry)
             => CreateFromEntry(entry, (e, p) => e.GetCurrentValue(p));
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual IProperty FindNullPropertyInCurrentValues(InternalEntityEntry entry)
+            => _properties.FirstOrDefault(p => entry.GetCurrentValue(p) == null);
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual string BuildKeyValuesString(InternalEntityEntry entry) 
+            => string.Join(", ", _properties.Select(p => p.Name + ":" + entry.GetCurrentValue(p)));
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
@@ -93,7 +114,10 @@ public virtual object[] CreateFromRelationshipSnapshot(InternalEntityEntry entry
 
             foreach (var property in _properties)
             {
-                values[index++] = getValue(entry, property);
+                if ((values[index++] = getValue(entry, property)) == null)
+                {
+                    return null;
+                }
             }
 
             return values;

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/IPrincipalKeyValueFactory.cs

@@ -3,6 +3,7 @@
 
 using System.Collections.Generic;
 using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Storage;
 
 namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
@@ -25,12 +26,30 @@ public interface IPrincipalKeyValueFactory<TKey>
         /// </summary>
         object CreateFromBuffer(ValueBuffer valueBuffer);
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        IProperty FindNullPropertyInValueBuffer(ValueBuffer valueBuffer);
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
         TKey CreateFromCurrentValues([NotNull] InternalEntityEntry entry);
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        IProperty FindNullPropertyInCurrentValues([NotNull] InternalEntityEntry entry);
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        string BuildKeyValuesString([NotNull] InternalEntityEntry entry);
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/IdentityMap.cs

@@ -18,6 +18,7 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
     /// </summary>
     public class IdentityMap<TKey> : IIdentityMap
     {
+        private readonly bool _sensitiveLoggingEnabled;
         private readonly Dictionary<TKey, InternalEntityEntry> _identityMap;
         private readonly IForeignKey[] _foreignKeys;
         private Dictionary<IForeignKey, IDependentsMap> _dependentMaps;
@@ -28,8 +29,10 @@ public class IdentityMap<TKey> : IIdentityMap
         /// </summary>
         public IdentityMap(
             [NotNull] IKey key,
-            [NotNull] IPrincipalKeyValueFactory<TKey> principalKeyValueFactory)
+            [NotNull] IPrincipalKeyValueFactory<TKey> principalKeyValueFactory,
+            bool sensitiveLoggingEnabled)
         {
+            _sensitiveLoggingEnabled = sensitiveLoggingEnabled;
             Key = key;
             PrincipalKeyValueFactory = principalKeyValueFactory;
             _identityMap = new Dictionary<TKey, InternalEntityEntry>(principalKeyValueFactory.EqualityComparer);
@@ -99,7 +102,18 @@ public virtual InternalEntityEntry TryGetEntry(ValueBuffer valueBuffer, bool thr
             if (key == null
                 && throwOnNullKey)
             {
-                throw new InvalidOperationException(CoreStrings.InvalidKeyValue(Key.DeclaringEntityType.DisplayName()));
+                if (Key.IsPrimaryKey())
+                {
+                    throw new InvalidOperationException(
+                        CoreStrings.InvalidKeyValue(
+                            Key.DeclaringEntityType.DisplayName(),
+                            PrincipalKeyValueFactory.FindNullPropertyInValueBuffer(valueBuffer).Name));
+                }
+
+                throw new InvalidOperationException(
+                    CoreStrings.InvalidAlternateKeyValue(
+                        Key.DeclaringEntityType.DisplayName(),
+                        PrincipalKeyValueFactory.FindNullPropertyInValueBuffer(valueBuffer).Name));
             }
 
             try
@@ -187,7 +201,19 @@ protected virtual void Add([NotNull] TKey key, [NotNull] InternalEntityEntry ent
             {
                 if (existingEntry != entry)
                 {
-                    throw new InvalidOperationException(CoreStrings.IdentityConflict(entry.EntityType.DisplayName()));
+                    if (_sensitiveLoggingEnabled)
+                    {
+                        throw new InvalidOperationException(
+                            CoreStrings.IdentityConflictSensitive(
+                                entry.EntityType.DisplayName(),
+                                PrincipalKeyValueFactory.BuildKeyValuesString(entry)));
+
+                    }
+
+                    throw new InvalidOperationException(
+                        CoreStrings.IdentityConflict(
+                            entry.EntityType.DisplayName(),
+                            string.Join(", ", Key.Properties.Select(p => p.Name))));
                 }
             }
             else

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/IdentityMapFactoryFactory.cs

@@ -19,20 +19,20 @@ public class IdentityMapFactoryFactory : IdentityMapFactoryFactoryBase
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
-        public virtual Func<IIdentityMap> Create([NotNull] IKey key)
-            => (Func<IIdentityMap>)typeof(IdentityMapFactoryFactory).GetTypeInfo()
+        public virtual Func<bool, IIdentityMap> Create([NotNull] IKey key)
+            => (Func<bool, IIdentityMap>)typeof(IdentityMapFactoryFactory).GetTypeInfo()
                 .GetDeclaredMethod(nameof(CreateFactory))
                 .MakeGenericMethod(GetKeyType(key))
                 .Invoke(null, new object[] { key });
 
         [UsedImplicitly]
-        private static Func<IIdentityMap> CreateFactory<TKey>(IKey key)
+        private static Func<bool, IIdentityMap> CreateFactory<TKey>(IKey key)
         {
             var factory = key.GetPrincipalKeyValueFactory<TKey>();
 
             return typeof(TKey).IsNullableType()
-                ? (Func<IIdentityMap>)(() => new NullableKeyIdentityMap<TKey>(key, factory))
-                : () => new IdentityMap<TKey>(key, factory);
+                ? (Func<bool, IIdentityMap>)(sensitiveLoggingEnabled => new NullableKeyIdentityMap<TKey>(key, factory, sensitiveLoggingEnabled))
+                : sensitiveLoggingEnabled => new IdentityMap<TKey>(key, factory, sensitiveLoggingEnabled);
         }
     }
 }

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/KeyValueFactoryFactory.cs

@@ -28,8 +28,7 @@ public virtual IPrincipalKeyValueFactory<TKey> Create<TKey>([NotNull] IKey key)
         private static SimplePrincipalKeyValueFactory<TKey> CreateSimpleFactory<TKey>(IKey key)
         {
             var dependentFactory = new DependentKeyValueFactoryFactory();
-            var principalKeyValueFactory
-                = new SimplePrincipalKeyValueFactory<TKey>(key.Properties.Single().GetPropertyAccessors());
+            var principalKeyValueFactory = new SimplePrincipalKeyValueFactory<TKey>(key.Properties.Single());
 
             foreach (var foreignKey in key.GetReferencingForeignKeys())
             {

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/NullableKeyIdentityMap.cs

@@ -21,8 +21,9 @@ public class NullableKeyIdentityMap<TKey> : IdentityMap<TKey>
         /// </summary>
         public NullableKeyIdentityMap(
             [NotNull] IKey key,
-            [NotNull] IPrincipalKeyValueFactory<TKey> principalKeyValueFactory)
-            : base(key, principalKeyValueFactory)
+            [NotNull] IPrincipalKeyValueFactory<TKey> principalKeyValueFactory,
+            bool sensitiveLoggingEnabled)
+            : base(key, principalKeyValueFactory, sensitiveLoggingEnabled)
         {
         }
 
@@ -36,7 +37,18 @@ public override void Add(InternalEntityEntry entry)
 
             if (key == null)
             {
-                throw new InvalidOperationException(CoreStrings.InvalidKeyValue(entry.EntityType.DisplayName()));
+                if (Key.IsPrimaryKey())
+                {
+                    throw new InvalidOperationException(
+                        CoreStrings.InvalidKeyValue(
+                            entry.EntityType.DisplayName(),
+                            PrincipalKeyValueFactory.FindNullPropertyInCurrentValues(entry).Name));
+                }
+
+                throw new InvalidOperationException(
+                    CoreStrings.InvalidAlternateKeyValue(
+                        entry.EntityType.DisplayName(),
+                        PrincipalKeyValueFactory.FindNullPropertyInCurrentValues(entry).Name));
             }
 
             Add(key, entry);

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/SimplePrincipalKeyValueFactory.cs

@@ -6,6 +6,7 @@
 using System.Collections.Generic;
 using System.Reflection;
 using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Storage;
 
@@ -17,15 +18,17 @@ namespace Microsoft.EntityFrameworkCore.ChangeTracking.Internal
     /// </summary>
     public class SimplePrincipalKeyValueFactory<TKey> : IPrincipalKeyValueFactory<TKey>
     {
+        private readonly IProperty _property;
         private readonly PropertyAccessors _propertyAccessors;
 
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
-        public SimplePrincipalKeyValueFactory([NotNull] PropertyAccessors propertyAccessors)
+        public SimplePrincipalKeyValueFactory([NotNull] IProperty property)
         {
-            _propertyAccessors = propertyAccessors;
+            _property = property;
+            _propertyAccessors = _property.GetPropertyAccessors();
             EqualityComparer = typeof(IStructuralEquatable).GetTypeInfo().IsAssignableFrom(typeof(TKey).GetTypeInfo())
                 ? (IEqualityComparer<TKey>)new NoNullsStructuralEqualityComparer()
                 : new NoNullsEqualityComparer();
@@ -45,13 +48,34 @@ public virtual object CreateFromKeyValues(object[] keyValues)
         public virtual object CreateFromBuffer(ValueBuffer valueBuffer)
             => _propertyAccessors.ValueBufferGetter(valueBuffer);
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual IProperty FindNullPropertyInValueBuffer(ValueBuffer valueBuffer)
+            => _property;
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
         public virtual TKey CreateFromCurrentValues(InternalEntityEntry entry)
             => ((Func<InternalEntityEntry, TKey>)_propertyAccessors.CurrentValueGetter)(entry);
 
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual IProperty FindNullPropertyInCurrentValues(InternalEntityEntry entry)
+            => _property;
+
+        /// <summary>
+        ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
+        ///     directly from your code. This API may change or be removed in future releases.
+        /// </summary>
+        public virtual string BuildKeyValuesString(InternalEntityEntry entry)
+            => _property.Name + ":" + entry.GetCurrentValue(_property);
+
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.

src/Microsoft.EntityFrameworkCore/ChangeTracking/Internal/StateManager.cs

@@ -7,6 +7,7 @@
 using System.Threading;
 using System.Threading.Tasks;
 using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
@@ -36,6 +37,7 @@ public class StateManager : IStateManager
         private TrackingQueryMode _trackingQueryMode = TrackingQueryMode.Simple;
         private IEntityType _singleQueryModeEntityType;
 
+        private readonly bool _sensitiveLoggingEnabled;
         private readonly IInternalEntityEntryFactory _factory;
         private readonly IInternalEntityEntrySubscriber _subscriber;
         private readonly IModel _model;
@@ -54,7 +56,8 @@ public class StateManager : IStateManager
             [NotNull] IModel model,
             [NotNull] IDatabase database,
             [NotNull] IConcurrencyDetector concurrencyDetector,
-            [NotNull] ICurrentDbContext currentContext)
+            [NotNull] ICurrentDbContext currentContext,
+            [NotNull] IDbContextOptions contextOptions)
         {
             _factory = factory;
             _subscriber = subscriber;
@@ -64,6 +67,15 @@ public class StateManager : IStateManager
             _database = database;
             _concurrencyDetector = concurrencyDetector;
             Context = currentContext.Context;
+
+            if (contextOptions
+                .Extensions
+                .OfType<CoreOptionsExtension>()
+                .FirstOrDefault()
+                ?.IsSensitiveDataLoggingEnabled == true)
+            {
+                _sensitiveLoggingEnabled = true;
+            }
         }
 
         /// <summary>
@@ -216,7 +228,7 @@ private IIdentityMap GetOrCreateIdentityMap(IKey key)
         {
             if (_identityMap0 == null)
             {
-                _identityMap0 = key.GetIdentityMapFactory()();
+                _identityMap0 = key.GetIdentityMapFactory()(_sensitiveLoggingEnabled);
                 return _identityMap0;
             }
 
@@ -227,7 +239,7 @@ private IIdentityMap GetOrCreateIdentityMap(IKey key)
 
             if (_identityMap1 == null)
             {
-                _identityMap1 = key.GetIdentityMapFactory()();
+                _identityMap1 = key.GetIdentityMapFactory()(_sensitiveLoggingEnabled);
                 return _identityMap1;
             }
 
@@ -244,7 +256,7 @@ private IIdentityMap GetOrCreateIdentityMap(IKey key)
             IIdentityMap identityMap;
             if (!_identityMaps.TryGetValue(key, out identityMap))
             {
-                identityMap = key.GetIdentityMapFactory()();
+                identityMap = key.GetIdentityMapFactory()(_sensitiveLoggingEnabled);
                 _identityMaps[key] = identityMap;
             }
             return identityMap;

src/Microsoft.EntityFrameworkCore/Metadata/Internal/Key.cs

@@ -21,7 +21,7 @@ public class Key : ConventionalAnnotatable, IMutableKey
         private ConfigurationSource _configurationSource;
 
         // Warning: Never access these fields directly as access needs to be thread-safe
-        private Func<IIdentityMap> _identityMapFactory;
+        private Func<bool, IIdentityMap> _identityMapFactory;
         private Func<IWeakReferenceIdentityMap> _weakReferenceIdentityMap;
         private object _principalKeyValueFactory;
 
@@ -88,7 +88,7 @@ public virtual IEnumerable<ForeignKey> GetReferencingForeignKeys()
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used
         ///     directly from your code. This API may change or be removed in future releases.
         /// </summary>
-        public virtual Func<IIdentityMap> IdentityMapFactory
+        public virtual Func<bool, IIdentityMap> IdentityMapFactory
             => NonCapturingLazyInitializer.EnsureInitialized(
                 ref _identityMapFactory, this, k => new IdentityMapFactoryFactory().Create(k));