Don't detect public service properties in hierarchy as duplicates (#2…

…4462)

* Don't detect public service properties in hierarchy as duplicates

Fixes #23968

Service properties are most commonly private. We were missing test coverage for public service properties in a type hierarchy, which was hiding this bug where we detect the property reflected from the base type as a duplicate. Fix is to match duplicates by name.

Modification: Updated to handle duplicates rather than prevent them.

src/EFCore.Proxies/Proxies/Internal/ProxyBindingRewriter.cs

@@ -186,7 +186,9 @@ public virtual void ProcessModelFinalizing(
         }
 
         private InstantiationBinding UpdateConstructorBindings(
-            IConventionEntityType entityType, Type proxyType, InstantiationBinding binding)
+            IConventionEntityType entityType,
+            Type proxyType,
+            InstantiationBinding binding)
         {
             if (_options?.UseLazyLoadingProxies == true)
             {
@@ -212,31 +214,29 @@ private InstantiationBinding UpdateConstructorBindings(
                 }
 
                 return new FactoryMethodBinding(
-                        _proxyFactory,
-                        _createLazyLoadingProxyMethod,
-                        new List<ParameterBinding>
-                        {
-                            new ContextParameterBinding(typeof(DbContext)),
-                            new EntityTypeParameterBinding(),
-                            new DependencyInjectionParameterBinding(
-                                typeof(ILazyLoader), typeof(ILazyLoader), (IPropertyBase)serviceProperty),
-                            new ObjectArrayParameterBinding(binding.ParameterBindings)
-                        },
-                        proxyType);
-            }
-            else
-            {
-                return new FactoryMethodBinding(
-                        _proxyFactory,
-                        _createProxyMethod,
-                        new List<ParameterBinding>
-                        {
-                            new ContextParameterBinding(typeof(DbContext)),
-                            new EntityTypeParameterBinding(),
-                            new ObjectArrayParameterBinding(binding.ParameterBindings)
-                        },
-                        proxyType);
+                    _proxyFactory,
+                    _createLazyLoadingProxyMethod,
+                    new List<ParameterBinding>
+                    {
+                        new ContextParameterBinding(typeof(DbContext)),
+                        new EntityTypeParameterBinding(),
+                        new DependencyInjectionParameterBinding(
+                            typeof(ILazyLoader), typeof(ILazyLoader), new[] { (IPropertyBase)serviceProperty }),
+                        new ObjectArrayParameterBinding(binding.ParameterBindings)
+                    },
+                    proxyType);
             }
+
+            return new FactoryMethodBinding(
+                _proxyFactory,
+                _createProxyMethod,
+                new List<ParameterBinding>
+                {
+                    new ContextParameterBinding(typeof(DbContext)),
+                    new EntityTypeParameterBinding(),
+                    new ObjectArrayParameterBinding(binding.ParameterBindings)
+                },
+                proxyType);
         }
     }
 }

src/EFCore/ChangeTracking/Internal/InternalEntityEntry.cs

@@ -1188,6 +1188,7 @@ private void SetProperty(
                                     var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex();
                                     _storeGeneratedValues.SetValue(asProperty, defaultValue, storeGeneratedIndex);
                                 }
+
                                 break;
                             case CurrentValueType.Temporary:
                                 if (!_temporaryValues.IsEmpty)
@@ -1196,6 +1197,7 @@ private void SetProperty(
                                     var storeGeneratedIndex = asProperty.GetStoreGeneratedIndex();
                                     _temporaryValues.SetValue(asProperty, defaultValue, storeGeneratedIndex);
                                 }
+
                                 break;
                         }
                     }
@@ -1742,8 +1744,7 @@ public virtual void SetIsLoaded(INavigationBase navigation, bool loaded = true)
 
             _stateData.FlagProperty(navigation.GetIndex(), PropertyFlag.IsLoaded, isFlagged: loaded);
 
-            var lazyLoaderProperty = EntityType.GetServiceProperties().FirstOrDefault(p => p.ClrType == typeof(ILazyLoader));
-            if (lazyLoaderProperty != null)
+            foreach (var lazyLoaderProperty in EntityType.GetServiceProperties().Where(p => p.ClrType == typeof(ILazyLoader)))
             {
                 ((ILazyLoader?)this[lazyLoaderProperty])?.SetLoaded(Entity, navigation.Name, loaded);
             }

src/EFCore/Metadata/ContextParameterBinding.cs

@@ -2,8 +2,6 @@
 // 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 System.Linq;
 using System.Linq.Expressions;
 using Microsoft.EntityFrameworkCore.Storage;
 using Microsoft.EntityFrameworkCore.Utilities;
@@ -20,11 +18,11 @@ public class ContextParameterBinding : ServiceParameterBinding
         ///     Creates a new <see cref="ServiceParameterBinding" /> instance for the given service type.
         /// </summary>
         /// <param name="contextType"> The <see cref="DbContext" /> CLR type. </param>
-        /// <param name="serviceProperty"> The associated <see cref="IServiceProperty" />, or <see langword="null" />. </param>
+        /// <param name="serviceProperties"> The associated <see cref="IServiceProperty" /> objects, or <see langword="null" />. </param>
         public ContextParameterBinding(
             Type contextType,
-            IPropertyBase? serviceProperty = null)
-            : base(contextType, contextType, serviceProperty)
+            IPropertyBase[]? serviceProperties = null)
+            : base(contextType, contextType, serviceProperties)
         {
         }
 
@@ -57,7 +55,7 @@ var propertyExpression
         /// </summary>
         /// <param name="consumedProperties"> The new consumed properties. </param>
         /// <returns> A copy with replaced consumed properties. </returns>
-        public override ParameterBinding With(IReadOnlyList<IPropertyBase> consumedProperties)
-            => new ContextParameterBinding(ParameterType, consumedProperties.SingleOrDefault());
+        public override ParameterBinding With(IPropertyBase[] consumedProperties)
+            => new ContextParameterBinding(ParameterType, consumedProperties);
     }
 }

src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs

@@ -100,7 +100,6 @@ public virtual ConventionSet CreateConventionSet()
             conventionSet.EntityTypeMemberIgnoredConventions.Add(inversePropertyAttributeConvention);
             conventionSet.EntityTypeMemberIgnoredConventions.Add(relationshipDiscoveryConvention);
             conventionSet.EntityTypeMemberIgnoredConventions.Add(foreignKeyPropertyDiscoveryConvention);
-            conventionSet.EntityTypeMemberIgnoredConventions.Add(servicePropertyDiscoveryConvention);
 
             var keyAttributeConvention = new KeyAttributeConvention(Dependencies);
             var backingFieldConvention = new BackingFieldConvention(Dependencies);
@@ -231,7 +230,6 @@ public virtual ConventionSet CreateConventionSet()
             conventionSet.ModelFinalizingConventions.Add(new ConstructorBindingConvention(Dependencies));
             conventionSet.ModelFinalizingConventions.Add(foreignKeyIndexConvention);
             conventionSet.ModelFinalizingConventions.Add(foreignKeyPropertyDiscoveryConvention);
-            conventionSet.ModelFinalizingConventions.Add(servicePropertyDiscoveryConvention);
             conventionSet.ModelFinalizingConventions.Add(nonNullableReferencePropertyConvention);
             conventionSet.ModelFinalizingConventions.Add(nonNullableNavigationConvention);
             conventionSet.ModelFinalizingConventions.Add(new QueryFilterRewritingConvention(Dependencies));

src/EFCore/Metadata/Conventions/ServicePropertyDiscoveryConvention.cs

@@ -1,16 +1,10 @@
 // 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 System.Linq;
 using System.Reflection;
-using Microsoft.EntityFrameworkCore.Diagnostics;
-using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata.Builders;
 using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata.Internal;
-using Microsoft.EntityFrameworkCore.Utilities;
 
 namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
 {
@@ -19,9 +13,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions
     /// </summary>
     public class ServicePropertyDiscoveryConvention :
         IEntityTypeAddedConvention,
-        IEntityTypeBaseTypeChangedConvention,
-        IEntityTypeMemberIgnoredConvention,
-        IModelFinalizingConvention
+        IEntityTypeBaseTypeChangedConvention
     {
         /// <summary>
         ///     Creates a new instance of <see cref="ServicePropertyDiscoveryConvention" />.
@@ -90,134 +82,9 @@ private void Process(IConventionEntityTypeBuilder entityTypeBuilder)
                     continue;
                 }
 
-                var duplicateMap = GetDuplicateServiceProperties(entityType);
-                if (duplicateMap != null
-                    && duplicateMap.TryGetValue(propertyInfo.PropertyType, out var duplicateServiceProperties))
-                {
-                    duplicateServiceProperties.Add(propertyInfo);
-
-                    return;
-                }
-
-                var otherServicePropertySameType = entityType.GetServiceProperties()
-                    .FirstOrDefault(p => p.ClrType == propertyInfo.PropertyType);
-                if (otherServicePropertySameType != null)
-                {
-                    if (ConfigurationSource.Convention.Overrides(otherServicePropertySameType.GetConfigurationSource()))
-                    {
-                        otherServicePropertySameType.DeclaringEntityType.RemoveServiceProperty(otherServicePropertySameType.Name);
-                    }
-
-                    AddDuplicateServiceProperty(entityTypeBuilder, propertyInfo);
-                    AddDuplicateServiceProperty(entityTypeBuilder, otherServicePropertySameType.GetIdentifyingMemberInfo()!);
-
-                    return;
-                }
-
                 entityTypeBuilder.ServiceProperty(propertyInfo)?.HasParameterBinding(
                     (ServiceParameterBinding)factory.Bind(entityType, propertyInfo.PropertyType, name));
             }
         }
-
-        /// <summary>
-        ///     Called after an entity type member is ignored.
-        /// </summary>
-        /// <param name="entityTypeBuilder"> The builder for the entity type. </param>
-        /// <param name="name"> The name of the ignored member. </param>
-        /// <param name="context"> Additional information associated with convention execution. </param>
-        public virtual void ProcessEntityTypeMemberIgnored(
-            IConventionEntityTypeBuilder entityTypeBuilder,
-            string name,
-            IConventionContext<string> context)
-        {
-            var entityType = entityTypeBuilder.Metadata;
-            var duplicateMap = GetDuplicateServiceProperties(entityType);
-            if (duplicateMap == null)
-            {
-                return;
-            }
-
-            var member = (MemberInfo?)entityType.GetRuntimeProperties().Find(name)
-                ?? entityType.GetRuntimeFields().Find(name)!;
-            var type = member.GetMemberType();
-            if (duplicateMap.TryGetValue(type, out var duplicateServiceProperties)
-                && duplicateServiceProperties.Remove(member))
-            {
-                if (duplicateServiceProperties.Count != 1)
-                {
-                    return;
-                }
-
-                var otherMember = duplicateServiceProperties.First();
-                var otherName = otherMember.GetSimpleMemberName();
-                var factory = Dependencies.ParameterBindingFactories.FindFactory(type, otherName)!;
-                entityType.Builder.ServiceProperty(otherMember)?.HasParameterBinding(
-                    (ServiceParameterBinding)factory.Bind(entityType, type, otherName));
-                duplicateMap.Remove(type);
-                if (duplicateMap.Count == 0)
-                {
-                    SetDuplicateServiceProperties(entityType.Builder, null);
-                }
-            }
-        }
-
-        /// <inheritdoc />
-        public virtual void ProcessModelFinalizing(
-            IConventionModelBuilder modelBuilder,
-            IConventionContext<IConventionModelBuilder> context)
-        {
-            foreach (var entityType in modelBuilder.Metadata.GetEntityTypes())
-            {
-                var duplicateMap = GetDuplicateServiceProperties(entityType);
-                if (duplicateMap == null)
-                {
-                    continue;
-                }
-
-                foreach (var duplicateServiceProperties in duplicateMap.Values)
-                {
-                    foreach (var duplicateServiceProperty in duplicateServiceProperties)
-                    {
-                        if (entityType.FindProperty(duplicateServiceProperty.GetSimpleMemberName()) == null
-                            && entityType.FindNavigation(duplicateServiceProperty.GetSimpleMemberName()) == null)
-                        {
-                            throw new InvalidOperationException(
-                                CoreStrings.AmbiguousServiceProperty(
-                                    duplicateServiceProperty.Name,
-                                    duplicateServiceProperty.GetMemberType().ShortDisplayName(),
-                                    entityType.DisplayName()));
-                        }
-                    }
-                }
-
-                SetDuplicateServiceProperties(entityType.Builder, null);
-            }
-        }
-
-        private static void AddDuplicateServiceProperty(IConventionEntityTypeBuilder entityTypeBuilder, MemberInfo serviceProperty)
-        {
-            var duplicateMap = GetDuplicateServiceProperties(entityTypeBuilder.Metadata)
-                ?? new Dictionary<Type, HashSet<MemberInfo>>(1);
-
-            var type = serviceProperty.GetMemberType();
-            if (!duplicateMap.TryGetValue(type, out var duplicateServiceProperties))
-            {
-                duplicateServiceProperties = new HashSet<MemberInfo>();
-                duplicateMap[type] = duplicateServiceProperties;
-            }
-
-            duplicateServiceProperties.Add(serviceProperty);
-
-            SetDuplicateServiceProperties(entityTypeBuilder, duplicateMap);
-        }
-
-        private static Dictionary<Type, HashSet<MemberInfo>>? GetDuplicateServiceProperties(IConventionEntityType entityType)
-            => entityType.FindAnnotation(CoreAnnotationNames.DuplicateServiceProperties)?.Value
-                as Dictionary<Type, HashSet<MemberInfo>>;
-
-        private static void SetDuplicateServiceProperties(
-            IConventionEntityTypeBuilder entityTypeBuilder,
-            Dictionary<Type, HashSet<MemberInfo>>? duplicateServiceProperties)
-            => entityTypeBuilder.HasAnnotation(CoreAnnotationNames.DuplicateServiceProperties, duplicateServiceProperties);
     }
 }

src/EFCore/Metadata/Conventions/SlimModelConvention.cs

@@ -222,7 +222,7 @@ private ParameterBinding Create(ParameterBinding parameterBinding, SlimEntityTyp
             (entityType.FindProperty(property.Name)
                 ?? entityType.FindServiceProperty(property.Name)
                 ?? entityType.FindNavigation(property.Name)
-                ?? (IPropertyBase?)entityType.FindSkipNavigation(property.Name))!).ToList());
+                ?? (IPropertyBase?)entityType.FindSkipNavigation(property.Name))!).ToArray());
 
         private InstantiationBinding? Create(InstantiationBinding? instantiationBinding, SlimEntityType entityType)
             => instantiationBinding?.With(instantiationBinding.ParameterBindings.Select(binding => Create(binding, entityType)).ToList());

src/EFCore/Metadata/DependencyInjectionMethodParameterBinding.cs

@@ -24,13 +24,13 @@ public class DependencyInjectionMethodParameterBinding : DependencyInjectionPara
         /// <param name="parameterType"> The parameter CLR type. </param>
         /// <param name="serviceType"> The service CLR types, as resolved from dependency injection </param>
         /// <param name="method"> The method of the service to bind to. </param>
-        /// <param name="serviceProperty"> The associated <see cref="IServiceProperty" />, or null. </param>
+        /// <param name="serviceProperties"> The associated <see cref="IServiceProperty" /> objects, or <see langword="null" />. </param>
         public DependencyInjectionMethodParameterBinding(
             Type parameterType,
             Type serviceType,
             MethodInfo method,
-            IPropertyBase? serviceProperty = null)
-            : base(parameterType, serviceType, serviceProperty)
+            IPropertyBase[]? serviceProperties = null)
+            : base(parameterType, serviceType, serviceProperties)
         {
             Check.NotNull(method, nameof(method));
 
@@ -89,7 +89,7 @@ public override Expression BindToParameter(
         /// </summary>
         /// <param name="consumedProperties"> The new consumed properties. </param>
         /// <returns> A copy with replaced consumed properties. </returns>
-        public override ParameterBinding With(IReadOnlyList<IPropertyBase> consumedProperties)
-            => new DependencyInjectionMethodParameterBinding(ParameterType, ServiceType, Method, consumedProperties.SingleOrDefault());
+        public override ParameterBinding With(IPropertyBase[] consumedProperties)
+            => new DependencyInjectionMethodParameterBinding(ParameterType, ServiceType, Method, consumedProperties);
     }
 }

src/EFCore/Metadata/DependencyInjectionParameterBinding.cs

@@ -2,8 +2,6 @@
 // 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 System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
 using Microsoft.EntityFrameworkCore.Infrastructure;
@@ -28,12 +26,12 @@ private static readonly MethodInfo _getServiceMethod
         /// </summary>
         /// <param name="parameterType"> The parameter CLR type. </param>
         /// <param name="serviceType"> The service CLR types, as resolved from dependency injection </param>
-        /// <param name="serviceProperty"> The associated <see cref="IServiceProperty" />, or null. </param>
+        /// <param name="serviceProperties"> The associated <see cref="IServiceProperty" /> objects, or <see langword="null" />. </param>
         public DependencyInjectionParameterBinding(
             Type parameterType,
             Type serviceType,
-            IPropertyBase? serviceProperty = null)
-            : base(parameterType, serviceType, serviceProperty)
+            IPropertyBase[]? serviceProperties = null)
+            : base(parameterType, serviceType, serviceProperties)
         {
         }
 
@@ -65,7 +63,7 @@ public override Expression BindToParameter(
         /// </summary>
         /// <param name="consumedProperties"> The new consumed properties. </param>
         /// <returns> A copy with replaced consumed properties. </returns>
-        public override ParameterBinding With(IReadOnlyList<IPropertyBase> consumedProperties)
-            => new DependencyInjectionParameterBinding(ParameterType, ServiceType, consumedProperties.SingleOrDefault());
+        public override ParameterBinding With(IPropertyBase[] consumedProperties)
+            => new DependencyInjectionParameterBinding(ParameterType, ServiceType, consumedProperties);
     }
 }