efcore / EFCore.NamingConventions

Entity Framework Core plugin to apply naming conventions to table and column names (e.g. snake_case)
Apache License 2.0
731 stars 74 forks source link

Support rewriting primary key name with TPT #57

Open roji opened 3 years ago

roji commented 3 years ago

Rewriting is currently disabled for primary key names with TPT, because of https://github.com/dotnet/efcore/issues/23444 (https://github.com/dotnet/efcore/issues/19811).

ghost commented 3 years ago

Is this being looked into? It is a fairly critical feature for us, we have a whole existing database model we are trying to connect to using ef core and this is a blocking issue. Can be easily reproduced with the following SQL model derived from the documentation itself:

CREATE TABLE [Blog] (
  [PkBlogId] int NOT NULL IDENTITY,
  [Url] nvarchar(max) NULL,
  CONSTRAINT [PK_Blogs] PRIMARY KEY ([PkBlogId])
);

CREATE TABLE [RssBlog] (
  [FkBlogId] int NOT NULL,
  [RssUrl] nvarchar(max) NULL,
  CONSTRAINT [PK_RssBlogs] PRIMARY KEY ([FkBlogId]),
  CONSTRAINT [FK_RssBlogs_Blogs_BlogId] FOREIGN KEY ([FkBlogId]) REFERENCES [Blogs] ([PkBlogId]) ON DELETE NO ACTION
);
roji commented 3 years ago

@DMX-David-Cardinal as written above, the root issue is missing support in EF Core itself - tracked by dotnet/efcore#19811. In the meantime, you can explicitly name your PKs using the Fluent API - this should unblock you.

ghost commented 3 years ago

When you say "the issue is missing support", I understand that no one is working on this at the moment and there is no estimate about when this will be ready?

Also, can you precise what you mean by explicitly naming the PKs using the fluent API? Because from what I tried it seemed like an impass though I might be missing something.

When you say explicitly naming the Pk, this is what I understand:

modelBuilder.Entity<RssBlog>(entity =>
{
  entity.Property(e => e.PkBlogId).HasColumnName("FkBlogId");
}

Yet this gives the following error: "Invalid column name 'FkBlogId'" (It isn't a typo; if I change the F for a P in both the database and the model and remove that line in the modelBuild, everything seems to run fine and as intended)

This would be the class definitions:

public partial class Blog
{
  public int PkBlogId { get; set; }
  public string Url { get; set; }
}

public partial class RssBlog : Blog
{
  public string RssUrl { get; set; }
}
roji commented 3 years ago

When you say "the issue is missing support", I understand that no one is working on this at the moment and there is no estimate about when this will be ready?

dotnet/efcore#19811 is in the 6.0.0 milestone, meaning that we plan to do it for the next release of EF Core, planned for November.

When you say explicitly naming the Pk, this is what I understand:

This isn't about naming the column that's part of the PK, it's about the name of the PK itself, as in the docs. The issue is about having EFCore.NamingConventions rewrite that name, and that's blocked by dotnet/efcore#19811.

For your issue with changing HasColumnName, can you please open a new issue with a full code sample? Also, please test whether your issue is actually related to EFCore.NamingConventions; if not, please open your issue in https://github.com/dotnet/efcore/issues.

ghost commented 3 years ago

dotnet/efcore#19811 is in the 6.0.0 milestone, meaning that we plan to do it for the next release of EF Core, planned for November.

My apologies, I wasn't familiar with the process.

This isn't about naming the column that's part of the PK, it's about the name of the PK itself, as in the docs. The issue is about having EFCore.NamingConventions rewrite that name, and that's blocked by dotnet/efcore#19811.

I don't see how to get around that since there is a clash between both PKs. Attempting to name the key on RssBlog gives the error A key cannot be configured on 'RssBlog' because it is a derived type. The key must be configured on the root type 'Blog'. From my understanding, this is blocking because the inherited column PkBlogId is a primary key on Blog, while the column FkBlogId is the primary key on RssBlog and both of them exist in the RssBlog class. I guess we will have to wait until november and hack our way around it in the meantime.

For your issue with changing HasColumnName, can you please open a new issue with a full code sample?

I don't think that's necessary.. It really seems like a byproduct of the column/key issues where either two properties are trying to map to the same column or the mapper tries to find PkBlogId in RssBlog which doesn't exist or whatnot.

roji commented 3 years ago

@DMX-David-Cardinal just to make sure, it's perfectly OK to set the key name in a TPT hierarchy - but you have to do it at the root. Being able to set it on derived types would be part of dotnet/efcore#19811.

ghost commented 3 years ago

Oh! Misunderstood the difference between the two issues. Apologies once again

gojanpaolo commented 2 years ago

Is this the issue we're getting with the following code? If so, what is the current workaround? I see comments above regarding explicitly naming the PK but I think I have tried that and it didn't work. Thank you!

I believe this is fixed in .NET 6 but we're stuck at .NET 5 atm.

using Microsoft.EntityFrameworkCore;
using System;
using System.Threading.Tasks;

namespace ConsoleApp1
{
    class Program
    {
        static async Task Main()
        {
            await using var ctx = new MyDbContext(
                new DbContextOptionsBuilder()
                .UseSqlite("datasource=:memory:")
                .UseSnakeCaseNamingConvention()
                .Options);
            Console.WriteLine(ctx.Database.GenerateCreateScript());
            await ctx.Database.EnsureDeletedAsync();
            await ctx.Database.EnsureCreatedAsync();
        }
    }
    public class MyDbContext : DbContext
    {
        public MyDbContext(DbContextOptions options) : base(options) { }
        public DbSet<Employee> Employee { get; set; }
        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Person>(_ =>
            {
                _.HasKey(_ => _.Name);
                _.OwnsOne(_ => _.Address);
            });
            modelBuilder.Entity<Employee>().ToTable("employee");
        }
    }
    public class Employee : Person
    {
        public int Salary { get; set; }
    }
    public abstract class Person
    {
        public Address Address { get; set; }
        public string Name { get; set; }
    }
    public class Address
    {
        public string Street { get; set; }
    }
}

csproj

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="5.0.13" />
    <PackageReference Include="EFCore.NamingConventions" Version="5.0.2" />
  </ItemGroup>
</Project>

exception

System.InvalidOperationException
  Message=Cannot use table 'person' for entity type 'Person' since it is being used for entity type 'Address' and the name 'PK_person' of the primary key {'Name'} does not match the name 'pk_person' of the primary key {'PersonName'}.
roji commented 2 years ago

@gojanpaolo no, that is an unrelated bug which indeed no longer occurs in 6.0. Unfortunately I'm not going to be able to backport the fix to 5.0, both because it's complex and because 5.0 goes out of support in May.

As an aside, your usage of underscore as a variable identifier is a bit odd - the C# usage is to use it only when the variable/parameter isn't going to be used - that's why it's called a "discard" (see docs. So writing _.HasKey doesn't conform to that.

gojanpaolo commented 2 years ago

@roji Thanks for the response. Is there a workaround for .NET 5? Thank you

roji commented 2 years ago

Not as far as I'm aware. Note that the end-of-life of 5 is in May, so upgrading to 6.0 is a good idea at this point.