SqlConnectionX Instantiation via SqlConnection #2570
Conversation
… to experimental connection based on _isExperimental setting.
mdaigle
requested review from
cheenamalhotra,
benrr101,
saurabh500,
VladimirReshetnikov and
samsharma2700
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnectionX.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
Show resolved
Hide resolved
|
What is SqlConnectionX and why does it exist? |
@Wraith2 Part of the SqlClientX project. The goal is to address core issues in SqlClient under an experimental code path in the existing package. You might find this project particularly interesting and your feedback and expertise would be beneficial, if you have time to take a look at the PRs coming in. |
| @@ -214,6 +228,11 @@ public SqlConnection(string connectionString, SqlCredential credential) : this() | |||
| // checking pool groups which is not necessary. All necessary operation is already done by calling "ConnectionString = connectionString" | |||
| } | |||
|
|
|||
| /// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/ctorConnectionStringCredential/*' /> | |||
| public SqlConnection(string connectionString, SqlCredential credential) : this(connectionString, credential, false) | |||
There was a problem hiding this comment.
Instead of setting useExperimental to false, have you thought about setting it via AppContext switch that can be configured by user?
There was a problem hiding this comment.
I'm open to it. In practice, is AppContext only used like this article describes (as an opt-out mechanism for new behavior) or is it used as a general settings store? https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-appcontext
There was a problem hiding this comment.
Another possible direction here is to use the DbDataSource abstraction as the factory for the new experimental connection objects (#2119 generally tracked introducing a SqlDataSourcei n SqlClient). In other words, you would have a SqlDataSourceBuilder which can produce a SqlDataSource with a particular configuration; one of these configurable options would be to produce "experimental" connections. The disadvantage of this method is that users must use the SqlDataSource mechanism to create such experimental connections (but people would have to modify their code in any case). @saurabh500 and I discussed these possibilities a bit.
Whatever's done, I'd recommend putting the [Experimental] attribute on any programmatic API here; this attribute was introduced in .NET 8.0, and causes a warning with a specific diagnostic ID to be reported when a user uses an API (the user can then decide to suppress that specific diagnostic ID). This would make it easier to remove later as an experimental, non-supported API etc.
There was a problem hiding this comment.
@roji, thanks! We do plan to follow the lead of npgsql and use the data source builder pattern. This is a start at integrating with the existing call pattern to reduce the burden of transitioning to the new implementation. The idea being we could seamlessly default you to the new driver if you're on the right .NET version and platform. But I'm thinking I'll shelve this PR for a bit until the other pieces are in place. I'm learning more about the builder and data source pattern as we implement those pieces and I think this PR will change slightly.
There was a problem hiding this comment.
Sure thing! Please feel free to ping me or reach out on Teams with any questions.
I'd be maybe wary of varying the default implementation based on the .NET version targeted - it's quite unexpected for some to switch the targeted TFM and for SqlClient to suddenly be a completely different implementation. But anyway, I'm sure all this will be discussed at some point.
| return _sqlConnectionX.OpenAsync(cancellationToken); | ||
| } | ||
|
|
||
| return IsProviderRetriable? |
| @@ -33,6 +36,19 @@ public SqlConnection() : base() | |||
| _innerConnection = DbConnectionClosedNeverOpened.SingletonInstance; | |||
| } | |||
|
|
|||
| internal SqlConnection(bool useExperimental): this() | |||
| } | ||
| else | ||
| { | ||
| _innerConnection = DbConnectionClosedNeverOpened.SingletonInstance; |
There was a problem hiding this comment.
Hasn't this assignment already happened on line 36?
| { | ||
| } | ||
|
|
||
| internal SqlConnectionX(string connectionString, SqlCredential credential): this() |
mdaigle
force-pushed
the
sqlconnectionx-instantiation
branch
from
19e361e to
e6fc7fc
Compare
…lconnectionx-instantiation
No description provided.