Skip to content

Conversation

benrr101
Copy link
Contributor

Description

In this installment of merging SqlCommand, the focus is on the ExecuteReader methods and supporting structures. As with previous PRs in this series, a new partial has been introduced for the Reader methods. Every commit is a bite-sized merge of a single method or so. This is probably the largest of the partials due to the many methods that make up execution of readers. It can definitely be simplified by using proper async primitives, and I have plans to do so. Please also take special look at this PR since all the other execute partials call into this partial.

The following methods were merged:

  • BeginExecuteReader
  • BeginExecuteReaderInternal
  • BeginExecuteReaderInternalReadStage
  • BuildExecute
  • BuildExecuteSql
  • BuildPrpeExec
  • BuildRPC
  • CleanupExecuteReaderAsync
  • CompleteAsyncExecuteReader - both arguments were made required to clear up usages of the method
  • EndExecuteReader
  • EndExecuteReaderAsync
  • EndExecuteReaderInternal
  • ExecuteDbDataReader
  • ExecuteDbDataReaderAsync
  • ExecuteReader
  • ExecuteReaderAsync
  • ExecuteReaderWithRetry
  • ExecuteReaderWithRetryAsync
  • InternalEndExecuteReader
  • InternalExecuteReaderAsync
  • InternalExecuteReaderWithRetryAsync
  • GenerateEnclavePackage
  • RunExecuteReader
  • RunExecuteReaderTds
  • RunExecuteReaderTdsSetupContinuation
  • RunExecuteReaderTdsSetupReconnectionContinuation - This was factored out of the netfx implementation to match the netcore implementation
  • RunExecuteReaderTdsWithTransparentParameterEncryption
  • SetCachedCommandExecuteReaderAsyncContext

And also the ExecuteReaderAsyncCallContext internal class

Issues

Continuation of work for #1261

Testing

Code still compiles, CI suite will validate

@benrr101 benrr101 added this to the 7.0-preview2 milestone Sep 29, 2025
@benrr101 benrr101 requested a review from a team as a code owner September 29, 2025 23:14
@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Sep 29, 2025
@Copilot Copilot AI review requested due to automatic review settings September 29, 2025 23:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR consolidates all SqlCommand ExecuteReader-related methods and functionality into a new shared partial class file (SqlCommand.Reader.cs). It removes duplicate implementations from both .NET Framework and .NET Core platform-specific files, eliminating code duplication and centralizing the ExecuteReader logic.

  • Merges 30+ ExecuteReader-related methods from platform-specific files into a shared implementation
  • Updates method signatures to make parameters required and clarify usage patterns
  • Adds the new shared file to both platform-specific project files

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
SqlCommand.Reader.cs New shared partial containing all ExecuteReader methods and supporting infrastructure
SqlCommand.NonQuery.cs Updates CompleteAsyncExecuteReader call to use new required parameter signature
SqlCommand.netfx.cs Removes duplicated ExecuteReader methods and classes, updates CompleteAsyncExecuteReader calls
SqlCommand.netcore.cs Removes duplicated ExecuteReader methods and classes, updates CompleteAsyncExecuteReader calls
Microsoft.Data.SqlClient.csproj (netfx) Adds reference to new shared SqlCommand.Reader.cs file
Microsoft.Data.SqlClient.csproj (netcore) Adds reference to new shared SqlCommand.Reader.cs file
Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs:1

  • The TODO comment expresses confusion about the logic. Since this is in a text command execution path, the comment is correct that CommandType should never be StoredProcedure here. Consider removing this parameter or clarifying the logic.
// Licensed to the .NET Foundation under one or more agreements.

paulmedynski
paulmedynski previously approved these changes Sep 30, 2025
@mdaigle mdaigle self-assigned this Sep 30, 2025
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 82.93217% with 156 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.16%. Comparing base (16e2cbe) to head (cb11f43).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
.../src/Microsoft/Data/SqlClient/SqlCommand.Reader.cs 82.92% 154 Missing ⚠️
...src/Microsoft/Data/SqlClient/SqlCommand.netcore.cs 80.00% 1 Missing ⚠️
...x/src/Microsoft/Data/SqlClient/SqlCommand.netfx.cs 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3644       +/-   ##
===========================================
- Coverage   90.82%   77.16%   -13.66%     
===========================================
  Files           6      274      +268     
  Lines         316    45659    +45343     
===========================================
+ Hits          287    35233    +34946     
- Misses         29    10426    +10397     
Flag Coverage Δ
addons 90.82% <ø> (ø)
netcore 77.10% <82.89%> (?)
netfx 76.40% <82.34%> (?)

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.

mdaigle
mdaigle previously approved these changes Oct 1, 2025
@paulmedynski paulmedynski self-assigned this Oct 3, 2025
@benrr101 benrr101 merged commit b555adf into main Oct 3, 2025
252 checks passed
@benrr101 benrr101 deleted the dev/russellben/merge/sqlcommand-nocer-reader branch October 3, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants