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

Type mapping plugins and NpgsqlDataSource #2518

Closed
roji opened this issue Oct 7, 2022 · 5 comments
Closed

Type mapping plugins and NpgsqlDataSource #2518

roji opened this issue Oct 7, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@roji
Copy link
Member

roji commented Oct 7, 2022

#2400 introduced basic support for NpgsqlDataSource in EFCore.PG; that means you can now call UseNpgsql with a data source etc.

However, npgsql/npgsql#4494 is also introducing type mapping plugins and configuration at the data source level, replacing the previous connection-level type mapping support. We should think about how to react to this in EFCore.PG.

Right now, calling UseNodaTime() for EFCore.PG also globally registers the ADO-level NodaTime plugin; this is necessary in order for the traditional approach to work (without data source). Note that it also doesn't work for data source, since it's too late: when UseNpgsql is called, the data source has already been built, and so the global mapping (which is done later) doesn't take effect.

We can simply remove the global ADO mapping from UseNodaTime, and require users to handle things themselves (i.e. add the ADO plugin either to their data source, or globally (which they already need to do with enums)). This would be a breaking change and make things less seamless, but I'm not seeing anything better. Note that this would align plugins with how enums currently work: you have to do the ADO global mapping explicitly before everything else (but with enums you don't also have to add an EF plugin...).

We could also allow users to provide an NpgsqlDataSourceBuilder (instead of an NpgsqlDataSource), allowing us to add the ADO plugin before building the data source. But that's weird (register a NpgsqlDataSourceBuilder in DI??), and opens the question of Build() vs. BuildMultiHost().

On the enum mapping side,EFCore.PG's NpgsqlTypeMappingSource currently uses reflection to see which enums are registered globally. We could maybe improve this by exposing info from NpgsqlDataSource about mapped enums, though we'd still need to support the global mode...

@roji roji self-assigned this Oct 7, 2022
@roji roji added the enhancement New feature or request label Oct 7, 2022
@roji roji added this to the 8.0.0 milestone Oct 7, 2022
@roji
Copy link
Member Author

roji commented Oct 7, 2022

We could also allow users to provide an NpgsqlDataSourceBuilder (instead of an NpgsqlDataSource), allowing us to add the ADO plugin before building the data source.

A much better version of this is to allow users to configure EF via an NpgsqlDataSourceBuilder directly. So instead of passing an NpgsqlDataSource to UseNpgsql, we could have an overload accepting an Action<NpgsqlDataSourceBuilder>. This is a nice way to allow the user to configure the ADO options, and it should also allow us internally to add ADO plugins to the builder (based on EF plugins) before we call Build() on it.

It might get confusing with the overload that accepts Action<NpgsqlDbContextOptionsBuilder>, maybe use a different method name (test before). Consider this for 7.0.

@roji roji modified the milestones: 8.0.0, 9.0.0 Nov 15, 2023
@roji
Copy link
Member Author

roji commented Sep 13, 2024

This was mostly done in #3167: calling UseNodaTime() at the EF level now internally builds an NpgsqlDataSource, calling UseNodaTime() on its builder, and everything just works.

The last missing piece is to expose NpgsqlDataSourceBuilder via EF's UseNpgsql, to allow users to configure their NpgsqlDataSource without having to build it externally (because for externally-built data sources, users have to manually do UseNodaTime() for the Npgsql level).

@roji roji closed this as completed Sep 13, 2024
@azan-n
Copy link

azan-n commented Oct 23, 2024

The last missing piece is to expose NpgsqlDataSourceBuilder via EF's UseNpgsql, to allow users to configure their NpgsqlDataSource without having to build it externally (because for externally-built data sources, users have to manually do UseNodaTime() for the Npgsql level).

@roji do you think I can take this up?

@roji
Copy link
Member Author

roji commented Oct 23, 2024

@azan-n this was already done, see ConfigureDataSource() in the 9.0 release notes.

@azan-n
Copy link

azan-n commented Oct 24, 2024

@azan-n this was already done, see ConfigureDataSource() in the 9.0 release notes.

My bad, I didn't check. Thanks!

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

2 participants