efcore / EFCore.NamingConventions

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

8.0.2 doesn't rewrite TPH indexes #256

Closed aradalvand closed 8 months ago

aradalvand commented 8 months ago

I'm now on 8.0.2 (although I did observe this same problem in 8.0.1 too) and the plugin seems to be messing with TPH index names this time.

Updating EFCore.NamingConventions to 8.0.2 and then running dotnet ef migrations add ... creates this weird migration for me:

image

It apparently wants to make the ix prefix uppercase, which definitely looks like a bug; and this is, once again, only happening for TPH tables.

Minimal repro: .csproj:

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net8.0</TargetFramework>
        <RootNamespace>efcore_npgsql_json</RootNamespace>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="EFCore.NamingConventions" Version="8.0.2" />
        <PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="8.0.0">
            <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
            <PrivateAssets>all</PrivateAssets>
        </PackageReference>
        <PackageReference Include="Npgsql.EntityFrameworkCore.PostgreSQL" Version="8.0.0" />
    </ItemGroup>

</Project>

Program.cs:

using Microsoft.EntityFrameworkCore;
using Npgsql;

using var db = new AppDbContext();
db.Database.EnsureDeleted();
db.Database.EnsureCreated();

class AppDbContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder) =>
        optionsBuilder
            .UseNpgsql("YOUR_CONNECTION_STRING")
            .UseSnakeCaseNamingConvention()
            .LogTo(Console.WriteLine, Microsoft.Extensions.Logging.LogLevel.Information);

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Lesson>()
            .HasDiscriminator<string>("Type")
                .HasValue<VideoLesson>("video")
                .HasValue<ArticleLesson>("article");
    }

    public DbSet<Lesson> Lessons => Set<Lesson>();
}

[Index(nameof(Title))]
public abstract class Lesson
{
    public int Id { get; set; }
    public required string Title { get; set; }
}

public class VideoLesson : Lesson
{
    public required string VideoId { get; set; }
}

public class ArticleLesson : Lesson
{
    public required string Content { get; set; }
}

dotnet run output — notice how the IX prefix is uppercased.

info: 01/05/2024 20:25:19.518 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command)
      Executed DbCommand (262ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      DROP DATABASE idk_whatever WITH (FORCE);
info: 01/05/2024 20:25:19.792 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command)
      Executed DbCommand (118ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE DATABASE idk_whatever;
info: 01/05/2024 20:25:19.995 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command)
      Executed DbCommand (23ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE TABLE lessons (
          id integer GENERATED BY DEFAULT AS IDENTITY,
          title text NOT NULL,
          type character varying(8) NOT NULL,
          content text,
          video_id text,
          CONSTRAINT pk_lessons PRIMARY KEY (id)
      );
info: 01/05/2024 20:25:20.003 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command)
      Executed DbCommand (8ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE INDEX "IX_lessons_title" ON lessons (title);
roji commented 8 months ago

Thanks @aradalvand, keep 'em coming...

This plugin is much more complicated than it seems, unfortunately. But we're hopefully close to getting rid of the issues.

aradalvand commented 8 months ago

This plugin is much more complicated than it seems

Very true, I experienced that firsthand when I tried to give fixing #234 a shot, but the plugin's implementation was nowhere near as straightforward as I thought it would be, so I had to give up :D Thanks for all the hard work!

roji commented 8 months ago

@aradalvand I've fixed this - will give it a few more days to see if other bug reports popup before releasing 8.0.3.

roji commented 8 months ago

@aradalvand can I ask you to give the latest version a test? You can download the nupkg here from the build artifacts - it would be good to get confirmation that at least in your case there are no other pending problems.

aradalvand commented 8 months ago

@roji Sure. I just tested it and the weird RenameIndexes are now gone. So, this has been fixed. There also don't seem to be any more issues in my case related to this plugin. Thanks.

roji commented 8 months ago

Thank you! I'll give it a couple more days and then release.

DenisSikic commented 8 months ago

Hi Guys (@roji )

Has the default TPH behavior been changed to TPT in 8.0.1?

We're upgrading from 8.0.0 and EF is trying to generate a table for each type and blowing up, where-as before it was all in the inherited class' table.

Also, we started seeing errors that we had to suppress using Microsoft.EntityFrameworkCore.Diagnostics.RelationalEventId.ForeignKeyPropertiesMappedToUnrelatedTables which we're nervous about. Also related to inheritance in our case.

Is 8.0.1 safe? Are there breaking changes somewhere?

Thanks! Denis

roji commented 8 months ago

@DenisSikic 8.0.3 is out, have you tried that?

DenisSikic commented 8 months ago

@roji I have 8.0.3 naming conventions, and 8.0.1 everything else (all latest).

roji commented 8 months ago

Are you saying you're seeing a behavior difference between EF 8.0.0 and 8.0.1 (while keeping EFCore.NamingConventions at 8.0.3)? If so, that's definitely I'd like to see in a minimal, runnable repro - can you please open a new issue with one?

DenisSikic commented 8 months ago

@roji Yes I'm seeing TPH no longer as the default behaviour. Trying to currently force TPH via OnModelCreating reflection loop. Still researching, if I can fix it I'll try to create you a sample otherwise I'm going to revert back to 8.0.0.

roji commented 8 months ago

@DenisSikic I'd very much appreciate seeing a sample - just reverting back wouldn't give us the opportunity to investigate and fix a possible bug (either here in EFCore.NamingConventions or in EF itself).

DenisSikic commented 8 months ago

@roji I couldn't reproduce it in a separate project. I had this code to fix some snake case bug that showed up Sep 29, 2023. Now when I remove it, everything is fine and it appears that bug was fixed and now my code was causing issues. image

roji commented 8 months ago

@DenisSikic thanks for spending the time to look into it! Please don't hesitate to open a new issue if you do run into trouble.