Cleanup | Consolidate connection capability metadata by edwardneal · Pull Request #3862 · dotnet/SqlClient

edwardneal · 2025-12-28T23:23:41Z

Description

This PR emerges in part from discussion with @paulmedynski on #3858. We noticed that the json datatype wouldn't appear when running SqlConnection.GetSchema("DataTypes") against an Azure SQL instance. This is because of an underlying design choice in SqlMetaDataFactory: it only uses the SQL Server version number to control whether specific queries are used and specific records are returned. This isn't compatible with Azure SQL (which always returns a version of 12.x.) To fix this, we need SqlMetaDataFactory to be able to make decisions based upon the server capabilities, whether determined by the version, the presence of a FEATUREEXTACK token, or the contents of one of those tokens' values.

In this PR, we introduce a new type, ConnectionCapabilities. This class takes on the responsibility of parsing LOGINACK and FEATUREEXTACK tokens, then converting them into an object which SqlConnectionInternal, SqlMetaDataFactory and TdsParser can interrogate to check for feature availability. For good measure, I then plumb this object through to SqlMetaDataFactory. A subsequent PR can work out the best way to implement filtering.

LOGINACK parsing is handled by the new object's ProcessLoginAck method. This will look very different to the original TryProcessLoginAck method in TdsParser; the latter had a great deal of legacy baggage from needing to deal with SQL Server 2000 compatibility, which we no longer need to carry.

FEATUREEXTACK parsing is handled by ProcessFeatureExtAck. I've kept this fairly close to the original OnFeatureExtAck method in SqlConnectionInternal.

Other points of errata:

Moving the capability/version interrogation into the new type means that we no longer need to keep the original SqlLoginAck instance as a field on SqlConnectionInternal, which means that it never leaves the scope it was created in. I've turned it into a readonly ref struct.
There are a few places where SqlClient's processing of FEATUREEXTACK doesn't align with the TDS specification. I've not touched these to preserve the original behaviour, and have added a note where applicable.
229a04d enables SqlClient to build under the Release configuration. This was necessary to get the benchmarking to work.
Benchmarking confirms that the CPU time, number of Gen0 GCs and memory usage remains identical (or very slightly better - slightly fewer Gen0 collections) between this branch and main.

This is a comparatively large PR, so I've tried to make it practical to move commit-by-commit.

Issues

Builds on a conversation in #3858.
Prerequisite to #3833.

Testing

I'm planning to make the coverage more explicit with unit tests for each relevant FEATUREEXTACK, but the existing test coverage should be fine.

We already have coverage (ConnectionTests.ConnectionTestDeniedVersion and ConnectionTestPermittedVersion) which specifically tests the LOGINACK component. Existing test functionality should prove the FEATUREEXTACK parsing - if there are issues parsing the relevant FEATUREEXTACK structures, the AlwaysEncrypted/vector/JSON/etc. tests will fail.

This class parses FEATUREEXTACK and LOGINACK streams, updating an object which specifies the capabilities of a connection.

This also means that we no longer need to hold the original SqlLoginAck record in memory.

This is adjacent cleanup which is best kept separate from the next step.

This includes the now-redundant checking (and associated exception) of the TDS version, and the assignment to _is20XX. Remove the now-redundant _is20XX fields.

We only ever use two of these versions, and they're based on TDS versions rather than SQL Server versions. Convert the Major/Minor/Increment bit-shifting to a constant value for clarity, and associate it with ConnectionCapabilities to eliminate duplication. Also add explanatory comment to describe reason for big-endian vs. little-endian reads.

Move UTF8 feature detection handling to ConnectionCapabilities.

This was always equal to !Capability.DnsCaching.

This enables the if condition from OnFeatureExtAck to continue to apply to capability processing. Also remove now-redundant comments, and clean up one reference to IsSQLDNSCachingSupported.

This is never persisted, and eliminates an allocation

One missing validation path. Also switched conditions to use pattern matching and to better align with original method for easier comparison and better codegen.

github-actions · 2026-02-12T03:01:01Z

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

edwardneal · 2026-02-12T16:58:57Z

This PR is not stale.

paulmedynski · 2026-02-12T18:02:18Z

@edwardneal - Can you resolve the conflicts? I'll kick a new PR run once that's done.

paulmedynski · 2026-02-12T19:38:57Z

/azp run

azure-pipelines · 2026-02-12T19:39:24Z

Azure Pipelines successfully started running 2 pipeline(s).

paulmedynski · 2026-02-17T12:27:04Z

@edwardneal - The PR pipeline issues on main have been fixed. Can you re-merge this and your owht Cleanup PRs? I'll get the pipelines started on all of them. Sorry!

edwardneal · 2026-02-17T18:27:40Z

Thanks @paulmedynski. I've just merged main into this PR. ~~I'll merge into the rest of the PRs over the next few days - if you're waiting on any in particular for GA then just let me know and I'll prioritise those.~~

I've just merged main into the other two cleanup PRs.

paulmedynski · 2026-02-18T18:20:25Z

/azp run

azure-pipelines · 2026-02-18T18:20:50Z

Azure Pipelines successfully started running 2 pipeline(s).

mdaigle

I'm ok to proceed with this, but something about it rubs me the wrong way, and I think it's that there's no way to know when the class is fully populated. We have two separate "process" methods that need to be called before you can read from the class's properties. If you do read before it's fully populated you'll get different values out. That said, the same critique applies to what we have now. I think this is a step in the right direction, but want to keep iterating on how to have strong guardrails during token processing so that it's hard to make mistakes.

Will take another look when the enhanced routing support is incorporated!

mdaigle · 2026-03-25T20:48:03Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ConnectionCapabilities.cs

+    /// Indicates support for user-defined CLR types (up to a length of 8000
+    /// bytes.) This was introduced in SQL Server 2005.
+    /// </summary>
+    public bool UserDefinedTypes => true;


Note to self: expecting to see these get cleaned up

mdaigle · 2026-03-26T17:47:01Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/ConnectionCapabilities.cs

+                JsonType = !featureData.IsEmpty && featureData[0] != 0x00
+                    && featureData[0] <= TdsEnums.MAX_SUPPORTED_JSON_VERSION;
+                break;
+


missing a case for enhanced routing

mdaigle · 2026-03-26T18:03:42Z

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs

-        // @TODO: This feature is *far* too big, and has the same issues as the above OnEnvChange
-        // @TODO: Consider individual callbacks for the supported features and perhaps an interface of feature callbacks. Or registering with the parser what features are handleable.
+        internal bool ShouldProcessFeatureExtAck(byte featureId) =>
+            RoutingInfo is null || featureId is TdsEnums.FEATUREEXT_SQLDNSCACHING;


needs updating for enhanced routing

This check needs to include enhanced routing as an "or" condition. We get envchange tokens before feature ext tokens, so if we don't have a special case for it we won't process the enhanced routing ack.

This condition is already in the latest version of the file (L1265.)

edwardneal · 2026-03-26T19:55:48Z

Thanks @mdaigle - I've moved the enhanced routing detection.

I think part of the issue here is that the LOGINACK and the FEATUREEXTACK tokens are treated independently in TdsParser. The logic to read these tokens from the wire is blended with the parsing and processing, and that flexibility has meant that anything downstream has to assume it can receive those tokens in any order; it never knows precisely when the item has finished processing, so we can't strictly guarantee that anything will be read-only.

If we could get to grips with the message sequencing, start to become more stringent about the order we expect to receive token streams from the server and start to factor that parsing out from TdsParser, I'd quite like to have some form of "capability builder" class which accepts a single LOGINACK token, any number of FEATUREEXTACK tokens and constructs a static ConnectionCapabilities instance for downstream call sites.

edwardneal added 23 commits December 27, 2025 15:04

Add ConnectionCapabilities class

a6646c0

This class parses FEATUREEXTACK and LOGINACK streams, updating an object which specifies the capabilities of a connection.

Hook LOGINACK handling

e37f3a2

This also means that we no longer need to hold the original SqlLoginAck record in memory.

Update reference to _is20XX to reference Capabilities property

de5c285

Errata: use BinaryPrimitives

65e701f

This is adjacent cleanup which is best kept separate from the next step.

Move handling of SqlLoginAck to ConnectionCapabilities

943c7db

This includes the now-redundant checking (and associated exception) of the TDS version, and the assignment to _is20XX. Remove the now-redundant _is20XX fields.

Hook FEATUREEXT handling

d68b90c

Move UTF8 feature detection handling to ConnectionCapabilities.

Move JSON feature detection handling

936218e

Move float32 vector feature detection handling

53f8947

Move Azure SQL feature detection handling

8ee0fb4

Add additional detection logic for Global Transactions

7713018

Move Global Transactions feature detection handling

0e1fcc5

Move data classification feature detection handling

ade2336

Move initial SQL DNS caching feature detection handling

ab9ebff

Remove unnecessary _cleanSQLDNSCaching member

abd69b7

This was always equal to !Capability.DnsCaching.

Move column encryption feature detection handling

50ccf47

Move logic to throw on unknown FEATUREEXT tokens into TdsParser

24caa2a

Refactor parsing condition into ShouldProcessFeatureExtAck

984ac2e

This enables the if condition from OnFeatureExtAck to continue to apply to capability processing. Also remove now-redundant comments, and clean up one reference to IsSQLDNSCachingSupported.

Maintain original server version behaviour

9380711

Performance: convert SqlLoginAck to a readonly ref struct

95ac4f5

This is never persisted, and eliminates an allocation

Enable Release mode build

229a04d

Plumb new ConnectionCapabilities to SqlMetaDataFactory

25b01d4

Cross-check FEATUREEXT validation to original OnFeatureExtAck

6560f5f

One missing validation path. Also switched conditions to use pattern matching and to better align with original method for easier comparison and better codegen.

edwardneal requested a review from a team as a code owner December 28, 2025 23:23

github-project-automation bot added this to SqlClient Board Dec 28, 2025

github-project-automation bot moved this to To triage in SqlClient Board Dec 28, 2025

cheenamalhotra moved this from To triage to In review in SqlClient Board Jan 5, 2026

cheenamalhotra added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label Jan 13, 2026

github-actions bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 12, 2026

paulmedynski self-assigned this Feb 12, 2026

Merge main

7c86769

github-actions bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Feb 13, 2026

Merge branch 'main' into cleanup/unified-connection-capabilities

2402cd7

paulmedynski added this to the 7.1.0-preview1 milestone Mar 9, 2026

mdaigle requested changes Mar 26, 2026

View reviewed changes

github-project-automation bot moved this from In review to In progress in SqlClient Board Mar 26, 2026

edwardneal added 2 commits March 26, 2026 19:02

Merge main

f532f60

Move enhanced routing feature detection support

a67a8a8

edwardneal force-pushed the cleanup/unified-connection-capabilities branch from d3feb15 to a67a8a8 Compare March 26, 2026 19:26

Conversation

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

Uh oh!

Description

Issues

Testing

Uh oh!

github-actions bot commented Feb 12, 2026

Uh oh!

edwardneal commented Feb 12, 2026

Uh oh!

paulmedynski commented Feb 12, 2026

Uh oh!

paulmedynski commented Feb 12, 2026

Uh oh!

azure-pipelines bot commented Feb 12, 2026

Uh oh!

paulmedynski commented Feb 17, 2026

Uh oh!

edwardneal commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulmedynski commented Feb 18, 2026

Uh oh!

azure-pipelines bot commented Feb 18, 2026

Uh oh!

mdaigle left a comment • 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!

mdaigle Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

mdaigle Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

edwardneal Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

edwardneal commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

edwardneal commented Dec 28, 2025 •

edited

Loading

edwardneal commented Feb 17, 2026 •

edited

Loading

mdaigle left a comment •

edited

Loading