Critical Bug with HierarchyId Serialization/Deserialization

[vyrotek]

We ran into a show-stopping issue with the writing and reading of HierarchyId binary data.

If findings below are accurate then folks need to know that this wonderful library is not production ready at the moment!
V 2.1.0 now pulls in V 1.3.0 of dotMorten which addresses this specific bug. Thanks everyone!

This library uses dotMorten/Microsoft.SqlServer.Types as the backing SqlHierarchyId implementation which has an outstanding bug with how it reads and writes Id values.

For example: Using GetDescendant on SQL Server generates the following:

declare @h HIERARCHYID = '/1/1/'
select
[Binary] = @h.GetDescendant('/1/1/3/', '/1/1/4/'),
[String] = @h.GetDescendant('/1/1/3/', '/1/1/4/').ToString()

Binary		String
0x5AE058	/1/1/3.1/

But in C# it generates the following invalid binary value:

var newId = HierarchyId.Parse("/1/1/").GetDescendant(HierarchyId.Parse("/1/1/3/"), HierarchyId.Parse("/1/1/4/"));

using (var ms = new MemoryStream())
using (var binWriter = new BinaryWriter(ms))
{
    newId.Write(binWriter);
    var byteString = BitConverter.ToString(ms.ToArray()).Replace("-", "");
    var result = String.Format("0x{0}", byteString);
}

// Result = 0x5AE0B0

Here are some related issues and PRs:
dotMorten/Microsoft.SqlServer.Types#35
dotMorten/Microsoft.SqlServer.Types#55

I'm no expert, but after reading through this article and the PRs above it seems maybe the issue occurs when you try to use any Id value which is on the end of the special ranges? (ex. 0 through 3, 4 through 7, etc.)
http://www.adammil.net/blog/v100_how_the_SQL_Server_hierarchyid_data_type_works_kind_of_.html

I also verified that Using the .NET Framework version of Microsoft.SqlServer.Types does not have the issue.

I'm not sure what options are available until this is resolved. The maintainer of the dotMorten library has not committed to getting this resolved and does not seem to feel confident in maintaining HierarchyId features in his library. In fact, he may remove it completely.
dotMorten/Microsoft.SqlServer.Types#55 (comment)

[bricelam]

That library is the main thing preventing this from being an “official” extension.

To do it properly, we should create something like NTS and NetTopologySuite.IO.SqlServerBytes but for hierarchyid.

The spec for the serialization format is here: https://docs.microsoft.com/openspecs/sql_server_protocols/ms-ssclrt/6f82da7e-f487-4bb1-afa3-4b0ce0acb2db

[bricelam]

Please upvote these issues to let Microsoft know this is a priority:

dotnet/efcore#365
dotnet/SqlClient#322

[bricelam]

@olmobrutall Let me know if you're interested in working on an implementation more like the NTS model. It would be completely decoupled from Microsoft.SqlServer.Types and SqlClient.

We'd provide a friendlier type for using, serializing, and deserializing hierarchyid values:

public class HierarchyId : IComparable {
    public HierarchyId(string value);
    public HierarchyId(byte[] bytes);
    
    public override string ToString();
    public byte[] ToByteArray();
    
    public HierarchyId GetAncestor(int n);
    public HierarchyId GetDescendant(HierarchyId child1, HierarchyId child2);
    public short GetLevel();
    public HierarchyId GetReparentedValue(HierarchyId oldRoot, HierarchyId newRoot);
    public bool IsDescendantOf(HierarchyId parent);
}

That could easily be used with SqlClient:

// Reading
var bytes = dataReader.GetSqlBytes(columnOrdinal).Value;
var hierarchyId = new HierarchyId(bytes);

// Writing
var bytes = hierarchyId.ToByteArray();
var parameter = command.Parameters
    .AddWithValue(parameterName, new SqlBytes(bytes));
parameter.SqlDbType = SqlDbType.Udt;
parameter.UdtTypeName = "hierarchyid";

[vyrotek]

1.3 was released which contains fixes for this issue!

https://github.com/dotMorten/Microsoft.SqlServer.Types/releases/tag/release%2Fv1.3

[olmobrutall]

@bricelam to give a little bit of context, I'm the maintainer of https://github.com/signumsoftware/framework for like 10 years and I don't use EF on a daily basis (I would love to discuss why with the EF team by the way!). I'm also father of two toddlers and I have a day job.

I'm worried of the burden on support request that building an official piece of EF will put on my shoulders. Also, SQL Server is an expensive Microsoft product and we're talking about .Net Core support, not some corner language... shouldn't SQL Server team take his share of work?.

Complains aside, I see that the current dependency chain HierarhyId (efcore) -> SqlHierarhyId (dorMorten) -> HierarhyId (dotMorten) it's kind of ugly and should be fixed. I would also like to have one HierarhyId type that can easily be used in EFCore, Signum Framework, or any other .Net Core code.

About your proposed APIs:

• Should this type have all the Equal / GetChasCode / CompareTo / overloaded operators... I think so.
• What about the annoying SqlBoolean / SqlInt16? I just took it from the official .Net Framework implementation but.. could we use bool/short instead?

So, I can build / help build such a class but will be cool to have someone with a Microsoft salary that understand the code and can maintain it in the future.

[bricelam]

We've been asking the SQL Server team for years to support the spatial and hierarchyid types .NET Core, but it seems to ...fall on deaf ears.

EF Core's use of NTS and this unofficial hierarchyid side project of mine (and others from the community) represent our efforts to unblock users. But of course, we'll keep pressuring the SQL Server team too.

My draft proposal was just an illustration of how we could cut dotMorten.Microsoft.SqlServer.Types out of the picture and still support hierarchyid on .NET Core using amore ORM-friendly type (those SqlBoolean types just make me cringe).

If you (and Morten) are happy with the current state of things, I'm happy to continue as we are for now and eagerly await official SQL Server support for these types on .NET Core.

I don't use EF on a daily basis (I would love to discuss why with the EF team by the way!)

@JeremyLikness is currently gathering feedback to help direct the future development of EF Core and, more generally, data access on .NET. Always feel free to reach out to us.

[bricelam]

Closing this issue as resolved by PR #38, but feel free to continue the discussion.

The issues tracking official .NET Core support are:

• Implement SqlGeography and SqlGeometry for Spatial Types support for .Net Core dotnet/SqlClient#30 (spatial)
• Implement SqlHierarchyId type for .NET Core dotnet/SqlClient#322 (hierarhcyid)

[olmobrutall]

@bricelam if you think SQL Server team support is not going to come any soon, I think I could find time to make a cleaner HierarchyId implementation about Feb 2021.

where should be the repo be created? As a project from dotnet? NetTopologySuite? signumsoftware? or my very professional olmobrutall account?

@vyrotek @ppasieka feel like helping on it? There is not too much coding to be made but maybe some reviewing/CI stuff etc?

[bricelam]

We're reaching out to the SQL Sever team (again). Let's give them a chance to respond before moving forward.