Work towards merging TdsParser #2953

Activity

1. MichelZ

   added a commit that references this issue on Nov 2, 2024

   Align newlines between netfx and netcore dotnet#2953

   ce66c57
2. edwardneal commented on Nov 2, 2024

   

   edwardneal

   on Nov 2, 2024

   Contributor

   More actions

   Thanks for this @MichelZ - these bundles of changes look good to me.

   Another pattern of changes will also need to be made at some point, across TdsParser and other core parts of SqlClient: Constrained Execution Region support exists in netfx, but was removed in netcore. This is (roughly) a no-op, but each instance adds ~45 lines to the diff and it's usually inside methods which are otherwise identical. It's probably not necessary as part of this cleanup, but it'd be a good idea to port the CER support to netcore before starting to merge identical methods.

   On a wider point: I think it makes sense to use the TdsParser approach as a template for the core parts of SqlClient.

   👍React with 👍1Reacted by Michel Zehnder
3. MichelZ commented on Nov 2, 2024

   

   MichelZ

   on Nov 2, 2024

   ContributorAuthor

   More actions

   I think that's it for now with the more trivial stuff. Once these are merged I'll continue with some more "meaty" one's :)
   I do hope these are mostly straight forward to review and don't take up too much of anyone's time.

   🎉React with 🎉2Reacted by Erik Ejlskov Jensen and Edward Neal
4. benrr101

   added

   Common Project 🚮Things that relate to the common project projectThings that relate to the common project project

   on Nov 4, 2024
5. benrr101 commented on Nov 4, 2024

   

   benrr101

   on Nov 4, 2024

   Contributor

   More actions

   Thanks for taking a stab at merging this behemoth of a class 👍 I'll do my best to look at these PRs in a timely manner, but keep in mind we need two signoffs on them to get them merged. I'll also do my best to encourage my teammates to look at your PRs as well 😅

   That being said, I think for the smaller changes, it might be more beneficial to combine a couple together - eg, comments and comment typos could be rolled into a single comment PR. I've been trying to structure my PRs such that each commit is a complete item. So, eg, if you had a PR for comments, one commit could be getting the blocks aligned, the second could be to fix the typos. You don't need to go back recall the PRs that are currently out, but it might be beneficial going forward. It's a delicate balance between too much overhead per PR and too much to review in a single PR.

   My strategy for merging classes has been to create a "merge file" in the commit history of the PR. Basically indicate what blocks of code are from only one side of the merge. Then, merge work through each chunk of conflicts to either resolve as one or the other (eg, Int32 vs int) or leave with an #if NETFRAMEWORK sort of thing.
6. cheenamalhotra

   assigned

   benrr101

   on Nov 4, 2024
7. benrr101

   added a commit that references this issue on Nov 5, 2024

   Align newlines between netfx and netcore #2953 (#2954)

   69ac9c5
8. cheenamalhotra

   added a parent issue on Jan 21, 2025

   • Possible mergeable classes for merging code bases #1261
9. edwardneal commented on May 1, 2025

   

   edwardneal

   on May 1, 2025

   Contributor

   More actions

   At the moment, TdsParser has a diff of -550/+430. The tasks I'd identified to finish this merge are:

   Trivial

   • Sync tracing - netcore takes precedence
   • Sync exception messages
   • Sync code style in declaration of TdsOperationStatus variables
   • Sync order of TryProcessUDTMetadata and Write[X]FeatureRequest methods
   • Move TdsParser.Unix and TdsParser.Windows to shared directory, refactor netfx TdsParser to use the methods in TdsParser.Windows
   • netcore: mask server encryption option with OPTIONS_MASK, similar to netfx
   • netfx: rename SniError's sniError field to sniErrorNumber
   • netfx: don't write data to the login ack's program name - it's never referenced anywhere
   • netfx: port endianness handling in CopyCharsToBytes
   • bugfix: when performing a bulk copy of a JSON column in WriteBulkCopyMetaData, netfx will write the type information for an XML column
   • bugfix: when writing an XmlDataFeed, netfx needs to dispose of the ConstrainedTextWriter and XmlWriter objects it creates

   Functionality ports

   • LocalDB: allow named instances to be created during the connection process
   • Port TdsParserStateObjectFactory: Merge | TdsParserStateObjectFactory, TdsParserStateObject #3291
   • Where possible, port guid, integer, float, double, decimal, SqlGuid, SqlBinary parsing/writing from netcore to netfx
   • Port lambda expression allocation improvements from netcore to netfx

   Complex

   • Decide what to do with Transparent Network IP Resolution. netfx supports this, netcore doesn't
   • Port public interface of TdsParserStateObject
   • When processing ALTMETADATA, if a null/empty column name is supplied then netfx tries to synthesise it from the operand. Verify this - it might just be an artefact of SQL Server 2000, since netcore works without it
   • netfx throws an exception when trying to add a UDT parameter with a negative length, netcore does not. This needs a behaviour change (and cross-reference to the TDS specification to check for possible special meanings.)
   • When tracing is enabled, netfx will include a Trace Activity header with every SQL command. Decide if we want this functionality - and if we want to control it in this manner

   I'll work through these as I can, unless anyone else gets to them first.
10. benrr101

    closed this as completedin #3622on Sep 30, 2025
11. github-project-automation

    added this to SqlClient Boardon Oct 16, 2025
12. github-project-automation

    moved this to To triage in SqlClient Boardon Oct 16, 2025
13. cheenamalhotra

    moved this from To triage to Done in SqlClient Boardon Oct 16, 2025