Skip to content

Cleanup | SMI Cleanup (part 3) #3431

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.

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

Merged
merged 18 commits into from
Jun 23, 2025
Merged

Conversation

benrr101
Copy link
Contributor

@benrr101 benrr101 commented Jun 18, 2025

Description

This PR is a continuation of work done in parts 1 and 2 to clean up dead code in SMI. This time I took a slightly more heavy handed approach - if the class isn't being constructed anywhere, I deleted it. I then went through and removed any methods, properties, and variables that weren't being used in the Server folder, using IDE analysis of the files.

One of the biggest not-strictly-delete changes is to the methods in ITypedGetters/ITypedSetters. The methods in ITypedGetters/ITypedSetters did not take an EventSink in their arguments, but the interfaces themselves were not being used. The methods in ITypedGettersV3 and ITypedSettersV3 were being used around the codebase and take an EventSink in their arguments, but were never using the sink. As such, I deleted the ITypedGetters/ITypedSetters, and modified the ITypedGettersV3/ITypedSettersV3 to not take an EventSink in their arguments.

Issues

Believe it or not, this actually contributes to #1261 - two four of the classes wholesale deleted from the project were slated for merging!

  • SqlDataReaderSmi.cs
  • SqlInternalConnectionSmi
  • SqlSequentialStreamSmi
  • SqlSequentialTextReaderSmi

Testing

I attempted to run tests locally and identified a few that were failing due to reflection accessing methods that were removed. I suspect there will be more of these, but I was too impatient with the tests to wait for more to tests to complete.

@benrr101 benrr101 added this to the 7.0-preview1 milestone Jun 18, 2025
@Copilot Copilot AI review requested due to automatic review settings June 18, 2025 22:52
@benrr101 benrr101 added Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project labels Jun 18, 2025
@benrr101 benrr101 requested a review from a team as a code owner June 18, 2025 22:52
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 removes unused SMI/server-side code and obsolete interfaces, refactors typed getters/setters to drop the unused EventSink parameter, and cleans up related project file entries.

  • Deleted dead classes, methods, and netfx-specific SMI files no longer in use
  • Updated ITypedGettersV3/ITypedSettersV3 and callers to remove the EventSink argument
  • Removed legacy tests for private methods and streamlined compile includes in csproj

Reviewed Changes

Copilot reviewed 33 out of 37 changed files in this pull request and generated no comments.

Show a summary per file
File Description
SqlMetaDataTest.cs Removed reflection-based tests for GetPartialLengthMetaData
SmiEventSink_Default.cs Simplified message handling and removed parent-sink logic
SqlBulkCopy.cs Dropped _rowSourceIsSqlDataReaderSmi flag in streaming checks
ITypedGettersV3.cs, ITypedSettersV3.cs, SmiTypedGetterSetter.cs, TdsRecordBufferSetter.cs, TdsParameterSetter.cs, ValueUtilsSmi.cs, SqlUtil.cs, MetadataUtilsSmi.cs, etc. Refactored to remove SmiEventSink from signatures and cleanup obsolete code
*.csproj Removed compile references to deleted netfx SMI files
Comments suppressed due to low confidence (3)

src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlMetaDataTest.cs:613

  • These tests covered the behavior of the now-removed GetPartialLengthMetaData methods; consider adding new tests for the adjusted typed getters/setters or removing references to ensure coverage isn't inadvertently reduced.
        [Fact]

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiEventSink_Default.cs:20

  • The refactored HasMessages no longer considers the parent sink, so nested sinks may miss messages propagated from their parent; restore or extend this logic to include parent.HasMessages when _parent is set.
        internal bool HasMessages => _errors is not null || _warnings is not null;

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlBulkCopy.cs:1255

  • By removing the _rowSourceIsSqlDataReaderSmi check, streaming may now be enabled for SqlDataReaderSmi where it was previously disabled; reintroduce or replace this guard if streaming should remain unsupported for that reader type.
            else if (_enableStreaming && (metadata.length == MAX_LENGTH || metadata.metaType.SqlDbType == SqlDbTypeExtensions.Json))

Copy link
Contributor Author

@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.

Calling out some notable non-deletion changes

mdaigle
mdaigle previously approved these changes Jun 19, 2025
Copy link

codecov bot commented Jun 19, 2025

Codecov Report

Attention: Patch coverage is 76.43678% with 41 lines in your changes missing coverage. Please review.

Project coverage is 64.82%. Comparing base (12f742d) to head (e04b4e9).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...c/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs 74.10% 29 Missing ⚠️
...soft/Data/SqlClient/Server/SmiEventSink_Default.cs 16.66% 5 Missing ⚠️
...src/Microsoft/Data/SqlClient/TdsParameterSetter.cs 0.00% 3 Missing ⚠️
...Microsoft/Data/SqlClient/Server/SmiRecordBuffer.cs 0.00% 2 Missing ⚠️
.../Microsoft/Data/SqlClient/TdsRecordBufferSetter.cs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3431      +/-   ##
==========================================
+ Coverage   63.51%   64.82%   +1.31%     
==========================================
  Files         293      282      -11     
  Lines       63810    62307    -1503     
==========================================
- Hits        40526    40391     -135     
+ Misses      23284    21916    -1368     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 69.01% <76.07%> (+2.08%) ⬆️
netfx 66.30% <76.74%> (+0.08%) ⬆️

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.

Copy link
Contributor

@paulmedynski paulmedynski left a comment

Choose a reason for hiding this comment

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

One improvement suggested.

@paulmedynski paulmedynski dismissed their stale review June 23, 2025 17:11

Changing to Approved.

@benrr101 benrr101 merged commit b9b9153 into main Jun 23, 2025
251 checks passed
@benrr101 benrr101 deleted the dev/russellben/cleanup/smi-dead-code3 branch June 23, 2025 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Health 💊 Issues/PRs that are targeted to source code quality improvements. Common Project 🚮 Things that relate to the common project project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants