Cleanup | Remove Dead SMI Code (Part 1) #3393
Conversation
…they are no longer being used
There was a problem hiding this comment.
Pull Request Overview
This PR removes dead code paths tied to the always-false ContextConnection flag and cleans up related SMI/server-side classes and connection logic.
- Deleted the
ContextConnectionproperty and all context-connection-specific branches - Refactored
SqlConnectionFactoryto simplify connection creation and pool options - Marked numerous obsolete SMI classes/methods (
SmiRequestExecutor,SmiEventStream,SmiContext, etc.) for removal
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SqlConnectionString.cs | Removed always-false ContextConnection property |
| SmiRequestExecutor.netfx.cs | Flagged unused methods with @TODO |
| SmiEventStream.netfx.cs | Added @TODO to confirm usage |
| SmiContext.netfx.cs | Added @TODO to identify unused abstractions |
| SqlInternalConnectionTds.cs | Removed obsolete ContextConnection assertion |
| SqlInternalConnectionSmi.cs | Marked constructors and methods for deletion |
| SqlDataReaderSmi.cs | Flagged unused constructor with @TODO |
| SqlConnectionFactory.cs | Simplified CreateConnection & pool options logic |
| SqlConnection.cs | Removed context-connection properties and checks |
| SqlCommand.cs | Eliminated context-connection SMI execution paths |
| SqlBulkCopy.cs | Removed context-connection guard in validation |
Comments suppressed due to low confidence (4)
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:40
- Refactoring of pool group options logic should be accompanied by unit tests to ensure pooling behavior remains correct across all edge cases.
override protected DbConnectionPoolGroupOptions CreateConnectionPoolGroupOptions(DbConnectionOptions connectionOptions)
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:1326
- [nitpick] The context-connection guard is marked for deletion; remove this unused block to streamline the method.
// @TODO: No longer used -- delete!
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:140
- [nitpick] Passing a null literal for the nullable
setEnlistValueparameter can be confusing; consider using a named boolean or introducing an explicit overload to make the intent clearer.
setEnlistValue: null); // Do not modify the enlist value
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlConnectionFactory.cs:143
- The updated constructor invocation uses
string.Emptyandnullfor the password parameters, whereas previously the credential was passed through. Verify that this change preserves the intended authentication behavior.
return new SqlInternalConnectionTds(
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiEventStream.netfx.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiContext.netfx.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiRequestExecutor.netfx.cs
Show resolved
Hide resolved
|
Great! As a rule of thumb, the context connection removal will eventually result in the removal of almost every file containing "Smi" in the name, and will leave a few of the netfx-specific files unused. It casts a very long shadow, and I think this PR handles the first 10-15%. Once this is merged I'll update #2996 which should handle a few more parts of the codebase. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3393 +/- ##
===========================================
- Coverage 92.58% 68.24% -24.34%
===========================================
Files 6 300 +294
Lines 310 65412 +65102
===========================================
+ Hits 287 44639 +44352
- Misses 23 20773 +20750
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
If I did all of this work in a single PR, I think it would be far too difficult to review. And to be honest, I'm not entirely sure how far down this rabbit hole goes. So, let's start with this and go from there.
While I was working on merging the SMI classes from netfx, I discovered that there was a lot of code that was marked "obsolete" or wasn't being used. This was generally the Context Connection. While I'm not entirely sure what it was supposed to do, there were large swaths of code that would change behavior if the connection was a context connection. However, it all hinged on a property in SqlConnectionString being true - but the property always returns false. So, once I delete that property, large chunks of code become broken. Going through those chunks of code, I see that entire methods and classes can be safely removed since they are no longer being used or doing anything functional.
This PR is structured to follow this thread. Each commit is self-contained and self-explanatory. I delete a chunk of code and anything that becomes unused, I mark with a
@TODO. The subsequent commit deletes the code marked with a@TODO. The PR stops when the next batch of deletions start breaking the build.Issues
N/A
Testing
CI testing should validate that behavior continues as expected. Though theoretically we're just deleting code paths that were never being exercised.