Skip to content

Conversation

edwardneal
Copy link
Contributor

Description

This follows up #3394, merging a number of methods relating to packet handling and some final errata. This is split into multiple commits for ease of review.

I'm moving a few members from TdsParserStateObject to TdsParserStateObjectNative:

  • SetPacketData
  • CreateAndSetAttentionPacket
  • CheckConnection
  • EnableSSL
  • Handle

SetPacketData is the only truly standout method here. The netfx implementation was structured slightly differently to the netcore implementation, and it also cleared _outBuff after writing a SecureString to the packet. I've added this feature in dcd231e.

This is the final PR prior to the merge of TdsParserStateObject and TdsParserStateObjectNative.

Issues

Relates to #1261.

Testing

Automated tests pass. Can someone run CI please?

@edwardneal edwardneal requested a review from a team as a code owner September 5, 2025 21:07
@benrr101
Copy link
Contributor

benrr101 commented Sep 8, 2025

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link

codecov bot commented Sep 10, 2025

Codecov Report

❌ Patch coverage is 90.76923% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.32%. Comparing base (1765de5) to head (98aa20d).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...osoft/Data/SqlClient/TdsParserStateObject.netfx.cs 88.46% 3 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs 85.71% 1 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 1 Missing ⚠️
...osoft/Data/SqlClient/TdsParserStateObjectNative.cs 95.65% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (1765de5) and HEAD (98aa20d). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (1765de5) HEAD (98aa20d)
addons 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3603       +/-   ##
===========================================
- Coverage   69.61%   59.32%   -10.30%     
===========================================
  Files         276      270        -6     
  Lines       61714    61431      -283     
===========================================
- Hits        42964    36441     -6523     
- Misses      18750    24990     +6240     
Flag Coverage Δ
addons ?
netcore 61.99% <93.33%> (-11.31%) ⬇️
netfx 62.63% <90.19%> (-5.62%) ⬇️

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
Copy link
Contributor Author

I've brought this up-to-date following the TdsParser merge work, can someone run CI please?

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Sep 17, 2025
@benrr101 benrr101 added this to the 7.0-preview2 milestone Sep 17, 2025
@paulmedynski
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

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.

Generally speaking, looks good to me!

@edwardneal edwardneal mentioned this pull request Sep 20, 2025
@benrr101 benrr101 merged commit 10ccb20 into dotnet:main Sep 22, 2025
236 checks passed
@edwardneal edwardneal deleted the merge/tdsparserstateobject-packets-and-cleanup branch September 23, 2025 05:11
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.

3 participants