efcore.pg icon indicating copy to clipboard operation
efcore.pg copied to clipboard

Global option to always use PostgreSQL native enums?

Open aradalvand opened this issue 3 years ago • 7 comments

Hi there. Thank you for all your hard work. Currently, if you want all your enums to be mapped to corresponding Postgres enums, you'll have to register each one of them explicitly twice in two different places, like so:

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

    static MyDbContext()
    {
        NpgsqlConnection.GlobalTypeMapper
            .MapEnum<Foo>()
            .MapEnum<Bar>()
            .MapEnum<Buzz>();
    }

    protected override void OnModelCreating(ModelBuilder builder)
    {
        builder
            .HasPostgresEnum<Foo>()
            .HasPostgresEnum<Bar>()
            .HasPostgresEnum<Buzz>();
    }

    public DbSet<Whatever> Whatevers { get; set; }
    // ...
}

This indeed works, but it's easy to see how it could become tedious and error-prone over time; you could add a new enum to your schema and very easily forget to come add it here. One workaround is to use reflection to search in the assembly and discover all the enums and map them, but this can become tricky in cases where the assembly contains other non-relevant enums but you only want to map those that are actually used in your entity classes. And even if you manage to pull that off cleanly, you'll still have to have two methods and call them in two different places (i.e. OnModelCreating and the static DbContext contructor). I think it's fair to say this provides a sub-par developer experience.

Ideally, all you would have to do to achieve this, would be to turn on some sort of a "switch", as it were, to let Npgsql know that you want your enums to be mapped to Postgres enums, as opposed to integer columns which is the default behavior in EF Core:

optionsBuilder.UseNpgsql(
    connectionString,
    o => o.UsePostgresEnums()
);

I would assume this was at some point considered but for some reason eventually discarded, if so I'm curious what that reason was and whether it still applies. I think having a feature like this would certainly help improve the developer experience, this is definitely one of those things that makes you scratch your head and go "Wait, why isn't there a simple option for this?".

Thank you in advance.

aradalvand avatar Jul 18 '22 17:07 aradalvand

This indeed works, but it's easy to see how it could become tedious and error-prone over time; you could add a new enum to your schema and very easily forget to come add it here.

There's not much risk here; if you forget to add it, it simply would throw an exception when you try to use it. At that point you'd be reminded to add it.

Stepping back... As you write, it's not appropriate to automatically map all auto-discovered enums to the database, since many enums aren't meant to be mapped; this is why a user gesture is needed to indicate that a specific enum should be mapped. In addition, auto-discovery of types in an assembly isn't friendly to trimming, and we'd also somehow have to know which assembly (or assemblies) to scan. That's nothing something I'd want Npgsql to do.

I do agree that having to specify the enum twice is sub-par; #1026 tracks improving that. However, in practice this is very rarely a problem, and nobody has actually complained about it as far as I can remember. So this isn't a very high-priority issue.

roji avatar Jul 19 '22 08:07 roji

Closing as no response was provided.

roji avatar Aug 01 '22 09:08 roji

@roji Oh sorry, I missed this one!

There's not much risk here; if you forget to add it, it simply would throw an exception when you try to use it. At that point you'd be reminded to add it.

Well, wouldn't the enum property result in an integer column in the database in that case? What exception?!

In addition, auto-discovery of types in an assembly isn't friendly to trimming, and we'd also somehow have to know which assembly (or assemblies) to scan. That's nothing something I'd want Npgsql to do.

But why would you want to do an assembly scan? Why not just look at the properties of entity classes that have an enum as their type and map those? That's what I had in mind personally, not an assembly scan. Is that not feasible?

aradalvand avatar Aug 01 '22 09:08 aradalvand

Well, wouldn't the enum property result in an integer column in the database in that case? What exception?!

You're right, I had the scenario in mind where you do call MapEnum, but forget to have EF create it via HasPostgresEnum - that would cause an exception. But you're right that if you forget both, you'll just get the default int mapping instead.

Thinking about this again, I do think you're right and there's value in having a global opt-in to native PostgreSQL enums. The provider could have an (optional) convention that goes over the model, and for every enum CLR type it finds there, configures it as a PostgreSQL enum.

However, there's a technical stumbling block - the MapEnum call. The below shouldn't really interest you - it's mostly implementation notes for myself. I'm reopening to think more about this.

  • MapEnum tells the lower-level, ADO.NET provider to do the enum mapping at its level. This is totally outside of EF's scope, and right now I'm not sure how that could be made to run automatically on startup.
  • In normal usage, the model gets built on startup and the convention could theoretically call MapEnum there. Since the compiled model feature may be in use, this part needs to be a runtime convention.
  • More importantly, with the introduction of DbDatasource, we should be moving from using global enum mapping to mapping at the data source level (depends on #4494). There isn't going to be any way that EF can interact with the data source builder though - the building happens earlier, and EF just gets a built, fully-baked DbDataSource with all the mappings already done.
  • A possible way forward is to allow full usage of enums at the ADO level without requiring up-front mapping. For example, we could allow sending parameters with NpgsqlParameter.DataTypeName referencing the PG enum name; when reading, the type in GetFieldValue<T> would instruct Npgsql which CLR enum type to return. This will likely be challenging, and probably wouldn't allow stuff like case-by-case name translators (though a global one at the data source level should be fine).

In short, I agree there's something to be done here, but it isn't going to be simple.

roji avatar Aug 01 '22 10:08 roji

Hello Roji, has there been any traction on this?

I've just started looking into migrating to Npgsql + Entity Framework 7 and already got the warning that using NpgsqlConnection.GlobalTypeMapper.MapEnum<T>(pgName); is obsolete.

For now, it's just a warning, the GlobalTypeMapper is still supported, but at some point this will be removed I assume.

carlosrfernandez avatar Nov 11 '22 15:11 carlosrfernandez

@carlosrfernandez GlobalTypeMapper.MapEnum definitely won't be removed until an alternative mapping solution is provided for EF, it's definitely OK to continue using that. Note that this issue is about a global option to always use native enums, which is different from a mechanism map a single enum that doesn't depend on GlobalTypeMapper. I have some thoughts on how to do things better here, but it depends on some EF-side changes (https://github.com/dotnet/efcore/issues/29489).

roji avatar Nov 11 '22 16:11 roji

This would be really convenient. I'm experiencing this exact issue. It's a bit tedious to have to map each and every single enum manually in two places when you just want all the CLR enums in the model to be mapped to Postgres ones. A simple opt-in switch would be perfect.

xamir82 avatar Aug 01 '23 03:08 xamir82