Extensions: Consider changing Microsoft.Extensions.Caching.SqlServer to depend on Microsoft.Data.SqlClient and/or making it OOB

[divega]

Creating this as a follow up from https://github.com/aspnet/AspNetCore/issues/10013#issuecomment-492811495.

Currently Microsoft.Extensions.Caching.SqlServer depends on System.Data.SqlClient, which is still supported, but the new provider Microsoft.Data.SqlClient is where all new features of SQL Server will be supported. In the 3.0 time frame the main functional difference is Always Encrypted support in .NET Core.

I haven't checked if this ships only as a package or as part of the shared framework.

[natemcmaster]

This package already ships OOB and is not part of the aspnet shared framework.

cc @anurse for triage.

[anurse]

I guess my first question is what is the primary motivation here? Are there features we need or is it just about "completeness" and transitioning to the new thing in major release?

[ajcvickers]

@anurse All .NET Core applications that use SQL Server should switch to this new package. It should be a trivial amount of work--if it isn't, then that's something we need to follow up on.

I don't expect you need any features of the new package, but going forward it's the only package that will get bug fixes or new features.

[anurse]

Cool, just clarifying the situation.

[anurse]

I notice that the Microsoft.Data.SqlClient package is preview on NuGet. Do we have a commitment that there will be an RTM build well in advance of 3.0 RTM? We can't depend upon a preview package in our RTM build. As long as we're confident it will be RTM with plenty of runway before our RTM though I'm OK with this. If there's a reasonable chance that it will slip though, we should consider doing this after their RTM _actually ships_. Looks like EF Core will be adopting it in 3.0 which means we have fairly strong confidence it will RTM in time.

Does anyone see any concern with just swapping this dependency? Often when we change the actual assembly identity of a package we consume we create a new package (i.e. the StackExchange.Redis situation) because it's a binary breaking change (even if the source is identical). Are we concerned about that here? Users who are currently only getting System.Data.SqlClient through this package will now be getting Microsoft.Data.SqlClient. Users who are directly referencing System.Data.SqlClient (or getting it through a different package) will now have a reference to both System.Data.SqlClient _and_ Microsoft.Data.SqlClient. Do we know what happens in that situation? My presumption is it would be OK but you'd end up with two separate connection pools. (cc @davidfowl)

All .NET Core applications that use SQL Server should switch to this new package.

Given this, it seems OK to just swap it. Is that what EF is doing @ajcvickers @divega ?

I'll try to schedule this in for preview 6. It sounds like the code change is trivial, it's the knock-on effects I'm more worried about so getting feedback early is critical here. This kind of change is precisely the kind of thing that has bitten us in the past ;).

[ajcvickers]

@anurse

• Yes, we have commitment from them that it will RTM by 3.0.
• Referencing both should work.
• We are just swapping for now. Since we have a provider model, we are going to track requests to support the old package and if there is a a lot of demand we may consider shipping a provider that uses it. However, it is not in the plan to do this unless we need to.

[anurse]

• Yes, we have commitment from them that it will RTM by 3.0.

Most important here is that it has to RTM before 3.0 so that we can take a dependency on the RTM version BEFORE the final check-in for 3.0.

[ajcvickers]

@anurse Yes. That applies to all the dependencies we're taking on it, including EF.

[anurse]

Hrm, I did the upgrade but I get BadImageFormatExceptions running the tests on net461. Looks like this is somewhat expected according to the repo README:

Currently x64 is the only platform supported when targeting .NET Framework. To resolve the warning, change your application to target x64.

This means we would have to change the Microsoft.Extensions.Caching.SqlServer library to be x64-only on .NET Framework. Are we OK with this limitation? (EF folks, is this limitation something you've accepted? @divega @ajcvickers)

This does not apply to .NET Core. The library works fine on .NET Core as an AnyCPU library.

Thoughts @davidfowl ?

[divega]

@David-Engel is the constraint to x64 temporary in the current preview or something that you expect to still have on 1.0 RTM? (it isn't very clear in the readme).

[David-Engel]

@divega It's temporary. It will be fixed in the next preview (already fixed in our current code).

[divega]

Thanks @David-Engel. Is there an ETA for the next preview that you can share?

[David-Engel]

@divega No promises, but I'm hoping within the next 2 weeks.

[anurse]

Given that, I think we should merge this for preview 7. It means we would likely be shipping preview 7 with this known regression, but since changing this dependency is a breaking change, it's important for us to get that done in preview 7.

We might be able to squeak in a dependency bump if the new version is published in 2 weeks (it's not likely though, since it's cutting it super close) but we can definitely bump to the x86-compatible version in preview 8.

Since .NET Core does not appear to be affected by this, and that's where all the ASP.NET Core usage of this will be, I think we're good with that. Thoughts @Pilchie @davidfowl (@glennc ? @DamianEdwards ?)

TL;DR: For Preview 7, after this change, 32-bit applications running on .NET Framework will not be able to use Microsoft.Extensions.Caching.SqlServer. This will be resolved in Preview 8.

[Pilchie]

Yes, I agree.