From 92dcb80495ce566e780e6587676b2c90eff06526 Mon Sep 17 00:00:00 2001 From: AndriySvyryd Date: Thu, 14 Sep 2017 14:31:29 -0700 Subject: [PATCH] Default to the principal value generation strategy for the primary key columns for entity types sharing the same table. Validate that properties sharing the same column have the same value generation strategy. Fixes #9652 --- ...igationsOwnedQueryRelationalFixtureBase.cs | 3 +- .../Metadata/Internal/PropertyExtensions.cs | 59 ++++++++++++++ .../Metadata/RelationalPropertyAnnotations.cs | 80 +++++++------------ .../Metadata/RelationalPropertyExtensions.cs | 10 +-- .../Internal/SqlServerModelValidator.cs | 38 ++++++++- .../Metadata/SqlServerPropertyAnnotations.cs | 6 +- .../Properties/SqlServerStrings.Designer.cs | 8 ++ .../Properties/SqlServerStrings.resx | 3 + src/EFCore/Internal/EnumerableExtensions.cs | 23 ++++++ .../RelationalModelValidatorTest.cs | 5 ++ .../Migrations/SqlServerModelDifferTest.cs | 35 ++++++++ .../SqlServerModelValidatorTest.cs | 26 +++++- 12 files changed, 230 insertions(+), 66 deletions(-) create mode 100644 src/EFCore.Relational/Metadata/Internal/PropertyExtensions.cs diff --git a/src/EFCore.Relational.Specification.Tests/Query/ComplexNavigationsOwnedQueryRelationalFixtureBase.cs b/src/EFCore.Relational.Specification.Tests/Query/ComplexNavigationsOwnedQueryRelationalFixtureBase.cs index 92011baf4f8..f9caffc78e3 100644 --- a/src/EFCore.Relational.Specification.Tests/Query/ComplexNavigationsOwnedQueryRelationalFixtureBase.cs +++ b/src/EFCore.Relational.Specification.Tests/Query/ComplexNavigationsOwnedQueryRelationalFixtureBase.cs @@ -45,7 +45,6 @@ protected override void Configure(ReferenceOwnershipBuilder l4) public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder) => base.AddOptions(builder).ConfigureWarnings( - c => c - .Log(RelationalEventId.QueryClientEvaluationWarning)); + c => c.Log(RelationalEventId.QueryClientEvaluationWarning)); } } diff --git a/src/EFCore.Relational/Metadata/Internal/PropertyExtensions.cs b/src/EFCore.Relational/Metadata/Internal/PropertyExtensions.cs new file mode 100644 index 00000000000..7b3ea4868b1 --- /dev/null +++ b/src/EFCore.Relational/Metadata/Internal/PropertyExtensions.cs @@ -0,0 +1,59 @@ +// 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 JetBrains.Annotations; +using Microsoft.EntityFrameworkCore.Internal; + +namespace Microsoft.EntityFrameworkCore.Metadata.Internal +{ + /// + /// Extension methods for for relational database metadata. + /// + public static class PropertyExtensions + { + /// + /// This API supports the Entity Framework Core infrastructure and is not intended to be used + /// directly from your code. This API may change or be removed in future releases. + /// + public static IProperty FindSharedTablePrincipalPrimaryKeyProperty([NotNull] this IProperty property) + { + var linkingRelationship = property.FindSharedTableLink(); + return linkingRelationship?.PrincipalKey.Properties[linkingRelationship.Properties.IndexOf(property)]; + } + + /// + /// This API supports the Entity Framework Core infrastructure and is not intended to be used + /// directly from your code. This API may change or be removed in future releases. + /// + public static IForeignKey FindSharedTableLink([NotNull] this IProperty property) + { + var pk = property.GetContainingPrimaryKey(); + if (pk == null) + { + return null; + } + + var entityType = property.DeclaringEntityType; + + foreach (var fk in entityType.FindForeignKeys(pk.Properties)) + { + if (!fk.PrincipalKey.IsPrimaryKey() + || fk.PrincipalEntityType == fk.DeclaringEntityType) + { + continue; + } + + var principalEntityType = fk.PrincipalEntityType; + var entityTypeAnnotations = fk.DeclaringEntityType.Relational(); + var principalTypeAnnotations = principalEntityType.Relational(); + if (entityTypeAnnotations.TableName == principalTypeAnnotations.TableName + && entityTypeAnnotations.Schema == principalTypeAnnotations.Schema) + { + return fk; + } + } + + return null; + } + } +} diff --git a/src/EFCore.Relational/Metadata/RelationalPropertyAnnotations.cs b/src/EFCore.Relational/Metadata/RelationalPropertyAnnotations.cs index faf4f92757d..885d69bbd10 100644 --- a/src/EFCore.Relational/Metadata/RelationalPropertyAnnotations.cs +++ b/src/EFCore.Relational/Metadata/RelationalPropertyAnnotations.cs @@ -93,67 +93,49 @@ public virtual string ColumnName private string GetDefaultColumnName() { + var sharedTablePrincipalPrimaryKeyProperty = Property.FindSharedTablePrincipalPrimaryKeyProperty(); + if (sharedTablePrincipalPrimaryKeyProperty != null) + { + return GetAnnotations(sharedTablePrincipalPrimaryKeyProperty).ColumnName; + } + var entityType = Property.DeclaringEntityType; - var pk = Property.GetContainingPrimaryKey(); - if (pk != null) + StringBuilder builder = null; + do { - foreach (var fk in entityType.FindForeignKeys(pk.Properties)) + var ownership = entityType.GetForeignKeys().SingleOrDefault(fk => fk.IsOwnership); + if (ownership == null) { - if (!fk.PrincipalKey.IsPrimaryKey() - || fk.PrincipalKey == pk) - { - continue; - } - - var principalEntityType = fk.PrincipalEntityType; - var entityTypeAnnotations = GetAnnotations(entityType); - var principalTypeAnnotations = GetAnnotations(principalEntityType); - if (entityTypeAnnotations.TableName == principalTypeAnnotations.TableName - && entityTypeAnnotations.Schema == principalTypeAnnotations.Schema) - { - return GetAnnotations(principalEntityType.FindPrimaryKey().Properties[pk.IndexOf(Property)]).ColumnName; - } + entityType = null; } - } - else - { - StringBuilder builder = null; - do + else { - var ownership = entityType.GetForeignKeys().SingleOrDefault(fk => fk.IsOwnership); - if (ownership == null) + var ownerType = ownership.PrincipalEntityType; + var entityTypeAnnotations = GetAnnotations(entityType); + var ownerTypeAnnotations = GetAnnotations(ownerType); + if (entityTypeAnnotations.TableName == ownerTypeAnnotations.TableName + && entityTypeAnnotations.Schema == ownerTypeAnnotations.Schema) { - entityType = null; + if (builder == null) + { + builder = new StringBuilder(); + } + builder.Insert(0, "_"); + builder.Insert(0, ownership.PrincipalToDependent.Name); + entityType = ownerType; } else { - var ownerType = ownership.PrincipalEntityType; - var entityTypeAnnotations = GetAnnotations(entityType); - var ownerTypeAnnotations = GetAnnotations(ownerType); - if (entityTypeAnnotations.TableName == ownerTypeAnnotations.TableName - && entityTypeAnnotations.Schema == ownerTypeAnnotations.Schema) - { - if (builder == null) - { - builder = new StringBuilder(); - } - builder.Insert(0, "_"); - builder.Insert(0, ownership.PrincipalToDependent.Name); - entityType = ownerType; - } - else - { - entityType = null; - } + entityType = null; } } - while (entityType != null); + } + while (entityType != null); - if (builder != null) - { - builder.Append(Property.Name); - return builder.ToString(); - } + if (builder != null) + { + builder.Append(Property.Name); + return builder.ToString(); } return Property.Name; diff --git a/src/EFCore.Relational/Metadata/RelationalPropertyExtensions.cs b/src/EFCore.Relational/Metadata/RelationalPropertyExtensions.cs index e0dbbfdfb9b..56f18df21db 100644 --- a/src/EFCore.Relational/Metadata/RelationalPropertyExtensions.cs +++ b/src/EFCore.Relational/Metadata/RelationalPropertyExtensions.cs @@ -37,14 +37,8 @@ public static bool IsColumnNullable([NotNull] this IProperty property) return false; } - var pk = property.DeclaringEntityType.FindPrimaryKey(); - return pk != null - && property.DeclaringEntityType.FindForeignKeys(pk.Properties) - .Any( - fk => fk.PrincipalKey.IsPrimaryKey() - && fk.PrincipalEntityType.BaseType != null - && fk.DeclaringEntityType.Relational().TableName == fk.PrincipalEntityType.Relational().TableName - && fk.DeclaringEntityType.Relational().Schema == fk.PrincipalEntityType.Relational().Schema); + return property.DeclaringEntityType.FindPrimaryKey()?.Properties.First() + ?.FindSharedTableLink()?.PrincipalEntityType.BaseType != null; } } } diff --git a/src/EFCore.SqlServer/Internal/SqlServerModelValidator.cs b/src/EFCore.SqlServer/Internal/SqlServerModelValidator.cs index c35c0b865a9..849ce0bd116 100644 --- a/src/EFCore.SqlServer/Internal/SqlServerModelValidator.cs +++ b/src/EFCore.SqlServer/Internal/SqlServerModelValidator.cs @@ -125,10 +125,40 @@ protected override void ValidateSharedColumnsCompatibility(IReadOnlyList et.GetDeclaredProperties()) - .Where(p => p.SqlServer().ValueGenerationStrategy == SqlServerValueGenerationStrategy.IdentityColumn) - .Distinct((p1, p2) => p1.Name == p2.Name) - .ToList(); + var identityColumns = new List(); + var propertyMappings = new Dictionary(); + + foreach (var property in mappedTypes.SelectMany(et => et.GetDeclaredProperties())) + { + var propertyAnnotations = property.Relational(); + var columnName = propertyAnnotations.ColumnName; + if (propertyMappings.TryGetValue(columnName, out var duplicateProperty)) + { + var propertyStrategy = property.SqlServer().ValueGenerationStrategy; + var duplicatePropertyStrategy = duplicateProperty.SqlServer().ValueGenerationStrategy; + if (propertyStrategy != duplicatePropertyStrategy + && (propertyStrategy == SqlServerValueGenerationStrategy.IdentityColumn + || duplicatePropertyStrategy == SqlServerValueGenerationStrategy.IdentityColumn)) + { + throw new InvalidOperationException( + SqlServerStrings.DuplicateColumnNameValueGenerationStrategyMismatch( + duplicateProperty.DeclaringEntityType.DisplayName(), + duplicateProperty.Name, + property.DeclaringEntityType.DisplayName(), + property.Name, + columnName, + tableName)); + } + } + else + { + propertyMappings[columnName] = property; + if (property.SqlServer().ValueGenerationStrategy == SqlServerValueGenerationStrategy.IdentityColumn) + { + identityColumns.Add(property); + } + } + } if (identityColumns.Count > 1) { diff --git a/src/EFCore.SqlServer/Metadata/SqlServerPropertyAnnotations.cs b/src/EFCore.SqlServer/Metadata/SqlServerPropertyAnnotations.cs index e813616e955..898f83e3c7b 100644 --- a/src/EFCore.SqlServer/Metadata/SqlServerPropertyAnnotations.cs +++ b/src/EFCore.SqlServer/Metadata/SqlServerPropertyAnnotations.cs @@ -133,7 +133,6 @@ public virtual SqlServerValueGenerationStrategy? ValueGenerationStrategy var relationalProperty = Property.Relational(); if (!fallbackToModel - || Property.ValueGenerated != ValueGenerated.OnAdd || relationalProperty.DefaultValue != null || relationalProperty.DefaultValueSql != null || relationalProperty.ComputedColumnSql != null) @@ -141,6 +140,11 @@ public virtual SqlServerValueGenerationStrategy? ValueGenerationStrategy return null; } + if (Property.ValueGenerated != ValueGenerated.OnAdd) + { + return Property.FindSharedTablePrincipalPrimaryKeyProperty()?.SqlServer().ValueGenerationStrategy; + } + var modelStrategy = Property.DeclaringEntityType.Model.SqlServer().ValueGenerationStrategy; if (modelStrategy == SqlServerValueGenerationStrategy.SequenceHiLo diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index ad3d2ffd09e..737b007f4e1 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -279,6 +279,14 @@ public static readonly EventDefinition LogForeignKeyTableNotInSe SqlServerEventId.ForeignKeyTableMissingWarning, _resourceManager.GetString("LogForeignKeyTableNotInSelectionSet"))); + /// + /// '{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different value generation strategies. + /// + public static string DuplicateColumnNameValueGenerationStrategyMismatch([CanBeNull] object entityType1, [CanBeNull] object property1, [CanBeNull] object entityType2, [CanBeNull] object property2, [CanBeNull] object columnName, [CanBeNull] object table) + => string.Format( + GetString("DuplicateColumnNameValueGenerationStrategyMismatch", nameof(entityType1), nameof(property1), nameof(entityType2), nameof(property2), nameof(columnName), nameof(table)), + entityType1, property1, entityType2, property2, columnName, table); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index c129b2c7b40..68686a3ea2a 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -214,4 +214,7 @@ Foreign key {fkName} is defined on table {tableName} which is not included in the selection set. Skipping. Debug SqlServerEventId.ForeignKeyTableMissingWarning string string + + '{entityType1}.{property1}' and '{entityType2}.{property2}' are both mapped to column '{columnName}' in '{table}' but are configured with different value generation strategies. + \ No newline at end of file diff --git a/src/EFCore/Internal/EnumerableExtensions.cs b/src/EFCore/Internal/EnumerableExtensions.cs index 7914b120373..95df88a9e4b 100644 --- a/src/EFCore/Internal/EnumerableExtensions.cs +++ b/src/EFCore/Internal/EnumerableExtensions.cs @@ -122,5 +122,28 @@ public static bool StartsWith( return true; } + + /// + /// This API supports the Entity Framework Core infrastructure and is not intended to be used + /// directly from your code. This API may change or be removed in future releases. + /// + public static int IndexOf([NotNull] this IEnumerable source, [NotNull] T item) + => source.Select((x, index) => + EqualityComparer.Default.Equals(item, x) ? index : -1) + .FirstOr(x => x != -1, -1); + + /// + /// This API supports the Entity Framework Core infrastructure and is not intended to be used + /// directly from your code. This API may change or be removed in future releases. + /// + public static T FirstOr([NotNull] this IEnumerable source, [NotNull] T alternate) + => source.DefaultIfEmpty(alternate).First(); + + /// + /// This API supports the Entity Framework Core infrastructure and is not intended to be used + /// directly from your code. This API may change or be removed in future releases. + /// + public static T FirstOr([NotNull] this IEnumerable source, [NotNull] Func predicate, [NotNull] T alternate) + => source.Where(predicate).FirstOr(alternate); } } diff --git a/test/EFCore.Relational.Tests/RelationalModelValidatorTest.cs b/test/EFCore.Relational.Tests/RelationalModelValidatorTest.cs index 45ce640a5ea..9b1716e87c9 100644 --- a/test/EFCore.Relational.Tests/RelationalModelValidatorTest.cs +++ b/test/EFCore.Relational.Tests/RelationalModelValidatorTest.cs @@ -14,6 +14,7 @@ using Microsoft.EntityFrameworkCore.TestUtilities; using Xunit; +// ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore { public class RelationalModelValidatorTest : ModelValidatorTest @@ -764,6 +765,8 @@ protected class Cat : Animal [NotMapped] public string Type { get; set; } + + public int Identity { get; set; } } protected class Dog : Animal @@ -772,6 +775,8 @@ protected class Dog : Animal [NotMapped] public int Type { get; set; } + + public int Identity { get; set; } } protected class Person diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs index 252c6e3a8f6..d87ec176ed4 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs @@ -278,6 +278,41 @@ public void Alter_unique_constraint_clustering() }); } + [Fact] + public void Create_shared_table_with_two_entity_types() + { + Execute( + _ => { }, + modelBuilder => + { + modelBuilder.Entity("Order", eb => + { + eb.Property("Id"); + eb.ToTable("Orders"); + }); + modelBuilder.Entity("Details", eb => + { + eb.Property("Id"); + eb.Property("Time"); + eb.HasOne("Order").WithOne().HasForeignKey("Details", "Id"); + eb.ToTable("Orders"); + }); + }, + operations => + { + Assert.Equal(1, operations.Count); + + var createTableOperation = Assert.IsType(operations[0]); + Assert.Equal(2, createTableOperation.Columns.Count); + var idColumn = createTableOperation.Columns[0]; + Assert.Equal("Id", idColumn.Name); + Assert.Equal(SqlServerValueGenerationStrategy.IdentityColumn, idColumn["SqlServer:ValueGenerationStrategy"]); + var timeColumn = createTableOperation.Columns[1]; + Assert.Equal("Time", timeColumn.Name); + Assert.False(timeColumn.IsNullable); + }); + } + [Fact] public void Alter_index_clustering() { diff --git a/test/EFCore.SqlServer.Tests/SqlServerModelValidatorTest.cs b/test/EFCore.SqlServer.Tests/SqlServerModelValidatorTest.cs index 2ca9df9d848..dea43d663fd 100644 --- a/test/EFCore.SqlServer.Tests/SqlServerModelValidatorTest.cs +++ b/test/EFCore.SqlServer.Tests/SqlServerModelValidatorTest.cs @@ -11,6 +11,7 @@ using Microsoft.EntityFrameworkCore.TestUtilities; using Xunit; +// ReSharper disable InconsistentNaming namespace Microsoft.EntityFrameworkCore { public class SqlServerModelValidatorTest : RelationalModelValidatorTest @@ -70,14 +71,35 @@ public virtual void Detects_duplicate_column_names_within_hierarchy_with_differe { var modelBuilder = new ModelBuilder(TestRelationalConventionSetBuilder.Build()); modelBuilder.Entity(); - modelBuilder.Entity().Ignore(e => e.Type).Property(c => c.Breed).IsUnicode(false); - modelBuilder.Entity().Ignore(e => e.Type).Property(d => d.Breed).IsUnicode(); + modelBuilder.Entity().Property(c => c.Breed).IsUnicode(false); + modelBuilder.Entity().Property(d => d.Breed).IsUnicode(); VerifyError( RelationalStrings.DuplicateColumnNameDataTypeMismatch( nameof(Cat), nameof(Cat.Breed), nameof(Dog), nameof(Dog.Breed), nameof(Cat.Breed), nameof(Animal), "varchar(max)", "nvarchar(max)"), modelBuilder.Model); } + [Fact] + public virtual void Detects_duplicate_column_names_within_hierarchy_with_different_value_generation_strategy() + { + var modelBuilder = new ModelBuilder(TestRelationalConventionSetBuilder.Build()); + modelBuilder.Entity(); + modelBuilder.Entity(cb => + { + cb.Property(c => c.Identity).UseSqlServerIdentityColumn(); + cb.Property(c => c.Identity).HasColumnName(nameof(Cat.Identity)); + }); + modelBuilder.Entity(db => + { + db.Property(d => d.Identity).ValueGeneratedNever(); + db.Property(c => c.Identity).HasColumnName(nameof(Dog.Identity)); + }); + + VerifyError( + SqlServerStrings.DuplicateColumnNameValueGenerationStrategyMismatch( + nameof(Cat), nameof(Cat.Identity), nameof(Dog), nameof(Dog.Identity), nameof(Cat.Identity), nameof(Animal)), modelBuilder.Model); + } + [Fact] public virtual void Passes_for_incompatible_foreignKeys_within_hierarchy_when_one_name_configured_explicitly_for_sqlServer() {