Cleanup | SqlConnectionFactory <- DbConnectionFactory

[benrr101]

Description

This is the first PR in a bit of a fun experiment where we see if we can seriously clean up the codebase. According to @saurabh500 the codebase we've inherited dates back to the days when it was part of System.Data that not only contained the SqlClient but OdbcClient and even OracleClient. This is why there's Db* classes buried in the codebase that are passed around instead of the Sql* classes. In the best cases, these base implementations are fully abstract or just provide fallback implementations that throw. In the worst cases, they create confusing loops of implementations. And regardless, we are constantly passing them around as Db* and doing a lot of unnecessary checks and casts to the Sql* classes. These seemingly redundant base classes have a lot of unnecessary code and make the codebase as a whole more confusing. If we can do away with them, it would be idea. So, as an experiment, I decided SqlConnectionFactory might be a good place to see if we can simply merge the Db* class into the Sql* class.

As with most of my other merge project PRs, this one can be digested commit by commit (although be forewarned these ones are a bit stranger than usual due to protection levels). Important callouts have been added via my own review of the commit. Keep in mind although this could easily turn into a gigantic PR that removes all Db* classes, I've tried my best to be surgical and only eliminate the DbConnectionFactory class.

Issues

N/A

Testing

I'm still having trouble running test projects locally, so I have attempted to run them, but I can't guarantee I caught all the issues. CI will hopefully catch the remainder.

[Copilot]

Pull Request Overview

This PR replaces the legacy DbConnectionFactory abstractions with a single SqlConnectionFactory and cleans up related code, while also adding a new Timer overload.

• Swaps all DbConnectionFactory references for SqlConnectionFactory and updates method signatures accordingly.
• Introduces a TimeSpan-based overload for AdapterUtil.UnsafeCreateTimer.
• Changes all SqlConnectionFactory.SingletonInstance usages to the new SqlConnectionFactory.Instance and removes now-unused compile links.

Reviewed Changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/WaitHandleDbConnectionPool.cs	Switched to SqlConnectionFactory and updated pooled-connection call
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/IDbConnectionPool.cs	Changed ConnectionFactory property to SqlConnectionFactory
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/DbConnectionPoolGroup.cs	Updated methods to accept SqlConnectionFactory
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ConnectionPool/ChannelDbConnectionPool.cs	Adjusted ConnectionFactory property type
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionInternal.cs	Modified open/close schema methods to use SqlConnectionFactory
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbConnectionClosed.cs	Updated overrides to accept SqlConnectionFactory
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs	Added TimeSpan overload for UnsafeCreateTimer
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs	Revised TryReplaceConnection override signature
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs	Replaced SingletonInstance with Instance
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnection.cs	Swapped all SingletonInstance calls for Instance
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj	Removed DbConnectionFactory compile include
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs	Revised TryReplaceConnection override signature
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnectionHelper.cs	Replaced SingletonInstance with Instance
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlConnection.cs	Swapped SingletonInstance usage for Instance
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj	Removed DbConnectionFactory compile include

Comments suppressed due to low confidence (2)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs:136

• The new TimeSpan-based overload should be covered by unit tests to verify correct timer creation and disposal behavior under different intervals.

        internal static Timer UnsafeCreateTimer(TimerCallback callback, object state, TimeSpan dueTime, TimeSpan period)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs:133

• [nitpick] Consider adding XML doc comments to both overloads to clarify the expected units for the int parameters and explain that the new overload accepts TimeSpans.

        internal static Timer UnsafeCreateTimer(TimerCallback callback, object state, int dueTime, int period) =>

[paulmedynski]

I love removing unnecessary polymorphism! Just a few comments.

[mdaigle]

ProviderFactory always calls through to the singleton

[benrr101]

Makes sense to remove - my guess is it was some kinda thing where everything comes from the connection factory, implying that there's more than one kind of connection factory.

[mdaigle]

I did find it useful to add stub implementations for DbConnectionFactory when writing unit tests for the connection pool. It's the best spot to stub opening a new connection. If this class becomes sealed, that pattern will break (can't even do inheritance and override). I'd consider adding an interface (It's just a bummer that this class has such an unnecessarily big API at the moment.)

[benrr101]

I agree. Singletons/static classes are always a kind of code smell - totally valid to have, but they should used as interfaces so they can be replaced in test scenarios (what I think of as a facade pattern).

I kinda don't want to try and implement an entire facade pattern as part of this change. I'll add a note to facade it. But what if I unseal the class for now?

[codecov]

Codecov Report

Attention: Patch coverage is 91.31737% with 29 lines in your changes missing coverage. Please review.

Project coverage is 64.73%. Comparing base (12f742d) to head (36ee48c).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/Microsoft/Data/SqlClient/SqlConnectionFactory.cs	91.24%	26 Missing ⚠️
.../Microsoft/Data/ProviderBase/DbConnectionClosed.cs	50.00%	2 Missing ⚠️
...core/src/Microsoft/Data/SqlClient/SqlConnection.cs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3435      +/-   ##
==========================================
+ Coverage   63.51%   64.73%   +1.22%     
==========================================
  Files         293      279      -14     
  Lines       63810    62398    -1412     
==========================================
- Hits        40526    40396     -130     
+ Misses      23284    22002    -1282     

Flag	Coverage Δ
addons	92.58% <ø> (ø)
netcore	68.96% <91.15%> (+2.02%)	⬆️
netfx	66.20% <91.38%> (-0.02%)	⬇️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.