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

Implement SqlDataSource #2119

Open
roji opened this issue Aug 10, 2023 · 7 comments
Open

Implement SqlDataSource #2119

roji opened this issue Aug 10, 2023 · 7 comments
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain.

Comments

@roji
Copy link
Member

roji commented Aug 10, 2023

.NET 7.0 introduced the new DbDataSource abstraction to System.Data (issue), here's a video discussing it in depth.

Now, the .NET 7.0 implementation provides a default implementation which can be obtained from the ADO.NET provider's DbProviderFactory without the driver needing to implement anything. So the following code already works today:

await using var dataSource = SqlClientFactory.Instance.CreateDataSource("<connection string>");
await using var connection = await dataSource.OpenConnectionAsync();
await using var command = connection.CreateCommand();
command.CommandText = "SELECT 1";
Console.WriteLine(command.ExecuteScalar());

However, this provides a database-agnostic DbDataSource which isn't typed to any specific provider, so it hands out DbConnections rather than SqlConnections, which may be cumbersome to work with if the user is only targeting SQL Server.

A proper SqlDataSource implementation in SqlClient would solve this, and also provide a good configuration/setup point (e.g. for authentication/access tokens, rather than the current static methods).

@JRahnama JRahnama added the 🆕 Triage Needed For new issues, not triaged yet. label Aug 11, 2023
@JRahnama JRahnama added the 💡 Enhancement Issues that are feature requests for the drivers we maintain. label Aug 11, 2023
@JRahnama
Copy link
Contributor

Thanks @roji. We will discuss this internally and will get back to you.

@roji
Copy link
Member Author

roji commented Aug 11, 2023

Yep, something like that. Npgsql has an NpgsqlDataSourceBuilder type for configuring an NpgsqlDataSource (when you're done you call Build() on it and get your data source); that would be the type of the setupAction parameter in your lambda above. There's also an Npgsql.DependencyInjection package which does precisely this kind of nice integration with DI that you're showing above.

NpgsqlDataSourceBuilder has all manner of configuration on it: stuff for managing access tokens, tweaking type mappings, logging and telemetry configuration, etc. Basically anything that you need to pre-configure for all the connections you'll be using from that data source. This is a much better alternative than static configuration, which is what you have to do if there's no data source and users are directly instantiating SqlConnections.

(see the video I posted above for more discussion on how this is useful etc.)

@roji
Copy link
Member Author

roji commented Aug 11, 2023

Note that as a first step, I'd just implement SqlDataSource so that users can use it as a simple connection factory (e.g. to be registered in DI). Adding actual support for e.g. access token management is additional work on top of that which would require a bit more refactoring inside SqlClient to support.

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 11, 2023

Cool, I might look into that, since I already published a package which does that 😄

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 11, 2023

@roji I presume then something like ErikEJ.Data.SqlClient.DependencyInjection would be next (and maybe eventually Microsoft.Data.SqlClient.DependencyInjection) ?

@roji
Copy link
Member Author

roji commented Aug 11, 2023

Yeah, exactly. SqlDataSource is something that needs to be implemented inside SqlClient (since ideally it would go on to support actual SqlClient-specific configurations like around access tokens). A DI integration package - similar to Npgsql.DependencyInjection - could be external (e.g. ErikEJ...), though ideally it would indeed be Microsoft.Data.SqlClient.DependencyInjection at some point.

@adamsitnik
Copy link
Member

Once it gets implemented, AspNetCore.HealthChecks.SqlServer (a very popular health check package for SQL Server with 37 milion downloads) should start using it too. I've created Xabaril/AspNetCore.Diagnostics.HealthChecks#2123 to track it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Enhancement Issues that are feature requests for the drivers we maintain.
Projects
None yet
Development

No branches or pull requests

5 participants