Make the enumName parameter in NpgsqlDbContextOptionsBuilder.MapEnum<T>() optional.

[Brar]

#3167 added NpgsqlDbContextOptionsBuilder.MapEnum<T>() with the required string parameter enumName.

We should allow that to get inferred from the CLR type like we do in NpgsqlDataSourceBuilder.MapEnum<T>()

[Brar]

The new API actually seems to be even more problematic since it currently doesn't seem to work with the NamingConventions package.

[roji]

@Brar that's (currently) expected, since the enum name must be provided explicitly by the user, no?

We still have to think about the precise behavior we want to implement here... i.e. if we allow the enum name to be omitted from the new EF MapEnum, should it use the INameTranslator configured on the global type mapper? (note that there's no access currently to NpgsqlDataSourceBuilder, so there's no place to configure an INameTranslator except the global one, which is an obsolete API). Or should we just make this managed via the NamingConventions plugin?

Note that in the future I do plan to expose an EF UseNpgsql API() which provides access to an NpgsqlDataSourceBuilder, at which point it would technically be possible for the user to set an INameTranslator. But I'm not sure EF would have access to that ADO-level setting...

In other words, this is a bit more complex than it seems... :(

[Brar]

I see.
The way I (and probably others) would like to approach naming conventions is to enable them and then forget about my names at the EF level. They are mostly to save me from having to quote everything in cases where I access the database directly via SQL.
With that it is at least surprising, if parts of EF support that and others don't.

Whether naming conventions should be applied to names you pass in explicitly or not is a separate question to me.
You could also see them as a convention that should be applied to all names - even the ones you set explicitly - and that can be changed across the whole codebase with a single gesture. Then again, that would probably require the presence of yet another API gesture to explicitly opt out of applying naming conventions in individual cases where you really want/have to enforce a certain name.

[roji]

Yeah, that all makes sense.

So first, EF has a very clear layered configuration model, where conventions are the weakest, and are always overridden by data annotations (attributes), which are themselves always overridden by the Fluent API; so if the user specifies something explicitly (say, a table name), it's not possible for a convention to then modify that. I think that generally makes sense - if you're explicitly setting a table name, a convention shouldn't then change it (just apply the naming convention yourself to whatever name you set explicitly). And as you say, if conventions did apply to explicitly-set config, there would probably need to be further config APIs to opt out of that somehow, which is yet more complexity etc.

Now, regardless of explicit vs. convention... I agree that ideally enums in EF would behave just like tables - there would be a default database name for the enum (and its labels) that's derived from the CLR type, and then you'd be able to override that via a convention or explicit config. However, enums are unfortnuately a bit of a strange beast... Tables, for example, exist purely in the EF model, which is built up via conventions (and of course explicit user config gestures); EFCore.NamingConventions is a pure convention plugin, which means it simply plugs in an additional convention into the model building process, modifying table/column names etc.

Enums also exist in the model - this is what allows us to create them in the database via migrations (in that sense they're just like tables); and EFCore.NamingConventions could easily affect enum names (and enum label names!) at that level (so determining how the migration is created). However, enums also exist as a type, which means they're known to NpgsqlTypeMappingSource, which is where all the EF type mappings are managed (this is the service that resolves a RelationalTypeMapping given a CLR type, or a database store type as a string). For example, when you do something like Where(b => b.SomeEnum == MyEnum.Happy), the query pipeline find the type mapping on the b.SomeEnum property and applies it to the other side (MyEnum.Happy), and this is how we know which literal representation to generate for Happy (or if it's a parameter, which NpgsqlParameter.DataTypeName to set!).

The thing is that type mappings aren't a part of the model, and so cannot be affected by conventions; so at least the way things currently work, EFCore.NamingConventions would be able to affect the enum migration, but not the query pipeline. So I'm currently unsure how it would be possible to make things work.

BTW thanks, this conversation is helping me think through all these things.

[Brar]

Thanks!
I'm slowly getting an understanding of the inner model.
This makes it even more problematic if I am forced to name things explicitly because I have to remember to also rename B if I rename A.
In C# I typically solve this via nameof() and let the IDE do it's Job when renaming things but here I'd need nameof().ApplyNamingConvention() or at least do nameof().ToLower() if I'm willing to accept the ugly name and only want to get rid of the quoting.

Also, I'm pretty sure it didn't even work without me renaming something, so there might be a clash where the naming conventions are applied partially.

[roji]

This makes it even more problematic if I am forced to name things explicitly because I have to remember to also rename B if I rename A. In C# I typically solve this via nameof() and let the IDE do it's Job when renaming things [...]

Can you provide a concrete example of what you have in mind here? At least in theory, the database name of something (table, enum) should only ever need to be specified once; in the rest of the .NET program, you're supposed to only use the .NET type/property/whatever. So I'm not sure why naming something explicitly means you need to rename B if you rename A...

Also, I'm pretty sure it didn't even work without me renaming something, so there might be a clash where the naming conventions are applied partially.

Do you mean that you think there's a problem/bug with the current bits? If so maybe open an issue?

[Brar]

Do you mean that you think there's a problem/bug with the current bits?

Ok, it dose not repro.

At least in theory, the database name of something (table, enum) should only ever need to be specified once; in the rest of the .NET program, you're supposed to only use the .NET type/property/whatever.

Yeah, it works exactly like this, so everything should be great.

My attempt to repro the problem I thought I had, which actually just works as it should except for the looged parameter values:

Code

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();
context.CustomerBlogs.Add(new() { CustomerId = 123});
await context.SaveChangesAsync();
var blogType = BlogType.CustomerBlog;
var someBlog = await context.Blogs.Where(b => b.BlogType == blogType).SingleAsync();


public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }
    public DbSet<CustomerBlog> CustomerBlogs { get; set; }
    public DbSet<EmployeeBlog> EmployeeBlogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseNpgsql("Host=localhost;Username=npgsql_tests;Password=npgsql_tests", builder => builder.MapEnum<BlogType>("blog_type")).UseSnakeCaseNamingConvention()
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>()
            .HasDiscriminator(type => type.BlogType)
            .HasValue<CustomerBlog>(BlogType.CustomerBlog)
            .HasValue<EmployeeBlog>(BlogType.EmployeeBlog);
    }
}

public abstract class Blog
{
    public int Id { get; set; }
    public abstract BlogType BlogType { get; set; }
}

public sealed class CustomerBlog : Blog
{
    public required int CustomerId { get; set; }

    public override BlogType BlogType
    {
        get => BlogType.CustomerBlog;
        set { }
    }
}

public sealed class EmployeeBlog : Blog
{
    public required int EmployeeId { get; set; }

    public override BlogType BlogType
    {
        get => BlogType.EmployeeBlog;
        set { }
    }

}

public enum BlogType
{
    CustomerBlog,
    EmployeeBlog,
}

[zotanmew]

Shouldn't this be inferred through the PgName attribute? That's how the old API worked at least

[roji]

@zotanmew I intend to look at this before 9.0 releases - but for now the name needs to be explicitly specified.

[zotanmew]

Perfect, thank you! I'm happy to finally have everything working in our net90 branch :)

[Atulin]

FWIW, I'll be updating Atulin/NpgsqlSourceGenerators to work with the new API as well, certainly with an option to just infer the names

[zotanmew]

Hmm, it seems like this is ignoring the [PgName] attributes that the old MapEnum respects 🤔

[roji]

@zotanmew are you referring to [PgName] on the enum labels, or on the enum itself? Can you open a new issue with a minimal repro?

[zotanmew]

On the enum itself, and gladly.

[roji]

Yeah, makes sense that I made an oversight there - please open the new issue with a repro and I'll take care of this for 9.0.1.