-
Notifications
You must be signed in to change notification settings - Fork 228
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
Support DbDataSource #2429
Support DbDataSource #2429
Conversation
// Otherwise check the application service provider to see if one is registered there. | ||
static DbDataSource? GetDataSource(IDbContextOptions options, NpgsqlOptionsExtension npgsqlOptions) | ||
=> npgsqlOptions.DataSource | ||
?? options.FindExtension<CoreOptionsExtension>()?.ApplicationServiceProvider?.GetService<DbDataSource>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safer to do the following
?? options.FindExtension<CoreOptionsExtension>()?.ApplicationServiceProvider?.GetService<DbDataSource>(); | |
?? options.FindExtension<CoreOptionsExtension>()?.ApplicationServiceProvider?.GetService<NpgsqlDataSource>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that we want to support a wrapping data source as well, which wouldn't be an NpgsqlDataSource.
Basically I think that if a DbDataSource is registered in DI, and the user hasn't specified a connection string or connection in UseNpgsql, it makes sense to pick that up. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to leave the wrapping DbDataSource scenario to manual config. The reason I gave the suggestion is that you don't want to blow up down the line because DbDataSource is actually of type SqliteDataSource and it doesn't quite work the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But wouldn't that happen only if you don't otherwise explicitly configure EF (i.e. with a connection string), meaning that it will blow up anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, we'll start by only resolving NpgsqlDataSource from DI, to minimize confusion in case an incompatible data source is present. Users who want wrappers can just use the explicit option with UseNpgsql.
Part of npgsql#2400
This provides the basic support, allowing:
... or with DI:
Notably excluded are:
Part of #2400
/cc @ajcvickers @NinoFloris @vonzshik