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

Global option to always use PostgreSQL native enums? #2435

Open
aradalvand opened this issue Jul 18, 2022 · 7 comments
Open

Global option to always use PostgreSQL native enums? #2435

aradalvand opened this issue Jul 18, 2022 · 7 comments
Labels
enhancement New feature or request
Milestone

Comments

@aradalvand
Copy link

aradalvand commented Jul 18, 2022

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.

@roji
Copy link
Member

roji commented Jul 19, 2022

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
Copy link
Member

roji commented Aug 1, 2022

Closing as no response was provided.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Aug 1, 2022
@aradalvand
Copy link
Author

aradalvand commented Aug 1, 2022

@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?

@roji
Copy link
Member

roji commented Aug 1, 2022

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 roji reopened this Aug 1, 2022
@roji roji added this to the Backlog milestone Aug 1, 2022
@roji roji added the enhancement New feature or request label Aug 1, 2022
@carlosrfernandez
Copy link

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.

@roji
Copy link
Member

roji commented Nov 11, 2022

@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 (dotnet/efcore#29489).

@xamir82
Copy link

xamir82 commented Aug 1, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants