Sqlclient: 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();
        }
    }
}

```xml

Exe
netcoreapp2.2

It apparently works on .NET Core 3.0 with FrameworkReference, atleast I don't get any error
```xml
<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.

[mgravell]

@divega being pragmatic and respecting assembly conventions, perhaps simply: Microsoft.Data.SqlClient.Something.SqlDataRecord etc? Where Something could also just be nothing, i.e. move them to the SqlClient namespace?

[mgravell]

@Wraith2 just to note on "MSS isn't exposed through SqlClient" - in terms of assembly location, it is... complex

in .NET Framework, it comes from:

Assembly System.Data, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089

in .NET Standard / .NET Core App, however, it comes from:

Assembly System.Data.SqlClient, Version=4.5.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a

(with a type-forward in System.Data.dll)

[Wraith2]

Hmm, yes the original split out of System.Data is still there. Which i think makes the option of moving the type into M.D.SqlClient more workable. My only concern on that front is that consumers will end up with references to (and loading) both S.D.SqlClient and M.D.SqlClient

[mgravell]

@Wraith2 in which scenario would the user still need/have a S.D.SqlClient reference, assuming the approach of "those things that are currently causing this pain in M.D.SqlClient - change their namespace"?

[Wraith2]

The Dapper api definition will include two different methods, one defined in terms of each of the different fully qualified names of SqlDataRecord. That will mean both assemblies need to be present and loaded won't it?

[mgravell]

Oh, I see, you're talking about that Dapper API. Yes, it is unfortunate that this is baked in. I do not propose to add a M.S. specific API - the proposed new API is <T> where T : IDataRecord - I will verify that this works as soon as I can do so without the build failing because of the current name conflict. My plan is also to do a "major" rev on Dapper, which removes the old non-generic API, while also removing the entire S.D.SqlClient dependency (moving to just System.Data.Commoin)
However! Assuming that the consumer isn't using that one single API, the current status (without my removing that API) would be if they are using M.D.SqlClient.SqlConnection:

• their code is referencing Dapper
• Dapper (currently) referencs S.D.SqlClient directly, and System.Data.Common transitively
• so the consuming code is taking System.Data.Common transitively, and M.D.SqlClient directly (to use M.D.SqlClient.SqlConnection)
• at runtime, S.D.SqlClient is never loaded - edit: this was wrong; see next comment

Make sense?

Additional work for me to do to remove the S.D.SqlClient dependency:

• in NS1.3, use hard coded value of 30 instead of SqlDbType.Structured in some IL emit code - trivial, just a #if
• add code to dynamically detect this scenario and add the correct type-handler (previously a static type-handler was added for IEnumerable<SqlDataRecord>; "type-handler" here is an internal Dapper concept) - more complex, probably not too hard, but I need the code to be testable before I can implement this

[mgravell]

hmmm... actually, I was wrong about the current status above: the fact that the current code adds the static type handler of course means that Dapper will forcibly load S.D.SqlClient at the moment during the .cctor - so yeah, I need to get that done; again, my barrier is the name conflict

[David-Engel]

It seems like the best approach will be to bring the M.S.S classes forward into M.D.SqlClient under a new namespace.

I'm not crazy about dumping them straight into M.D.SqlClient. How about Microsoft.Data.SqlClient.Server?

This will allow applications to not depend on S.D.SqlClient for those classes. This also means M.S.S.SqlDataRecord will not be compatible with M.D.SqlClient.SqlParameter, which we believe should be fine. Applications need their entire stack to use SqlDataRecord with M.D.SqlClient, just like everything else in M.D.SqlClient (zero object interchangeability between S.D.SqlClient).

[Wraith2]

I'm not crazy about dumping them straight into M.D.SqlClient. How about Microsoft.Data.SqlClient.Server?

Would you be ok with moving SqlDataRecord, SqlMetaData and InvalidUdtException ? into M.D.SqlClient? those are the only things commonly used.

Everything else could be dropped into something like M.D.SqlClient.Compatibility so they have a non-clashing name and are clearly for back compat, As far as I know SQLCLR is never going to be possible with core because the runtime requirements (CAS, CER) aren't present to make it safe.

(zero object interchangeability between S.D.SqlClient).

This is going to be the only way to allow them to co-exist so it get my vote

[mgravell]

@David-Engel can I just make sure I'm 100% clear about this line:

I'm not crazy about dumping them straight into M.D.SqlClient. How about Microsoft.Data.SqlClient.Server?

Do you mean the namespace, or the assembly, or the package here? Namespace: fine. Package gets more complex (I'm trying to see how that would work in the "parameter value for a TVP" scenario).

[David-Engel]

Do you mean the _namespace_, or the _assembly_, or the _package_ here? Namespace: fine. Package gets more complex (I'm trying to see how that would work in the "parameter value for a TVP" scenario).

@mgravell Yes, I was referring to namespace.

[mgravell]

@David-Engel out of curiosity, is there an ETA on the next drop? this is kinda blocking me from testing many deeper things. Just so I can try to arrange my time - any ideas on when this is likely to be?

[DannyMeister]

If anyone ends up here needing a temporary work around, here's one that worked for me.

Background for my use case: I have a project that needs to use SqlDataRecord and tries to do so via referencing package Microsoft.Data.SqlClient. It appears to be gettings its conflict via some kind of transitive dependency within Microsoft.NETCore.Platforms (3.0.0 preview). I'm using an extern alias to alias the Microsoft.Data.SqlClient copy of the conflicting namespace.

In my .csproj:

  <Target Name="ResolveNamespaceConflict" BeforeTargets="FindReferenceAssembliesForReferences;ResolveReferences">
    <ItemGroup>
      <ReferencePath Condition="'%(FileName)' == 'Microsoft.Data.SqlClient'">
        <Aliases>MDSqlClient</Aliases>
      </ReferencePath>
    </ItemGroup>
  </Target>

In my .cs file in this project:

extern alias MDSqlClient;
using MDSqlClient::Microsoft.SqlServer.Server;

[...snip...]

    IEnumerable<SqlDataRecord> Entities { get; set; }

[David-Engel]

@mgravell My goal was to have another one by the end of June. But it's looking tight at the moment. Hopefully we can have a new drop within the next two weeks.

[cheenamalhotra]

Closing the issue as Preview Release v1.0.19189.1 contains the fix.