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.

Sign up for GitHub

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

SqlConnectionX Instantiation via SqlConnection #2570

Closed
wants to merge 10 commits into from
Closed

SqlConnectionX Instantiation via SqlConnection #2570

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c3c07c7
Add SqlConnectionX
mdaigle Jun 11, 2024
413fd13
Add missing methods.
mdaigle Jun 11, 2024
4279051
Remove extra docs
mdaigle Jun 11, 2024
7b308ec
Fix formatting and visibility.
mdaigle Jun 12, 2024
a10aa68
Add constructors with experimental argument. Route Open and OpenAsync…
mdaigle Jun 12, 2024
6af9a3b
Avoid double instantiation of SqlConnectionX instance. Add additional…
mdaigle Jun 12, 2024
38a0b45
merge feature branch
mdaigle Jun 12, 2024
7ff8100
Set _isExperimental. Add comment to describe the property.
mdaigle Jun 14, 2024
e6fc7fc
Accept connection overrides in Open and OpenAsync.
mdaigle Jun 14, 2024
fb36882
Merge branch 'feat/sqlclientx' of github.com:dotnet/SqlClient into sq…
mdaigle Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,15 +153,29 @@ public SqlRetryLogicBaseProvider RetryLogicProvider
[ResDescription(StringsHelper.ResourceNames.TCE_SqlConnection_TrustedColumnMasterKeyPaths)]
public static IDictionary<string, IList<string>> ColumnEncryptionTrustedMasterKeyPaths => _ColumnEncryptionTrustedMasterKeyPaths;

internal SqlConnection(string connectionString, bool useExperimental) : this()
{
_isExperimental = useExperimental;
if (_isExperimental)
{
_sqlConnectionX = new SqlClientX.SqlConnectionX(connectionString);
}
ConnectionString = connectionString; // setting connection string first so that ConnectionOption is available
}

/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/ctorConnectionString/*' />
public SqlConnection(string connectionString) : this()
public SqlConnection(string connectionString) : this(connectionString, false)
{
ConnectionString = connectionString; // setting connection string first so that ConnectionOption is available
}

/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/ctorConnectionStringCredential/*' />
public SqlConnection(string connectionString, SqlCredential credential) : this()
internal SqlConnection(string connectionString, SqlCredential credential, bool useExperimental) : this()
{
_isExperimental = useExperimental;
if (_isExperimental)
{
_sqlConnectionX = new SqlClientX.SqlConnectionX(connectionString, credential);
}

ConnectionString = connectionString;
if (credential != null)
{
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of setting useExperimental to false, have you thought about setting it via AppContext switch that can be configured by user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
}

private SqlConnection(SqlConnection connection)
{
GC.SuppressFinalize(this);
Expand Down Expand Up @@ -1077,7 +1096,7 @@ private void CheckAndThrowOnInvalidCombinationOfConnectionOptionAndAccessToken(S
throw ADP.InvalidMixedUsageOfCredentialAndAccessToken();
}

if(_accessTokenCallback != null)
if (_accessTokenCallback != null)
{
throw ADP.InvalidMixedUsageOfAccessTokenAndTokenCallback();
}
Expand All @@ -1099,7 +1118,7 @@ private void CheckAndThrowOnInvalidCombinationOfConnectionOptionAndAccessTokenCa
throw ADP.InvalidMixedUsageOfAccessTokenCallbackAndAuthentication();
}

if(_accessToken != null)
if (_accessToken != null)
{
throw ADP.InvalidMixedUsageOfAccessTokenAndTokenCallback();
}
Expand Down Expand Up @@ -1395,6 +1414,12 @@ private bool TryOpenWithRetry(TaskCompletionSource<DbConnectionInternal> retry,
/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/OpenWithOverrides/*' />
public void Open(SqlConnectionOverrides overrides)
{
if (_isExperimental)
{
_sqlConnectionX.Open();
return;
}

using (TryEventScope.Create("SqlConnection.Open | API | Correlation | Object Id {0}, Activity Id {1}", ObjectID, ActivityCorrelator.Current))
{
SqlClientEventSource.Log.TryCorrelationTraceEvent("SqlConnection.Open | API | Correlation | Object Id {0}, Activity Id {1}", ObjectID, ActivityCorrelator.Current);
Expand Down Expand Up @@ -1665,10 +1690,16 @@ private Task InternalOpenWithRetryAsync(CancellationToken cancellationToken)
=> RetryLogicProvider.ExecuteAsync(this, () => InternalOpenAsync(cancellationToken), cancellationToken);

/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/OpenAsync/*' />
public override Task OpenAsync(CancellationToken cancellationToken)
=> IsProviderRetriable ?
public override Task OpenAsync(CancellationToken cancellationToken) {
if (_isExperimental)
{
return _sqlConnectionX.OpenAsync(cancellationToken);
}

return IsProviderRetriable?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space before ?

InternalOpenWithRetryAsync(cancellationToken) :
InternalOpenAsync(cancellationToken);
}

private Task InternalOpenAsync(CancellationToken cancellationToken)
{
Expand Down Expand Up @@ -2416,6 +2447,8 @@ private void CopyFrom(SqlConnection connection)
ADP.CheckArgumentNull(connection, nameof(connection));
_userConnectionOptions = connection.UserConnectionOptions;
_poolGroup = connection.PoolGroup;
_sqlConnectionX = connection._sqlConnectionX;
_isExperimental = connection._isExperimental;

if (DbConnectionClosedNeverOpened.SingletonInstance == connection._innerConnection)
{
Expand Down
7 changes: 7 additions & 0 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Transactions;
using Microsoft.Data.Common;
using Microsoft.Data.ProviderBase;
using Microsoft.Data.SqlClientX;


namespace Microsoft.Data.SqlClient
Expand All @@ -21,6 +22,12 @@ public sealed partial class SqlConnection : DbConnection
private DbConnectionOptions _userConnectionOptions;
private DbConnectionPoolGroup _poolGroup;
private DbConnectionInternal _innerConnection;
/// <summary>
/// Indicates that this connection uses the experimental SqlClientX code path.
/// When true, operations pass through _sqlConnectionX.
/// </summary>
private bool _isExperimental;
private SqlConnectionX _sqlConnectionX;
private int _closeCount;

private static int _objectTypeCount; // EventSource Counter
Expand Down
16 changes: 15 additions & 1 deletion src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClientX/SqlConnectionX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading;
using System.Threading.Tasks;
using System.Transactions;
using Microsoft.Data.SqlClient;

namespace Microsoft.Data.SqlClientX
{
Expand All @@ -28,6 +29,14 @@ internal SqlConnectionX() : base()
{
}

internal SqlConnectionX(string connectionString) : this()
{
}

internal SqlConnectionX(string connectionString, SqlCredential credential): this()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: space before :

{
}

/// <inheritdoc/>
[Browsable(false)]
public override ConnectionState State
Expand Down Expand Up @@ -68,7 +77,6 @@ public override bool CanCreateBatch
protected override DbProviderFactory? DbProviderFactory
=> throw new NotImplementedException();


/// <inheritdoc/>
public override void ChangeDatabase(string databaseName)
=> throw new NotImplementedException();
Expand Down Expand Up @@ -123,10 +131,16 @@ public override Task<DataTable> GetSchemaAsync(string collectionName, string?[]

/// <inheritdoc/>
public override void Open()
=> Open(SqlConnectionOverrides.None);

public void Open(SqlConnectionOverrides overrides)
=> throw new NotImplementedException();

/// <inheritdoc/>
public override Task OpenAsync(CancellationToken cancellationToken)
=> OpenAsync(cancellationToken, SqlConnectionOverrides.None);

public Task OpenAsync(CancellationToken cancellationToken, SqlConnectionOverrides overrides)
=> throw new NotImplementedException();

/// <inheritdoc/>
Expand Down