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

Enum mapping with v9 and ordering #3390

Closed
bmarkovic17 opened this issue Nov 27, 2024 · 14 comments
Closed

Enum mapping with v9 and ordering #3390

bmarkovic17 opened this issue Nov 27, 2024 · 14 comments

Comments

@bmarkovic17
Copy link

Hello,

I have a question about the new enum mapping with which we are having some issues while doing our migration from v8 to v9. We are migrating a project that has 3 enums mapped I would say in the standard way:

  1. On our NpgsqlDataSource we used MapEnum<T> to map each enum
  2. In our OnModelCreating method we used HasPostgresEnum<T> to register each of the enums

After updating the nuget packages and dotnet-ef tool, we created a new migration that wanted to revert our enum based table columns back to integers. We saw in the documentation that enum mapping is changing considerably with v9, so we removed invocations of the HasPostgresEnum<T> from our OnModelCreating method and added MapEnum<T> calls to UseNpgsql's npgsqlOptionAction delegate (both on our IDesignTimeDbContextFactory implementation and AddDbContextFactory<T>'s optionsAction). Unfortunately, that didn't work out as the migration created with this configuration resulted in recreating the same DB enums but with different sort order (the order before was the same as in our C# enum, but with v9 it is alphabetically)

...
    .Annotation("Npgsql:Enum:offering.market_status", "active,off,resulted,suspended")
    .OldAnnotation("Npgsql:Enum:offering.market_status", "off,suspended,active,resulted")
...

After further analysis and troubleshooting we noticed that if we also add back the HasPostgresEnum<T> part to our OnModelCreating method we get it to work as it was, i.e. new migrations aren't changing anything about our existing mapped enums. I'm not sure if we are doing something wrong or this is supposed to work like this, but with v9 we now have to keep our enum mappings in 4 places, while before we had to keep it in 2, which was also not ideal.

Thanks.

@roji
Copy link
Member

roji commented Nov 27, 2024

@bmarkovic17 thanks for reporting.. It looks like the new automatic support in EF 9 (which obviates HasPostgresEnum) is simply configuring the enums in a different order, nothing more. That means that you should be able to generate a new migration with 9.0, and then empty the migration of all contents - so have an empty Up/Down migration - since no actual database change needs to happen (enum label ordering is irrelevant. This will ensure that your context snapshot is now aligned with what EF 9 generates, and from that point on everything should be OK.

In other words, this seems like a one-time wrinkle in the transition from the pre-9 explicit HasPostgresEnum() to the new 9 support. I'll go ahead and close this for now as I don't see what can be done to avoid it, and on the other hand it's very easy to work around. But please post back here to let me know how it goes, and I'll reopen if needed.

@roji roji closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2024
@bmarkovic17
Copy link
Author

Thanks @roji for the quick reply.

I can confirm that with an empty migration I could remove the HasPostgresEnum<T> calls from my codebase. With this change and moving NpgsqlDbContextOptionsBuilder's MapEnum<T> calls into an extension method I'm back to mapping my enums only in two places.

@roji
Copy link
Member

roji commented Nov 28, 2024

Great, thanks for confirming! Though if you're on 9 and aer using the new ConfigureDataSource(), you should only need to be mapping your enums in a single place (docs).

@bmarkovic17
Copy link
Author

Sorry for my late reply and thanks for the heads up on this, I don't know how we missed this. We got rid of the external NpgsqlDataSource and are mappings our enums only in the UseNpgsql method call.

Thanks again for all the help and quick replys.

@or2e
Copy link

or2e commented Dec 1, 2024

Hello!

After migrating to .net 9, we created an empty migration, but still, each subsequent migration tries to change the order of our Enums.

It's not a problem for us, but we'll have to keep a "close watch" on each migration and change manually )

@roji
Copy link
Member

roji commented Dec 2, 2024

@or2e that definitely shouldn't happen - try taking a look at the context snapshot (that's the file next to the migration which captures the model as it is after the migration). If you can put together some sort of minimal repro for this, please open a new issue and I'll investigate.

@or2e
Copy link

or2e commented Dec 2, 2024

Can I ask a question?
Did I have to manually modify *_*.Designer.cs and CrpgDbContextModelSnapshot.cs, because the enum sort order has changed in them too?

I suspect that this is why the order changes in subsequent migrations, because I didn't commit it in the snapshot.

@roji
Copy link
Member

roji commented Dec 2, 2024

You are not supposed to modify those files yourself - EF generates them when you add a new migration (and they're supposed to contain up-to-date info about your model, including the correct enum ordering). However, you do have to commit them.

@blastrock
Copy link

We just got hit by this issue too.

That means that you should be able to generate a new migration with 9.0, and then empty the migration of all contents

I think this works indeed

enum label ordering is irrelevant

But this seems wrong. In PostgreSQL, enums are ordered and you can compare enum values to each other. We currently don't do that in our code, but we must be careful not to do it in the future as it won't behave as expected.

@roji
Copy link
Member

roji commented Dec 5, 2024

But this seems wrong. In PostgreSQL, enums are ordered and you can compare enum values to each other. We currently don't do that in our code, but we must be careful not to do it in the future as it won't behave as expected.

Columns in a table also have an ordering (the order in which they were created), but that shouldn't be relevant to you when using EF in any way.

@blastrock
Copy link

I agree for columns, but it's not the same for enums since EF allows you to write something like

myTable.Where(o => o.MyEnum > Enum.SomeValue);

And the meaning of this line changes depending on the order of the enum.

@roji
Copy link
Member

roji commented Dec 5, 2024

Yes, .NET allows you to order by enums; but this will not necessarily have the same meaning when doing it in .NET (the ordering is based on the underlying int value, which itself is based on declaration order in the .NET enum) and in SQL (PostgreSQL enums aren't numbers).

It's true that we could (by default) create enum labels in the order in which they were defined in .NET, making the two consistent; this would allow PG enums to e.g. sort in the same way as their .NET counterparts. But PG is limited in terms of altering existing enums (e.g. reordering/removing existing labels isn't supported). In any case, I've opened #3398 to track this.

In any case, any user who really cares about label ordering can use HasPostgresEnum as before and manually specify the exist ordering they want. The only thing that changed is that version 9 of the provider effectively does HasPostgresEnum implicitly for you, in alphabetical order.

@ValeriyGourov
Copy link

@roji
The alphabetical ordering breaks the upgrade from EF 8 to EF 9. For example, I have an enum:

public enum SessionStatus
{
	Created = 1,
	InProgress = 2,
	Completed = 3,
	Failed = 4
}

ModelSnapshot:

NpgsqlModelBuilderExtensions.HasPostgresEnum(modelBuilder, "SessionStatus", new[] { "Created", "InProgress", "Completed", "Failed" });

After upgrading to EF 9 I create a new migration and in that migration I have changes for the enums:

migrationBuilder.AlterDatabase()
	.Annotation("Npgsql:Enum:SessionStatus", "Completed,Created,Failed,InProgress")
	.OldAnnotation("Npgsql:Enum:SessionStatus", "Created,InProgress,Completed,Failed");

And ModelSnapshot:

NpgsqlModelBuilderExtensions.HasPostgresEnum(modelBuilder, "SessionStatus", new[] { "Completed", "Created", "Failed", "InProgress" });

A SQL script is executed while the database is being updated:

ALTER TYPE "SessionStatus" ADD VALUE 'Completed' BEFORE 'Created';

and there's an exception:

Npgsql.PostgresException (0x80004005): 42710: enum label "Completed" already exists
at Npgsql.Internal.NpgsqlConnector.ReadMessageLong(Boolean async, DataRowLoadingMode dataRowLoadingMode, Boolean readingNotifications, Boolean isReadingPrependedMessage)
at System.Runtime.CompilerServices.PoolingAsyncValueTaskMethodBuilder1.StateMachineBox1.System.Threading.Tasks.Sources.IValueTaskSource.GetResult(Int16 token)
at Npgsql.NpgsqlDataReader.NextResult(Boolean async, Boolean isConsuming, CancellationToken cancellationToken)
at Npgsql.NpgsqlDataReader.NextResult(Boolean async, Boolean isConsuming, CancellationToken cancellationToken)
at Npgsql.NpgsqlDataReader.NextResult()
at Npgsql.NpgsqlCommand.ExecuteReader(Boolean async, CommandBehavior behavior, CancellationToken cancellationToken)
at Npgsql.NpgsqlCommand.ExecuteReader(Boolean async, CommandBehavior behavior, CancellationToken cancellationToken)
at Npgsql.NpgsqlCommand.ExecuteNonQuery(Boolean async, CancellationToken cancellationToken)
at Npgsql.NpgsqlCommand.ExecuteNonQuery()
at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary2 parameterValues) at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.Execute(IReadOnlyList1 migrationCommands, IRelationalConnection connection, MigrationExecutionState executionState, Boolean beginTransaction, Boolean commitTransaction, Nullable1 isolationLevel) at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.<>c.<ExecuteNonQuery>b__3_1(DbContext _, ValueTuple6 s)
at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded)
at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IReadOnlyList1 migrationCommands, IRelationalConnection connection, MigrationExecutionState executionState, Boolean commitTransaction, Nullable1 isolationLevel)
at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.MigrateImplementation(DbContext context, String targetMigration, MigrationExecutionState state, Boolean useTransaction)
at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.<>c.b__20_1(DbContext c, ValueTuple4 s) at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.<>c__DisplayClass28_02.b__0(DbContext context, TState state)
at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.ExecuteImplementation[TState,TResult](Func3 operation, Func3 verifySucceeded, TState state)
at Microsoft.EntityFrameworkCore.Storage.ExecutionStrategy.Execute[TState,TResult](TState state, Func3 operation, Func3 verifySucceeded)
at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
at Npgsql.EntityFrameworkCore.PostgreSQL.Migrations.Internal.NpgsqlMigrator.Migrate(String targetMigration)
at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.UpdateDatabase(String targetMigration, String connectionString, String contextType)
at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabaseImpl(String targetMigration, String connectionString, String contextType)
at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabase.<>c__DisplayClass0_0.<.ctor>b__0()
at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)
Exception data:
Severity: ERROR
SqlState: 42710
MessageText: enum label "Completed" already exists
File: pg_enum.c
Line: 348
Routine: AddEnumLabel

@roji
Copy link
Member

roji commented Dec 28, 2024

@ValeriyGourov thanks for flagging this - I can see it happening. I opened #3424 to track this separately, as this issues already discusses various other, unrelated aspects of this.

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

No branches or pull requests

5 participants