Repository navigation
Change DbDataReader.{GetSchemaTable,GetColumnSchema} to return empty results when there is no resultset #417
Description
As discussed in dotnet/runtime#509 and recently offline. This would be a small breaking change (so ideally done in the upcoming 2.0.0), which would make these methods more consistent and more usable in the new null-annotated world.
Activity
cheenamalhotra commentedon Feb 10, 2020
Thanks @roji
We will investigate and address soon!
Wraith2 commentedon Feb 10, 2020
For compatibility we could put in an AppContext switch allowing people to revert to the previous behaviour if they need to, it isn't a high performance method so the overhead would be irrelevant.
If I want I can make these changes @cheenamalhotra
Return empty DataTable from GetSchemaTable.
ErikEJ commentedon Feb 10, 2020
@Wraith2 Sorry, already on it. I assume 2.0 means "ok with breaking changes" ?
Wraith2 commentedon Feb 10, 2020
No problem, all yours 😁
There are other breaking changes going into to 2.0 so yes as discussed in the original thread referenced above making the new default different should be ok, I just wondered if since we can give it a configuration switch it might be worth doing so.
ErikEJ commentedon Feb 10, 2020
I will await comments from the product team
cheenamalhotra commentedon Feb 10, 2020
We're accepting breaking changes for this release if the changes have been long awaited and there is no workaround, or make us in-alignment with Specs.
Basically we're open for improvements that make most sense! :)
Wraith2 commentedon Feb 11, 2020
Would you like to have an AppContext switch to enable the previous behaviour @cheenamalhotra or @roji
roji commentedon Feb 11, 2020
I'm not super knowledgeable about our practices, but AFAIK we tend to do that more for risky patches, rather than for intentional behavior changes such as this. Maybe @ajcvickers has an opinion?
cheenamalhotra commentedon Feb 12, 2020
I agree with @roji
If there's no special request for AppContext, we're ready to take in changes.
@ajcvickers kindly confirm!
ajcvickers commentedon Feb 12, 2020
@cheenamalhotra I don't think the AppContext switch would be right here. We expect the impact of this change to be very minimal, and if people do hit it then they should be able to easily change their code to work again. This is better than having customers set a switch if they hit this break because we would then have to support the switch and both behaviors going forward.
So, yeah, agreeing that we should just make the change.
ErikEJ commentedon Feb 13, 2020
I have looked in GitHub codebase, and seen many usages of GetSchemaTable where the user never expects it to return null.
Wraith2 commentedon Feb 13, 2020
As above I think the solution to that would be to change the default as agreed since it's a desired breaking change and then add an appcontext switch for people that want to put it back. They'll need to investigate and read the release notes to find the switch and in doing so will now understand that they may be doing something wrong. At that point if they're the author they can change their code and if they're a consumer then can either push back on the author or if absolutely required set the context switch in their own app.
ErikEJ commentedon Feb 13, 2020
@Wraith2 I do not understand? Just now, nobody expects the return value to be null - so nothing will break for them.
Wraith2 commentedon Feb 13, 2020
Oh, negation. Ignore me, carry on.