Add a safeguard for fks updated in recursive calls to ForeignKeyPrope…

…rtyDiscoveryConvention

Make InternalEntityTypeBuilder.HasKey more resilient by removing conflicting fks of lower source
Reset FieldInfo when lifting properties if the field doesn't exist on the new type
Improve the exception message for removing a property that is still being referenced

Fixes #6867

src/Microsoft.EntityFrameworkCore/Internal/TypeExtensions.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Globalization;
+using System.Linq;
 using System.Reflection;
 using System.Text;
 using JetBrains.Annotations;
@@ -60,6 +61,25 @@ public static string DisplayName([NotNull] this Type type, bool fullName = true)
             return sb.ToString();
         }
 
+        /// <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 static FieldInfo GetFieldInfo([NotNull] this Type type, [NotNull] string fieldName)
+        {
+            var typesInHierarchy = type.GetTypesInHierarchy().ToList();
+            foreach (var typeInHierarchy in typesInHierarchy)
+            {
+                var fields = typeInHierarchy.GetRuntimeFields().ToDictionary(f => f.Name);
+                FieldInfo fieldInfo;
+                if (fields.TryGetValue(fieldName, out fieldInfo))
+                {
+                    return fieldInfo;
+                }
+            }
+            return null;
+        }
+
         private static void AppendGenericArguments(Type[] args, int startIndex, int numberOfArgsToAppend, StringBuilder sb, bool fullName)
         {
             var totalArgs = args.Length;

src/Microsoft.EntityFrameworkCore/Metadata/Conventions/Internal/ForeignKeyPropertyDiscoveryConvention.cs

@@ -353,10 +353,12 @@ public virtual InternalKeyBuilder Apply(InternalKeyBuilder keyBuilder)
         /// </summary>
         public virtual void Apply(InternalEntityTypeBuilder entityTypeBuilder, Key key)
         {
-            foreach (var foreignKey in key.DeclaringEntityType.GetDerivedForeignKeysInclusive().ToList())
+            var fks = key.DeclaringEntityType.GetDerivedForeignKeysInclusive().ToList();
+            foreach (var foreignKey in fks)
             {
-                if (!foreignKey.IsUnique
-                    || foreignKey.DeclaringEntityType.BaseType != null)
+                if (foreignKey.Builder != null
+                    && (!foreignKey.IsUnique
+                        || foreignKey.DeclaringEntityType.BaseType != null))
                 {
                     Apply(foreignKey.Builder);
                 }

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

@@ -465,16 +465,13 @@ public virtual Key AddKey([NotNull] IReadOnlyList<Property> properties,
 
             foreach (var property in properties)
             {
-                var currentKeys = property.Keys;
-                if (currentKeys == null)
+                if (property.Keys == null)
                 {
-                    property.Keys = new IKey[] { key };
+                    property.Keys = new List<IKey> { key };
                 }
                 else
                 {
-                    var newKeys = currentKeys.ToList();
-                    newKeys.Add(key);
-                    property.Keys = newKeys;
+                    property.Keys.Add(key);
                 }
             }
 
@@ -566,11 +563,14 @@ private Key RemoveKey([NotNull] Key key, bool runConventions)
 
             foreach (var property in key.Properties)
             {
-                property.Keys =
-                    property.Keys != null
-                    && property.Keys.Count > 1
-                        ? property.Keys.Where(k => k != key).ToList()
-                        : null;
+                if (property.Keys != null)
+                {
+                    property.Keys.Remove(key);
+                    if (property.Keys.Count == 0)
+                    {
+                        property.Keys = null;
+                    }
+                }
             }
 
             PropertyMetadataChanged();
@@ -673,16 +673,13 @@ public virtual ForeignKey AddForeignKey(
 
             foreach (var property in properties)
             {
-                var currentKeys = property.ForeignKeys;
-                if (currentKeys == null)
+                if (property.ForeignKeys == null)
                 {
-                    property.ForeignKeys = new IForeignKey[] { foreignKey };
+                    property.ForeignKeys = new List<IForeignKey> { foreignKey };
                 }
                 else
                 {
-                    var newKeys = currentKeys.ToList();
-                    newKeys.Add(foreignKey);
-                    property.ForeignKeys = newKeys;
+                    property.ForeignKeys.Add(foreignKey);
                 }
             }
 
@@ -909,11 +906,14 @@ private ForeignKey RemoveForeignKey([NotNull] ForeignKey foreignKey, bool runCon
 
             foreach (var property in foreignKey.Properties)
             {
-                property.ForeignKeys =
-                    property.ForeignKeys != null
-                    && property.ForeignKeys.Count > 1
-                        ? property.ForeignKeys.Where(k => k != foreignKey).ToList()
-                        : null;
+                if (property.ForeignKeys != null)
+                {
+                    property.ForeignKeys.Remove(foreignKey);
+                    if (property.ForeignKeys.Count == 0)
+                    {
+                        property.ForeignKeys = null;
+                    }
+                }
             }
 
             foreignKey.PrincipalKey.ReferencingForeignKeys.Remove(foreignKey);
@@ -1194,16 +1194,13 @@ public virtual Index AddIndex([NotNull] IReadOnlyList<Property> properties,
 
             foreach (var property in properties)
             {
-                var currentIndexes = property.Indexes;
-                if (currentIndexes == null)
+                if (property.Indexes == null)
                 {
-                    property.Indexes = new IIndex[] { index };
+                    property.Indexes = new List<IIndex> { index };
                 }
                 else
                 {
-                    var newIndex = currentIndexes.ToList();
-                    newIndex.Add(index);
-                    property.Indexes = newIndex;
+                    property.Indexes.Add(index);
                 }
             }
 
@@ -1312,11 +1309,14 @@ private Index RemoveIndex(Index index)
 
             foreach (var property in index.Properties)
             {
-                property.Indexes =
-                    property.Indexes != null
-                    && property.Indexes.Count > 1
-                        ? property.Indexes.Where(k => k != index).ToList()
-                        : null;
+                if (property.Indexes != null)
+                {
+                    property.Indexes.Remove(index);
+                    if (property.Indexes.Count == 0)
+                    {
+                        property.Indexes = null;
+                    }
+                }
             }
 
             Model.ConventionDispatcher.OnIndexRemoved(Builder, index);
@@ -1535,21 +1535,27 @@ private Property RemoveProperty(Property property)
 
         private void CheckPropertyNotInUse(Property property)
         {
-            CheckPropertyNotInUse(property, this);
+            var containingKey = property.Keys?.FirstOrDefault();
+            if (containingKey != null)
+            {
+                throw new InvalidOperationException(
+                    CoreStrings.PropertyInUseKey(property.Name, this.DisplayName(), Property.Format(containingKey.Properties)));
+            }
 
-            foreach (var entityType in GetDerivedTypes())
+            var containingForeignKey = property.ForeignKeys?.FirstOrDefault();
+            if (containingForeignKey != null)
             {
-                CheckPropertyNotInUse(property, entityType);
+                throw new InvalidOperationException(
+                    CoreStrings.PropertyInUseForeignKey(property.Name, this.DisplayName(),
+                        Property.Format(containingForeignKey.Properties), containingForeignKey.DeclaringEntityType.DisplayName()));
             }
-        }
 
-        private void CheckPropertyNotInUse(Property property, EntityType entityType)
-        {
-            if (entityType.GetDeclaredKeys().Any(k => k.Properties.Contains(property))
-                || entityType.GetDeclaredForeignKeys().Any(k => k.Properties.Contains(property))
-                || entityType.GetDeclaredIndexes().Any(i => i.Properties.Contains(property)))
+            var containingIndex = property.Indexes?.FirstOrDefault();
+            if (containingIndex != null)
             {
-                throw new InvalidOperationException(CoreStrings.PropertyInUse(property.Name, this.DisplayName()));
+                throw new InvalidOperationException(
+                    CoreStrings.PropertyInUseIndex(property.Name, this.DisplayName(),
+                        Property.Format(containingIndex.Properties), containingIndex.DeclaringEntityType.DisplayName()));
             }
         }
 

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

@@ -139,20 +139,47 @@ private InternalKeyBuilder HasKeyInternal(IReadOnlyList<Property> properties, Co
             var key = Metadata.FindDeclaredKey(actualProperties);
             if (key == null)
             {
-                if ((configurationSource != ConfigurationSource.Explicit) // let it throw for explicit
-                    && (configurationSource == null
-                        || actualProperties.Any(p => p.GetContainingForeignKeys().Any(k => k.DeclaringEntityType != Metadata))
-                        || actualProperties.Any(p => !p.Builder.CanSetRequired(true, configurationSource))))
+                if (configurationSource == null)
                 {
                     return null;
                 }
 
+                var containingForeignKeys = actualProperties
+                    .SelectMany(p => p.GetContainingForeignKeys().Where(k => k.DeclaringEntityType != Metadata))
+                    .ToList();
+
+                if (containingForeignKeys.Any(fk => !configurationSource.Overrides(fk.GetForeignKeyPropertiesConfigurationSource())))
+                {
+                    return null;
+                }
+
+                if (configurationSource != ConfigurationSource.Explicit // let it throw for explicit
+                    && actualProperties.Any(p => !p.Builder.CanSetRequired(true, configurationSource)))
+                {
+                    return null;
+                }
+
+                var modifiedRelationships = containingForeignKeys
+                    .Where(fk => fk.GetForeignKeyPropertiesConfigurationSource() != ConfigurationSource.Explicit)  // let it throw for explicit
+                    .Select(foreignKey => foreignKey.Builder.HasForeignKey(null, configurationSource, runConventions: false))
+                    .ToList();
+
                 foreach (var actualProperty in actualProperties)
                 {
                     actualProperty.Builder.IsRequired(true, configurationSource.Value);
                 }
 
                 key = Metadata.AddKey(actualProperties, configurationSource.Value);
+
+                foreach (var foreignKey in containingForeignKeys)
+                {
+                    ModelBuilder.Metadata.ConventionDispatcher.OnForeignKeyRemoved(foreignKey.DeclaringEntityType.Builder, foreignKey);
+                }
+
+                foreach (var relationship in modifiedRelationships)
+                {
+                    ModelBuilder.Metadata.ConventionDispatcher.OnForeignKeyAdded(relationship);
+                }
             }
             else if (configurationSource.HasValue)
             {
@@ -939,15 +966,17 @@ private static Dictionary<EntityType, List<RelationshipSnapshot>> GroupRelations
             var detachedRelationships = property.GetContainingForeignKeys().ToList()
                 .Select(DetachRelationship).ToList();
 
-            foreach (var key in Metadata.GetKeys().Where(i => i.Properties.Contains(property)).ToList())
+            var removedKeys = new List<Key>();
+            foreach (var key in property.GetContainingKeys().ToList())
             {
                 detachedRelationships.AddRange(key.GetReferencingForeignKeys().ToList()
                     .Select(DetachRelationship));
-                var removed = RemoveKey(key, configurationSource);
+                var removed = RemoveKey(key, configurationSource, runConventions: false);
                 Debug.Assert(removed.HasValue);
+                removedKeys.Add(key);
             }
 
-            foreach (var index in Metadata.GetIndexes().Where(i => i.Properties.Contains(property)).ToList())
+            foreach (var index in property.GetContainingIndexes().ToList())
             {
                 var removed = RemoveIndex(index, configurationSource);
                 Debug.Assert(removed.HasValue);
@@ -959,6 +988,11 @@ private static Dictionary<EntityType, List<RelationshipSnapshot>> GroupRelations
                 Debug.Assert(removedProperty == property);
             }
 
+            foreach (var key in removedKeys)
+            {
+                Metadata.Model.ConventionDispatcher.OnKeyRemoved(this, key);
+            }
+
             foreach (var detachedRelationship in detachedRelationships)
             {
                 detachedRelationship.Attach();

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

@@ -132,13 +132,27 @@ public virtual bool HasValueGenerator(
         /// </summary>
         public virtual bool HasField([CanBeNull] string fieldName, ConfigurationSource configurationSource)
         {
-            if (configurationSource.Overrides(Metadata.GetFieldInfoConfigurationSource()))
+            if (Metadata.FieldInfo?.Name == fieldName)
             {
                 Metadata.SetField(fieldName, configurationSource);
                 return true;
             }
 
-            return false;
+            if (!configurationSource.Overrides(Metadata.GetFieldInfoConfigurationSource()))
+            {
+                return false;
+            }
+
+            if (fieldName != null)
+            {
+                var fieldInfo = PropertyBase.GetFieldInfo(fieldName, Metadata.DeclaringType.ClrType, Metadata.Name,
+                    shouldThrow: configurationSource == ConfigurationSource.Explicit);
+                Metadata.SetFieldInfo(fieldInfo, configurationSource);
+                return true;
+            }
+
+            Metadata.SetField(fieldName, configurationSource);
+            return true;
         }
 
         /// <summary>
@@ -147,7 +161,12 @@ public virtual bool HasField([CanBeNull] string fieldName, ConfigurationSource c
         /// </summary>
         public virtual bool HasFieldInfo([CanBeNull] FieldInfo fieldInfo, ConfigurationSource configurationSource)
         {
-            if (configurationSource.Overrides(Metadata.GetFieldInfoConfigurationSource()))
+            if ((configurationSource.Overrides(Metadata.GetFieldInfoConfigurationSource())
+                 && (fieldInfo == null
+                     || PropertyBase.IsCompatible(
+                         fieldInfo, Metadata.ClrType, Metadata.DeclaringType.ClrType, Metadata.Name,
+                         shouldThrow: configurationSource == ConfigurationSource.Explicit)))
+                || Metadata.FieldInfo == fieldInfo)
             {
                 Metadata.SetFieldInfo(fieldInfo, configurationSource);
                 return true;

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

@@ -533,19 +533,19 @@ public virtual PropertyIndexes PropertyIndexes
         ///     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 IReadOnlyList<IKey> Keys { get; [param: CanBeNull] set; }
+        public virtual List<IKey> Keys { get; [param: CanBeNull] set; }
 
         /// <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 IReadOnlyList<IForeignKey> ForeignKeys { get; [param: CanBeNull] set; }
+        public virtual List<IForeignKey> ForeignKeys { get; [param: CanBeNull] set; }
 
         /// <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 IReadOnlyList<IIndex> Indexes { get; [param: CanBeNull] set; }
+        public virtual List<IIndex> Indexes { get; [param: CanBeNull] set; }
 
         /// <summary>
         ///     This API supports the Entity Framework Core infrastructure and is not intended to be used