Skip to content

Conversation

@edwardneal
Copy link
Contributor

Description

This continues some of the earlier work from #3435, which replaced a layer of inheritance between DbConnectionFactory and SqlConnectionFactory, and #3773, which does the same between SqlInternalConnection and SqlInternalConnectionTds.

Here, we remove DbMetaDataFactory and fold its responsibilities into SqlMetaDataFactory. These are comparatively large files, so it's simplest to move commit-by-commit with whitespace changes hidden. I proceed in stages:

  1. Replace necessary references to DbMetaDataFactory with SqlMetaDataFactory.
  2. Identify a dead code path: the ability to supply a normalized (i.e. zero-padded) server version string. SqlClient never used this capability.
  3. Perform general code cleanup prior to the merge. This cleanup was a handful of fairly small points which are designed to bring the final file into alignment with the current code style.
  4. Move methods and constants. At this stage, methods are being copied and pasted from source to destination (plus some touch-ups of their scope between protected and private as appropriate.)
  5. Post-hoc touch-ups to remove the inheritance and unused methods.

Issues

No associated issues.

Testing

Automated tests continue to pass.

SqlConnectionFactory always passed the same value to both parameters
Convert the list of unsupported engines to a static variable.
Make the UDT/TVP processing methods static.
Change name of constants and ServerVersion parameter to fit naming conventions.
Introduce constant for EngineEdition query.
Reuse SqlCommand where possible.
Remove layer of indentation for using block.
Remove redundant client-side checks for null on fields which are declared as non-nullable within SQL Server.
Slightly simplify building of assembly qualified names.
Convert checks whether columns exist in a DataTable (and DataTables in a DataSet) to debug assertions, since these are statically built now.
Move constants which were only used in one method to that method.
Replace an unnecessary string[] allocation with a stack-allocated ReadOnlySpan<string>.
Where appropriate, use ?..
Replace == null and != null with pattern matching.
Convert checks whether columns exist in a DataTable (and DataTables in a DataSet) to debug assertions, since these are statically built now.
Replace use of GetSchemaTable with GetColumnSchema to reduce allocations.
Where appropriate, make methods private and/or static.
Remove unnecessary ADP.CompareInsensitiveInvariant helper.
Simplify IncludeThisColumn to make use of Contains or IndexOf where necessary.
Also implement IDisposable on SqlMetaDataFactory
@edwardneal edwardneal requested a review from a team as a code owner December 1, 2025 18:24
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.

Looks good!

@benrr101
Copy link
Contributor

benrr101 commented Dec 2, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@benrr101 benrr101 added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Dec 2, 2025
@benrr101 benrr101 added this to the 7.0.0-preview4 milestone Dec 2, 2025
@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 89.85240% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.20%. Comparing base (995df25) to head (0722b56).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...src/Microsoft/Data/SqlClient/SqlMetaDataFactory.cs 89.77% 55 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (995df25) and HEAD (0722b56). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (995df25) HEAD (0722b56)
addons 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3818      +/-   ##
==========================================
- Coverage   76.61%   69.20%   -7.41%     
==========================================
  Files         274      267       -7     
  Lines       43393    66325   +22932     
==========================================
+ Hits        33247    45903   +12656     
- Misses      10146    20422   +10276     
Flag Coverage Δ
addons ?
netcore 69.17% <89.81%> (-7.51%) ⬇️
netfx 68.57% <89.81%> (-7.63%) ⬇️

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.

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants