runtime: Scan SqlClient for missing overrides and interface implementations
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)
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Reactions: 3
- Comments: 24 (22 by maintainers)
@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. 😃
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 😃
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.
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
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
DataTablebeing 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.0said this was there, it lied, it only goes boom”.What can we do to help?