Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Foreign Keys and Indices added for inherited items #12963

Closed
razzemans opened this issue Aug 10, 2018 · 18 comments
Closed

Multiple Foreign Keys and Indices added for inherited items #12963

razzemans opened this issue Aug 10, 2018 · 18 comments
Labels
area-migrations area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@razzemans
Copy link

razzemans commented Aug 10, 2018

I'm running into an issue where EF Core generates incorrect migrations. Consider the following code:

public class BasePage
{
    [Key]
    public Guid Id { get; protected set; }
}

public class ContentBlock
{
    [Key]
    public Guid Id { get; protected set; }
    public string Title { get; set; }
}

// Derived pages:

public class ContentPage : BasePage
{
    public ContentBlock ContentBlock { get; set; }

    [Column(nameof(ContentBlockId))]
    public Guid? ContentBlockId { get; set; }
}

public class HomePage : BasePage
{
    public ContentBlock ContentBlock { get; set; }

    [Column(nameof(ContentBlockId))]
    public Guid? ContentBlockId { get; set; }
}

public class PageWithoutContentBlock : BasePage
{
    public string Header { get; set; }
}

// Context file:

public class DatabaseContext : DbContext
{
    public DatabaseContext(DbContextOptions<DatabaseContext> options)
            : base(options)
    { }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);
           
        modelBuilder.Entity<HomePage>().HasBaseType<BasePage>();
        modelBuilder.Entity<ContentPage>().HasBaseType<BasePage>();
        modelBuilder.Entity<PageWithoutContentBlock>().HasBaseType<BasePage>();
    }

    public DbSet<BasePage> Pages { get; set; }

    public DbSet<ContentBlock> ContentBlocks { get; set; }
}

This creates the following migration:

 public partial class initial : Migration
    {
        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.CreateTable(
                name: "ContentBlocks",
                columns: table => new
                {
                    Id = table.Column<Guid>(nullable: false),
                    Title = table.Column<string>(nullable: true)
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_ContentBlocks", x => x.Id);
                });

            migrationBuilder.CreateTable(
                name: "Pages",
                columns: table => new
                {
                    Id = table.Column<Guid>(nullable: false),
                    Discriminator = table.Column<string>(nullable: false),
                    ContentBlockId = table.Column<Guid>(nullable: true),
                    Header = table.Column<string>(nullable: true)
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_Pages", x => x.Id);
                    table.ForeignKey(
                        name: "FK_Pages_ContentBlocks_ContentBlockId",
                        column: x => x.ContentBlockId,
                        principalTable: "ContentBlocks",
                        principalColumn: "Id",
                        onDelete: ReferentialAction.Restrict);
                    table.ForeignKey(
                        name: "FK_Pages_ContentBlocks_ContentBlockId1",
                        column: x => x.ContentBlockId,
                        principalTable: "ContentBlocks",
                        principalColumn: "Id",
                        onDelete: ReferentialAction.Restrict);
                });

            migrationBuilder.CreateIndex(
                name: "IX_Pages_ContentBlockId",
                table: "Pages",
                column: "ContentBlockId");

            migrationBuilder.CreateIndex(
                name: "IX_Pages_ContentBlockId1",
                table: "Pages",
                column: "ContentBlockId");
        }

        protected override void Down(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.DropTable(
                name: "Pages");

            migrationBuilder.DropTable(
                name: "ContentBlocks");
        }
    }

This feels incorrect to me. It has the same foreign key and index added twice. Worse, if I have more items that inherit from BasePage (which I have in a real world example) I get the same number of indices and foreign keys added as the number of inherited pages (in my case 6).

I also tried explicitly adding the following line to the ContentBlock entity:

public List<BasePage> Pages { get; set; }

But funny enough this only adds another FK and index. I tried setting the FK explicitly using the Fluent API, but it seemed I failed, as it is expecting for example a List<HomePage> instead of a List<BasePage> in the WithMany method.

Am I missing something or is this a bug? I think that this issue appeared after upgrading to EF Core 2.1.1 but I am not 100% sure. I'm quite sure it worked correctly in 1.0 at least.

Further technical details

EF Core version: 2.1
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10
IDE: Visual Studio 2017 15.7.4

@ajcvickers
Copy link
Contributor

This is a duplicate of #12702, but leaving open because it seems we could create only one constraint for this scenario, since the constraints are identical.

@razzemans The way to stop two constraints being created is to explicitly specify the constraint name to use:

modelBuilder.Entity<HomePage>()
    .HasOne(e => e.ContentBlock)
    .WithMany()
    .HasConstraintName("FK_Pages_ContentBlocks_ContentBlockId");

modelBuilder.Entity<ContentPage>()
    .HasOne(e => e.ContentBlock)
    .WithMany()
    .HasConstraintName("FK_Pages_ContentBlocks_ContentBlockId");

@razzemans
Copy link
Author

That indeed works. Do I understand correctly that this is something you consider fixing in EF itself in a future release?
The reason I ask is that this fix is a bit troublesome for us. We have created a CMS which allows users to create their own pages (in code) but must inherit from a base page. The situation outlined in my post is something that can easily happen. We don't want to burden users with having to specify FK constraint names themselves, so it would be great if this would work out of the box.

For now, we can continue with the above solution, thank you.

@ajcvickers
Copy link
Contributor

@razzemans Are you seeing a functional issue with creating two constraints? As in, does it cause an exception/crash/incorrect behavior somewhere?

@razzemans
Copy link
Author

@ajcvickers To be fair I haven't tested it, but I figured mutiple indices would definitely have some performance impacts for non-read operations? If that is not the case I'd gladly hear it!
Beside that, there is the esthetic argument which might not be as important but well, after running the latest migration we ended up having 7 duplicate FK's and indices. Looking at those tables it brings tears to my eyes, and not in a good way ;-)

If it is relatively easy to fix in EF Core I'd be happy if it gets implemented. It's not a showstopper as there are ways around it.

@GeorgePetri
Copy link

@ajcvickers It does crash in my use case. I have a hierarchy with lots of children with many foreign keys each and for each FK EF Core adds an index. It creates more than 999 indexes, causing sql server exception.

Would appreciate a fix for this.

@ajcvickers ajcvickers removed this from the Backlog milestone Dec 11, 2018
@ajcvickers
Copy link
Contributor

Removing from backlog to consider the index limitation.

@GeorgePetri As a workaround, you should be able to delete from the migration all the lines that create the duplicate indexes.

@ajcvickers ajcvickers added this to the 3.0.0 milestone Dec 12, 2018
@ajcvickers
Copy link
Contributor

@AndriySvyryd Assigned to you initially as a model change, which seems ideal. However, it could just be consolidation in Migrations if necessary.

@AndriySvyryd
Copy link
Member

Related to #10446

@ajcvickers
Copy link
Contributor

Closing in favor of #10446

@ajcvickers
Copy link
Contributor

Closed the wrong one.

@AndriySvyryd
Copy link
Member

Verified fixed by afe9774

@AndriySvyryd AndriySvyryd modified the milestones: 5.0.0, 5.0.0-preview7 Jul 21, 2020
@AndriySvyryd AndriySvyryd added type-bug closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed type-enhancement verify-fixed This issue is likely fixed in new query pipeline. labels Jul 21, 2020
@ajcvickers ajcvickers modified the milestones: 5.0.0-preview7, 5.0.0 Nov 7, 2020
@cyvocross
Copy link

Vérifié fixé par afe9774

Hello, is there any plan to have a version compatible with .net standard 2.0 including this fix?

@ajcvickers
Copy link
Contributor

@cyvocross No. The last EF Core version to support .NET Standard is EF Core 3.1. It will be completely out of support in December.

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 20, 2022

@ajcvickers ".NET Standard 2.0" ?

@ajcvickers
Copy link
Contributor

@ErikEJ Good point. I tend to forget about .NET Standard 2.1, since it's largely useless. :-)

@cyvocross
Copy link

@cyvocross No. The last EF Core version to support .NET Standard is EF Core 3.1. It will be completely out of support in December.

Thank you for your answer, unfortuntly my project run on .Net framework 4.8 and cannot be migrated before before long time due to specific dependecies.

@ErikEJ
Copy link
Contributor

ErikEJ commented Oct 20, 2022

@cyvocross What are those "specific dependencies" ??

@cyvocross
Copy link

@cyvocross Quels sont ces "dépendances spécifiques" ??

Dear @ErikEJ my project depend by internal libs that cannot be migrated on dot net core, therefore i cannot migrate before a long time to dot net 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-migrations area-model-building closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

No branches or pull requests

6 participants