Microsoft.SqlServer.Server conflicts between S.D.SqlClient and M.D.SqlClient

[Tornhoof]

As soon as I reference this package from a 2.2 project with a reference to "Microsoft.AspNetCore.App" I get the following error:

CS0433	The type 'SqlDataRecord' exists in both 'Microsoft.Data.SqlClient, Version=1.0.19128.1, Culture=neutral, PublicKeyToken=23ec7fc2d6eaa4a5' and 'System.Data.SqlClient, Version=4.5.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

Repro:

1. Create a new project
2. Reference both the Microsoft.AspNetCore.App package and the Microsoft.Data.SqlClient package
3. Use a type from the package

using Microsoft.SqlServer.Server;

namespace ConsoleApp44
{
    class Program
    {
        static void Main(string[] args)
        {
            var sqlDataRecord = new SqlDataRecord();
        }
    }
}

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.2</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Data.SqlClient" Version="1.0.19128.1-Preview" />
    <PackageReference Include="Microsoft.AspNetCore.App" />
  </ItemGroup>

</Project>

It apparently works on .NET Core 3.0 with FrameworkReference, atleast I don't get any error

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp3.0</TargetFramework>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Data.SqlClient" Version="1.0.19128.1-Preview" />
    <FrameworkReference Include="Microsoft.AspNetCore.App" />
  </ItemGroup>

</Project>

dotnet --info .NET Core SDK (gemäß "global.json"): Version: 3.0.100-preview4-010643 Commit: 88e7cb2515

Laufzeitumgebung:
OS Name: Windows
OS Version: 10.0.17134
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\3.0.100-preview4-010643\

Host (useful for support):
Version: 3.0.0-preview4-27506-14
Commit: 98641ad643

[David-Engel]

Thanks for filing the issue. I can reproduce it simply by adding a reference to both System.Data.SqlClient and Microsoft.Data.SqlClient and same code snippet you have. We are looking into it.

In the meantime, what is your use case for referencing classes under Microsoft.SqlServer.Server? Are you working with UDTs or something else?

[Tornhoof]

Yeah, I tried to do a repro without the normal SqlClient package to make it more realistic.
As for my use case:
Table-Valued-Parameters together with Dapper, so from that namespace, I use SqlDataRecord and SqlMetadata.

[mgravell]

Yeah, this is definitely going to cause a few problems... I added a dup, but closed it - same problem as this.

[Wraith2]

What's is missing in M.D for TVP's that needs to come in from S.D?

[mgravell]

@Wraith2 quite likely nothing is missing; the problem is that it already has come over, and has the exact same namespace and type name. Which makes it unusable if you have (direct or transitive) references to both M.D and S.D

Of course, since this means I can't compile the tests that touch SqlDataRecord, I can't actually test whether they work correctly ... and unfortunately Dapper has SqlDataRecord on one of the public APIs, so I can't remove S.D to do an isolated test. I might have to say "to hell with it" and do a major on Dapper to change that method from SqlDataRecord to <T> where T : IDataRecord, so I can remove the S.D dependency completely

[mgravell]

To restate that more concisely:

• Dapper currently has the old SqlDataRecord on a public API that can be used with TVPs as an alternative to DataTable
• because of this, Dapper refs S.D
• which means that anything that refs Dapper has a transitive ref to S.D
• which means that anything that refs Dapper cannot use SqlDataRecord at all if it refs M.D - it will be an unresolvable ambiguous fully qualified name

In the above, Dapper can be a placeholder for anything else that refs, directly or transitively, S.D

However, IMO the problem remains even if I remove the dep from Dapper.

IMO, having the same fully qualified type name in two assemblies is just asking for pain.

[David-Engel]

We've been thinking about this to come up with a workable solution. Opening it up to discussion.

Problem Summary: The Microsoft.SqlServer.Server namespace lives inside the SqlClient assembly in both System.Data.SqlClient and Microsoft.Data.SqlClient. (This is likely because it references internal classes in SqlClient.) If an application references any class under Microsoft.SqlServer.Server and the project pulls in dependencies for S.D.SqlClient and M.D.SqlClient (directly or indirectly), they will get an error(s) about conflicting types in S.D.SqlClient and M.D.SqlClient.

In an effort to minimize conflicting scenarios, our thought is to simply pull Microsoft.SqlServer.Server out of M.D.SqlClient (make anything actually used by SqlClient internal):

• Applications needing Microsoft.SqlServer.Server can take a dependency on S.D.SqlClient for that namespace.

To allow applications to eliminate the S.D.SqlClient dependency in the future, we could:

• Create a standalone Microsoft.SqlServer.Server package that is dependent on M.D.SqlClient
• Add InternalsVisibleTo to M.D.SqlClient to make internal classes visible to Microsoft.SqlServer.Server

This would eliminate the conflict in scenarios where an application references Microsoft.SqlServer.Server and has dependencies on both S.D.SqlClient and M.D.SqlClient. If we created the new Microsoft.SqlServer.Server package, applications could move to the new Microsoft.SqlServer.Server package only when all their dependencies have moved off of S.D.SqlClient.

CC: @divega

[mgravell]

@David-Engel question, though: there are scenarios where those types are useful today; if you remove them from the public API in M.D, will they work with S.D? Example: the TVP scenario mentioned above. I don't know how often SqlMetaData is used (not one I'm very familiar with). If they don't work, there may be an API gap. If they do work inwards, you'd presumably need to use reflection. And I'm not sure they could ever work outwards reliably.

Follow up question: is there no imaginative alternative namespace that could be used instead, even if slightly ugly?

[Wraith2]

(This is likely because it references internal classes in SqlClient.)

It goes the other way from what I can see. The M.S.Server namespace is referenced inside core bits of sqlclient (connection, parameter, parser etc) anytime it has to deal with udt or tvp it uses the extended metadata format in the M.S.S namespace to describe it. So it might be that if we break out M.S.S into it's own assembly it would need it's internals to be visible to M.D.SqlClient. I'm not sure if that makes things better or not.

I'm not a fan of InternalsVisibleTo, it always struck me as cheating. In this case given the support burden exposing the extended metadata api would come with I could be convinced that it's the best way to solve the problem.

[Wraith2]

I went through all the files In the MSS namespace and removed the references to S.D.SqlClient (on the corefx codebase, so i'm assuming no major re-org) then worked through all the errors the compiler flagged working out how each one could be resolved. If we break out MSS M.D.SqlClient can depend on it but MSS can't depend back. Overall positive but there are a couple of gnarly internal references that we'd need a good way to tackle.

Visibility	TypeName	Purpose	Suggested Action
public	SqlErrorCollection		eliminate the reference, they are never used?
public	SqlException		problem, would need a new exception type that cannot derive from SqlException
public	SqlParameter		only internals function ParseTypeName is used, could be copied
public	SortOrder		Problem, copy and provide map function?
public	SqlDataReader		problem, this is used for discovery and there is a fallback path
internal	ParameterPeekAheadValue		problem, this is an Ienumerable and SqlDataRecord, these could both be passed separately
internal	TdsEnums	protocol constants	create separate copy
internal	MetaType	type mapping	problem
internal	SqlBuffer.StorageType	type mapping	problem
internal	DataFeed	stream support	move to MSS
internal	StreamDataFeed	stream support	move to MSS
internal	TextDataFeed	stream support	move to MSS
internal	XmlDataFeed	stream support	move to MSS
internal	SQL	static helper	create separate copy

[David-Engel]

@mgravell I've not worked much with TVPs and UDTs in SqlClient (yet) so I'm learning these dependencies as we go. The thought of changing the namespace did cross my mind.

On the Framework side of M.D.SqlClient, we brought in all the MSS classes but set them all to internal so as not to conflict with S.D.SqlClient. I don't know why we didn't do that when we brought in the Core side other than the fact that two different people did it at two different times. I'm hoping @saurabh500 chimes in on this thread.

@Wraith2 Nice! I've been looking at the API surface of SqlClient and I don't see anywhere that MSS is used in the SqlClient API which leads me to believe we can simply hide MSS internally in M.D.SqlClient on Core like we have done on Framework. We can always choose to expose them publicly under a different namespace later, if needed.

[Wraith2]

MSS isn't exposed through SqlClient but you can't use udt and tvp without them and if there are internal versions then users can't refer to them. In the case @mgravell gave above he uses SqlDataRecord so he'd end up referring to [S.D.SqlClient]M.S.S.SqlDataRecord and that wouldn't be the same as the internal [M.D.SqlClient]M.S.S.SqlDataRecord so it wouldn't work. It would move the ambiguous throwing location from client code into SqlClient. Or am I missing something?

Changing the M.S.S namespace is probably the cleanest way to deal with it but it stops M.D.SqlClient being drop-in compatible in cases where tvp or udt are used. It'll cause some library authors pain but there would be a clear path resolve them which didn't require binding redirects and other such things. It does mean that people using libraries that support both will end up loading both because the interfaces are used.

[divega]

I agree with @mgravell that the simple solution should be renaming Microsoft.SqlServer.Server (and any other namespace duplicated between Microsoft.Data.SqlClient and System.Data.SqlClient) in the Microsoft.Data.SqlClient package so that there is no conflict. We cannot assume by default that these types can stop being public without loss of functionality. Also, using InternalsVisibleToAttribute is a smell. We should avoid it if we can.

I'm hoping @saurabh500 chimes in on this thread.

Me too. I remember Saurabh identified some issues around these types when he did the initial analysis for Microsoft.Data.SqlClient, but I don't keep all the details in my mind. I think it would probably also help to include the document that he produced in this repo (or any more up to date version of it), so that we all have it as a reference.