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

Merge ContextConnection handling #2862

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

edwardneal
Copy link
Contributor

Relates to #2838, #1261.

This is partially to fix the documentation and the obsolescence message on SqlConnectionStringBuilder.ContextConnection. It goes a little further though: setting ContextConnection to true will now always throw an ArgumentException, and manually specifying it in the connection string will now throw an explicit error highlighting that Microsoft.Data.SqlClient doesn't support it.

Doing this also meant merging the handling of it between .NET Core and Framework. Once that was done, I've reintroduced a number of places where references to SqlConnectionString.ContextConnection had been removed from the .NET Core code. This is a prerequisite to merge SqlDataReaderSmi et al.

One point of note here is that SqlConnectionString.ContextConnection (line 763 of SqlConnectionString.cs) is hardcoded to return false. This is so that the .NET JIT can optimise away any evaluation - example here.

This changes the public API surface slightly: it introduces the SqlConnectionStringBuilder.ContextConnection property to .NET Core, and changes the message on its Obsolete attribute on both platforms.

Copy link

codecov bot commented Sep 15, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.84%. Comparing base (ca9272d) to head (3800bd9).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
...osoft/Data/SqlClient/SqlConnectionStringBuilder.cs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2862      +/-   ##
==========================================
- Coverage   72.22%   71.84%   -0.38%     
==========================================
  Files         297      294       -3     
  Lines       61091    60307     -784     
==========================================
- Hits        44120    43330     -790     
- Misses      16971    16977       +6     
Flag Coverage Δ
addons 92.90% <ø> (ø)
netcore 75.79% <75.00%> (-0.17%) ⬇️
netfx 70.37% <91.66%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DavoudEshtehari DavoudEshtehari added this to the 6.0-preview2 milestone Sep 16, 2024
@DavoudEshtehari DavoudEshtehari added the ➕ Code Health Issues/PRs that are targeted to source code quality improvements. label Sep 16, 2024
@cheenamalhotra
Copy link
Member

@edwardneal Please resolve conflicts so we can continue with reviews.
Thanks!

@edwardneal
Copy link
Contributor Author

edwardneal commented Oct 9, 2024

Thanks @cheenamalhotra - now resolved, pending CI.
Edit: just noticed a stray #if NET, re-resolved.

@cheenamalhotra cheenamalhotra added the 🆕 Public API Issues/PRs that introduce new APIs to the driver. label Oct 18, 2024
@cheenamalhotra
Copy link
Member

cheenamalhotra commented Oct 18, 2024

I would vote for removing the property from .NET Framework too instead of adding and making it obsolete in .NET Core.. This property was first marked obsolete in v3.0.

Does that sound acceptable @David-Engel ?

@cheenamalhotra cheenamalhotra removed this from the 6.0-preview2 milestone Oct 18, 2024
@edwardneal
Copy link
Contributor Author

Thanks @cheenamalhotra. Part of the reason I picked this approach is to make a specific statement that SqlClient doesn't support connecting to SQL Server's context connection, so if the documentation is updated to make that lack of support clear then I don't have any objections to removing the property from .NET Framework (besides the API compatibility concerns - but a v6.0 release is probably our last opportunity to make this kind of breaking change for a while.)

The SqlConnectionStringBuilder.ContextConnection property also backs a Context Connection key in the connection string. If we decide to remove the property, I'd prefer to keep treating this as a valid key and to throw a specific exception if its value parses to true, since it's a behavioural change from S.D.S.

@David-Engel
Copy link
Contributor

I would vote for removing the property from .NET Framework too instead of adding and making it obsolete in .NET Core.. This property was first marked obsolete in v3.0.

Does that sound acceptable @David-Engel ?

@saurabh500 - Do you think MDS will ever get into the CLR? If not, I agree we should just remove the ContextConnection property.

@saurabh500
Copy link
Contributor

@David-Engel @cheenamalhotra dont know about forever. 😀
But this hasn't been on the radar or been tested in the past 5 years.
I will vote for removal from netfx.

@cheenamalhotra
Copy link
Member

cheenamalhotra commented Oct 23, 2024

@edwardneal Could you please update the PR as per comments from David and Saurabh?

Thanks!

Only removing the public-facing references, and the references added by previous commits.
@edwardneal
Copy link
Contributor Author

Thanks all - I've just rolled back the related additions to .NET Core's codebase and removed the ContextConnection property from both codebases. There's one piece of new functionality: specifying Context Connection=true in the connection string will now throw a new exception which explicitly states that this behaviour isn't supported.

SQLCLR casts quite a long shadow here. I've handled the API surface in this PR, and will remove the various Smi classes and code paths once 6.0 is released. Once those are gone, I'll close my original issue 2838.

@cheenamalhotra cheenamalhotra added this to the 6.0-preview3 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ Code Health Issues/PRs that are targeted to source code quality improvements. 🆕 Public API Issues/PRs that introduce new APIs to the driver.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants