DbDataReader.GetSchemaTable without a resultset should return empty table instead of null

@cheenamalhotra

While working on nullability annotation for System.Data.Common, I ran across some odd and inconsistent behavior when GetSchemaTable and GetColumnSchema are called and a resultset isn't present (e.g. a non-SELECT statement was executed, or NextResult was called and returned false).

Provider behavior

GetSchemaTable: SqlClient and Npgsql return null when there is no resultset, Sqlite returns an table (may be empty?), MySQL throws. No doc/spec info exists on this.
GetColumnSchema: SqlClient and Npgsql return an empty list of columns when there is no resultset. Sqlite returns a non-empty column list (?), MySQL throws. No doc/spec info exists on this.

Notes

Other reader metadata methods which require a resultset - GetName, GetDataTypeName, etc. - throw InvalidOperationException, so this method we have a behavior inconsistency.
There are some rare dynamic scenarios (especially with stored procedures) where a user legitimately cannot be expected to know in advance whether the reader has a resultset or not. User can check FieldCount == 0 to identify whether a resultset exists or not.

Options

We could make GetSchemaTable return a nullable DataTable, but that would make it harder to use for everyone in the 99% case.
We could make SqlClient and Npgsql to throw for this scenario, aligning with GetName and other metadata methods (minor breaking change). We would want to do this for GetColumnSchema as well to make sure they behave the same way. This would require a non-breaking change from MySqlConnector and possibly Sqlite.
We could make SqlClient and Npgsql return an empty DataTable (minor breaking change), aligning with GetColumnSchema. GetSchemaTable/GetColumnSchema are different from GetName/GetDataType since they can return an empty table/list, which clearly expresses the lack of a resultset.

I vote for option 3. It would allow the method to cleanly return a non-nullable DataTable

Test code

[Fact]
public virtual void GetSchemaTable_returns_null_when_no_resultset()
{
	using var connection = CreateOpenConnection();
	using var command = connection.CreateCommand();
	command.CommandText = "SELECT 1";
	using var reader = command.ExecuteReader();
	reader.NextResult();
	Assert.Null(reader.GetSchemaTable());
}

/cc @cheenamalhotra @David-Engel @saurabh500 @Wraith2 @bgrainger @bricelam @ajcvickers @AndriySvyryd

@ErikEJ

/cc @ErikEJ

@FransBouma

/cc @FransBouma

The current behaviour in MySqlConnector was due to mysql-net/AdoNetApiTest#28; see also mysql-net/MySqlConnector#678.

Forgot about that conversation :)

How would you feel about my suggestion of returning an empty DataTable/column collection for these methods? It wouldn't be a breaking change for you (as the methods currently throw), and it seems consistent with the idea of no resultset having "zero columns" (as expressed by FieldCount).

I vote for option 3, too. Seems cleaner, more logical, and easier to code against.

We could make SqlClient and Npgsql return an empty list

Do you mean "DataTable", not "list"?

How would you feel about my suggestion of returning an empty DataTable/column collection for these methods?

I have no objection; there's value in consistency and I doubt anyone is relying on the current behaviour (of throwing an exception).

Do you mean "DataTable", not "list"?

Yeah, thanks - corrected it (thinking about GetSchemaTable and GetColumnSchema at the same time).

@roji

I like the idea of an empty DataTable instead of a null. However I wonder if any code takes a decision based on the value of the schema object returned like don't show a UI grid if a null value is received. @roji has already mentioned that it is a breaking change.
I don't have data though. Putting a hypothetical situation here.
It might be worth following up with SMO.

SMO - SQL management objects library

If you execute queries without knowing whether they will return a resultset, you in general will do that with ExecuteReader(), and first examine the GetSchemaTable. If it's null, you can assume there wasn't a resultset returned, so the query likely was a DML query.
If there is a datatable with columns, you can assume there's a resultset.

But indeed the 'null' isn't reliable as there's no documentation that this should happen (in my code I assume it's null, but perhaps some obscure providers fail here). So I opt for option 3: the empty datatable is a good solution, as its emptiness (no columns nor rows) is a reliable indicator no resultset will be returned by the reader.

It would be a breaking change for my desktop facing designer system but it's a simple change to make. What's more important tho is: how to enforce all ADO.NET providers to do the right thing here. As there's code out there relying on this (has to be) for e.g. 'mysql's behavior', it won't be easy to persuade these provider writers to change their API too (as it enforces a breaking change onto their users they might not want to do that)

A consensus has been reached to return an empty table when there is no resultset - when we annotate System.Data.Common for nullability, we can annotate the returned DataTable as non-nullable. Related ADO provider issues:

Change DbDataReader.{GetSchemaTable,GetColumnSchema} to return empty results when there is no resultset SqlClient#417
GetSchemaTable/GetColumnSchema should return empty object mysql-net/MySqlConnector#744
Change DbDataReader.{GetSchemaTable,GetColumnSchema} to return empty results when there is no resultset npgsql/npgsql#2842
(Sqlite is already doing the right thing)
Test DbDataReader.{GetSchemaTable,GetColumnSchema} without a resultset mysql-net/AdoNetApiTest#87

@terrajobst

@terrajobst @stephentoub this discussion was the basis of runtime changes in ADO providers but not in System.Data itself (though it will influence how we null-annotate it). OK to leave the labels/milestone like this or should we remove from the milestone as no actual change happened in the runtime?

Annotate DbDataReader.GetSchemaTable as nullable

Everyone, unfortunately I have to propose to revert this change for 5.0. When we originally had the discussion above, I didn't sufficiently consider the wider effect of the breaking change... since then, I've done a lot of work on System.Data nullability, and have come to better understand the bar for breaking changes in System.Data and their overall impact. I apologize for the mess this is creating.

Some reasoning:

Since this is an ADO.NET change, we'd also need to change the behavior of System.Data.Odbc and System.Data.OleDb. These are old providers, and we have very little knowledge on who is using them or how. I'm now more familiar with the difficulty of pushing breaking changes in the BCL, and this is a potentially risky one.
There's also System.Data.SqlClient. I think it would be out of the question to introduce such a breaking change there, with the very high backwards compat bar.
If we indeed leave older providers with the old, null-returning behavior, then we've fragmented the behavior - newer providers would return an empty table, while older ones would return null. Anyone coding against ADO.NET would therefore have to handle both behaviors, which is definitely worse than having just one (regardless of what we think of it).
At the end of the day, the change simply doesn't seem to justify the issues and risks it creates - requiring users to check GetSchemaTable for null isn't ideal, but it isn't the end of the world either. Had I designed a new API today, I'd definitely have adopted the proposed new behavior, but changing it at this point is something entirely different.
Unfortunately, SqlClient 2.0.0 has changed its behavior (Change DbDataReader.{GetSchemaTable,GetColumnSchema} to return empty results when there is no resultset SqlClient#417), as well as MySqlConnector (GetSchemaTable/GetColumnSchema should return empty object mysql-net/MySqlConnector#744). I'm hoping that since these are relatively recent changes, reverting the behavior back would impact few users (the lack of any feedback after introducing the original break in these two providers may support this). Also, considering the age of ADO.NET and old applications/libraries relying on it, in the long run re-adopting the previous behavior may spare you and your users more headaches than it saves.

Once again, apologies for this.

PS The PR making GetSchemaTable nullable in System.Data (and Odbc/OleDb) is #41082.

To confirm: the desired new behaviour is that when there is no result set, GetSchemaTable will return null (and GetColumnSchema will continue returning an empty collection)?

Since MySqlConnector changed from throwing an exception to returning an empty DataTable, I don't think there would be many users relying on the new return value, and changing it to null shouldn't have a significant impact.

To confirm: the desired new behaviour is that when there is no result set, GetSchemaTable will return null (and GetColumnSchema will continue returning an empty collection)?

Yeah, that's correct. This includes the case where NextResult has been called on the reader, and it returned false (i.e. consumed all result sets). You should see this behavior with System.Data.SqlClient, and with Microsoft.Data.SqlClient prior to 2.0.0; if you're seeing anything different please let me know...

Since MySqlConnector changed from throwing an exception to returning an empty DataTable, I don't think there would be many users relying on the new return value, and changing it to null shouldn't have a significant impact.

That's good to hear.. I now understand the extent to which behavioral changes in the public-facing abstraction methods of ADO.NET should be done really, really carefully.

Or not at all? 😀

A certain number of "reallys" starts to be equivalent to "not at all" for sure ☹️

Annotate DbDataReader.GetSchemaTable as nullable (#41082)

Specify when DbDataReader.GetSchemaTable returns null

Navigation Menu

Repository navigation

DbDataReader.GetSchemaTable without a resultset should return empty table instead of null #509

Provider behavior

Notes

Options

Test code

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

DbDataReader.GetSchemaTable without a resultset should return empty table instead of null #509

Description

Provider behavior

Notes

Options

Test code

Activity

roji commented on Dec 4, 2019

roji commented on Dec 4, 2019

bgrainger commented on Dec 4, 2019

roji commented on Dec 4, 2019

David-Engel commented on Dec 4, 2019

bgrainger commented on Dec 4, 2019

roji commented on Dec 4, 2019

saurabh500 commented on Dec 4, 2019

saurabh500 commented on Dec 4, 2019

FransBouma commented on Dec 4, 2019

7 remaining items

roji commented on Mar 7, 2020

roji commented on Mar 7, 2020

roji commented on Aug 20, 2020

bgrainger commented on Aug 20, 2020

roji commented on Aug 20, 2020

ErikEJ commented on Aug 20, 2020

roji commented on Aug 20, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions