Fix for 20014. KeyAttribute does not override Keyless. Log warning. (#…

…20050)

Fix for #20014. KeyAttribute does not override the Keyless attribute. Log warning.

src/EFCore/Diagnostics/CoreEventId.cs

@@ -80,7 +80,7 @@ private enum Id
             ServiceProviderDebugInfo,
             RedundantAddServicesCallWarning,
 
-            // Model events
+            // Model and ModelValidation events
             ShadowPropertyCreated = CoreBaseId + 600,
             RedundantIndexRemoved,
             IncompatibleMatchingForeignKeyProperties,
@@ -102,6 +102,7 @@ private enum Id
             RequiredAttributeInverted,
             RequiredAttributeOnCollection,
             CollectionWithoutComparer,
+            ConflictingKeylessAndKeyAttributesWarning,
 
             // ChangeTracking events
             DetectChangesStarting = CoreBaseId + 800,
@@ -615,6 +616,21 @@ public static readonly EventId PossibleUnintendedReferenceComparisonWarning
         /// </summary>
         public static readonly EventId RedundantForeignKeyWarning = MakeModelValidationId(Id.RedundantForeignKeyWarning);
 
+        /// <summary>
+        ///     <para>
+        ///         A <see cref="KeylessAttribute"/> attribute on the entity type is conflicting
+        ///         with a <see cref="KeyAttribute"/> attribute on at least one of its properties.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Model" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="PropertyEventData" /> payload when used with a
+        ///         <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId ConflictingKeylessAndKeyAttributesWarning = MakeModelId(Id.ConflictingKeylessAndKeyAttributesWarning);
+
         private static readonly string _changeTrackingPrefix = DbLoggerCategory.ChangeTracking.Name + ".";
         private static EventId MakeChangeTrackingId(Id id) => new EventId((int)id, _changeTrackingPrefix + id);
 

src/EFCore/Diagnostics/CoreLoggerExtensions.cs

@@ -2912,5 +2912,41 @@ private static string ContextDisposed(EventDefinitionBase definition, EventData
             var p = (DbContextEventData)payload;
             return d.GenerateMessage(p.Context.GetType().ShortDisplayName());
         }
+
+        /// <summary>
+        ///     Logs for the <see cref="CoreEventId.ConflictingKeylessAndKeyAttributesWarning" /> event.
+        /// </summary>
+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="property"> The property which is being defined as part of a key. </param>
+        public static void ConflictingKeylessAndKeyAttributesWarning(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model> diagnostics,
+            [NotNull] IProperty property)
+        {
+            var definition = CoreResources.LogConflictingKeylessAndKeyAttributes(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, property.Name, property.DeclaringEntityType.DisplayName());
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new PropertyEventData(
+                    definition,
+                    ConflictingKeylessAndKeyAttributesWarning,
+                    property);
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string ConflictingKeylessAndKeyAttributesWarning(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition<string, string>)definition;
+            var p = (PropertyEventData)payload;
+            return d.GenerateMessage(
+                p.Property.Name,
+                p.Property.DeclaringEntityType.DisplayName());
+        }
     }
 }

src/EFCore/Diagnostics/LoggingDefinitions.cs

@@ -636,5 +636,14 @@ public abstract class LoggingDefinitions
         /// </summary>
         [EntityFrameworkInternal]
         public EventDefinitionBase LogRedundantForeignKey;
+
+        /// <summary>
+        ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+        ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+        ///     any release. You should only use it directly in your code with extreme caution and knowing that
+        ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+        /// </summary>
+        [EntityFrameworkInternal]
+        public EventDefinitionBase LogConflictingKeylessAndKeyAttributes;
     }
 }

src/EFCore/Metadata/Conventions/KeyAttributeConvention.cs

@@ -42,6 +42,21 @@ protected override void ProcessPropertyAdded(
             IConventionContext context)
         {
             var entityType = propertyBuilder.Metadata.DeclaringEntityType;
+            if (entityType.IsKeyless)
+            {
+                switch (entityType.GetIsKeylessConfigurationSource())
+                {
+                    case ConfigurationSource.DataAnnotation:
+                        Dependencies.Logger
+                            .ConflictingKeylessAndKeyAttributesWarning(propertyBuilder.Metadata);
+                        return;
+
+                    case ConfigurationSource.Explicit:
+                        // fluent API overrides the attribute - no warning
+                        return;
+                }
+            }
+
             if (entityType.BaseType != null)
             {
                 return;

src/EFCore/Properties/CoreStrings.resx

@@ -1269,4 +1269,8 @@
   <data name="LastUsedWithoutOrderBy" xml:space="preserve">
     <value>Queries performing '{method}' operation must have a deterministic sort order. Rewrite the query to apply an OrderBy clause on the sequence before calling '{method}'.</value>
   </data>
+  <data name="LogConflictingKeylessAndKeyAttributes" xml:space="preserve">
+    <value>Conflicting attributes have been applied: the 'Key' attribute on property '{property}' and the 'Keyless' attribute on its entity '{entity}'. Note that the entity will have no key unless you use fluent API to override this.</value>
+    <comment>Warning CoreEventId.ConflictingKeylessAndKeyAttributesWarning string string</comment>
+  </data>
 </root>

test/EFCore.Specification.Tests/DataAnnotationTestBase.cs

@@ -765,6 +765,49 @@ public virtual ModelBuilder Key_specified_on_multiple_properties_can_be_overridd
             return modelBuilder;
         }
 
+        [ConditionalFact]
+        public virtual void Keyless_and_key_attributes_which_conflict_cause_warning()
+        {
+            var modelBuilder = CreateModelBuilder();
+            var entity = modelBuilder.Entity<KeylessAndKeyAttributes>();
+
+            Assert.True(entity.Metadata.IsKeyless);
+            Assert.Null(entity.Metadata.FindPrimaryKey());
+
+            var logEntry = Fixture.ListLoggerFactory.Log.Single();
+            Assert.Equal(LogLevel.Warning, logEntry.Level);
+            Assert.Equal(
+                CoreResources.LogConflictingKeylessAndKeyAttributes(new TestLogger<TestLoggingDefinitions>())
+                    .GenerateMessage("NotAKey", nameof(KeylessAndKeyAttributes)),
+                logEntry.Message);
+        }
+
+        [ConditionalFact]
+        public virtual void Keyless_fluent_api_and_key_attribute_do_not_cause_warning()
+        {
+            var modelBuilder = CreateModelBuilder();
+            var entity = modelBuilder.Entity<KeylessFluentApiAndKeyAttribute>();
+            entity.HasNoKey();
+
+            Assert.True(entity.Metadata.IsKeyless);
+            Assert.Null(entity.Metadata.FindPrimaryKey());
+
+            Assert.Empty(Fixture.ListLoggerFactory.Log);
+        }
+
+        [ConditionalFact]
+        public virtual void Key_fluent_api_and_keyless_attribute_do_not_cause_warning()
+        {
+            var modelBuilder = CreateModelBuilder();
+            var entity = modelBuilder.Entity<KeyFluentApiAndKeylessAttribute>();
+            entity.HasKey("MyKey");
+
+            Assert.False(entity.Metadata.IsKeyless);
+            Assert.NotNull(entity.Metadata.FindPrimaryKey());
+
+            Assert.Empty(Fixture.ListLoggerFactory.Log);
+        }
+
         private class CompositeKeyAttribute
         {
             [Key]
@@ -2319,14 +2362,18 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
                 modelBuilder.Ignore<BookLabel>();
                 modelBuilder.Entity<BookDetails>();
                 modelBuilder.Entity<Book>().Property(d => d.Id).ValueGeneratedNever();
+                modelBuilder.Entity<KeylessAndKeyAttributes>();
+                modelBuilder.Entity<KeylessFluentApiAndKeyAttribute>();
+                modelBuilder.Entity<KeyFluentApiAndKeylessAttribute>();
             }
 
             public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
                 => base.AddOptions(builder).ConfigureWarnings(
                     c => c
                         .Log(CoreEventId.ConflictingForeignKeyAttributesOnNavigationAndPropertyWarning)
                         .Log(CoreEventId.ForeignKeyAttributesOnBothNavigationsWarning)
-                        .Log(CoreEventId.ForeignKeyAttributesOnBothPropertiesWarning));
+                        .Log(CoreEventId.ForeignKeyAttributesOnBothPropertiesWarning)
+                        .Log(CoreEventId.ConflictingKeylessAndKeyAttributesWarning));
 
             protected override bool ShouldLogCategory(string logCategory)
                 => logCategory == DbLoggerCategory.Model.Name;
@@ -2404,5 +2451,24 @@ protected class Details
         {
             public string Name { get; set; }
         }
+
+        [Keyless]
+        protected class KeylessAndKeyAttributes
+        {
+            [Key]
+            public int NotAKey { get; set; }
+        }
+
+        protected class KeylessFluentApiAndKeyAttribute
+        {
+            [Key]
+            public int NotAKey { get; set; }
+        }
+
+        [Keyless]
+        protected class KeyFluentApiAndKeylessAttribute
+        {
+            public int MyKey { get; set; }
+        }
     }
 }

test/EFCore.Tests/Metadata/Conventions/EntityTypeAttributeConventionTest.cs

@@ -120,7 +120,7 @@ public void KeylessAttribute_can_be_overriden_using_explicit_configuration()
         }
 
         [ConditionalFact]
-        public void KeyAttribute_overrides_keyless_attribute()
+        public void KeyAttribute_does_not_override_keyless_attribute()
         {
             var modelBuilder = InMemoryTestHelpers.Instance.CreateConventionBuilder();