Cleanup | Consolidate connection capability metadata

[edwardneal]

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.

[github-actions]

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.

[paulmedynski]

@edwardneal - New conflicts here.

[edwardneal]

Thanks @paulmedynski - just merged.

[mdaigle]

On second review, I don't think I'm comfortable with shifting so much parsing logic around without a long term plan or vision in place. I think the piece that clearly makes sense is a record to store and pass capabilities. Can we reduce the changes to just that?

[mdaigle]

Can this be pushed up to some level where it's clear it will succeed? Then we can scope down the param type.

[edwardneal]

I've pushed this up to DbConnectionInternal, returning null by default and matching the throwing/non-throwing behaviour on all derived classes.

[priyankatiwari08]

Behavior change worth confirming: this was previously if (_connHandler._cleanSQLDNSCaching). That field defaults to false and was only set to true in OnFeatureExtAck when the server sent SQLDNSCACHING with data[0] == 0. So in main, connections to servers that don't send SQLDNSCACHING at all (i.e., non-Azure) did not call DeleteDNSInfo. With if (!Capabilities.DnsCaching) the inverse is true — those connections will now hit DeleteDNSInfo on every login. Intentional?

[edwardneal]

Yes, correct - unintentional and removed. Thanks for pointing that out.

[priyankatiwari08]

Agree with @mdaigle's ask to reduce scope to just the capability record + SqlMetaDataFactory plumbing and defer the parsing relocation. Left one inline on TdsParser.cs about a DNS-caching cleanup behavior change in that relocated code — worth confirming before any of it lands.

[edwardneal]

Thanks both. I've pared this back, so ConnectionCapabilities is just a resettable POCO now. This exposed that the SqlLoginAck class was no longer necessary, so I've removed it.

[mdaigle]

Thanks, I will take another look on Monday.

[mdaigle]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[mdaigle]

Looking good. just feeling hesitant about the changes to server-side version calculation.

[mdaigle]

Can you take a close look at this and double check that this is safe? The masking behavior is making me think that there are a range of accepted server-side values.

[edwardneal]

I've re-checked. That first impression was part of the reason why I've left it in here post-simplification - the switch is a little odd.

majorMinor and increment are calculated as below in TdsParser (lines 4321-4322):

// Bits 0-15 and 24-31
majorMinor = [TdsVersion] & 0xFF00FFFF;

// Shift right by 16 bits and take lower 8 bits. Equivalent maths is below.
increment = ([TdsVersion] >> 16) & 0xFF;
increment = ([TdsVersion] & 0x00FF0000) >> 16;

There are four cases: one per combination of majorMinor. Each case has one valid increment value, and anything outside of these cases fails. They're present in TdsParser (lines 4332-4360.)

	SQL 2005	SQL 2008	SQL 2012 / TDS 7.x	TDS 8
majorMinor expression	(SQL2005_MAJOR << 24) OR SQL2005_RTM_MINOR	(SQL2008_MAJOR << 24) OR SQL2008_MINOR	(SQL2012_MAJOR << 24) OR SQL2012_MINOR	(TDS8_MAJOR << 24) OR TDS8_MINOR
Major version constant	0x72	0x73	0x74	0x08
Minor version constant	0x0002	0x0003	0x0004	0x0000
Resultant majorMinor	0x72_00_0002	0x73_00_0003	0x74_00_0004	0x08_00_0000
Acceptable increment	SQL2005_INCREMENT	SQL2008_INCREMENT	SQL2012_INCREMENT	TDS8_INCREMENT
Resultant increment	0x09	0x0B	0x00	0x00

Since both majorMinor and increment come from the same [TdsVersion] value, we can OR them together and perform the comparison in one step, resulting in four cases:

• SQL 2005: 0x72_09_0002 (SQL2005_VERSION)
• SQL 2008: 0x73_0B_0003 (SQL2008_VERSION)
• SQL 2012: 0x74_00_0004 (TDS7X_VERSION)
• TDS 8: 0x08_00_0000 (TDS80_VERSION)

This corresponds to the values specified in MS-TDS for the LOGINACK token stream for every value besides TDS 8 - Appendix A, <72>.

There's also prior art in the tediousjs version mapping here and in mssql-jdbc here.

This switch also contains the logic determining whether the remote server is SQL 2022 or newer / SQL 2012 or newer / SQL 2008 R2 or newer. I've moved this logic to ConnectionCapabilities, lines 54-69.

[mdaigle]

Makes sense, and the new code is much cleaner. Can we refactor this logic into a static method and add some unit testing to prove equivalency (copy old code into test and compare)? I think I need to see that to feel comfortable with this. Plus, it will give a good spot to verify version parsing if/when we add a new version in the future.

[edwardneal]

I've just shuffled the validation logic and added a unit test in AdapterUtilTest.

[codecov]

Codecov Report

❌ Patch coverage is 85.26316% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.55%. Comparing base (be95ca2) to head (0c41cf4).
⚠️ Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
...ata/SqlClient/Connection/ConnectionCapabilities.cs	88.37%	5 Missing ⚠️
...Data/SqlClient/Connection/SqlConnectionInternal.cs	58.33%	5 Missing ⚠️
...qlClient/src/Microsoft/Data/SqlClient/TdsParser.cs	93.75%	2 Missing ⚠️
.../Microsoft/Data/ProviderBase/DbConnectionClosed.cs	0.00%	1 Missing ⚠️
...icrosoft/Data/ProviderBase/DbConnectionInternal.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3862      +/-   ##
==========================================
+ Coverage   65.96%   69.55%   +3.59%     
==========================================
  Files         275      273       -2     
  Lines       42993    70290   +27297     
==========================================
+ Hits        28361    48892   +20531     
- Misses      14632    21398    +6766     

Flag	Coverage Δ
CI-SqlClient	?
PR-SqlClient-Project	69.55% <85.26%> (?)

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.

[mdaigle]

Looks good. Thanks!