Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with specific values #35

Closed
Jabe opened this issue May 4, 2020 · 9 comments · Fixed by #56
Closed

Issues with specific values #35

Jabe opened this issue May 4, 2020 · 9 comments · Fixed by #56

Comments

@Jabe
Copy link

Jabe commented May 4, 2020

There is an issue with specific values. Using the following code in linqpad:

var parsed = HierarchyId.Parse("/1.3.2/").Dump("parsed");
var ms = new MemoryStream();
parsed.Write(new BinaryWriter(ms));
ms.Position = 0;
var roundTrip = new Microsoft.SqlServer.Types.SqlHierarchyId();
roundTrip.Read(new BinaryReader(ms));
roundTrip.Dump("roundTrip");
Input Value of parsed Value of roundTrip
/1.0.2/ /1.0.2/ /1.0.2/
/1.1.2/ /1.1.2/ /1.1.2/
/1.2.2/ /1.2.2/ /1.2.2/
/1.3.2/ /1.3.2/ InvalidCastException: No pattern found for: 11010
/3.0/ /3.0/ /

Using Microsoft.SqlServer.Types.SqlHierarchyId (Microsoft.SqlServer.Types, Version=1.1.0.0, Culture=neutral, PublicKeyToken=null) directly in netcore31 (linqpad6) produces the same bad result.

Using Microsoft.SqlServer.Types.SqlHierarchyId (Microsoft.SqlServer.Types, Version=13.0.0.0, Culture=neutral, PublicKeyToken=89845dcd8080cc91) directly in net46+ (linqpad5) works with all tested values as expected.

(via efcore/EFCore.SqlServer.HierarchyId#7)

@ppasieka
Copy link
Contributor

@dotMorten do you have any materials where I could learn more about the bit patterns used in implementing HierarchyId?

Or maybe you have some ideas how to fix this issue?

@dotMorten
Copy link
Owner

dotMorten commented Sep 28, 2020

do you have any materials where I could learn more about the bit patterns used in implementing HierarchyId?

Nope. I didn't even provide this implementation (community contribution), and honestly don't even know what it is used for. Part of me wish I didn't accept the PR, because I have a really hard time maintaining that part of the source code, and really rely on the community to do this.

@ppasieka
Copy link
Contributor

@olmobrutall Would you be able to help with this issue? Or provide some materials where I could learn more about these bits patterns?
Definitely, serialization part is broken. When I try to look at data in SQL management studio I'm getting an exception
after using .ToString on HierarchyId

A .NET Framework error occurred during execution of user-defined routine or aggregate "hierarchyid": 
Microsoft.SqlServer.Types.HierarchyIdException: 24000: SqlHierarchyId operation failed because HierarchyId object was constructed from an invalid binary string. 
Microsoft.SqlServer.Types.HierarchyIdException: 
   at Microsoft.SqlServer.Types.ex_raise(Int32 major, Int32 minor, Int32 sev, Int32 state, Object param1, Object param2, Object param3)
   at Microsoft.SqlServer.Types.OrdPath.GetBits(UInt16 startBit, UInt16 nBits, UInt64& bits)
   at Microsoft.SqlServer.Types.OrdPath.ExtractOrd(UInt16& bitOffset, UInt32 stage, Int64& ord)
   at Microsoft.SqlServer.Types.OrdPath.ExtractComponent(UInt16& bitOffset, SComponent& component, levelType& type)
   at Microsoft.SqlServer.Types.OrdPath.ToString()

@olmobrutall
Copy link
Contributor

Hi @ppasieka,

the bit patterns come from http://www.adammil.net/blog/v100_how_the_SQL_Server_hierarchyid_data_type_works_kind_of_.html

Not sure if I’ll find time to work on it, but I can review a potential change if you go for it.

Cheers

@olmobrutall
Copy link
Contributor

It's definitely an issue with serialization, I've converted the code to a Unit Test and also compare with values comming from the database:

        [DataTestMethod]
        [DataRow("/1.0.2/")]
        [DataRow("/1.1.2/")]
        [DataRow("/1.2.2/")]
        [DataRow("/1.3.2/")]
        [DataRow("/3.0/")]
        public void SerializeDeserialize(string route)
        {
            var parsed = SqlHierarchyId.Parse(route);
            //var ms = new MemoryStream();
            //parsed.Write(new BinaryWriter(ms));
            //ms.Position = 0;
            //var roundTrip = new Microsoft.SqlServer.Types.SqlHierarchyId();
            //roundTrip.Read(new BinaryReader(ms));
            //Assert.AreEqual(parsed, roundTrip);

            using (SqlConnection con = new SqlConnection(@"Data Source=.\SQLEXPRESS;Initial Catalog=HierarchyTest;Integrated Security=true"))
            {
                con.Open();
                var id = new SqlCommand($"INSERT INTO [dbo].[TreeNode] (Route) output INSERTED.ID VALUES ('{route}') ", con).ExecuteScalar();

                using (var reader = new SqlCommand($"SELECT Route FROM [dbo].[TreeNode] WHERE ID = " + id, con).ExecuteReader())
                {
                    while (reader.Read())
                    {
                        var roundTrip = new Microsoft.SqlServer.Types.SqlHierarchyId();
                        roundTrip.Read(new BinaryReader(reader.GetStream(0)));
                        Assert.AreEqual(parsed, roundTrip);
                    }
                }
            }

The deserializer works, the serializer produces the error. Maybe tomorrow I can take a deeper look.

@ppasieka
Copy link
Contributor

@olmobrutall Thanks. I will also look into it tomorrow :) And yes I came to the same conclusion that the issue is with a serializer. Deserialization part work for me without any problems so far

@olmobrutall
Copy link
Contributor

Fixed in #55

@ppasieka
Copy link
Contributor

@olmobrutall amazing 👍 I will check it against my project. Thanks for that fast response 😊

@ppasieka
Copy link
Contributor

ppasieka commented Nov 3, 2020

@dotMorten Fix provided in #55 works nicely. What do you think about creating a release? With PRs #55 and #47 out, the list of HierarychId problems would be reduced to only one (#36)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants