Skip to content

Commit 5f05e45

Browse files
edwardnealedwardneal
committed
Response to code review
Remove SqlDataReader.GetSchemaTableAsync (and references to such.) Expand test coverage of both GetSchema and GetSchemaAsync. Correct typo in documentation for GetSchema and GetSchemaAsync.
1 parent eb164d6 commit 5f05e45

File tree

6 files changed

+62
-76
lines changed

6 files changed

+62
-76
lines changed

‎doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1357,7 +1357,7 @@ For more information on working with events, see [Connection Events](https://lea
13571357
</FireInfoMessageEventOnUserErrors>
13581358
<GetSchema2>
13591359
<summary>
1360-
Returns schema information for the data source of this <see cref="T:Microsoft.Data.SqlClient.SqlConnection" />. For more information about scheme, see <see href="https://learn.microsoft.com/sql/connect/ado-net/sql-server-schema-collections">SQL Server Schema Collections</see>.
1360+
Returns schema information for the data source of this <see cref="T:Microsoft.Data.SqlClient.SqlConnection" />. For more information about schemas, see <see href="https://learn.microsoft.com/sql/connect/ado-net/sql-server-schema-collections">SQL Server Schema Collections</see>.
13611361
</summary>
13621362
<returns>
13631363
A <see cref="T:System.Data.DataTable" /> that contains schema information.
@@ -1368,7 +1368,7 @@ For more information on working with events, see [Connection Events](https://lea
13681368
The cancellation token.
13691369
</param>
13701370
<summary>
1371-
An asynchronous version of <see cref="M:Microsoft.Data.SqlClient.SqlConnection.GetSchema()" />, which returns schema information for the data source of this <see cref="T:Microsoft.Data.SqlClient.SqlConnection" />. For more information about scheme, see <see href="https://learn.microsoft.com/sql/connect/ado-net/sql-server-schema-collections">SQL Server Schema Collections</see>.
1371+
An asynchronous version of <see cref="M:Microsoft.Data.SqlClient.SqlConnection.GetSchema()" />, which returns schema information for the data source of this <see cref="T:Microsoft.Data.SqlClient.SqlConnection" />. For more information about schemas, see <see href="https://learn.microsoft.com/sql/connect/ado-net/sql-server-schema-collections">SQL Server Schema Collections</see>.
13721372
</summary>
13731373
<returns>
13741374
A task representing the asynchronous operation.

‎doc/snippets/Microsoft.Data.SqlClient/SqlDataReader.xml‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -794,26 +794,6 @@ The <xref:Microsoft.Data.SqlClient.SqlDataReader.GetSchemaTable%2A> method retur
794794
The <see cref="T:Microsoft.Data.SqlClient.SqlDataReader" /> is closed.
795795
</exception>
796796
</GetSchemaTable>
797-
<GetSchemaTableAsync>
798-
<param name="cancellationToken">
799-
The cancellation token.
800-
</param>
801-
<summary>
802-
An asynchronous version of <see cref="M:Microsoft.Data.SqlClient.SqlDataReader.GetSchemaTable()" />, which returns a <see cref="T:System.Data.DataTable" /> that describes the column metadata of the <see cref="T:Microsoft.Data.SqlClient.SqlDataReader" />.
803-
</summary>
804-
<returns>
805-
A task representing the asynchronous operation.
806-
</returns>
807-
<remarks>
808-
<para>
809-
For more information about asynchronous programming in the .NET Framework Data Provider for SQL Server, see <see href="https://learn.microsoft.com/sql/connect/ado-net/asynchronous-programming">Asynchronous Programming</see>.
810-
</para>
811-
</remarks>
812-
<exception cref="T:System.InvalidOperationException">
813-
The <see cref="T:Microsoft.Data.SqlClient.SqlDataReader" /> is closed.
814-
</exception>
815-
<seealso cref="M:Microsoft.Data.SqlClient.SqlDataReader.GetSchemaTable()" />
816-
</GetSchemaTableAsync>
817797
<GetSqlBinary>
818798
<param name="i">
819799
The zero-based column ordinal.

‎src/Microsoft.Data.SqlClient/netfx/ref/Microsoft.Data.SqlClient.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -876,14 +876,14 @@ public void EnlistDistributedTransaction(System.EnterpriseServices.ITransaction
876876
public override void EnlistTransaction(System.Transactions.Transaction transaction) { }
877877
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchema2/*'/>
878878
public override System.Data.DataTable GetSchema() { throw null; }
879-
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaAsync/*'/>
880-
public System.Threading.Tasks.Task<System.Data.DataTable> GetSchemaAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
881879
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaCollectionName/*'/>
882880
public override System.Data.DataTable GetSchema(string collectionName) { throw null; }
883-
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaCollectionNameAsync/*'/>
884-
public System.Threading.Tasks.Task<System.Data.DataTable> GetSchemaAsync(string collectionName, System.Threading.CancellationToken cancellationToken = default) { throw null; }
885881
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaCollectionNameRestrictionValues/*'/>
886882
public override System.Data.DataTable GetSchema(string collectionName, string[] restrictionValues) { throw null; }
883+
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaAsync/*'/>
884+
public System.Threading.Tasks.Task<System.Data.DataTable> GetSchemaAsync(System.Threading.CancellationToken cancellationToken = default) { throw null; }
885+
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaCollectionNameAsync/*'/>
886+
public System.Threading.Tasks.Task<System.Data.DataTable> GetSchemaAsync(string collectionName, System.Threading.CancellationToken cancellationToken = default) { throw null; }
887887
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/GetSchemaCollectionNameRestrictionValuesAsync/*'/>
888888
public System.Threading.Tasks.Task<System.Data.DataTable> GetSchemaAsync(string collectionName, string[] restrictionValues, System.Threading.CancellationToken cancellationToken = default) { throw null; }
889889
/// <include file='../../../../doc/snippets/Microsoft.Data.SqlClient/SqlConnection.xml' path='docs/members[@name="SqlConnection"]/Open/*'/>

‎src/Microsoft.Data.SqlClient/src/Microsoft/Data/ProviderBase/DbMetaDataFactory.cs‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -176,18 +176,25 @@ private async ValueTask<DataTable> ExecuteCommandAsync(DataRow requestedCollecti
176176
Locale = CultureInfo.InvariantCulture
177177
};
178178

179-
cancellationToken.ThrowIfCancellationRequested();
180-
DataTable schemaTable = isAsync ? await reader.GetSchemaTableAsync(cancellationToken).ConfigureAwait(false) : reader.GetSchemaTable();
179+
// We would ordinarily call reader.GetSchemaTableAsync, but this waits synchronously for the reader to receive its type metadata.
180+
// Instead, we invoke reader.ReadAsync outside of the while loop, which will implicitly ensure that the metadata is available.
181+
// ReadAsync/Read will throw an exception if necessary, so we can trust that the list of fields is available if the call returns.
182+
bool firstResultAvailable = isAsync ? await reader.ReadAsync(cancellationToken).ConfigureAwait(false) : reader.Read();
183+
DataTable schemaTable = reader.GetSchemaTable();
181184

182185
foreach (DataRow row in schemaTable.Rows)
183186
{
184-
resultTable.Columns.Add(row["ColumnName"] as string, (Type)row["DataType"] as Type);
187+
resultTable.Columns.Add((string)row["ColumnName"], (Type)row["DataType"]);
185188
}
186-
object[] values = new object[resultTable.Columns.Count];
187-
while (isAsync ? await reader.ReadAsync(cancellationToken).ConfigureAwait(false) : reader.Read())
189+
190+
if (firstResultAvailable)
188191
{
189-
reader.GetValues(values);
190-
resultTable.Rows.Add(values);
192+
object[] values = new object[resultTable.Columns.Count];
193+
do
194+
{
195+
reader.GetValues(values);
196+
resultTable.Rows.Add(values);
197+
} while (isAsync ? await reader.ReadAsync(cancellationToken).ConfigureAwait(false) : reader.Read());
191198
}
192199
}
193200
finally

‎src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs‎

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1638,31 +1638,6 @@ public override DataTable GetSchemaTable()
16381638
}
16391639
}
16401640

1641-
/// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlDataReader.xml' path='docs/members[@name="SqlDataReader"]/GetSchemaTableAsync/*' />
1642-
#if NETFRAMEWORK
1643-
internal Task<DataTable> GetSchemaTableAsync(CancellationToken cancellationToken = default)
1644-
#else
1645-
public override Task<DataTable> GetSchemaTableAsync(CancellationToken cancellationToken = default)
1646-
#endif
1647-
{
1648-
// This method wraps GetSchemaTable in a Task, introducing async-over-sync. It should not be publicly exposed until
1649-
// this has been removed and replaced with an async path to guarantee that metadata has been read. Its purpose meanwhile
1650-
// is to enable code sharing between netcore and netfx.
1651-
if (cancellationToken.IsCancellationRequested)
1652-
{
1653-
return Task.FromCanceled<DataTable>(cancellationToken);
1654-
}
1655-
1656-
try
1657-
{
1658-
return Task.FromResult(GetSchemaTable());
1659-
}
1660-
catch (Exception ex)
1661-
{
1662-
return Task.FromException<DataTable>(ex);
1663-
}
1664-
}
1665-
16661641
/// <include file='../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlDataReader.xml' path='docs/members[@name="SqlDataReader"]/GetBoolean/*' />
16671642
override public bool GetBoolean(int i)
16681643
{

‎src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/SqlSchemaInfoTest/SqlSchemaInfoTest.cs‎

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,23 +23,35 @@ public static void TestGetSchema(bool openTransaction)
2323
{
2424
using (SqlConnection conn = new SqlConnection(DataTestUtility.TCPConnectionString))
2525
{
26-
conn.Open();
27-
DataTable dataBases;
26+
SqlTransaction transaction = null;
2827

29-
if (openTransaction)
28+
conn.Open();
29+
try
3030
{
31-
using (SqlTransaction transaction = conn.BeginTransaction())
31+
if (openTransaction)
3232
{
33-
dataBases = conn.GetSchema("DATABASES");
33+
transaction = conn.BeginTransaction();
3434
}
35+
36+
DataTable dataBases = conn.GetSchema("DATABASES");
37+
Assert.True(dataBases.Rows.Count > 0, "At least one database is expected");
38+
39+
string firstDatabaseName = dataBases.Rows[0]["database_name"] as string;
40+
dataBases = conn.GetSchema("DATABASES", [firstDatabaseName]);
41+
42+
Assert.Equal(1, dataBases.Rows.Count);
43+
Assert.Equal(firstDatabaseName, dataBases.Rows[0]["database_name"] as string);
44+
45+
string nonexistentDatabaseName = DataTestUtility.GenerateRandomCharacters("NonExistentDatabase_");
46+
dataBases = conn.GetSchema("DATABASES", [nonexistentDatabaseName]);
47+
48+
Assert.Equal(0, dataBases.Rows.Count);
3549
}
36-
else
50+
finally
3751
{
38-
dataBases = conn.GetSchema("DATABASES");
52+
transaction?.Dispose();
3953
}
4054

41-
Assert.True(dataBases.Rows.Count > 0, "At least one database is expected");
42-
4355
DataTable metaDataCollections = conn.GetSchema(DbMetaDataCollectionNames.MetaDataCollections);
4456
Assert.True(metaDataCollections != null && metaDataCollections.Rows.Count > 0);
4557

@@ -64,23 +76,35 @@ public static async Task TestGetSchemaAsync(bool openTransaction)
6476
{
6577
using (SqlConnection conn = new SqlConnection(DataTestUtility.TCPConnectionString))
6678
{
67-
await conn.OpenAsync();
68-
DataTable dataBases;
79+
SqlTransaction transaction = null;
6980

70-
if (openTransaction)
81+
await conn.OpenAsync();
82+
try
7183
{
72-
using (SqlTransaction transaction = conn.BeginTransaction())
84+
if (openTransaction)
7385
{
74-
dataBases = await conn.GetSchemaAsync("DATABASES");
86+
transaction = conn.BeginTransaction();
7587
}
88+
89+
DataTable dataBases = await conn.GetSchemaAsync("DATABASES");
90+
Assert.True(dataBases.Rows.Count > 0, "At least one database is expected");
91+
92+
string firstDatabaseName = dataBases.Rows[0]["database_name"] as string;
93+
dataBases = await conn.GetSchemaAsync("DATABASES", [firstDatabaseName]);
94+
95+
Assert.Equal(1, dataBases.Rows.Count);
96+
Assert.Equal(firstDatabaseName, dataBases.Rows[0]["database_name"] as string);
97+
98+
string nonexistentDatabaseName = DataTestUtility.GenerateRandomCharacters("NonExistentDatabase_");
99+
dataBases = await conn.GetSchemaAsync("DATABASES", [nonexistentDatabaseName]);
100+
101+
Assert.Equal(0, dataBases.Rows.Count);
76102
}
77-
else
103+
finally
78104
{
79-
dataBases = await conn.GetSchemaAsync("DATABASES");
105+
transaction?.Dispose();
80106
}
81107

82-
Assert.True(dataBases.Rows.Count > 0, "At least one database is expected");
83-
84108
DataTable metaDataCollections = await conn.GetSchemaAsync(DbMetaDataCollectionNames.MetaDataCollections);
85109
Assert.True(metaDataCollections != null && metaDataCollections.Rows.Count > 0);
86110

0 commit comments

Comments
 (0)