fix SqlHierarchyId serialization and deserialization

[olmobrutall]

Hi @dotMorten,

This PR solves the issue #35 when serializing SqlHierarchyId.

I've also converted the examples in the issues into unit test and, to be sure, tested with 10000 random SqlHierarchyId,

I discovered that the deserialization was also broken for big numbers (> int.MaxValue), an error since we converted from int to long. It's fixed now.

In order to make the random SqlHierarchyId distributed usefully I had to also Include KnownPatterns.cs, and this required to move Microsoft.SqlServer.Types.Tests.csproj to C# 8.0 not-nullable.

Cheers

[olmobrutall]

Oh! and I forgot to mention that I've included a piece of code that compares the results with the actual values generated by Sql Server.

This is necessary to determine if the problem occurs when serializing or deserializing.

The code inside of an if(false) to avoid requiring a SqlServer instance running, but is very useful for debugging or discovering more KnownPatterns in the future.

[ppasieka]

Would you be able to always include these tests? In this project, you can find the DBTests.cs file where geometry tests are executed against real DB.

Something in these lines:

HierarchyScripts.cs

    internal class HierarchyScripts
    {

        public static string DropTables = @"
IF OBJECT_ID('dbo.TreeNode', 'U') IS NOT NULL DROP TABLE TreeNode;
";

        public static string CreateTables = @"
-- hierarchy_tests
CREATE TABLE TreeNode(Id IDENTITY PRIMARY KEY, Route HierarchyId NOT NULL);
";
    }

DatabaseUtil.cs (extracted from DBTests.cs)

    static class DatabaseUtil
    {
        internal static void CreateSqlDatabase(string filename)
        {
            string databaseName = System.IO.Path.GetFileNameWithoutExtension(filename);
            if (File.Exists(filename))
                File.Delete(filename);
            if (File.Exists(filename.Replace(".mdf","_log.ldf")))
                File.Delete(filename.Replace(".mdf", "_log.ldf"));
            using (var connection = new SqlConnection(
                @"Data Source=(localdb)\mssqllocaldb;Initial Catalog=master; Integrated Security=true;"))
            {
                connection.Open();
                using (var command = connection.CreateCommand())
                {
                    command.CommandText =
                        String.Format("CREATE DATABASE {0} ON PRIMARY (NAME={0}, FILENAME='{1}')", databaseName, filename);
                    command.ExecuteNonQuery();

                    command.CommandText =
                        String.Format("EXEC sp_detach_db '{0}', 'true'", databaseName);
                    command.ExecuteNonQuery();
                }
            }
        }
    }

And actual tests class

    [TestCategory("Database")]
    [TestCategory("SqlHierarchyId")]
    public class HierarchyDbTests : IDisposable
    {
        const string DataSource = @"Data Source=(localdb)\mssqllocaldb;Integrated Security=True;AttachDbFileName=";

        private readonly SqlConnection _connection;
        private static string _path;
        private static readonly object LockObj = new object();
        static HierarchyDbTests()
        {
            Init();
        }

        private static void Init()
        {
            lock(LockObj)
                if(_path == null)
                {
                    _path = Path.Combine(new FileInfo(typeof(DBTests).Assembly.Location).Directory.FullName, "HierarchyUnitTestData.mdf");
                    DatabaseUtil.CreateSqlDatabase(_path);
                    using (var conn = new SqlConnection(DataSource + _path))
                    {
                        conn.Open();
                        var cmd = conn.CreateCommand();
                        cmd.CommandText = HierarchyScripts.DropTables;
                        cmd.ExecuteNonQuery();
                        cmd.CommandText = HierarchyScripts.CreateTables;
                        cmd.ExecuteNonQuery();
                        conn.Close();
                    }
                }
        }

        private static string ConnectionString => DataSource + _path;

        public HierarchyDbTests()
        {
            Init();
            _connection = new SqlConnection(ConnectionString);
            _connection.Open();
        }
        
        public void Dispose()
        {
            _connection.Close();
            _connection.Dispose();
        }
        
/*
Your tests here
*/
}

[olmobrutall]

Looks good to me, didn’t realize other test where already speaking with the database. Didn’t want to add a dependency because potential problems with CI. But locadb is a great solution.

[ppasieka]

Why have this if statement?

[olmobrutall]

Just so I could put a breakpoint in the assert when things went wrong. VS didn’t want to rewind the stacktrace after the exception inside AreEqual. But could be removed now.

[ppasieka]

You could also replace this test method with data-driven method

        [DataTestMethod]
        [DynamicData(nameof(GetData), DynamicDataSourceType.Method)]
        public void SerializeDeserializeRandom(string route)
        {
            // TEST body
        }

        private const int CountOfGeneratedCases = 1000;
        private static HashSet<string> _usedRoutes = new HashSet<string>();
        public static IEnumerable<object[]> GetData()
        {
            Random r = new Random();
            for (var i = 0; i < CountOfGeneratedCases; i++)
            {
                var randomHierarchyId = RandomHierarchyId(r);
                while (_usedRoutes.Contains(randomHierarchyId)) 
                    randomHierarchyId = RandomHierarchyId(r);

                _usedRoutes.Add(randomHierarchyId);
                yield return new object[] {randomHierarchyId};
            }
        }

[olmobrutall]

Hi @ppasieka , sorry for the delay. I hope everything is ok now.

[olmobrutall]

Hi @dotMorten, the PRs always take forever to to get merged / published.

Maybe is better to recover https://www.nuget.org/packages/Unofficial.Microsoft.SqlServer.Types/ and split the SqlHierarchyId stuff there?

[dotMorten]

Yes I'm considering removing the datatype from this library entirely, as I have neither an understanding or interest in it. It was a community contribution I accepted, and probably shouldn't have, as all it's done is generating lots of issues, and complaints that I'm quite honestly not interested in having to deal with for something I use my spare time to maintain, and make absolutely nothing on doing. My personal mental health comes first and foremost.

Maybe is better to recover https://www.nuget.org/packages/Unofficial.Microsoft.SqlServer.Types/

No please don't do that. Call it something completely different to avoid any confusion.

[ErikEJ]

@dotMorten happy to help in any way I can.

[ppasieka]

@dotMorten I forked this repo, and I'm using it with #46 and #55 PRs merged-in for over a month now without any problems. I also understand that it is not a comfortable situation to be in when you feel responsible for maintaining something you don't understand or don't need, but I as you see from PRs activity, contributors are trying to relieve you from this feeling :)

As a side note, your NuGet package is used in (unofficial) EF Core library EFCore.SqlServer.HierarchyId

[olmobrutall]

It was a community contribution I accepted, and probably shouldn't have, as all it's done is generating lots of issues, and complaints that I'm quite honestly not interested in having to deal with for something I use my spare time to maintain, and make absolutely nothing on doing.

I made the original contribution and I’m sorry that it caused problems. I also have a busy live and spend too much time on open source. From time to time I get mention in some of this issues and I have to stop what I’m doing to fix this bug, and then wait for weeks or months till I get feedback fro the gatekeepers. Typically some stylistic changes in the unit test, nothing in the core algorithm that is what could prevent the next issue.

So for me will also be easier to have a repository where I’m the collaborator (or one of them).

Unfortunately, this will create some frictions on the consumers that won’t have to reference just one package for getting all the types in Microsoft.SqlServer.Types.

So, your decision. If you want to remove all the HierarchyId code I will create a new Microsoft.SqlServer.Types.SqlHierarchyId package and inform EFCore.SqlServer.HierarchyId about the split.

[dotMorten]

@olmobrutall no reason to apologize. I appreciate your work.
The stylistic feedback is imho what's important to me here, not whether the fix works as intended or not. I want consistency throughout - otherwise it only gets more unmaintainable over time. After all if I'm going to maintain it, it'll have to be on my own terms.

[olmobrutall]

After all if I'm going to maintain it, it'll have to be on my own terms.

All yours then

[vyrotek]

Appreciate all the effort put into this so far!

Unfortunately we're impacted by these bugs due to EFCore.SqlServer.HierarchyId's dependency on this library.

It sounds like there's interest in forking the maintenance of the HierarchyId types separately.

@ppasieka @olmobrutall It may be worth having a discussion and seeing what you think of this idea.
efcore/EFCore.SqlServer.HierarchyId#37 (comment)

@dotMorten If there was an understanding that future/long-term HierarchyId support will be done separately would you feel more comfortable accepting these remaining crucial bug fixes to buy time while other libraries migrate off?