Skip to content

Conversation

edwardneal
Copy link
Contributor

Description

TdsParserStateObject currently comprises two target-specific files which have X differences:

  • TransparentNetworkIPResolution error handling - exists in netfx, not in netcore.
  • One debug assertion in ReadAsyncCallback which varies in a PacketHandle's type - present in netcore, not in netfx.
  • netfx will record the contents of outbound SNI packets (without their credentials) in the trace logging.

All of the other methods in TdsParserStateObject.netfx.cs and .netcore.cs are identical. I've thus merged them into the shared TdsParserStateObject.cs file in one PR; this does make it a little larger, but it's hopefully easy enough to review.

This also mops up the trailing TdsParser.SSPI.cs file, which only have a few methods but was large enough that I didn't want to deal with it as part of the large TdsParser.cs merge.

Finally, there were a handful of now-unnecessary .stub.cs files, which I've pruned.

Each commit moves one or two methods for ease of review.

Issues

Relates to #1261.

Testing

Automated tests pass, could someone run CI please?

@edwardneal edwardneal requested a review from a team as a code owner October 1, 2025 16:16
@benrr101
Copy link
Contributor

benrr101 commented Oct 3, 2025

Excited to get this one merged, but conflicts need addressing first :( (and they were probably caused by me anyhow...)

@benrr101 benrr101 added the Common Project 🚮 Things that relate to the common project project label Oct 3, 2025
@benrr101 benrr101 added this to the 7.0-preview2 milestone Oct 3, 2025
@edwardneal
Copy link
Contributor Author

Thanks - it looks like TdsParserStateObject had one instance of missing {}. I've merged and cleaned them up.

Copy link
Contributor

@mdaigle mdaigle left a comment

Choose a reason for hiding this comment

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

Getting so close!

@benrr101
Copy link
Contributor

benrr101 commented Oct 7, 2025

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

Looks good to me, thanks for this heavy lifting

@paulmedynski paulmedynski self-assigned this Oct 8, 2025
@paulmedynski
Copy link
Contributor

The CI errors appear to be transient, so I'll babysit this until it's green.

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.

4 participants