-
Notifications
You must be signed in to change notification settings - Fork 311
Implement SqlConnection.GetSchemaAsync #3005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbMetaDataFactory.cs
Outdated
Show resolved
Hide resolved
@@ -107,7 +109,7 @@ protected virtual void Dispose(bool disposing) | |||
} | |||
} | |||
|
|||
private DataTable ExecuteCommand(DataRow requestedCollectionRow, string[] restrictions, DbConnection connection) | |||
private async ValueTask<DataTable> ExecuteCommandAsync(DataRow requestedCollectionRow, string[] restrictions, DbConnection connection, bool isAsync, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice to be seeing proper async/await in SqlClient!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3005 +/- ##
==========================================
- Coverage 66.91% 63.00% -3.91%
==========================================
Files 280 273 -7
Lines 62386 61987 -399
==========================================
- Hits 41745 39058 -2687
- Misses 20641 22929 +2288
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This introduces async-over-sync, removing such requires work with the state machine and separate review.
@edwardneal - Can you resolve the conflicts, and then I will kick off another CI run? |
Thanks @paulmedynski, I've just brought the branch up to date. |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few inconsistencies and questions.
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlSchemaInfoTest/SqlSchemaInfoTest.cs
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Outdated
Show resolved
Hide resolved
Remove SqlDataReader.GetSchemaTableAsync (and references to such.) Expand test coverage of both GetSchema and GetSchemaAsync. Correct typo in documentation for GetSchema and GetSchemaAsync.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Contributes to #646, adding
SqlConnection.GetSchemaAsync
.I can't add the IDbColumnSchemaGenerator method because the .NET Framework version of SqlDataReader doesn't yet implement that interface. #2967 tracks this.
SqlDataReader.GetSchemaTableAsync
requires some extra work with the state machine to guarantee that the column metadata is available, and I'll submit a separate PR for this.I've added some basic documentation, but this effectively follows the
SqlCommand.[Something]Async
pattern of "An asynchronous version of [Something], which <etc.>"I've also moved the netfx GetSchema methods from SqlConnectionHelper.cs to SqlConnection.cs, to align that part of it with netcore. This contributes to the merge slightly.