nhibernate / nhibernate-core

NHibernate Object Relational Mapper
https://nhibernate.info
GNU Lesser General Public License v2.1
2.13k stars 925 forks source link

Breaking Change (in v5.5.0): Primary Key that is of Guid type is now a generated binary parameter instead of string parameter when running query #3511

Closed htmlsplash closed 6 months ago

htmlsplash commented 6 months ago

Environment: Asp.net core (.net 8) Database Driver: SQL Server

I discovered a breaking change going from v5.4.8 to v5.5.0.

For any entity that has PK mapped as of type Guid has stopped working correctly in queries. There's no exception, the queries return wrong results. I looked at the SQL trace, and now PK Guids are treated/sent as binary values to SQL server instead of string values as it was before. See below SQL traces:

v5.4.8 - Guids treated/sent as strings

WHERE pln.entity_id IN (@p0, ....
@p0 = 50f4280c-8e3f-4a97-ac61-cb9f6411e4c9 [Type: Guid (0:0:0)]

v5.5.0 - Guids treated/sent as binary value

WHERE pln.entity_id IN (@p0, ...
@p0 = 0x0001000000FFFFFFFF010000000000000004010000000B53797374656D2E477569640B000000025F61025F62025F63025F64025F65025F66025F67025F68025F69025F6A025F6B000000000000000000000008070702020202020202022AEBEA86C9F9D44699732E0E1D0F7D2C0B [Type: Binary (8000:0:0)]
fredericDelaporte commented 6 months ago

I do neither reproduce any such thing, nor see any change which could be related to such a trouble between 5.4.8 and 5.5.0. Can you provide a complete test case demonstrating the trouble please? See contributing.

htmlsplash commented 6 months ago

@fredericDelaporte I'll try to find out the cause and try to provide an example next week.

Curious, why change Guids from strings to binaries in quries? In the SQL Server's query pane, you enter the Guid as string query parameter, does SQL server support binary Guid parameters?

fredericDelaporte commented 6 months ago

We have done nothing to do such a change willingly.

htmlsplash commented 6 months ago

Okay ... I'll get to the bottom of this, see what's causing our queries return different results.

htmlsplash commented 6 months ago

Correction: I posted incorrect information about my environment. The issue occurs in our asp.net application that runs on asp.net framework 4.8. The queries run inside .net standard 2.0 class library.

I have corrected this information in the original post.

hazzik commented 6 months ago

@htmlsplash you would need to show your mapping, query and configuration, or even better, a full reproduction. As I could not reproduce the behavior reported even specifically using .NET Standard version of NHibernate.

hazzik commented 6 months ago

I had some ideas how to reproduce it, but all failed miserably.

fredericDelaporte commented 6 months ago

I also wondered if it could be a Microsoft.Data.SqlClient vs System.Data.SqlClient issue, or something specific to one of the SQL Server NHibernate dialect or driver available, or an unusual (at least for me) mapping of the Guid property, or specific to the querying API used (Linq, HQL, Criteria, QueryOver, native SQL, direct Load or Get, whatever), or if some custom interceptor or custom event is involved, ... That is too many possible combinations to test to find out if the trouble is actually within NHibernate.

We have over 10 000 tests, with many involving Guids, and none have such an issue with the 5.5.0 release. They are run with many different configurations, including several different ones for SQL Server. So, it sounds like your case needs special conditions to get triggered, and they need to be identified.

The best is to be able to provide a test case as a PR as explained in our contributing guidelines. But if you can provide instead a small standalone program code that does reproduce the issue, that would be good enough.

htmlsplash commented 6 months ago

@fredericDelaporte Will do. Just FYI for now until I find out the real reason: This change was made going from v5.4.8 to v5.5.0 The query that doesn't produce correct results is a native SQL server query that is loaded from resource file and is executed as a named query. I already tested executing native SQL query via "CreateSQLQuery" method and it works perfectly fine on my database.

We only have 1 interceptor, and it is Audit interreceptor, does nothing special but updates created/update fields.

Anyway, next week, I'll find the root cause, no worries.

Thanks for your tips.

htmlsplash commented 6 months ago

@htmlsplash you would need to show your mapping, query and configuration, or even better, a full reproduction. As I could not reproduce the behavior reported even specifically using .NET Standard version of NHibernate.

I'll provide all the information next week when I am at it again at work.

htmlsplash commented 6 months ago

Okay, found it. I have to admit, you guys made me work for this one :) wink wink

Summary of the problem: For named SQL queries (that use guid type field in the where clause with the in expression, I would check also other expressions), if your query parameter name (query-param name) is the same (I mean identical, letter for letter, and matching case) as the one passed to the select query (WHERE pln.entity_id IN (:guidList)) the query will run but treat Guid values as binary values:

Expected: @p0 = Ac030a64-6a03-4092-ade4-3f63bb94fc50 [Type: Guid (0:0:0)] Actual: @p0 = 0x0001000000FFFFFFFF010000000000000004010000000B53797374656D2E477569640B000000025F61025F62025F63025F64025F65025F66025F67025F68025F69025F6A025F6B000000000000000000000008070702020202020202022AEBEA86C9F9D44699732E0E1D0F7D2C0B [Type: Binary (8000:0:0)]

Test Environment: Bug introduced going from Nhibernate v5.4.8 to v5.5.0 .NET Framework used: v4.8 OS: Windows 10 *SQL Server: Microsoft SQL Server 2008 R2 (SP2) - 10.50.4000.0 (X64) Jun 28 2012 08:36:30 Copyright (c) Microsoft Corporation Express Edition (64-bit) on Windows NT 6.2 (Build 9200: )

*I bet you probably will get the same problem with later version of SQL server.

To reproduce this issue, create console app with the following:

Add the following SQL named query as embedded resource to your project and name it TestGuidQuery, but this name doesn't matter, you can name it whatever you like, just make sure you use the same name in your code:

<?xml version="1.0" encoding="utf-8" ?>
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2">
    <sql-query name="TestGuidQuery">
        <query-param name="guidList" type="System.Array" />
        <return-scalar column="planId" type="System.Guid" />
        <![CDATA[       
            SELECT
                pln.entity_id AS planId
            FROM plans pln
            WHERE 
                pln.entity_id IN (:guidList)
        ]]>
    </sql-query>
</hibernate-mapping>

(Or see attached file: TestGuidQuery.txt.)

NOTES: Create table named "plans", and add single column entity_id of type uniqueidentifier. The names probably don't matter but I am matching them to the attached query.

CAUSE of the Bug: The guidList string is used in both, for named query parameter, and as the SQL Query in clause parameter. This is the trigger of the problem I talked about. If you make the query parameter name different, even changing a single letter or its case, the problem goes away.

Test Code:

Guid[] guids = new[] { new Guid("a11473af-4c0e-4554-b43c-8f922337d407") };

var session = _sessionFactory.OpenSession();
var query = session.GetNamedQuery("TestGuidQuery");
var resultList = query
    .SetParameterList("guidList", guids)
    .List();

foreach (Guid resultGuid in resultList)
{
    Console.WriteLine($"plan Guid={resultGuid}");
}

Query String used for this Example:

<property name="connection.connection_string">Server=serverx\SQLEXPRESS2008R2;Database=dbname;uid=abc;pwd=abc;</property>

I think I gave you enough information to reproduce the error. I would first try it on current version of SQL server and I bet you'll experience the same problem.

Let me know if you cannot reproduce the problem or have any other questions.

fredericDelaporte commented 6 months ago

Does the named query needs the query-param declaration? This declaration tells it the parameter is a System.Array, meaning, an array of object. That is likely the root of the trouble.

The different name trick just causes the declaration to be ignored, further hinting at its uselessness. As for why 5.5 has a trouble with this declaration, that is simple: previously, they were ignored. See #3311. Now, it is taken into account. Your mapping tells this parameter is an object, so it gets treated as binary, now that the parameter declaration is no more ignored. You should fix your mapping: either remove that declaration which does no better than the default behavior with undeclared parameters, or declare the proper type. (Maybe <query-param name="guidList" type="System.Guid[]" /> would work.)

(I cannot even find some documentation about query-param. The named query documentation does not use it, neither for HQL nor for SQL.)

htmlsplash commented 6 months ago

So if I understand you correctly, the "query-param declaration" is optional for named queries? I thought that this was "always" required for any named query parameter. After all, I just copied the documentation for a named query. FYI: This code is like 6-7 years old and was written using older version of Nhibernate.

I get that the type declaration is wrong, I'll fix that.

Thanks for looking into this.

fredericDelaporte commented 6 months ago

Yes they are optional. And they were actually ignored since who knows when, until version 5.5.

I just copied the documentation for a named query.

The reference documentation does not mention the query-param declaration, as detailed by my previous comment links. Out of curiosity, in which documentation have you seen it used?

htmlsplash commented 6 months ago

"Out of curiosity, in which documentation have you seen it used?" - Don't remember, it has been so long ago. Thanks again for pointing this out.

htmlsplash commented 6 months ago

@fredericDelaporte

"(Maybe query-param name="guidList" type="System.Guid[]" would work." - So I followed your instructions with the following results:

1. <query-param name="guidList" type="System.Guid[]" /> - Did not work to my surprise, the Guid array is still treated as binary parameter.

2. <query-param name="guidList" type="Guid[]" /> -Worked fine without using System namespace.

3. Omitting query parameter name also worked.

  1. NOTE: This also works <query-param name="guidList" type="System.Collections.IEnumerable" />

  2. But also this works, which at minimum should throw an exception (the type specified is clearly wrong but it is just ignored; This silent treatment of user mistake in the mapping can lead to a problem later with future versions?):

<query-param name="guidList" type="asfasddf" />

So, the number 1 example still produces incorrect results. In my opinion that's a bug. What do you think? Number 5 example also surprised me and makes me wonder if in number 2, 4 examples the type is also ignored or used??

fredericDelaporte commented 6 months ago

I think 2, 3, and 4 are considered invalid and ignored, somehow. The parameter list case is a special case. It could be that the valid way to type it is to declare the element type, not the list type. So, this:

<query-param name="guidList" type="System.Guid" />

Or use the NHibernate type name:

<query-param name="guidList" type="Guid" />

When you have the parameter value converted to binary, it is likely happening because NHibernate has fallback to its last bet for the type, Serializable, which arrays are.

The point here is really to check, what value declaring the parameter has? It has some when NHibernate cannot guess properly the type from the value in all cases, like when the value may be null. That may not happen with a parameter list. Another useful case is when the .Net type alone cannot give all the information about the type, like with string: which length should be used, variable of fixed length, ASCII or Unicode for database which care about that, ... ? Declaring the parameter with a NHibernate type could give this additional information. But with a Guid, there is no such additional information to provide. So, it does not look to me there is any value in explicitly declaring the parameter for your case.

htmlsplash commented 6 months ago

So if understand you correctly, avoid using "query-param" ? What is the point of having it in v5.5 then? From the examples above, using the "query-param" is a bad practice since you (as the user) never know what's going behind the scene. If nhibernate just silently ignores your type, you will never know until it manifests itself as run-time problem; That's the worst case scenario for any developer and encourages logical bugs which are hard to trace down. At the minimum, if the user specified a type parameter and nhibernate ignored it, then there should be an exception.

"It could be that the valid way to type it is to declare the element type, not the list type" - Okay, and how would I know it is using my declaration??? If the current implementation silently ignores it.

fredericDelaporte commented 6 months ago

Avoid using it if useless for your case. I have given examples where it is useful. Your case is not among them.

htmlsplash commented 6 months ago

Avoid using it if useless for your case. I have given examples where it is useful. Your case is not among them.

Okay, still not a great implementation to say the least and being polite. I still don't understand of how silently ignoring user declared types is a good idea.

fredericDelaporte commented 6 months ago

Throwing would be better if it was a new feature. That is an old legacy thing seemingly forgiving bad typing in most cases. Changing its behavior for no more forgiving unrecognized types could be a breaking change for many consumers of NHibernate. If that is not needed to actually fix a functional trouble of the feature for a nominal case, it would have to be done only for a new major version.

htmlsplash commented 6 months ago

Alright ... For now, I have removed declarations of "query-param" for Guid arrays. Way to dangerous. Thanks for the help.