Merge | SqlCommand Prepare/Unprepare

[benrr101]

Description

This PR introduces the new merge style that I'm adopting for this class.

Merge Partials

Rather than try to do the merge as one big bang as I've done in the past, with simpler classes, this class needs some special attention. One particular issue that causes great headache is the disorder of the members of the class and the sheer size of the class. I would like to make improvements here while also making it clear what how things move from the separate projects to the common project. So, I've made the SqlCommand files in the separate projects partials, and added a partial to the common project. This allows me to move each member to the partial one at a time, give it access to what's in the separate project files, give the separate project files access to whats been merged, as well as sort the merged members and apply light cleanup in each merge.

Each commit has been tested to build - it is encouraged to step through this PR commit-by-commit

CER Helper

The netfx constrained execution region code has been an annoyance throughout the merge process. It's causing the worst of the merge conflicts in SqlCommand. I really really hate using #if blocks around { } blocks (ie, things that mess with indenting), and although I kinda get that the exceptions can be caught in netcore, I don't trust it. So, I've introduced something I've been playing with for a few months - helpers for CER. Basically, separate out the code that would be run inside the try of the CER, and replace the entire block with:

#if NET
DoTheThing();
#else
ConstrainedExecutionHelper.RunWithConnectionDoom(DoTheThing, _connectionToDoom);
#endif

Please let me know if I'm overlooking some obvious perf degradation (and potentially how to avoid it), or if the approach should be abandoned altogether.

Issues

Continuation of work for #1261?

Testing

Code still builds, CI should confirm

[Copilot]

Pull Request Overview

This PR implements a new merge strategy for the SqlCommand class by introducing partial classes and moving Prepare/Unprepare methods to a common shared file. It also introduces a ConstrainedExecutionHelper utility to simplify handling of constrained execution regions (CER) in .NET Framework.

• Introduces ConstrainedExecutionHelper to abstract CER handling between .NET Framework and .NET Core
• Moves SqlCommand Prepare/Unprepare functionality to a common partial class implementation
• Removes the stub SqlCommand class and establishes the partial class structure

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Utilities/ConstrainedExecutionHelper.netfx.cs	New helper class for CER handling in .NET Framework
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.stub.cs	Removes stub class entirely
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs	New common partial class with Prepare/Unprepare methods and shared fields
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlClientEventSource.cs	Adds TryCorrelationTraceEvent overload
src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs	Removes duplicated Prepare/Unprepare code, makes class partial
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs	Removes duplicated Prepare/Unprepare code, makes class partial
src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj	Updates project references for new file structure
src/Microsoft.Data.SqlClient/netcore/src/Microsoft.Data.SqlClient.csproj	Updates project references for new file structure

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.cs:247

• [nitpick] Using 'this.' prefix unnecessarily. The explicit 'this.' qualifier is redundant and should be removed for consistency with the rest of the codebase.

                this.Unprepare();

[codecov]

Codecov Report

Attention: Patch coverage is 80.41237% with 19 lines in your changes missing coverage. Please review.

Project coverage is 69.14%. Comparing base (9744a8e) to head (9bf1118).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ient/Utilities/ConstrainedExecutionHelper.netfx.cs	37.50%	10 Missing ⚠️
...lClient/src/Microsoft/Data/SqlClient/SqlCommand.cs	88.46%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3514      +/-   ##
==========================================
+ Coverage   67.81%   69.14%   +1.33%     
==========================================
  Files         276      278       +2     
  Lines       62417    62373      -44     
==========================================
+ Hits        42329    43130     +801     
+ Misses      20088    19243     -845     

Flag	Coverage Δ
addons	90.82% <ø> (ø)
netcore	72.94% <86.66%> (+0.04%)	⬆️
netfx	68.55% <79.34%> (+1.94%)	⬆️

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.

[edwardneal]

The CERs have definitely been frustrating - I've been working around the worst of their impact by asking people to ignore whitespace changes in specific merge PRs. The documentation for them would seem to rule out using delegates inside them though, so I don't think we can use a helper to simplify them.

[mdaigle]

The CERs have definitely been frustrating - I've been working around the worst of their impact by asking people to ignore whitespace changes in specific merge PRs. The documentation for them would seem to rule out using delegates inside them though, so I don't think we can use a helper to simplify them.

It looks like there's some functionality for preparing delegates for use as a CER. https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimehelpers.preparedelegate?view=netframework-4.8.1

[benrr101]

The CERs have definitely been frustrating - I've been working around the worst of their impact by asking people to ignore whitespace changes in specific merge PRs. The documentation for them would seem to rule out using delegates inside them though, so I don't think we can use a helper to simplify them.

I think this brings up some deeper questions about CER usage in our codebase in general - we've got loads of code that is doing unapproved things within CER blocks. Which is making me question whether we're doing anything right, and whether the CER blocks are useful at all. Currently discussing with @saurabh500

[mdaigle]

The CERs have definitely been frustrating - I've been working around the worst of their impact by asking people to ignore whitespace changes in specific merge PRs. The documentation for them would seem to rule out using delegates inside them though, so I don't think we can use a helper to simplify them.

I think this brings up some deeper questions about CER usage in our codebase in general - we've got loads of code that is doing unapproved things within CER blocks. Which is making me question whether we're doing anything right, and whether the CER blocks are useful at all. Currently discussing with @saurabh500

I was curious about the same thing! They feel marginally useful, but have they really been maintained and tested over time? They're probably not working the way they were originally intended to.

[mdaigle]

This all looks good to me. Any update on the CER investigation?

[mdaigle]

🫨

[benrr101]

uhhhhhhhhhhhhh ... the whole IsDirty thing is a complicated topic here and I need to untangle it as part of post merge cleanup. Specifically, the fact that there's different mechanism through which the command can become "dirty", and somehow whether or not the command is prepared. There's a lot of code in here that checks to IsDirty (which included IsPrepared) and IsPrepared in the same if statement. It's very confusing and the whole thing needs work to make it clearer what's actually going on. This was an attempt at indicating that it's the flag for when the user has done something to make the command dirty, and not the same as a flag indicating all scenarios when the command is dirty (which is the IsDirty property). The name could use some more work :)

[benrr101]

Marking as draft for now - if #3535 goes through, this will be replaced with version based on that PR