Sqlclient: Change DbDataReader.{GetSchemaTable,GetColumnSchema} to return empty results when there is no resultset

[roji]

As discussed in https://github.com/dotnet/runtime/issues/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.

/cc @David-Engel @cheenamalhotra @ajcvickers @bricelam

[cheenamalhotra]

Thanks @roji

We will investigate and address soon!

[Wraith2]

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

[ErikEJ]

@Wraith2 Sorry, already on it. I assume 2.0 means "ok with breaking changes" ?

[Wraith2]

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]

I will await comments from the product team

[cheenamalhotra]

I assume 2.0 means "ok with breaking changes"

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]

Would you like to have an AppContext switch to enable the previous behaviour @cheenamalhotra or @roji

[roji]

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]

I agree with @roji

If there's no special request for AppContext, we're ready to take in changes.
@ajcvickers kindly confirm!

[ajcvickers]

@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]

I have looked in GitHub codebase, and seen many usages of GetSchemaTable where the user never expects it to return null.

[Wraith2]

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]

@Wraith2 I do not understand? Just now, nobody expects the return value to be null - so nothing will break for them.

[Wraith2]

Oh, negation. Ignore me, carry on.