Cleanup | Consolidate connection capability metadata#3862
Conversation
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.
cheenamalhotra
added
the
Code Health 💊
|
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. |
github-actions
Bot
added
the
Stale
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
@priyankatiwari08 this is ready for another review |
mdaigle
added
the
Author attention needed
mdaigle
added
Author attention needed
mdaigle
removed
the
Author attention needed
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
…up/unified-connection-capabilities
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Great changes overall and a good step towards encapsulating concerns in this old spaghetti code! A few questions/comments for your consideration.
| /// then the connection is to SQL Server 2022 or newer. | ||
| /// </summary> | ||
| public bool Is2022OrNewer => | ||
| TdsVersion == TdsEnums.TDS80_VERSION; |
There was a problem hiding this comment.
Is it possible that we chose to negotiate TDS 7.4 when connecting to SqlServer 2022+ (i.e. for TLS version or session encryption reasons)?
There was a problem hiding this comment.
Absolutely. This is just a drop-in replacement for the existing _is2022 variable in TdsParser, which is itself used solely to make sure that TDS 8.0 doesn't break a few features which are gated upon SQL Server 2008 support (namely, UDTs and the XML data type.)
There was a problem hiding this comment.
Ahh I see - so _is2022 was also misleadingly named. It also really meant "We negotiated TDS 8.x". I guess all 3 of these properties suffers from confusing naming, but they retain the naming from the old TdsParser fields.
A connection to SQL 2025 using TDS 7.x will have Is2012OrNewer true and Is2022OrNewer false, which is making my head hurt 😄
Could you update the XML docs to be super clear that these are solely a reflection of the negoitated TDS version, and don't reflect the server version despite their names? Perhaps even consider renaming these to NegotiatedTds80 and similar.
There was a problem hiding this comment.
The field's logic was a little tangled. I've dug into a little detail though, and we never actually need to discriminate based upon having negotiated a TDS 8.0 connection - _is2022 was never necessary.
We need to care about the remote server running SQL Server 2012 or later, but SQL 2012 onwards always used the same pair of version fields: one for TDS 7.x, another for TDS 8.0.
With that in mind, I've removed the Is2022OrNewer property and changed Is2012OrNewer to match those semantics.
github-project-automation
Bot
moved this from In progress
to Waiting for customer
in SqlClient Board
Name is misleading, and this corrects the semantics of Is2012OrNewer
6 participants
Description
This PR emerges in part from discussion with @paulmedynski on #3858. We noticed that the
jsondatatype wouldn't appear when runningSqlConnection.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 hosting the various feature flags as 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.Other points of errata:
SqlLoginAckinstance 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.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
This is covered by existing tests.