From ed629d65089bc7b1bbd6853c335e541df5a5ae7e Mon Sep 17 00:00:00 2001 From: Brice Lambson Date: Tue, 3 Oct 2017 15:50:18 -0700 Subject: [PATCH] Migrations: Use reflection order for columns in CreateTable Fixes #2272 --- .../Internal/MigrationsModelDiffer.cs | 123 +++++- src/EFCore/Internal/EnumerableExtensions.cs | 10 +- .../Internal/MigrationsModelDifferTest.cs | 375 +++++++++++++++++- 3 files changed, 491 insertions(+), 17 deletions(-) diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs index 4522a972268..66216122c8f 100644 --- a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs +++ b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Reflection; using JetBrains.Annotations; using Microsoft.EntityFrameworkCore.ChangeTracking.Internal; using Microsoft.EntityFrameworkCore.Infrastructure; @@ -566,7 +567,7 @@ protected virtual IEnumerable Add( createTableOperation.AddAnnotations(MigrationsAnnotations.For(target.EntityTypes[0])); createTableOperation.Columns.AddRange( - target.GetProperties().SelectMany(p => Add(p, diffContext, inline: true)).Cast()); + GetSortedProperties(target).SelectMany(p => Add(p, diffContext, inline: true)).Cast()); var primaryKey = target.EntityTypes[0].FindPrimaryKey(); createTableOperation.PrimaryKey = Add(primaryKey, diffContext).Cast().Single(); createTableOperation.UniqueConstraints.AddRange( @@ -605,6 +606,126 @@ protected virtual IEnumerable Remove( yield return operation; } + private static IEnumerable GetSortedProperties(TableMapping target) + => target.EntityTypes + .Where( + t => t.BaseType == null + && t.FindForeignKeys(t.FindDeclaredPrimaryKey().Properties) + .All(fk => t.Relational().TableName != fk.PrincipalEntityType.Relational().TableName)) + .SelectMany(GetSortedProperties) + .Distinct((x, y) => x.Relational().ColumnName == y.Relational().ColumnName); + + private static IEnumerable GetSortedProperties(IEntityType entityType) + { + var shadowProperties = new List(); + var groups = new Dictionary>(); + var unorderedGroups = new Dictionary>(); + var types = new Dictionary>(); + + foreach (var property in entityType.GetDeclaredProperties()) + { + var clrProperty = property.PropertyInfo; + if (clrProperty == null) + { + var foreignKey = property.GetContainingForeignKeys() + .FirstOrDefault(fk => fk.DependentToPrincipal?.PropertyInfo != null); + if (foreignKey == null) + { + shadowProperties.Add(property); + + continue; + } + + clrProperty = foreignKey.DependentToPrincipal.PropertyInfo; + var groupIndex = foreignKey.Properties.IndexOf(property); + + unorderedGroups.GetOrAddNew(clrProperty).Add(groupIndex, property); + } + else + { + groups.Add(clrProperty, new List { property }); + } + + var clrType = clrProperty.DeclaringType; + var index = clrType.GetTypeInfo().DeclaredProperties + .IndexOf(clrProperty, new PropertyInfoEuqlityComparer()); + + types.GetOrAddNew(clrType)[index] = clrProperty; + } + + foreach (var group in unorderedGroups) + { + groups.Add(group.Key, group.Value.Values.ToList()); + } + + foreach (var definingForeignKey in entityType.GetDeclaredReferencingForeignKeys() + .Where(fk => fk.DeclaringEntityType.RootType() != entityType.RootType() + && fk.DeclaringEntityType.Relational().TableName == entityType.Relational().TableName + && fk == fk.DeclaringEntityType + .FindForeignKey( + fk.DeclaringEntityType.FindDeclaredPrimaryKey().Properties, + entityType.FindPrimaryKey(), + entityType))) + { + var clrProperty = definingForeignKey.PrincipalToDependent?.PropertyInfo; + var properties = GetSortedProperties(definingForeignKey.DeclaringEntityType).ToList(); + if (clrProperty == null) + { + shadowProperties.AddRange(properties); + + continue; + } + + groups.Add(clrProperty, properties); + + var clrType = clrProperty.DeclaringType; + var index = clrType.GetTypeInfo().DeclaredProperties + .IndexOf(clrProperty, new PropertyInfoEuqlityComparer()); + + types.GetOrAddNew(clrType)[index] = clrProperty; + } + + var graph = new Multigraph(); + graph.AddVertices(types.Keys); + + foreach (var left in types.Keys) + { + var found = false; + foreach (var baseType in left.GetBaseTypes()) + { + foreach (var right in types.Keys) + { + if (right == baseType + && baseType != left) + { + graph.AddEdge(right, left, null); + found = true; + + break; + } + } + + if (found) + { + break; + } + } + } + + return graph.TopologicalSort().SelectMany(t => types[t].Values).SelectMany(p => groups[p]) + .Concat(shadowProperties) + .Concat(entityType.GetDirectlyDerivedTypes().SelectMany(GetSortedProperties)); + } + + private class PropertyInfoEuqlityComparer : IEqualityComparer + { + public bool Equals(PropertyInfo x, PropertyInfo y) + => x.IsSameAs(y); + + public int GetHashCode(PropertyInfo obj) + => throw new NotImplementedException(); + } + #endregion #region IProperty diff --git a/src/EFCore/Internal/EnumerableExtensions.cs b/src/EFCore/Internal/EnumerableExtensions.cs index 95df88a9e4b..10e9022511c 100644 --- a/src/EFCore/Internal/EnumerableExtensions.cs +++ b/src/EFCore/Internal/EnumerableExtensions.cs @@ -128,8 +128,16 @@ public static bool StartsWith( /// 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) + => IndexOf(source, item, EqualityComparer.Default); + + /// + /// 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, + [NotNull] IEqualityComparer comparer) => source.Select((x, index) => - EqualityComparer.Default.Equals(item, x) ? index : -1) + comparer.Equals(item, x) ? index : -1) .FirstOr(x => x != -1, -1); /// diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index c4fc0de126d..d7c6763cf77 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -2,7 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Diagnostics; +using System.Collections.Generic; using System.Linq; using Microsoft.EntityFrameworkCore.ChangeTracking; using Microsoft.EntityFrameworkCore.Infrastructure; @@ -12,7 +12,6 @@ using Microsoft.EntityFrameworkCore.ValueGeneration; using Microsoft.Extensions.DependencyInjection; using Xunit; -using System.Collections.Generic; // ReSharper disable UnusedAutoPropertyAccessor.Local // ReSharper disable ClassNeverInstantiated.Local @@ -143,6 +142,348 @@ public void Create_table() })); } + class CreateTableEntity1 + { + public int Id { get; set; } + public int C { get; set; } + public int B { get; set; } + public int A { get; set; } + } + + class CreateTableEntity2 + { + public int Id { get; set; } + public int E { get; set; } + public CreateTableEntity2B D { get; set; } + public int A { get; set; } + } + + class CreateTableEntity2B + { + public int B { get; set; } + public int C { get; set; } + } + + [Fact] + public void Create_table_columns_use_property_order() + { + Execute( + _ => { }, + modelBuilder => modelBuilder.Entity(), + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("C", x.Name), + x => Assert.Equal("B", x.Name), + x => Assert.Equal("A", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_use_dependent_to_principal_and_key_order_when_shadow_fk() + { + Execute( + _ => { }, + modelBuilder => + { + modelBuilder.Entity(); + modelBuilder.Entity().HasKey(e => new { e.C, e.B }); + }, + operations => + { + Assert.Equal(3, operations.Count); + + Assert.IsType(operations[0]); + + var operation = Assert.IsType(operations[1]); + Assert.Equal("CreateTableEntity2", operation.Name); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("E", x.Name), + x => Assert.Equal("DC", x.Name), + x => Assert.Equal("DB", x.Name), + x => Assert.Equal("A", x.Name)); + + Assert.IsType(operations[2]); + }); + } + + [Fact] + public void Create_table_columns_uses_defining_navigation_order() + { + Execute( + _ => { }, + modelBuilder => modelBuilder.Entity().OwnsOne(e => e.D), + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("E", x.Name), + x => Assert.Equal("D_B", x.Name), + x => Assert.Equal("D_C", x.Name), + x => Assert.Equal("A", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_uses_principal_to_dependent_order_when_splitting() + { + Execute( + _ => { }, + modelBuilder => modelBuilder.Entity().ToTable("CreateTableEntity2") + .HasOne().WithOne(x => x.D).HasForeignKey("Id"), + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("E", x.Name), + x => Assert.Equal("B", x.Name), + x => Assert.Equal("C", x.Name), + x => Assert.Equal("A", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_groups_and_sorts_type_hierarchy() + { + Execute( + _ => { }, + modelBuilder => + { + modelBuilder.Entity("D").Property("Id"); + modelBuilder.Entity("C").HasBaseType("D").Property("C"); + modelBuilder.Entity("B").HasBaseType("D").Property("B"); + modelBuilder.Entity("A").HasBaseType("B").Property("A"); + }, + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("Discriminator", x.Name), + x => Assert.Equal("B", x.Name), + x => Assert.Equal("A", x.Name), + x => Assert.Equal("C", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_handles_aliased_columns() + { + Execute( + _ => { }, + modelBuilder => modelBuilder.Entity().Property(e => e.A).HasColumnName("C"), + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("C", x.Name), + x => Assert.Equal("B", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_handles_shadow_defining_navigation() + { + Execute( + _ => { }, + modelBuilder => modelBuilder.Entity( + "X", + x => + { + x.Property("Id"); + x.OwnsOne("Y", "Y").Property("A"); + }), + operations => + { + var operation = Assert.IsType(Assert.Single(operations)); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("Y_A", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_handles_shadow_principal_to_dependent_when_splitting() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity("X", x => x.Property("Id")) + .Entity( + "Y", + x => + { + x.ToTable("X"); + x.Property("A"); + x.HasOne("X").WithOne("Y").HasForeignKey("Y", "Id"); + }), + operations => + { + Assert.Equal(1, operations.Count); + + var operation = Assert.IsType(operations[0]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("A", x.Name)); + + }); + } + + [Fact] + public void Create_table_columns_handles_no_principal_to_dependent_when_splitting() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity("X", x => x.Property("Id")) + .Entity( + "Y", + x => + { + x.ToTable("X"); + x.Property("A"); + x.HasOne("X").WithOne().HasForeignKey("Y", "Id"); + }), + operations => + { + Assert.Equal(1, operations.Count); + + var operation = Assert.IsType(operations[0]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("A", x.Name)); + }); + } + + [Fact] + public void Create_table_columns_handles_shadow_dependent_to_principal() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity("X", x => x.Property("Id")) + .Entity( + "Y", + x => + { + x.Property("Id"); + x.HasOne("X", "X").WithMany("Ys").HasForeignKey("XId"); + }), + operations => + { + Assert.Equal(3, operations.Count); + + Assert.IsType(operations[0]); + + var operation = Assert.IsType(operations[1]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("XId", x.Name)); + + Assert.IsType(operations[2]); + }); + } + + [Fact] + public void Create_table_columns_handles_no_dependent_to_principal() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity("X", x => x.Property("Id")) + .Entity( + "Y", + x => + { + x.Property("Id"); + x.HasOne("X").WithMany().HasForeignKey("XId"); + }), + operations => + { + Assert.Equal(3, operations.Count); + + Assert.IsType(operations[0]); + + var operation = Assert.IsType(operations[1]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("XId", x.Name)); + + Assert.IsType(operations[2]); + }); + } + + [Fact] + public void Create_table_columns_handles_self_referencing_one_to_many() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity( + "X", + x => + { + x.Property("Id"); + x.HasOne("X").WithMany(); + }), + operations => + { + Assert.Equal(2, operations.Count); + + var operation = Assert.IsType(operations[0]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("XId", x.Name)); + + Assert.IsType(operations[1]); + }); + } + + [Fact] + public void Create_table_columns_handles_self_referencing_one_to_one() + { + Execute( + _ => { }, + modelBuilder => modelBuilder + .Entity( + "X", + x => + { + x.Property("Id"); + x.HasOne("X").WithOne(); + }), + operations => + { + Assert.Equal(2, operations.Count); + + var operation = Assert.IsType(operations[0]); + Assert.Collection( + operation.Columns, + x => Assert.Equal("Id", x.Name), + x => Assert.Equal("XId", x.Name)); + + Assert.IsType(operations[1]); + }); + } + [Fact] public void Rename_table() { @@ -409,16 +750,17 @@ public void Move_type_from_one_shared_table_to_another_with_seed_data() public void Can_split_entity_in_two_using_shared_table_with_seed_data() { Execute( - modelBuilder => { - modelBuilder.Entity( - "Animal", - x => - { - x.Property("Id"); - x.Property("MouseId"); - x.Property("BoneId"); - x.SeedData(new { Id = 42, MouseId = "1", BoneId = "2" }); - }); + modelBuilder => + { + modelBuilder.Entity( + "Animal", + x => + { + x.Property("Id"); + x.Property("MouseId"); + x.Property("BoneId"); + x.SeedData(new { Id = 42, MouseId = "1", BoneId = "2" }); + }); }, modelBuilder => { @@ -5547,7 +5889,8 @@ public void SeedData_all_operations() public void SeedData_with_shadow_navigation_properties() { SeedData_with_navigation_properties( - target => { + target => + { target.Entity( "Blog", x => @@ -5581,7 +5924,8 @@ public void SeedData_with_shadow_navigation_properties() public void SeedData_with_CLR_navigation_properties() { SeedData_with_navigation_properties( - target => { + target => + { target.Entity( x => { @@ -5613,7 +5957,8 @@ private void SeedData_with_navigation_properties(Action buildTarge { Execute( _ => { }, - source => { + source => + { source.Entity( "Blog", x =>