When entities are discovered through relationships the conventions ru…

…n in a different order. This can cause the KeyAttributeConvention to run on derived types before the base. Move the check to validation to avoid the false positive. Fixes #5898
dotnet · Jul 6, 2016 · 5ce5b86 · 5ce5b86
1 parent c65503b
commit 5ce5b86
Show file tree

Hide file tree

Showing 8 changed files with 149 additions and 13 deletions.
diff --git a/src/Microsoft.EntityFrameworkCore.Specification.Tests/DataAnnotationFixtureBase.cs b/src/Microsoft.EntityFrameworkCore.Specification.Tests/DataAnnotationFixtureBase.cs
@@ -4,6 +4,7 @@
 using System;
 using System.ComponentModel.DataAnnotations;
 using System.ComponentModel.DataAnnotations.Schema;
+using Microsoft.EntityFrameworkCore.Internal;
 
 namespace Microsoft.EntityFrameworkCore.Specification.Tests
 {
@@ -14,6 +15,8 @@ public abstract class DataAnnotationFixtureBase<TTestStore>
 
         public abstract DataAnnotationContext CreateContext(TTestStore testStore);
 
+        public abstract ModelValidator ThrowingValidator { get; }
+
         protected virtual void OnModelCreating(ModelBuilder modelBuilder)
         {
         }

diff --git a/src/Microsoft.EntityFrameworkCore.Specification.Tests/DataAnnotationTestBase.cs b/src/Microsoft.EntityFrameworkCore.Specification.Tests/DataAnnotationTestBase.cs
@@ -2,7 +2,13 @@
 // 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.ComponentModel.DataAnnotations;
 using System.Linq;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Metadata.Conventions.Internal;
+using Microsoft.EntityFrameworkCore.Storage;
 using Xunit;
 
 namespace Microsoft.EntityFrameworkCore.Specification.Tests
@@ -25,6 +31,53 @@ protected DataAnnotationTestBase(TFixture fixture)
         protected TTestStore TestStore { get; }
 
         public void Dispose() => TestStore.Dispose();
+
+        public virtual ModelBuilder CreateModelBuilder()
+        {
+            var context = CreateContext();
+            var conventionSetBuilder = context.GetService<IDatabaseProviderServices>().ConventionSetBuilder;
+            var conventionSet = context.GetService<ICoreConventionSetBuilder>().CreateConventionSet();
+            conventionSet = conventionSetBuilder == null
+                ? conventionSet
+                : conventionSetBuilder.AddConventions(conventionSet);
+            return new ModelBuilder(conventionSet);
+        }
+
+        protected virtual void Validate(IModel model) => Fixture.ThrowingValidator.Validate(model);
+
+        [Fact]
+        public virtual void Key_from_base_type_is_recognized_if_discovered_through_relationship()
+        {
+            var builder = CreateModelBuilder();
+
+            builder.Entity<SRelated>();
+
+            Validate(builder.Model);
+            Assert.True(GetProperty<OKeyBase>(builder, nameof(OKeyBase.OrderLineNo)).IsPrimaryKey());
+            Assert.True(GetProperty<DODerived>(builder, nameof(DODerived.OrderLineNo)).IsPrimaryKey());
+        }
+
+        protected class SRelated
+        {
+            public int SRelatedId { get; set; }
+
+            public ICollection<OKeyBase> OKeyBases { get; set; }
+            public ICollection<DODerived> DADeriveds { get; set; }
+        }
+
+        protected class OKeyBase
+        {
+            [Key]
+            public int OrderLineNo { get; set; }
+
+            public int Quantity { get; set; }
+        }
+
+        protected class DODerived : OKeyBase
+        {
+            public SRelated DARelated { get; set; }
+            public string Special { get; set; }
+        }
 
         [Fact]
         public virtual void ConcurrencyCheckAttribute_throws_if_value_in_database_changed()
@@ -193,5 +246,8 @@ public virtual void TimestampAttribute_throws_if_value_in_database_changed()
                 Assert.Throws<DbUpdateConcurrencyException>(() => context.SaveChanges());
             }
         }
+
+        protected static IMutableProperty GetProperty<TEntity>(ModelBuilder modelBuilder, string name)
+            => modelBuilder.Model.FindEntityType(typeof(TEntity)).FindProperty(name);
     }
 }
diff --git a/src/Microsoft.EntityFrameworkCore/Metadata/Conventions/Internal/KeyAttributeConvention.cs b/src/Microsoft.EntityFrameworkCore/Metadata/Conventions/Internal/KeyAttributeConvention.cs
@@ -30,7 +30,7 @@ public override InternalPropertyBuilder Apply(InternalPropertyBuilder propertyBu
             var entityType = propertyBuilder.Metadata.DeclaringEntityType;
             if (entityType.BaseType != null)
             {
-                throw new InvalidOperationException(CoreStrings.KeyAttributeOnDerivedEntity(entityType.DisplayName(), propertyBuilder.Metadata.Name));
+                return propertyBuilder;
             }
 
             var entityTypeBuilder = entityType.Builder;
@@ -64,20 +64,33 @@ public override InternalPropertyBuilder Apply(InternalPropertyBuilder propertyBu
         public virtual InternalModelBuilder Apply(InternalModelBuilder modelBuilder)
         {
             var entityTypes = modelBuilder.Metadata.GetEntityTypes();
-            foreach (var entityType in entityTypes.Where(et => et.BaseType == null))
+            foreach (var entityType in entityTypes)
             {
-                var currentPrimaryKey = entityType.FindPrimaryKey();
-                if ((currentPrimaryKey != null)
-                    && (currentPrimaryKey.Properties.Count > 1))
+                if (entityType.BaseType == null)
                 {
-                    var newKey = entityType.Builder.PrimaryKey(
-                        new List<string> { currentPrimaryKey.Properties.First().Name }, ConfigurationSource.DataAnnotation);
-                    if (newKey != null)
+                    var currentPrimaryKey = entityType.FindPrimaryKey();
+                    if (currentPrimaryKey != null
+                        && currentPrimaryKey.Properties.Count > 1
+                        && entityType.GetPrimaryKeyConfigurationSource() == ConfigurationSource.DataAnnotation)
                     {
                         throw new InvalidOperationException(CoreStrings.CompositePKWithDataAnnotation(entityType.DisplayName()));
                     }
                 }
+                else
+                {
+                    foreach (var declaredProperty in entityType.GetDeclaredProperties())
+                    {
+                        var propertyInfo = declaredProperty.PropertyInfo;
+                        var attributes = propertyInfo?.GetCustomAttributes<KeyAttribute>(true);
+                        if (attributes?.Any() == true)
+                        {
+                            throw new InvalidOperationException(
+                                CoreStrings.KeyAttributeOnDerivedEntity(entityType.DisplayName(), declaredProperty.Name));
+                        }
+                    }
+                }
             }
+
             return modelBuilder;
         }
     }

diff --git a/...icrosoft.EntityFrameworkCore/Metadata/Conventions/Internal/PropertyAttributeConvention.cs b/...icrosoft.EntityFrameworkCore/Metadata/Conventions/Internal/PropertyAttributeConvention.cs
@@ -25,10 +25,7 @@ public virtual InternalPropertyBuilder Apply(InternalPropertyBuilder propertyBui
         {
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
 
-            var clrType = propertyBuilder.Metadata.DeclaringEntityType.ClrType;
-            var propertyName = propertyBuilder.Metadata.Name;
-
-            var propertyInfo = clrType?.GetRuntimeProperties().FirstOrDefault(pi => pi.Name == propertyName);
+            var propertyInfo = propertyBuilder.Metadata.PropertyInfo;
             var attributes = propertyInfo?.GetCustomAttributes<TAttribute>(true);
             if (attributes != null)
             {

diff --git a/test/Microsoft.EntityFrameworkCore.InMemory.FunctionalTests/DataAnnotationInMemoryFixture.cs b/test/Microsoft.EntityFrameworkCore.InMemory.FunctionalTests/DataAnnotationInMemoryFixture.cs
@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.EntityFrameworkCore.Internal;
 using Microsoft.EntityFrameworkCore.Specification.Tests;
 using Microsoft.Extensions.DependencyInjection;
 
@@ -18,9 +19,22 @@ public DataAnnotationInMemoryFixture()
             _serviceProvider = new ServiceCollection()
                 .AddEntityFrameworkInMemoryDatabase()
                 .AddSingleton(TestInMemoryModelSource.GetFactory(OnModelCreating))
+                .AddSingleton<ThrowingModelValidator>()
                 .BuildServiceProvider();
         }
 
+        public override ModelValidator ThrowingValidator
+            => _serviceProvider.GetService<ThrowingModelValidator>();
+
+        // ReSharper disable once ClassNeverInstantiated.Local
+        private class ThrowingModelValidator : ModelValidator
+        {
+            protected override void ShowWarning(string message)
+            {
+                throw new InvalidOperationException(message);
+            }
+        }
+
         public override InMemoryTestStore CreateTestStore()
         {
             return InMemoryTestStore.GetOrCreateShared(DatabaseName, () =>

diff --git a/...Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests/DataAnnotationSqlServerFixture.cs b/...Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests/DataAnnotationSqlServerFixture.cs
@@ -2,8 +2,12 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.EntityFrameworkCore.Internal;
+using Microsoft.EntityFrameworkCore.Metadata;
 using Microsoft.EntityFrameworkCore.Specification.Tests;
 using Microsoft.EntityFrameworkCore.SqlServer.FunctionalTests.Utilities;
+using Microsoft.EntityFrameworkCore.Storage;
+using Microsoft.EntityFrameworkCore.Storage.Internal;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 
@@ -26,6 +30,28 @@ public DataAnnotationSqlServerFixture()
                 .BuildServiceProvider();
         }
 
+        public override ModelValidator ThrowingValidator
+            => new ThrowingModelValidator(
+                _serviceProvider.GetService<ILogger<RelationalModelValidator>>(),
+                new SqlServerAnnotationProvider(),
+                new SqlServerTypeMapper());
+
+        private class ThrowingModelValidator : RelationalModelValidator
+        {
+            public ThrowingModelValidator(
+                ILogger<RelationalModelValidator> loggerFactory,
+                IRelationalAnnotationProvider relationalExtensions,
+                IRelationalTypeMapper typeMapper)
+                : base(loggerFactory, relationalExtensions, typeMapper)
+            {
+            }
+
+            protected override void ShowWarning(string message)
+            {
+                throw new InvalidOperationException(message);
+            }
+        }
+
         public override SqlServerTestStore CreateTestStore()
         {
             return SqlServerTestStore.GetOrCreateShared(DatabaseName, () =>

diff --git a/test/Microsoft.EntityFrameworkCore.Sqlite.FunctionalTests/DataAnnotationSqliteFixture.cs b/test/Microsoft.EntityFrameworkCore.Sqlite.FunctionalTests/DataAnnotationSqliteFixture.cs
@@ -2,7 +2,12 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using Microsoft.EntityFrameworkCore.Internal;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Metadata.Internal;
 using Microsoft.EntityFrameworkCore.Specification.Tests;
+using Microsoft.EntityFrameworkCore.Storage;
+using Microsoft.EntityFrameworkCore.Storage.Internal;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 
@@ -25,6 +30,28 @@ public DataAnnotationSqliteFixture()
                 .BuildServiceProvider();
         }
 
+        public override ModelValidator ThrowingValidator
+            => new ThrowingModelValidator(
+                _serviceProvider.GetService<ILogger<RelationalModelValidator>>(),
+                new SqliteAnnotationProvider(),
+                new SqliteTypeMapper());
+
+        private class ThrowingModelValidator : RelationalModelValidator
+        {
+            public ThrowingModelValidator(
+                ILogger<RelationalModelValidator> loggerFactory,
+                IRelationalAnnotationProvider relationalExtensions,
+                IRelationalTypeMapper typeMapper)
+                : base(loggerFactory, relationalExtensions, typeMapper)
+            {
+            }
+
+            protected override void ShowWarning(string message)
+            {
+                throw new InvalidOperationException(message);
+            }
+        }
+
         public override SqliteTestStore CreateTestStore()
             => SqliteTestStore.GetOrCreateShared(DatabaseName, () =>
                 {

diff --git a/...ntityFrameworkCore.Tests/Metadata/Conventions/Internal/PropertyAttributeConventionTest.cs b/...ntityFrameworkCore.Tests/Metadata/Conventions/Internal/PropertyAttributeConventionTest.cs
@@ -192,7 +192,7 @@ public void KeyAttribute_throws_when_setting_key_in_derived_type()
 
             Assert.Equal(
                 CoreStrings.KeyAttributeOnDerivedEntity(derivedEntityTypeBuilder.Metadata.DisplayName(), propertyBuilder.Metadata.Name),
-                Assert.Throws<InvalidOperationException>(() => new KeyAttributeConvention().Apply(propertyBuilder)).Message);
+                Assert.Throws<InvalidOperationException>(() => new KeyAttributeConvention().Apply(derivedEntityTypeBuilder.ModelBuilder)).Message);
         }
 
         [Fact]