Issue 507: Cannot change base type after convention model builder

EntityKey convention is looking at Type.GetProperties() directly to figure
out the key property. So if base class a key property named Id, model
building would fail as GetProperties() would return that as well.

Modifying EnityKeyConvention to use properties on the configuration object
instead of from the clr type.

7 files changed, 114 insertions, 90 deletions

diff --git a/src/System.Web.Http.OData/OData/Builder/Conventions/ConventionsHelpers.cs b/src/System.Web.Http.OData/OData/Builder/Conventions/ConventionsHelpers.cs
index cb3c9d4e..36ce68d3 100644
--- a/src/System.Web.Http.OData/OData/Builder/Conventions/ConventionsHelpers.cs
+++ b/src/System.Web.Http.OData/OData/Builder/Conventions/ConventionsHelpers.cs
@@ -16,34 +16,6 @@ namespace System.Web.Http.OData.Builder.Conventions
     {
         private static HashSet<Type> _ignoredCollectionTypes = new HashSet<Type>(new Type[] { typeof(string) });
 
-        public static PropertyInfo GetKeyProperty(Type entityType, bool throwOnError = false)
-        {
-            IEnumerable<PropertyInfo> keys = entityType.GetProperties()
-                .Where(p => (p.Name.Equals(entityType.Name + "Id", StringComparison.OrdinalIgnoreCase) || p.Name.Equals("Id", StringComparison.OrdinalIgnoreCase))
-                && EdmLibHelpers.GetEdmPrimitiveTypeOrNull(p.PropertyType) != null);
-
-            if (keys.Count() == 0)
-            {
-                if (throwOnError)
-                {
-                    throw Error.InvalidOperation(SRResources.NoKeyFound, entityType.FullName);
-                }
-            }
-            else if (keys.Count() > 1)
-            {
-                if (throwOnError)
-                {
-                    throw Error.InvalidOperation(SRResources.MultipleKeysFound, entityType.FullName);
-                }
-            }
-            else
-            {
-                return keys.Single();
-            }
-
-            return null;
-        }
-
         public static string GetEntityKeyValue(EntityInstanceContext entityContext, IEntityTypeConfiguration entityTypeConfiguration)
         {
             // TODO: BUG 453795: reflection cleanup
diff --git a/src/System.Web.Http.OData/OData/Builder/Conventions/EntityKeyConvention.cs b/src/System.Web.Http.OData/OData/Builder/Conventions/EntityKeyConvention.cs
index 63ccf00e..1a1cea2d 100644
--- a/src/System.Web.Http.OData/OData/Builder/Conventions/EntityKeyConvention.cs
+++ b/src/System.Web.Http.OData/OData/Builder/Conventions/EntityKeyConvention.cs
@@ -1,12 +1,22 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.txt in the project root for license information.
 
+using System.Collections.Generic;
 using System.Linq;
-using System.Reflection;
+using System.Web.Http.OData.Formatter;
 
 namespace System.Web.Http.OData.Builder.Conventions
 {
+    /// <summary>
+    /// <see cref="EntityTypeConvention"/> for figuring out the entity keys.
+    /// <remarks>This convention configures properties that are named 'ID' (case-insensitive) or {EntityName}+ID (case-insensitive) as the key.</remarks>
+    /// </summary>
     public class EntityKeyConvention : EntityTypeConvention
     {
+        /// <summary>
+        /// Figures out the key properties and marks them as Keys in the EDM model.
+        /// </summary>
+        /// <param name="entity">The entity type being configured.</param>
+        /// <param name="model">The <see cref="ODataModelBuilder"/>.</param>
         public override void Apply(IEntityTypeConfiguration entity, ODataModelBuilder model)
         {
             if (entity == null)
@@ -14,11 +24,30 @@ namespace System.Web.Http.OData.Builder.Conventions
                 throw Error.ArgumentNull("entity");
             }
 
-            PropertyInfo key = ConventionsHelpers.GetKeyProperty(entity.ClrType);
-            if (key != null && !entity.IgnoredProperties.Contains(key))
+            // Try to figure out keys only if there is no base type.
+            if (entity.BaseType == null)
             {
-                entity.HasKey(key);
+                PropertyConfiguration key = GetKeyProperty(entity);
+                if (key != null)
+                {
+                    entity.HasKey(key.PropertyInfo);
+                }
             }
         }
+
+        private static PropertyConfiguration GetKeyProperty(IEntityTypeConfiguration entityType)
+        {
+            IEnumerable<PropertyConfiguration> keys =
+                entityType.Properties
+                .Where(p => (p.Name.Equals(entityType.Name + "Id", StringComparison.OrdinalIgnoreCase) || p.Name.Equals("Id", StringComparison.OrdinalIgnoreCase))
+                && EdmLibHelpers.GetEdmPrimitiveTypeOrNull(p.PropertyInfo.PropertyType) != null);
+
+            if (keys.Count() == 1)
+            {
+                return keys.Single();
+            }
+
+            return null;
+        }
     }
 }
diff --git a/src/System.Web.Http.OData/Properties/SRResources.Designer.cs b/src/System.Web.Http.OData/Properties/SRResources.Designer.cs
index 7537b7e3..845e2979 100644
--- a/src/System.Web.Http.OData/Properties/SRResources.Designer.cs
+++ b/src/System.Web.Http.OData/Properties/SRResources.Designer.cs
@@ -592,15 +592,6 @@ namespace System.Web.Http.OData.Properties {
         }
         
         /// <summary>
-        ///   Looks up a localized string similar to Entity type &apos;{0}&apos; has multiple keys..
-        /// </summary>
-        internal static string MultipleKeysFound {
-            get {
-                return ResourceManager.GetString("MultipleKeysFound", resourceCulture);
-            }
-        }
-        
-        /// <summary>
         ///   Looks up a localized string similar to More than one matching clr type found for the Edm type {0}.\nThe matching clr types are {1}..
         /// </summary>
         internal static string MultipleMatchingClrTypesForEdmType {
@@ -691,15 +682,6 @@ namespace System.Web.Http.OData.Properties {
         }
         
         /// <summary>
-        ///   Looks up a localized string similar to Entity type &apos;{0}&apos; has no keys..
-        /// </summary>
-        internal static string NoKeyFound {
-            get {
-                return ResourceManager.GetString("NoKeyFound", resourceCulture);
-            }
-        }
-        
-        /// <summary>
         ///   Looks up a localized string similar to No IEdmType could be found for &apos;{0}&apos;..
         /// </summary>
         internal static string NoMatchingIEdmTypeFound {
diff --git a/src/System.Web.Http.OData/Properties/SRResources.resx b/src/System.Web.Http.OData/Properties/SRResources.resx
index b5c15cb0..c6d2130a 100644
--- a/src/System.Web.Http.OData/Properties/SRResources.resx
+++ b/src/System.Web.Http.OData/Properties/SRResources.resx
@@ -270,9 +270,6 @@
   <data name="ItemMustBeOfType" xml:space="preserve">
     <value>The item must be of type '{0}'.</value>
   </data>
-  <data name="MultipleKeysFound" xml:space="preserve">
-    <value>Entity type '{0}' has multiple keys.</value>
-  </data>
   <data name="MustBeComplexProperty" xml:space="preserve">
     <value>The property '{0}' on type '{1}' must be a Complex property.</value>
   </data>
@@ -285,9 +282,6 @@
   <data name="MustHaveMatchingMultiplicity" xml:space="preserve">
     <value>The multiplicity of the '{0}' property must be '{1}'.</value>
   </data>
-  <data name="NoKeyFound" xml:space="preserve">
-    <value>Entity type '{0}' has no keys.</value>
-  </data>
   <data name="MustBePrimitiveType" xml:space="preserve">
     <value>The type '{0}' must be a primitive type.</value>
   </data>
diff --git a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ConventionsHelpersTests.cs b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ConventionsHelpersTests.cs
index 79975ef9..8a9f25eb 100644
--- a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ConventionsHelpersTests.cs
+++ b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ConventionsHelpersTests.cs
@@ -89,25 +89,6 @@ namespace System.Web.Http.OData.Builder.Conventions
             Assert.False(type.IsCollection());
         }
 
-        [Theory]
-        [InlineData(typeof(GetKeyProperty_validEntityType_TestClass_Id), "Id")]
-        [InlineData(typeof(GetKeyProperty_validEntityType_TestClass2_ClassName), "GetKeyProperty_validEntityType_TestClass2_ClassNameId")]
-        public void GetKeyProperty_ValidEntityType(Type type, string propertyName)
-        {
-            var key = ConventionsHelpers.GetKeyProperty(type, throwOnError: false);
-            Assert.NotNull(key);
-            Assert.Equal(key.Name, propertyName);
-        }
-
-        [Theory]
-        [InlineData(typeof(GetKeyProperty_InValidEntityType_NoId))]
-        [InlineData(typeof(GetKeyProperty_InValidEntityType_ComplexId))]
-        public void GetKeyProperty_InValidEntityType(Type type)
-        {
-            var key = ConventionsHelpers.GetKeyProperty(type, throwOnError: false);
-            Assert.Null(key);
-        }
-
         [Fact]
         public void GetProperties_ReturnsProperties_FromBaseAndDerived()
         {
@@ -316,7 +297,7 @@ namespace System.Web.Http.OData.Builder.Conventions
 
         private class GetKeyProperty_InValidEntityType_ComplexId
         {
-            public Type Id { get; set; }
+            public GetProperties_Complex Id { get; set; }
         }
     }
 
diff --git a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/EntityKeyConventionTests.cs b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/EntityKeyConventionTests.cs
index a16ac788..58859610 100644
--- a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/EntityKeyConventionTests.cs
+++ b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/EntityKeyConventionTests.cs
@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.txt in the project root for license information.
 
-using System.Linq;
-using System.Reflection;
+using System.ComponentModel.DataAnnotations.Schema;
+using Microsoft.Data.Edm;
 using Microsoft.TestCommon;
 using Moq;
 
@@ -9,18 +9,21 @@ namespace System.Web.Http.OData.Builder.Conventions
 {
     public class EntityKeyConventionTests
     {
-        [Fact]
-        public void Apply_Calls_HasKey_OnEdmType()
+        [Theory]
+        [InlineData("ID")]
+        [InlineData("Id")]
+        [InlineData("iD")]
+        [InlineData("SampleEntityID")]
+        public void Apply_Calls_HasKey_OnEdmType(string propertyName)
         {
             // Arrange
-            var mockEntityType = new Mock<IEntityTypeConfiguration>(MockBehavior.Strict);
-            mockEntityType
-                .Setup(edmType => edmType.ClrType)
-                .Returns(typeof(EntityKeyConventionTests_EntityType));
-            mockEntityType
-                .Setup(edmType => edmType.IgnoredProperties)
-                .Returns(Enumerable.Empty<PropertyInfo>());
-            mockEntityType.Setup(entityType => entityType.HasKey(typeof(EntityKeyConventionTests_EntityType).GetProperty("ID"))).Returns(mockEntityType.Object);
+            Mock<IEntityTypeConfiguration> mockEntityType = new Mock<IEntityTypeConfiguration>();
+            Mock<PropertyConfiguration> property = new Mock<PropertyConfiguration>(typeof(EntityKeyConventionTests_EntityType).GetProperty(propertyName), mockEntityType.Object);
+
+            mockEntityType.Setup(e => e.Properties).Returns(new[] { property.Object });
+            mockEntityType.Setup(e => e.Name).Returns("SampleEntity");
+
+            mockEntityType.Setup(entityType => entityType.HasKey(typeof(EntityKeyConventionTests_EntityType).GetProperty(propertyName))).Returns(mockEntityType.Object).Verifiable();
 
             var mockModelBuilder = new Mock<ODataModelBuilder>(MockBehavior.Strict);
 
@@ -31,9 +34,72 @@ namespace System.Web.Http.OData.Builder.Conventions
             mockEntityType.Verify();
         }
 
+        [Fact]
+        public void EntityKeyConvention_FiguresOutTheKeyProperty()
+        {
+            MockType baseType =
+                new MockType("BaseType")
+                .Property<uint>("ID");
+
+            ODataConventionModelBuilder builder = new ODataConventionModelBuilder();
+            builder.AddEntity(baseType);
+
+            IEdmModel model = builder.GetEdmModel();
+
+            IEdmEntityType entity = model.AssertHasEntityType(baseType);
+            entity.AssertHasKey(model, "ID", EdmPrimitiveTypeKind.Int64);
+        }
+
+        [Fact]
+        public void EntityKeyConvention_DoesnotFigureOutKeyPropertyOnDerivedTypes()
+        {
+            MockType baseType =
+                new MockType("BaseType")
+                .Property<uint>("ID");
+
+            MockType derivedType =
+                new MockType("DerivedType")
+                .Property<int>("DerivedTypeID")
+                .BaseType(baseType);
+
+            ODataConventionModelBuilder builder = new ODataConventionModelBuilder();
+            builder.AddEntity(derivedType).DerivesFrom(builder.AddEntity(baseType));
+
+            IEdmModel model = builder.GetEdmModel();
+
+            IEdmEntityType baseEntity = model.AssertHasEntityType(baseType);
+            baseEntity.AssertHasKey(model, "ID", EdmPrimitiveTypeKind.Int64);
+
+            IEdmEntityType derivedEntity = model.AssertHasEntityType(derivedType);
+            derivedEntity.AssertHasPrimitiveProperty(model, "DerivedTypeID", EdmPrimitiveTypeKind.Int32, isNullable: false);
+        }
+
+        [Fact]
+        public void EntityKeyConvention_DoesnotFigureOutKeyPropertyIfIgnored()
+        {
+            MockType baseType =
+                new MockType("BaseType")
+                .Property(typeof(int), "ID", new NotMappedAttribute());
+
+            ODataConventionModelBuilder builder = new ODataConventionModelBuilder();
+            builder.AddEntity(baseType);
+
+            IEdmModel model = builder.GetEdmModel();
+
+            IEdmEntityType baseEntity = model.AssertHasEntityType(baseType);
+            Assert.Empty(baseEntity.Properties());
+            Assert.Empty(baseEntity.Key());
+        }
+
         class EntityKeyConventionTests_EntityType
         {
             public string ID { get; set; }
+
+            public string Id { get; set; }
+
+            public string iD { get; set; }
+
+            public string SampleEntityID { get; set; }
         }
     }
 }
diff --git a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ODataConventionModelBuilderTests.cs b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ODataConventionModelBuilderTests.cs
index 588221e6..7ed989f0 100644
--- a/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ODataConventionModelBuilderTests.cs
+++ b/test/System.Web.Http.OData.Test/OData/Builder/Conventions/ODataConventionModelBuilderTests.cs
@@ -648,8 +648,8 @@ namespace System.Web.Http.OData.Builder.Conventions
         public void ModelBuilder_DerivedTypeDeclaringKeyThrows()
         {
             MockType baseType =
-                new MockType("BaseType")
-                .Property(typeof(int), "ID");
+                  new MockType("BaseType")
+                  .Property(typeof(int), "ID");
 
             MockType derivedType =
                 new MockType("DerivedType")