Skip to content

Remove SqlCommand code paths for context connections #2996

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Remove SqlCommand code paths for context connections #2996

wants to merge 10 commits into from

Conversation

edwardneal
Copy link
Contributor

Contributes to #2838. PR 2/7.

This doesn't need to be merged before 6.0. It removes the core SqlDataReaderSmi type, and the code paths in SqlCommand.ExecuteNonQuery and SqlCommand.ExecuteReader which reference it. There are also a handful of Stream and TextReader derivatives which are only used in those contexts.

One point of note is in SqlDataRecord. InOutOfProcHelper.InProc will always be false, so I've trimmed the dead code in the if condition. As a result, _recordContext will always be null, and null will always propagate to the context parameter in ValueUtilsSmi. I plan to clean this up in the next PR.

If this is too large to review, I can partially revert some of these removals.

@edwardneal edwardneal mentioned this pull request Nov 9, 2024
Closed
@codecov Codecov
Copy link

codecov bot commented Nov 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.

Project coverage is 59.60%. Comparing base (b8948f2) to head (43065ad).
Report is 45 commits behind head on main.

Files with missing lines Patch % Lines
...t/netfx/src/Microsoft/Data/SqlClient/SqlCommand.cs 61.53% 5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (b8948f2) and HEAD (43065ad). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (b8948f2) HEAD (43065ad)
addons 1 0
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2996      +/-   ##
==========================================
- Coverage   67.04%   59.60%   -7.45%     
==========================================
  Files         300      288      -12     
  Lines       65376    64484     -892     
==========================================
- Hits        43831    38433    -5398     
- Misses      21545    26051    +4506
Flag Coverage Δ
addons ?
netcore 62.95% <100.00%> (-9.25%) ⬇️
netfx 60.92% <70.58%> (-4.27%) ⬇️

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.

benrr101
benrr101 previously approved these changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

I am 10000% ok with these changes. My only complain its I'm kinda jealous I'm not the one getting to delete all this code 💣

@edwardneal
Copy link
Contributor Author

I've addressed the merge conflict on this, can someone re-run CI please?

@edwardneal
Copy link
Contributor Author

The new merge conflict is now resolved. I'd appreciate a CI run and review.

@David-Engel
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edwardneal edwardneal mentioned this pull request May 2, 2025
Merged
@edwardneal
Copy link
Contributor Author

I've dealt with the merge conflict - could this be run through CI and reviewed please?

@David-Engel
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@edwardneal edwardneal requested a review from a team as a code owner May 27, 2025 17:57
@mdaigle
Copy link
Contributor

mdaigle commented Jun 2, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@edwardneal edwardneal mentioned this pull request Jun 5, 2025
Merged
@benrr101
Copy link
Contributor

@edwardneal How's this one looking now that we've got most of the SMI code removed? Can we close it out or is there other stuff that we haven't covered yet?

@edwardneal
Copy link
Contributor Author

We're almost done. The only things remaining are the work submitted in #3438, the removal of SmiRecordBuffer and some updates to SqlDataRecord to remove the (now nearly identical) netcore and netfx versions.

I don't think we need this PR for that - once we're happy with #3438, I'll clean the two other points up separately.

@edwardneal edwardneal closed this Jun 25, 2025
@edwardneal edwardneal deleted the remove-contextconnection-sqlcommand-paths branch June 25, 2025 20:15
@edwardneal edwardneal mentioned this pull request Jun 26, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benrr101 benrr101 benrr101 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@edwardneal @David-Engel @mdaigle @benrr101