Merge | TdsParser functional changes #3555

edwardneal · 2025-08-10T20:33:51Z

Description

This should cover all necessary functional changes to TdsParser. With these changes merged, the class as a whole should be ready to merge after TdsParserStateObject is dealt with.

Not all of these changes are functional - some are simply necessary to see the real ones clearly. Possibly impactful commits here are:

878fcd4: when TdsParser was writing ints and there was space in the packet buffer, WriteInt would write the bytes to a stackalloc'd Span<byte> and then copy into the buffer. I've eliminated the unnecessary intermediary span, and we now write directly into the buffer.
a8be2f1: netfx had more detailed error messages when the parsing of a TDS stream failed. This ports those messages over to netcore.
01b02db: netfx will currently enforce the validation of a server's SSL certificate if Trust Server Certificate is false and the connection's AccessToken or AccessTokenCallback is set, but netcore doesn't currently check the AccessTokenCallback property. This commit changes the behaviour of netcore to match netfx.

There's also one other commit (6d98357) which looks more impactful than it is. s_xmlMetadataSubstituteSequence and s_jsonMetadataSubstituteSequence are the same length, so there's no runtime impact here.

Issues

Relates to #1261.

Testing

Unit tests passed locally.

netcore: removed a simple WriteInt which did nothing besides call BinaryPrimitives. netcore: when there's space in the packet buffer, WriteInt will now write to it directly (rather than write to a stack-allocated span and copy.)

This removes the need for TdsParser.netcore.cs.

…s never referenced in the format string

Messages of the netfx exceptions are more detailed

Added mask to netcore logic. Also removed mask on _encryptionOption from netfx - this will never be outside the range of EncryptionOptions.OPTIONS_MASK.

… is set and certificate is not automatically trusted

This passed state around, but never passed enough state to remove the state machine. Sync with netcore.

…instances from WriteUnterminatedSqlValue

Wraith2 · 2025-08-11T21:43:26Z

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

@@ -10342,27 +10377,23 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
        // This is in its own method to avoid always allocating the lambda in TDSExecuteRPCParameter
        private void TDSExecuteRPCParameterSetupWriteCompletion(SqlCommand cmd, IList<_SqlRPC> rpcArray, int timeout, bool inSchema, SqlNotificationRequest notificationRequest, TdsParserStateObject stateObj, bool isCommandProc, bool sync, TaskCompletionSource<object> completion, int startRpc, int startParam, Task writeParamTask)
        {
-            AsyncHelper.ContinueTaskWithState(
+            AsyncHelper.ContinueTask(


Why don't we want the state parameter anymore?

I'd switched away from using a state parameter because it wasn't doing anything and it wasn't present in netcore. We pass this as the state, cast it back to a TdsParser, then don't use it anywhere in the lambda expression (which closes over another 11 variables); it's used once in the failure path (which closes over another variable, so can't currently be made static either.)

The halfway house doesn't really help anyone, but I'm happy enough to reintroduce the state parameter and use a tuple in both places if you'd prefer?

Ok, if it's not used downstream that's fine. The netore version is typically better than the netfx.

Wraith2 · 2025-08-11T22:17:56Z

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

@@ -11160,7 +11191,7 @@ internal void WriteBulkCopyMetaData(_SqlMetaDataSet metadataCollection, int coun
                            stateObj.WriteByte(md.scale);
                            break;
                        case SqlDbTypeExtensions.Json:
-                            stateObj.WriteByteArray(s_jsonMetadataSubstituteSequence, s_xmlMetadataSubstituteSequence.Length, 0);
+                            stateObj.WriteByteArray(s_jsonMetadataSubstituteSequence, s_jsonMetadataSubstituteSequence.Length, 0);


Good catch.

Is there any test coverage for this?

I'd expect it to be implicitly covered by the SqlBulkCopy tests which handle JSON data. The variables in question are on lines 137-141 (link), and we can verify that they're the same length directly.

Wraith2 · 2025-08-11T22:19:49Z

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

+                        WriteGuid((Guid)value, stateObj);
+                        Debug.Assert(length == 16, "Invalid length for guid type in com+ object");


You should probably invert these lines. Generally asserts come before the work they are preconditions for.

paulmedynski · 2025-08-27T14:44:56Z

/azp run

azure-pipelines · 2025-08-27T14:45:19Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski

Looks good, with a few questions/suggestions.

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

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

Remove unnecessary conditional compilation. Add XML documentation to clarify SerializeShort, WriteShort et al.

paulmedynski · 2025-08-28T11:33:01Z

/azp run

azure-pipelines · 2025-08-28T11:33:23Z

Azure Pipelines successfully started running 2 pipeline(s).

mdaigle · 2025-08-28T21:01:54Z

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

+        //
+        // Writes a guid, either to a specific buffer or to the wire.
+        //
+        private static void SerializeGuid(in Guid v, Span<byte> buffer)


Would like to see some unit tests for these to ensure they behave the same between netfx and netcore.

The methods which write/serialize the numeric types are covered by the existing tests, but I've found and widened ParametersTest.Test_WithGuidValue_ShouldReturnGuid to handle SqlGuids (with and without values) to ensure these are covered.

mdaigle · 2025-08-28T22:11:26Z

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

@@ -13841,8 +13841,7 @@ internal string TraceString()
                            _statistics == null ? bool.TrueString : bool.FalseString,
                            _statisticsIsInTransaction ? bool.TrueString : bool.FalseString,
                            _fPreserveTransaction ? bool.TrueString : bool.FalseString,
-                            _connHandler == null ? "(null)" : _connHandler.ConnectionOptions.MultiSubnetFailover.ToString((IFormatProvider)null),
-                            _connHandler == null ? "(null)" : _connHandler.ConnectionOptions.TransparentNetworkIPResolution.ToString((IFormatProvider)null));


Does this get reintroduced under an #if NETFRAMEWORK somewhere? This option is still valid for netfx

The option's still valid, but the format string doesn't actually include an identifier referencing this argument. We could either remove the unused argument (current option) or change the format string so that it uses the argument - I've got no strong preference either way.

Let's add it to the format string. The trace message is misleading as-is because the parallel connectivity behavior can still be enabled (via TNIR) even if multisubnetfailover is disabled.

Sounds good. I've changed the format string to clear up whether the setting is for MSF or TNIR, although netcore will just print a hardcoded false for the latter.

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

mdaigle · 2025-08-28T22:25:48Z

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

@@ -11160,7 +11191,7 @@ internal void WriteBulkCopyMetaData(_SqlMetaDataSet metadataCollection, int coun
                            stateObj.WriteByte(md.scale);
                            break;
                        case SqlDbTypeExtensions.Json:
-                            stateObj.WriteByteArray(s_jsonMetadataSubstituteSequence, s_xmlMetadataSubstituteSequence.Length, 0);
+                            stateObj.WriteByteArray(s_jsonMetadataSubstituteSequence, s_jsonMetadataSubstituteSequence.Length, 0);


Is there any test coverage for this?

Both platforms will now show the TransparentNetworkIPResolution status. This is always disabled on netcore.

edwardneal added 16 commits August 9, 2025 21:46

netcore, netfx: sync declarations of TdsOperationStatus variables

cd2211a

netcore: reorder TryProcessUDTMetadata

076f87b

netfx: reorder WriteTceFeatureRequest

9499343

netfx: reorder WriteAzureSQLSupportFeatureRequest

06b292d

netcore, netfx: merge WriteInt and SerializeInt

878fcd4

netcore: removed a simple WriteInt which did nothing besides call BinaryPrimitives. netcore: when there's space in the packet buffer, WriteInt will now write to it directly (rather than write to a stack-allocated span and copy.)

netcore: remove ConstructGuid method

c19a10d

netcore, netfx: refactor and sync serialization of guids

8806a93

netcore, netfx: refactor and sync serialization of floats and doubles

b9d396f

This removes the need for TdsParser.netcore.cs.

netfx: adjust TraceString - parameter was being passed to it which wa…

43e90dd

…s never referenced in the format string

netcore: sync exception messages with netfx

a8be2f1

Messages of the netfx exceptions are more detailed

netfx, netcore: centralise masking of received server options

8793af7

Added mask to netcore logic. Also removed mask on _encryptionOption from netfx - this will never be outside the range of EncryptionOptions.OPTIONS_MASK.

netcore: enforce server certificate validation if AccessTokenCallback…

01b02db

… is set and certificate is not automatically trusted

netfx: add static lambda to TdsExecuteSQLBatch

209663e

netfx: remove failed attempt at static lambda

693069f

This passed state around, but never passed enough state to remove the state machine. Sync with netcore.

netfx: sync reference to length of JSON metadata substitution sequence

6d98357

netfx: pre-PR correction: match previous behaviour when writing Guid …

1d3930a

…instances from WriteUnterminatedSqlValue

edwardneal requested a review from a team as a code owner August 10, 2025 20:33

Wraith2 reviewed Aug 11, 2025

View reviewed changes

Flip order of debug assertion

539b993

paulmedynski self-assigned this Aug 27, 2025

paulmedynski requested changes Aug 27, 2025

View reviewed changes

Next round of code review

ea7f728

Remove unnecessary conditional compilation. Add XML documentation to clarify SerializeShort, WriteShort et al.

edwardneal requested a review from paulmedynski August 28, 2025 06:19

paulmedynski mentioned this pull request Aug 28, 2025

Add select docs to TdsParser methods per #3555 #3584

Open

mdaigle self-assigned this Aug 28, 2025

paulmedynski previously approved these changes Aug 28, 2025

View reviewed changes

mdaigle reviewed Aug 28, 2025

View reviewed changes

mdaigle linked an issue Aug 29, 2025 that may be closed by this pull request

Inconsistent server certificate validation between .NET Core and .NET Framework #3586

Open

edwardneal added 3 commits August 29, 2025 23:12

netcore, netfx: sync TraceString

3694fdd

Both platforms will now show the TransparentNetworkIPResolution status. This is always disabled on netcore.

Expand ParametersTest to include SqlGuid (including null value)

b475b85

Merge main

e079e09

edwardneal dismissed paulmedynski’s stale review via e079e09 August 29, 2025 23:23

		WriteGuid((Guid)value, stateObj);
		Debug.Assert(length == 16, "Invalid length for guid type in com+ object");

Merge | TdsParser functional changes #3555

Are you sure you want to change the base?

Merge | TdsParser functional changes #3555

Conversation

edwardneal commented Aug 10, 2025

Description

Issues

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwardneal Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmedynski commented Aug 27, 2025

Uh oh!

azure-pipelines bot commented Aug 27, 2025

Uh oh!

paulmedynski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulmedynski commented Aug 28, 2025

Uh oh!

azure-pipelines bot commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edwardneal Aug 12, 2025 •

edited

Loading