jonwagner / Insight.Database

Fast, lightweight .NET micro-ORM
Other
861 stars 145 forks source link

"Operation could destabilize the runtime" with Table Value Parameter with GUID column #456

Closed arikalish closed 3 years ago

arikalish commented 3 years ago

Describe the bug

We've got a few stored procedures that take TVPs where at least one of the columns is a GUID. When those procedures are called by Insight they throw an exception stating "Operation could destabilize the runtime".

Steps to reproduce

InsightTvpCrashRepro.zip

In the attached zip file, execute the sql file to setup a database with a single table with three entries, a custom table-type with a single GUID column, and a stored procedure that uses the custom type.

The included sample program crashes as soon as it attempts to execute the stored procedure.

Note: when I changed the custom type to use an nvarchar(1) instead of a uniqueidentifier and used A, B, and C for TableEntry in SampleTable the query executes successfully.

Expected behavior

The query should execute successfully.

jonwagner commented 3 years ago

I could imagine that the type conversion code could fail in an edge case like that. What is the C#/.NET type you're sending into Insight? is it a string or a GUID?

arikalish commented 3 years ago

In one case we pass in a string and in the other case we pass in GUIDs.

Something like this:

Guid[] orderedIds = new [] { a bunch of Guids };

var command = new SqlCommand("The_Sproc")
{
    CommandType = CommandType.StoredProcedure
};

var ids = CreateDataTableWithIncrementingIdAndGuid(orderedIds);

var sqlParameters = command.Parameters.AddWithValue("@ids", ids);
sqlParameters.SqlDbType = SqlDbType.Structured;

private static DataTable CreateDataTableWithIncrementingIdAndGuid(Guid[] orderedIds)
{
    var ids = new DataTable();

    ids.Columns.Add(new DataColumn("ItemOrder", typeof(int))
    {
        AutoIncrement = true,
        AutoIncrementSeed = 0,
        AutoIncrementStep = 1
    });

    ids.Columns.Add("ItemId", typeof(Guid));

    foreach (var ids in orderedIds)
        ids.Rows.Add(null, id);

    return ids;
}
arikalish commented 3 years ago

I went back and tried changing the sample test program from:

var orderedList = new[] { "a72863cf-c573-4bf8-9a0b-02212f84698a", "56a0c8ef-c826-45a5-bbce-fb334e59f4b7", "26525d03-1a64-4843-bab4-9daf88e9ae02" }; to var orderedList = new[] { new Guid("a72863cf-c573-4bf8-9a0b-02212f84698a"), new Guid("56a0c8ef-c826-45a5-bbce-fb334e59f4b7"), new Guid("26525d03-1a64-4843-bab4-9daf88e9ae02") };

and the crash goes away. Updated the in-app code that uses a similar pattern and things seem OK there. The example in my first response where the DataTable isn't crashing - looks like that was a misunderstanding. The first crash prevented me from easily getting to the second one.

jonwagner commented 3 years ago

Yup - my guess is that there's a missing conversion step when we go from an object type (string) to a value type (guid). Usually there's a missing boxing or unboxing in the code. I'll take a look at a fix.

You probably have workarounds making sure that you're mapping string => nvarchar and converting in the sql side, or making sure that you pass Guid => uniqueidentifier but who wants to do that? Am I right? :)

arikalish commented 3 years ago

or making sure that you pass Guid => uniqueidentifier but who wants to do that? Am I right? :)

Hah yes. It'd definitely save folks down the road from some surprises!

Thanks for taking a look.

jonwagner commented 3 years ago

I added a test case for this (see commit above). I was unable to reproduce the behavior you are describing on netcore5. I'll run it on other platforms. Can you modify the test case to fail in 6.3.5?

jonwagner commented 3 years ago

Interesting - this fails in net451 based builds but not netcore5 based builds.

jonwagner commented 3 years ago

Found the missing cast. This is fixed in 6.3.6 available now.

arikalish commented 3 years ago

Thanks! We'll give it a try on Monday!

arikalish commented 3 years ago

Seems to have fixed the issue we were seeing. Thanks for the quick turnaround!