Cleanup | Consolidate connection capability metadata#3862
Open
edwardneal wants to merge 25 commits intodotnet:mainfrom
Open
Cleanup | Consolidate connection capability metadata#3862edwardneal wants to merge 25 commits intodotnet:mainfrom
edwardneal wants to merge 25 commits intodotnet:mainfrom
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
Contributor
Author
|
This PR is not stale. |
Contributor
|
@edwardneal - Can you resolve the conflicts? I'll kick a new PR run once that's done. |
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
github-actions
bot
removed
the
Stale
Contributor
|
@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! |
Contributor
Author
|
Thanks @paulmedynski. I've just merged main into this PR. I've just merged main into the other two cleanup PRs. |
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 parsingLOGINACKandFEATUREEXTACKtokens, 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
ProcessLoginAckmethod. This will look very different to the originalTryProcessLoginAckmethod 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 originalOnFeatureExtAckmethod in SqlConnectionInternal.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
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.