Visitar URL original
Merge | TdsParser.GetSniErrorDetails by edwardneal · Pull Request #3434 · dotnet/SqlClient · GitHub
Skip to content

Merge | TdsParser.GetSniErrorDetails - #3434

Merged
benrr101 merged 3 commits into
dotnet:mainfrom
edwardneal:merge/tdsparser-snierror
Jul 8, 2025
Merged

Merge | TdsParser.GetSniErrorDetails#3434
benrr101 merged 3 commits into
dotnet:mainfrom
edwardneal:merge/tdsparser-snierror

Conversation

@edwardneal

@edwardneal edwardneal commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

Description

This is a slightly smaller merge, but it's the first of its type.

TdsParser.Windows.cs and TdsParser.Unix.cs have a number of methods which vary in behaviour based upon TdsParserStateObjectFactory.UseManagedSNI. This PR handles those methods by moving the method to the TdsParserStateObject base class, implementing them in the Native and Managed child classes, then updating the references to use the state object.

Once this process is complete, we'll be able to remove the OS-specific TdsParser files.

Issues

Contributes to #1261.

Testing

Unit tests pass locally, but I'd like a CI run please.

@edwardneal
edwardneal requested a review from a team as a code owner June 23, 2025 06:50
@benrr101

benrr101 commented Jun 23, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jun 23, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

benrr101
benrr101 previously approved these changes Jun 23, 2025
View reviewed changes

@benrr101 benrr101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes perfect sense to me - nice that we will be able to drop these OS-specific files!

@benrr101
benrr101 requested a review from a team June 23, 2025 16:43
@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 23, 2025
@benrr101 benrr101 added this to the 7.0-preview1 milestone Jun 23, 2025
paulmedynski
paulmedynski requested changes Jun 25, 2025
View reviewed changes

@paulmedynski paulmedynski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One tiny question.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs Resolved
paulmedynski
paulmedynski previously approved these changes Jun 25, 2025
View reviewed changes
@paulmedynski

paulmedynski commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jun 25, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

samsharma2700
samsharma2700 previously approved these changes Jul 3, 2025
View reviewed changes

@samsharma2700 samsharma2700 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look great! just a few nits.

Comment thread ...Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs Resolved
Comment thread src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectNative.cs Resolved
@edwardneal
edwardneal dismissed stale reviews from samsharma2700 and paulmedynski via d643b06 July 3, 2025 21:06
@paulmedynski

paulmedynski commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

azure-pipelines Bot commented Jul 4, 2025

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@codecov

codecov Bot commented Jul 4, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 43.10345% with 33 lines in your changes missing coverage. Please review.

Project coverage is 59.63%. Comparing base (12f742d) to head (d643b06).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 18 Missing ⚠️
...nt/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs 0.00% 13 Missing ⚠️
...c/Microsoft/Data/SqlClient/TdsParserStateObject.cs 88.88% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3434      +/-   ##
==========================================
- Coverage   63.51%   59.63%   -3.88%     
==========================================
  Files         293      274      -19     
  Lines       63810    62012    -1798     
==========================================
- Hits        40526    36983    -3543     
- Misses      23284    25029    +1745
Flag Coverage Δ
addons ?
netcore 61.48% <47.36%> (-5.46%) ⬇️
netfx 63.52% <56.66%> (-2.70%) ⬇️

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.

@benrr101
benrr101 merged commit 9e5eb97 into dotnet:main Jul 8, 2025
@edwardneal
edwardneal deleted the merge/tdsparser-snierror branch July 8, 2025 21:00
@edwardneal edwardneal mentioned this pull request Jul 9, 2025
Merged
This was referenced Mar 17, 2026
Closed
Closed
Merged
This was referenced Nov 6, 2026
Open
Open
Closed
Merged
Open
Closed
Closed
Closed
Open
Merged
Closed
Merged
Open
Open
@MZurmuehlen89 MZurmuehlen89 mentioned this pull request Nov 10, 2026
Closed
This was referenced Nov 10, 2026
Open
Closed
Open
Closed
Merged
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benrr101 benrr101 benrr101 approved these changes
@paulmedynski paulmedynski paulmedynski approved these changes
+1 more reviewer
@samsharma2700 samsharma2700 samsharma2700 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

7.0-preview1

Development

Successfully merging this pull request may close these issues.

4 participants

@edwardneal @benrr101 @paulmedynski @samsharma2700