Runtime: Scan SqlClient for missing overrides and interface implementations

[karelz]

We got 2 reports on .NET Core 2.0 Preview 1 which point to a class of problems we missed:

• dotnet/runtime#21675 - Missing interface implementation System.Data.SqlClient.SqlParameter : IDbDataParameter (fixed in PR dotnet/corefx#19734) (reported by @NickCraver)
• dotnet/runtime#21696 - Missing override System.Data.SqlClient.SqlDataReader.GetSchemaTable (reported by @NickCraver)
• dotnet/runtime#21643 & dotnet/runtime#21653 - Missing override System.Data.SqlClient.SqlClientFactory.CreateDataAdapter (fixed in PR dotnet/corefx#19682 by @justinvp) (reported by @RickStrahl & @FransBouma)

I think we need to boost our compat tooling, identify all missing overrides and interface (re)implementations and review all instances.
We should probably think deeply if we are missing any other problem classes - we had a list of things we didn't scan for, so maybe we should review it carefully again? (I remember I asked about the overrides, but didn't dig deeper into it)

cc @danmosemsft @stephentoub @Petermarcu

[justinvp]

dotnet/runtime#21643 was another case of a missing override.

[danmosemsft]

Generally overrides are filtered out of API diffs as removing an override is an implementation issue, not a breaking change. Sometime (often?) the override is intentionally omitted, because previously it didn't do anything or is no longer necessary. In short, we rely on having good tests because API scans are too noisy. @weshaggard do you have anything to add to that?

[danmosemsft]

Also as I mentioned elsewhere, we know there are a lot of omissions from SqlClient. (https://github.com/dotnet/corefx/issues/17126). There was insufficient time to fill it out.

[karelz]

@danmosemsft can we review the missing overrides now? Based on 3 regressions and not-so-high test coverage, I think it is our best chance to retain high-quality ...

[saurabh500]

SqlConnection.GetSchema() is another API which is missing. I have opened https://github.com/dotnet/corefx/issues/19797

[NickCraver]

@danmosemsft

Also as I mentioned elsewhere, we know there are a lot of omissions from SqlClient.

I think the point is that these aren't in that list. There's another class here that's been missed, and they are regressions. I think most of them stem from DataTable being not-a-thing early on and them coming back, with some edges missed. They were added to the contract but effectively aren't there. To me, there's an obvious mismatch here.

IMO, these APIs should ship in netstandard2.0, but only if they're there. If we have nothing but a lot of runtime exceptions there, we're basically lying to the user about what came back in 2.0 and leaving them with a bad experience. If they can't come back, they should be removed from the contract...and I'd hate to see that happen as well, but it's more honest to the user about what's there.

I get that technically they're there, due to this always being the case on the base classes, and it's SQL that needs to override. But to the user, it's: "netstandard2.0 said this was there, it lied, it only goes boom".

What can we do to help?

[saurabh500]

@NickCraver DataTable/DataSet and friends are available in System.Data.Common which is a part of netstandard2.0. However System.Data.SqlClient has been removed from netstandard and is not a part of it. System.Data.SqlClient is now another ADO.Net DB provider which doesn't fall into the netstandard API surface area and is a netstandard2.0 compliant library

This decoupling allows SqlClient to be shipped independent of netstandard.
I agree that SqlClient should support the given APIs and should bring back the internal implementations related to DataTable etc. , however the support in SqlClient is not necessarily tied to the surface area of netstandard any more, unlike netstandard 1.6

[NickCraver]

@saurabh500 Well yes, but I'm saying how a user sees it. They upgrade to netstandard2.0, grab the netstandard2.0 SqlClient, and it breaks. They don't make the distinction of what is split into what or ships when like we do here.

I understand you, and you are totally correct, but the reality is what most users simply won't care. IMO, SqlClient should not ship a non-pre-release netstandard2.0 library until it actually has one users expect. That's the most honest and least confusing thing to do. Hopefully that aligns with the .NET Core 2 ship date overall, but I understand reality is what it is and there are many bits left to go...some of them being non-trivial looking (at least in my comparisons to NetFx source).

[danmosemsft]

@NickCraver I hear you.

For overrides, as you saw I appended a list into dotnet/SqlClient#17. Even that doesn't catch the IDbDataParameter case because which is a bit unusual because the members were present but just ignored because the base class implemented it explicitly. All these are technically API compatible so they don't show up in eg https://github.com/dotnet/corefx/blob/master/src/shims/ApiCompatBaseline.netcoreapp.netfx461.txt

The best way to drive this to completion is probably to fill out the obviously missing API then throw in the desktop tests to help shake out anything else like this. API diffing is necessary but not sufficient.

[FransBouma]

I think the whole 'sql client isn't part of .netstandard2.0' aspect is a complicated issue to sell to the average user and I agree with @NickCraver on this. I also see why SqlClient is not part of netstandard2.0, as other 3rd party ADO.NET providers aren't either (which have the same problem) yet it does create a confusing situation.

As DbProviderFactory and the common interfaces/types used by the objects it creates are in netstandard2.0, code utilizing these should work at all times, and if SqlClient misses implementations, these should be added, otherwise IMHO it's not compatible with netstandard2.0, at least not semantically (if a lib implements a method by throwing 'not supported' is it really compatible? :))

So objects created by SqlClient's DbProviderFactory implementation should implement all features of netstandard provided by the base classes / interfaces like they do on .NET full to avoid incompatibility. IMHO what the user wants is running ADO.NET targeting code on either .NET core 2.x+ or .NET full without a lot of (preferably none) conversion/migration at all. It should be transparent to the user what to do when .net core or .net full is targeted using an ADO.NET utilizing library: namely the same thing, and it should work OOTB.

Netstandard2.0 marks the start of many people porting their code over to .net core, including myself, and missing functionality expected to be there by our own users would seriously mitigate these porting efforts as it would be confusing to our users what will and will not work: they don't know what our libraries do with SqlClient, that's abstracted away from them, they expect things to work as-is on .net full and .net core if the library using SqlClient says 'we support feature X'.

[FransBouma]

Found another one (unless I overlooked it): SqlConnection.DbProviderFactory isn't overridden. This is currently not that much of a deal, unless someone brings back DbProviderFactories, as the desktop implementation of DbProviderFactories contains a GetFactory(DbConnection) method which uses the internal DbConnection.ProviderFactory property, which reads the virtual DbProviderFactory property on the connection. Will file an issue.

(edit) filed 2 issues: dotnet/corefx#19825 (SqlConnection.DbProviderFactory) and dotnet/corefx#19826 (DbConnection.ProviderFactory, as it's not present and DbProviderFactories needs it so porting that code over will be easy).

[danmosemsft]

@FransBouma I fully agree. We're meeting in person this morning to see what can be done in the time remaining for 2.0.

[danmosemsft]

SqlConnection.DbProviderFactory isn't overridden.

Interesting. This wasn't in the list I posted of course. As far as I can tell, our API diff tools are semi arbitrary about which overrides they show -- because they are designed for discovering API-breaking changes only.

All this reinforces the point that the issue here is that the code is incomplete -- I'm not sure that missing overrides are special, they just happen to be what we're discovering here.

[danmosemsft]

Update, we met today and @saurabh500 is going to continue a code audit of SqlClient focusing on overrides. We have some other potential avenues of attack (porting samples, and running desktop SqlClient tests) that aren't scheduled. We're going to discuss status on Thursday. Our priorities are dotnet/corefx#1 do not break upgraders from Core, dotnet/corefx#2 code should just work against new System.Data API. Time remaining to finish 2.0 is quite limited, so increasing the surface area of SqlClient to match desktop is not currently feasible.

[danmosemsft]

Marking up for grabs since others are welcome to contribute by testing or looking at the code, if anyone is interested. I think porting code from desktop to try it would be particularly welcome.

[FransBouma]

I've some time this week, as I'm in a post-release lull at the moment, and ahead of my big port-to-corefx work this summer I think it's a good thing to familiarize myself with the codebase, so I'll try to submit fixes for the issues I reported today. If that goes well, I'll try to bring back DbProviderFactories as well, so porting code over utilizing that will be easier. Fingers crossed :)

[danmosemsft]

@FransBouma to give you an idea of schedule/context -- we fork off a 2.0 release branch from master potentially today. After that all changes that want to merge into 2.0 must go through our "shiproom" process (in person meeting where we examine the change, risk, and benefit). Shiproom will take minimal changes that fix key breaks such as those we're seeing in SqlClient, if the risk is acceptable. They won't take arbitrary changes. After about 2 weeks of this process we go into what we call "escrow" which is essentially the same but much more selective.

What this means is, please focus on helping find those key issues that shiproom would take fixes for. If you are inclined to do work beyond that, please keep it in separate commits in master, or possibly hold the PR for a while to avoid muddying the waters for such fixes we would take. We may not have time to code review larger changes right now anyway.

[danmosemsft]

@karelz and I agreed to limit this to jsut SqlClient. We don't have evidence that missing overrides are a widespread problem, and the scanning is noisy with lots of false positives and negatives. Made title more specific.

[RickStrahl]

Another related one I'm running into is that SqlDbType is missing. Looks like it's not coming over to .NET Core? Seems an odd thing to leave out. Easy enough to work around with DbType, but not all types are represented there.

[saurabh500]

Are you talking about the enum at https://msdn.microsoft.com/en-us/library/system.data.sqldbtype(v=vs.110).aspx
SqlDbType is available in the Data.Common references.

[FransBouma]

@danmosemsft understood. I don't think MS will wait a year or so before releasing 2.1 anyway ;). It's a bit unfortunate things are coming together right at this moment and not 3 months earlier, but I understand your point. :)

[RickStrahl]

@saurabh500 I think the problem is this:

[saurabh500]

@RickStrahl what's the version of SqlClient that you are using? It looks like you need to upgrade to 4.4.0-*

[saurabh500]

I am closing this issue as this is an aggregate of other issues which we have tackled in 2.0.