Merge | TdsParser.cs #3622

edwardneal · 2025-09-20T20:03:33Z

Description

This PR merges the TdsParser class in its entirety. It's currently waiting for the TdsParserStateObject interface to settle in #3603, but since this is one of the largest classes in the project I'm submitting it early so that we have a head start in reviewing it.

Prior to any changes, there was approximately a +/-200 line diff between the files. This PR currently closes that diff commit-by-commit, narrowing it to -20/+1. Once everyone's happy with #3603, I'll close that gap completely, then delete one of the now-identical files in the final commit.

I'd strongly recommend reviewing commit-by-commit, with whitespace changes disabled.

At present, this is marked as a draft. Once the files are merged, I'll mark it as ready for review.

@benrr101 and anyone else who wants to wade through the commits for an initial review.

Issues

Contributes to #1261.
Fixes #2953.

Testing

Automatic tests pass locally. I don't think there's any point running CI yet though.

netfx: remove empty try/finally structure netcore: add connection dooming

Also eliminates allocation of UnicodeEncoding

The exitContext parameter is ignored on netcore and applied on netfx

No need to call AsSpan, this conversion is implicit. No need to slice to length bytes, length is already defined as the length of unencryptedBytes.

Also enables use of cached _decimalBits in netfx, this wasn't present before

Remove Yoda condition from netcore. Remove comparison which is always true from netfx.

Only enabled on netfx for now

Also reduced indentation level in ProcessSNIError by swapping using block for using statement

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

edwardneal · 2025-09-23T17:21:29Z

I've seen that #3603 has been merged, and have been testing the git merge on a local branch. I think this needs a clean slate for the platform-specific TdsParser.cs files though - it's not workable otherwise. I'll keep waiting until #3605 has been merged, then proceed.

Incidentally, there's a stray TdsParser.cs file in $(CommonSourceRoot). This isn't displayed in Visual Studio right now, and that file will also need to be removed.

netfx-only features: * trigger debug assertion when client certificate bit is set. * hide passwords from TdsParserStateObject (when merged)

See line 6778 of change to netcore TdsParser in dotnet#3605

edwardneal · 2025-09-24T18:38:41Z

I'm opening this up for review, although I appreciate it'll almost certainly take a while.

The top-level PR's diff is appalling, I think the figure has been confused by the final hurdle of renaming the shared TdsParser.SSPI.cs to TdsParser.cs, then merging the existing files to TdsParser.cs. The only way to review this is going to be commit-by-commit. Could someone start a CI run please?

mdaigle · 2025-09-24T21:48:22Z

/azp run

azure-pipelines · 2025-09-24T21:48:42Z

Azure Pipelines successfully started running 2 pipeline(s).

benrr101

Only thing I'm blocking on is the _connHandler assignment line that should probably be removed. Otherwise, fantastic work, thank you so much for getting this worked out!

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

Add braces to TNIR handling, explicit parameter name for exitContext

mdaigle

Two small requests. Otherwise looks good 🥳

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

benrr101 · 2025-09-30T18:05:52Z

/azp run

azure-pipelines · 2025-09-30T18:06:14Z

Azure Pipelines successfully started running 2 pipeline(s).

codecov · 2025-09-30T22:34:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (37d8a3f) to head (0839219).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #3622       +/-   ##
==========================================
- Coverage   66.13%       0   -66.14%     
==========================================
  Files         276       0      -276     
  Lines       60765       0    -60765     
==========================================
- Hits        40184       0    -40184     
+ Misses      20581       0    -20581

Flag	Coverage Δ
addons	`?`
netcore	`?`
netfx	`?`

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 added 26 commits September 20, 2025 15:42

netfx, netcore: sync tracing

4689f8b

netfx, netcore: whitespace, comment and naming changes

19163f5

netfx, netcore: merge TNIR handling

69099ac

netfx: sync DNS caching

a5f114f

netfx, netcore: merge CER mopup

96ef625

netfx: remove empty try/finally structure netcore: add connection dooming

netfx, netcore: merge debug assertions

f2de25f

netfx: sync SNI-specific and endian-specific logic

83f112b

netcore: merge netfx synthesis of default column name from operand

b2e4fed

netfx: ensure disposal of ConstrainedTextWriter and XmlWriter

d937f90

Also eliminates allocation of UnicodeEncoding

netfx, netcore: sync handling of SqlError generation

1c03e7c

netfx: port dotnet#232

13f7a22

netfx: port TryRunSetupSpinWaitContinuation

894f8ca

netcore: sync _resetConnectionEvent.WaitOne

b73d240

The exitContext parameter is ignored on netcore and applied on netfx

netfx: port skipping read of fields from SqlLoginAck and SqlReturnValue

4eb8697

netcore: supporting infrastructure for CER cleanup

5c00879

netfx: sync GetCodePage

70a622f

netcore, netfx: sync SqlGuid handling

aeab751

netcore, netfx: sync SqlBinary handling

b2cb0fe

netcore: sync date/time handling

844ad1e

No need to call AsSpan, this conversion is implicit. No need to slice to length bytes, length is already defined as the length of unencryptedBytes.

netcore, netfx: sync decimal handling

3249a81

Also enables use of cached _decimalBits in netfx, this wasn't present before

netfx, netcore: sync minor cleanup of if conditions

1d1e925

Remove Yoda condition from netcore. Remove comparison which is always true from netfx.

netfx, netcore: sync creation of LocalDB instances

52dd551

Only enabled on netfx for now

netcore: run ApplyFeatureExData in a checked context

7880b5f

netcore, netfx: sync creation of a TryEventScope

143de69

Also reduced indentation level in ProcessSNIError by swapping using block for using statement

netfx: sync order of variable assignment

c9585a3

netcore: merge static constructor

5a21361

ErikEJ reviewed Sep 21, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs Show resolved Hide resolved

edwardneal added 2 commits September 24, 2025 19:01

Merge branch 'main' into merge/tdsparser-complete

ec02267

Merge conditional compilation of netfx-only features

e2d846b

netfx-only features: * trigger debug assertion when client certificate bit is set. * hide passwords from TdsParserStateObject (when merged)

edwardneal added 3 commits September 24, 2025 19:16

Port TryReadByteArray usage from netcore to netfx

af79e6b

See line 6778 of change to netcore TdsParser in dotnet#3605

Rename existing shared TdsParser.cs to TdsParser.SSPI.cs

69c81be

Final merge of TdsParser.cs and redirection of references

6bfb6ef

edwardneal marked this pull request as ready for review September 24, 2025 18:38

edwardneal requested a review from a team as a code owner September 24, 2025 18:38

edwardneal changed the title ~~[Draft] Merge | TdsParser~~ Merge | TdsParser.cs Sep 24, 2025

mdaigle self-assigned this Sep 24, 2025

paulmedynski self-assigned this Sep 25, 2025

benrr101 requested changes Sep 26, 2025

View reviewed changes

Code review response

0839219

Add braces to TNIR handling, explicit parameter name for exitContext

mdaigle requested changes Sep 26, 2025

View reviewed changes

mdaigle approved these changes Sep 26, 2025

View reviewed changes

benrr101 added the Common Project 🚮 Things that relate to the common project project label Sep 29, 2025

benrr101 added this to the 7.0-preview2 milestone Sep 29, 2025

benrr101 approved these changes Sep 30, 2025

View reviewed changes

src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs Outdated Show resolved Hide resolved

benrr101 merged commit 33a8d53 into dotnet:main Sep 30, 2025
236 checks passed

edwardneal deleted the merge/tdsparser-complete branch October 1, 2025 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merge | TdsParser.cs #3622

Merge | TdsParser.cs #3622

Uh oh!

edwardneal commented Sep 20, 2025

Uh oh!

Uh oh!

edwardneal commented Sep 23, 2025

Uh oh!

edwardneal commented Sep 24, 2025

Uh oh!

mdaigle commented Sep 24, 2025

Uh oh!

azure-pipelines bot commented Sep 24, 2025

Uh oh!

benrr101 left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 commented Sep 30, 2025

Uh oh!

azure-pipelines bot commented Sep 30, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 30, 2025

Uh oh!

Uh oh!

Merge | TdsParser.cs #3622

Merge | TdsParser.cs #3622

Uh oh!

Conversation

edwardneal commented Sep 20, 2025

Description

Issues

Testing

Uh oh!

Uh oh!

edwardneal commented Sep 23, 2025

Uh oh!

edwardneal commented Sep 24, 2025

Uh oh!

mdaigle commented Sep 24, 2025

Uh oh!

azure-pipelines bot commented Sep 24, 2025

Uh oh!

benrr101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdaigle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benrr101 commented Sep 30, 2025

Uh oh!

azure-pipelines bot commented Sep 30, 2025

Uh oh!

Uh oh!

codecov bot commented Sep 30, 2025

Codecov Report

Uh oh!

Uh oh!