diff --git a/src/EFCore/Metadata/Builders/IConventionEntityTypeBuilder.cs b/src/EFCore/Metadata/Builders/IConventionEntityTypeBuilder.cs index 5b5b81f1f98..4f7cdf41492 100644 --- a/src/EFCore/Metadata/Builders/IConventionEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Builders/IConventionEntityTypeBuilder.cs @@ -228,6 +228,37 @@ IConventionServicePropertyBuilder ServiceProperty( /// if the entity type can be marked as keyless. bool CanRemoveKey(bool fromDataAnnotation = false); + /// + /// Configures an index on the specified property names. + /// If there is an existing index on the given list of properyt names, + /// then the existing index will be returned for configuration. + /// + /// The names of the properties that make up the index. + /// Indicates whether the configuration was specified using a data annotation. + /// + /// An object that can be used to configure the index if it exists on the entity type, + /// otherwise. + /// + IConventionIndexBuilder HasIndex( + [NotNull] IReadOnlyList propertyNames, bool fromDataAnnotation = false); + + /// + /// Configures an index on the specified property names. + /// If there is an existing index on the given list of properyt names, + /// then the existing index will be returned for configuration. + /// + /// The names of the properties that make up the index. + /// The name of the index. + /// Indicates whether the configuration was specified using a data annotation. + /// + /// An object that can be used to configure the index if it exists on the entity type, + /// otherwise. + /// + IConventionIndexBuilder HasIndex( + [NotNull] IReadOnlyList propertyNames, + [NotNull] string name, + bool fromDataAnnotation = false); + /// /// Configures an index on the specified properties. /// If there is an existing index on the given list of properties, diff --git a/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs b/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs index d908053dc29..b3f461343d7 100644 --- a/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs +++ b/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Linq; using System.Reflection; using JetBrains.Annotations; using Microsoft.EntityFrameworkCore; @@ -18,7 +17,7 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions /// A convention that configures database indexes based on the . /// public class IndexAttributeConvention : IEntityTypeAddedConvention, - IEntityTypeBaseTypeChangedConvention, IPropertyAddedConvention, IModelFinalizingConvention + IEntityTypeBaseTypeChangedConvention, IModelFinalizingConvention { /// /// Creates a new instance of . @@ -39,8 +38,7 @@ public virtual void ProcessEntityTypeAdded( IConventionEntityTypeBuilder entityTypeBuilder, IConventionContext context) { - CheckIndexAttributesAndEnsureIndex( - new[] { entityTypeBuilder.Metadata }, true); + CheckIndexAttributesAndEnsureIndex(entityTypeBuilder.Metadata, false); } /// @@ -50,17 +48,12 @@ public virtual void ProcessEntityTypeBaseTypeChanged( IConventionEntityType oldBaseType, IConventionContext context) { - CheckIndexAttributesAndEnsureIndex( - entityTypeBuilder.Metadata.GetDerivedTypesInclusive(), true); - } + if (oldBaseType == null) + { + return; + } - /// - public virtual void ProcessPropertyAdded( - IConventionPropertyBuilder propertyBuilder, - IConventionContext context) - { - CheckIndexAttributesAndEnsureIndex( - propertyBuilder.Metadata.DeclaringEntityType.GetDerivedTypesInclusive(), true); + CheckIndexAttributesAndEnsureIndex(entityTypeBuilder.Metadata, false); } /// @@ -68,101 +61,143 @@ public virtual void ProcessModelFinalizing( IConventionModelBuilder modelBuilder, IConventionContext context) { - CheckIndexAttributesAndEnsureIndex( - modelBuilder.Metadata.GetEntityTypes(), false); + foreach (var entityType in modelBuilder.Metadata.GetEntityTypes()) + { + CheckIndexAttributesAndEnsureIndex(entityType, true); + } } private void CheckIndexAttributesAndEnsureIndex( - IEnumerable entityTypes, - bool shouldEnsureIndexOrFailSilently) + IConventionEntityType entityType, + bool shouldThrow) { - foreach (var entityType in entityTypes) + if (entityType.ClrType != null) { - if (entityType.ClrType != null) + foreach (var indexAttribute in + entityType.ClrType.GetCustomAttributes(true)) { - var ignoredMembers = entityType.GetIgnoredMembers(); - foreach (var indexAttribute in - entityType.ClrType.GetCustomAttributes(true)) + IConventionIndexBuilder indexBuilder = null; + if (!shouldThrow) { var indexProperties = new List(); foreach (var propertyName in indexAttribute.PropertyNames) { - if (ignoredMembers.Contains(propertyName)) + // TODO Change this to the IsIgnored() which takes + // fromDataAnnotation, when bug 21220 is fixed. + if (entityType.IsIgnored(propertyName)) { - if (shouldEnsureIndexOrFailSilently) - { - return; - } - - if (indexAttribute.Name == null) - { - throw new InvalidOperationException( - CoreStrings.UnnamedIndexDefinedOnIgnoredProperty( - entityType.DisplayName(), - indexAttribute.PropertyNames.Format(), - propertyName)); - } - else - { - throw new InvalidOperationException( - CoreStrings.NamedIndexDefinedOnIgnoredProperty( - indexAttribute.Name, - entityType.DisplayName(), - indexAttribute.PropertyNames.Format(), - propertyName)); - } + return; } var property = entityType.FindProperty(propertyName); if (property == null) { - if (shouldEnsureIndexOrFailSilently) - { - return; - } - - if (indexAttribute.Name == null) - { - throw new InvalidOperationException( - CoreStrings.UnnamedIndexDefinedOnNonExistentProperty( - entityType.DisplayName(), - indexAttribute.PropertyNames.Format(), - propertyName)); - } - else - { - throw new InvalidOperationException( - CoreStrings.NamedIndexDefinedOnNonExistentProperty( - indexAttribute.Name, - entityType.DisplayName(), - indexAttribute.PropertyNames.Format(), - propertyName)); - } + return; } - if (shouldEnsureIndexOrFailSilently) - { - indexProperties.Add(property); - } + indexProperties.Add(property); } - if (shouldEnsureIndexOrFailSilently) + indexBuilder = indexAttribute.Name == null + ? entityType.Builder.HasIndex( + indexProperties, fromDataAnnotation: true) + : entityType.Builder.HasIndex( + indexProperties, indexAttribute.Name, fromDataAnnotation: true); + } + else + { + // TODO See bug 21220 - we have to do this _before_ calling + // HasIndex() because during the call to HasIndex() + // IsIgnored (wrongly) returns false and we would end up + // creating a property where we shouldn't. + CheckIgnoredProperties(indexAttribute, entityType); + + try { - var indexBuilder = indexAttribute.Name == null + // Using the HasIndex(propertyNames) overload gives us + // a chance to create a missing property + // e.g. if the CLR property existed but was non-public. + indexBuilder = indexAttribute.Name == null ? entityType.Builder.HasIndex( - indexProperties, fromDataAnnotation: true) + indexAttribute.PropertyNames, fromDataAnnotation: true) : entityType.Builder.HasIndex( - indexProperties, indexAttribute.Name, fromDataAnnotation: true); + indexAttribute.PropertyNames, indexAttribute.Name, fromDataAnnotation: true); + } + catch(InvalidOperationException exception) + { + CheckMissingProperties(indexAttribute, entityType, exception); - if (indexBuilder != null) - { - if (indexAttribute.GetIsUnique().HasValue) - { - indexBuilder.IsUnique(indexAttribute.GetIsUnique().Value, fromDataAnnotation: true); - } - } + throw; } } + + if (indexBuilder != null + && indexAttribute.GetIsUnique().HasValue) + { + indexBuilder.IsUnique(indexAttribute.GetIsUnique().Value, fromDataAnnotation: true); + } + } + } + } + + private void CheckIgnoredProperties( + IndexAttribute indexAttribute, + IConventionEntityType entityType) + { + foreach (var propertyName in indexAttribute.PropertyNames) + { + if (entityType.IsIgnored(propertyName)) + { + if (indexAttribute.Name == null) + { + throw new InvalidOperationException( + CoreStrings.UnnamedIndexDefinedOnIgnoredProperty( + entityType.DisplayName(), + indexAttribute.PropertyNames.Format(), + propertyName)); + } + else + { + throw new InvalidOperationException( + CoreStrings.NamedIndexDefinedOnIgnoredProperty( + indexAttribute.Name, + entityType.DisplayName(), + indexAttribute.PropertyNames.Format(), + propertyName)); + } + } + } + } + + private void CheckMissingProperties( + IndexAttribute indexAttribute, + IConventionEntityType entityType, + InvalidOperationException innerException) + { + foreach (var propertyName in indexAttribute.PropertyNames) + { + var property = entityType.FindProperty(propertyName); + if (property == null) + { + if (indexAttribute.Name == null) + { + throw new InvalidOperationException( + CoreStrings.UnnamedIndexDefinedOnNonExistentProperty( + entityType.DisplayName(), + indexAttribute.PropertyNames.Format(), + propertyName), + innerException); + } + else + { + throw new InvalidOperationException( + CoreStrings.NamedIndexDefinedOnNonExistentProperty( + indexAttribute.Name, + entityType.DisplayName(), + indexAttribute.PropertyNames.Format(), + propertyName), + innerException); + } } } } diff --git a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs index 24d6a63a122..8e608207fc5 100644 --- a/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs +++ b/src/EFCore/Metadata/Conventions/Infrastructure/ProviderConventionSetBuilder.cs @@ -126,7 +126,6 @@ public virtual ConventionSet CreateConventionSet() conventionSet.PropertyAddedConventions.Add(backingFieldAttributeConvention); conventionSet.PropertyAddedConventions.Add(keyAttributeConvention); conventionSet.PropertyAddedConventions.Add(keyDiscoveryConvention); - conventionSet.PropertyAddedConventions.Add(indexAttributeConvention); conventionSet.PropertyAddedConventions.Add(foreignKeyPropertyDiscoveryConvention); conventionSet.EntityTypePrimaryKeyChangedConventions.Add(foreignKeyPropertyDiscoveryConvention); diff --git a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs index 049e92329c4..d7b64ad8ce3 100644 --- a/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs +++ b/src/EFCore/Metadata/Internal/InternalEntityTypeBuilder.cs @@ -4455,6 +4455,33 @@ IConventionEntityTypeBuilder IConventionEntityTypeBuilder.HasNoKey(IConventionKe bool IConventionEntityTypeBuilder.CanRemoveKey(bool fromDataAnnotation) => CanRemoveKey(fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); + /// + /// 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. + /// + [DebuggerStepThrough] + IConventionIndexBuilder IConventionEntityTypeBuilder.HasIndex( + IReadOnlyList propertyNames, bool fromDataAnnotation) + => HasIndex( + propertyNames, + fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); + + /// + /// 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. + /// + [DebuggerStepThrough] + IConventionIndexBuilder IConventionEntityTypeBuilder.HasIndex( + IReadOnlyList propertyNames, string name, bool fromDataAnnotation) + => HasIndex( + propertyNames, + name, + fromDataAnnotation ? ConfigurationSource.DataAnnotation : ConfigurationSource.Convention); + /// /// 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 diff --git a/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs index c508c8a15dc..f812da95a6d 100644 --- a/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs @@ -38,8 +38,6 @@ public void IndexAttribute_overrides_configuration_from_convention() indexBuilder.IsUnique(false, ConfigurationSource.Convention); RunConvention(entityBuilder); - RunConvention(propABuilder); - RunConvention(propBBuilder); RunConvention(modelBuilder); var index = entityBuilder.Metadata.GetIndexes().Single(); @@ -168,7 +166,8 @@ public virtual void IndexAttribute_without_name_and_an_ignored_property_causes_e public virtual void IndexAttribute_with_name_and_an_ignored_property_causes_error() { var modelBuilder = InMemoryTestHelpers.Instance.CreateConventionBuilder(); - modelBuilder.Entity(); + var entityBuilder = modelBuilder.Entity(); + Assert.Equal( CoreStrings.NamedIndexDefinedOnIgnoredProperty( @@ -215,27 +214,44 @@ public virtual void IndexAttribute_with_name_and_non_existent_property_causes_er public void IndexAttribute_index_replicated_to_derived_type_when_base_type_changes() { var modelBuilder = InMemoryTestHelpers.Instance.CreateConventionBuilder(); - var baseEntityBuilder = modelBuilder.Entity(typeof(BaseEntityWithIndex)); - var derivedEntityBuilder = modelBuilder.Entity(); + var grandparentEntityBuilder = modelBuilder.Entity(typeof(GrandparentEntityWithIndex)); + var parentEntityBuilder = modelBuilder.Entity(); + var childEntityBuilder = modelBuilder.Entity(); + + // Index is created on Grandparent type but not on Parent type. + // Child has its own index. + Assert.NotNull(parentEntityBuilder.Metadata.BaseType); + Assert.NotNull(childEntityBuilder.Metadata.BaseType); + Assert.Single(grandparentEntityBuilder.Metadata.GetDeclaredIndexes()); + Assert.Empty(parentEntityBuilder.Metadata.GetDeclaredIndexes()); + Assert.Single(childEntityBuilder.Metadata.GetDeclaredIndexes()); - // Index is created on base type but not on derived type. - Assert.NotNull(derivedEntityBuilder.Metadata.BaseType); - Assert.Single(baseEntityBuilder.Metadata.GetDeclaredIndexes()); - Assert.Empty(derivedEntityBuilder.Metadata.GetDeclaredIndexes()); + parentEntityBuilder.HasBaseType((string)null); - derivedEntityBuilder.HasBaseType((string)null); + Assert.Null(parentEntityBuilder.Metadata.BaseType); + Assert.NotNull(childEntityBuilder.Metadata.BaseType); - // Now the Index is replicated on the derived type. - Assert.Null(derivedEntityBuilder.Metadata.BaseType); + // The Index is replicated on the Parent type, but not on the Child. var index = (Metadata.Internal.Index) - Assert.Single(derivedEntityBuilder.Metadata.GetDeclaredIndexes()); + Assert.Single(parentEntityBuilder.Metadata.GetDeclaredIndexes()); Assert.Equal(ConfigurationSource.DataAnnotation, index.GetConfigurationSource()); - Assert.Equal("IndexOnBaseGetsReplicatedToDerived", index.Name); + Assert.Equal("IndexOnGrandparentGetsReplicatedToParent", index.Name); Assert.False(index.IsUnique); Assert.Null(index.GetIsUniqueConfigurationSource()); var indexProperty = Assert.Single(index.Properties); Assert.Equal("B", indexProperty.Name); + // The Child still has its own index even though + // the property is defined on the Grandparent type. + var childIndex = (Metadata.Internal.Index) + Assert.Single(childEntityBuilder.Metadata.GetDeclaredIndexes()); + Assert.Equal(ConfigurationSource.DataAnnotation, childIndex.GetConfigurationSource()); + Assert.Equal("IndexOnChildUnaffectedWhenParentBaseTypeRemoved", childIndex.Name); + Assert.True(childIndex.IsUnique); + Assert.Equal(ConfigurationSource.DataAnnotation, childIndex.GetIsUniqueConfigurationSource()); + var childIndexProperty = Assert.Single(childIndex.Properties); + Assert.Equal("A", childIndexProperty.Name); + // Check there are no errors. modelBuilder.Model.FinalizeModel(); } @@ -249,6 +265,7 @@ public void IndexAttribute_index_is_created_when_missing_property_added() Assert.Empty(entityBuilder.Metadata.GetDeclaredIndexes()); entityBuilder.Property("Y"); + modelBuilder.Model.FinalizeModel(); var index = (Metadata.Internal.Index) Assert.Single(entityBuilder.Metadata.GetDeclaredIndexes()); @@ -258,9 +275,23 @@ public void IndexAttribute_index_is_created_when_missing_property_added() Assert.Collection(index.Properties, prop0 => Assert.Equal("X", prop0.Name), prop1 => Assert.Equal("Y", prop1.Name)); + } - // Check there are no errors. + [ConditionalFact] + public void IndexAttribute_index_is_created_when_index_on_private_property() + { + var modelBuilder = InMemoryTestHelpers.Instance.CreateConventionBuilder(); + var entityBuilder = modelBuilder.Entity(typeof(EntityWithIndexOnPrivateProperty)); modelBuilder.Model.FinalizeModel(); + + var index = (Metadata.Internal.Index) + Assert.Single(entityBuilder.Metadata.GetDeclaredIndexes()); + + Assert.Equal(ConfigurationSource.DataAnnotation, index.GetConfigurationSource()); + Assert.Equal("IndexOnPrivateProperty", index.Name); + Assert.Collection(index.Properties, + prop0 => Assert.Equal("X", prop0.Name), + prop1 => Assert.Equal("Y", prop1.Name)); } #endregion @@ -273,14 +304,6 @@ private void RunConvention(InternalEntityTypeBuilder entityTypeBuilder) CreateIndexAttributeConvention().ProcessEntityTypeAdded(entityTypeBuilder, context); } - private void RunConvention(InternalPropertyBuilder propertyBuilder) - { - var context = new ConventionContext( - propertyBuilder.Metadata.DeclaringEntityType.Model.ConventionDispatcher); - - CreateIndexAttributeConvention().ProcessPropertyAdded(propertyBuilder, context); - } - private void RunConvention(InternalModelBuilder modelBuilder) { var context = new ConventionContext(modelBuilder.Metadata.ConventionDispatcher); @@ -392,25 +415,40 @@ private class EntityIndexWithNonExistentProperty public int B { get; set; } } - [Index(nameof(B), Name = "IndexOnBaseGetsReplicatedToDerived")] - private class BaseEntityWithIndex + [Index(nameof(B), Name = "IndexOnGrandparentGetsReplicatedToParent")] + private class GrandparentEntityWithIndex { public int Id { get; set; } public int A { get; set; } public int B { get; set; } } - private class DerivedEntity : BaseEntityWithIndex + private class ParentEntity : GrandparentEntityWithIndex { public int C { get; set; } public int D { get; set; } } + [Index(nameof(A), Name = "IndexOnChildUnaffectedWhenParentBaseTypeRemoved", IsUnique = true)] + private class ChildEntity : ParentEntity + { + public int E { get; set; } + public int F { get; set; } + } + [Index(nameof(X), "Y", Name = "IndexOnShadowProperty")] private class EntityWithIndexOnShadowProperty { public int Id { get; set; } public int X { get; set; } } + + [Index(nameof(X), nameof(Y), Name = "IndexOnPrivateProperty")] + private class EntityWithIndexOnPrivateProperty + { + public int Id { get; set; } + public int X { get; set; } + private int Y { get; set; } + } } }