Incorrect SQL when comparing a guid and string column in Sql Server

[micmerchant]

Hello,

we are currently updating from an old NHibernate 4 version to the current NHibernate 5 (5.2.3) version and are facing some issues.

One of our models contains a nullable string column which is joined on a column with a different type of an other table by using ToString() within the join, e.g:

q =
from o in db.Orders
join s in db.Shippers
	on o.ShippedTo equals s.Reference.ToString()
where o.ShippedTo != null
select o.OrderId;

This worked in NHibernate 4 but fails in NHibernate 5. Here is a logged query example from our production code with
Join-Exception.txt
NHibernate 4 and NHibernate 5.

-- NHibernate 4

select invruletem0_.ID as col_0_0_
from   InvRuleTemplate invruletem0_,
       InvRule invrule1_
where  (invruletem0_.Mandator & -1 /* @p0 */) != 0
       and (invrule1_.Mandator & -1 /* @p0 */) != 0
       and invrule1_.TargetID = cast(invruletem0_.ID as nvarchar(50))
       and (invrule1_.TargetID is not null)

-- NHibernate 5

select invruletem0_.ID as col_0_0_
from   InvRuleTemplate invruletem0_,
       InvRule invrule1_
where  (invruletem0_.Mandator & -1 /* @p0 */) != 0
       and (invrule1_.Mandator & -1 /* @p0 */) != 0
       and invrule1_.TargetID = invruletem0_.ID
       and (invrule1_.TargetID is not null)

It looks like that the .ToString() cast isn't translated into SQL anymore. This only happens when the string column is nullable. I guess the query translator recognizes that a cast isn't necessary for not nullable string columns.

This problem is reproducable with a unit test with the northwind domainmodel:

[TestFixture]
public class LinqQuerySamples : LinqTestCase
{
	...

	[Category("JOIN")]
	[Test(Description = "This sample shows how to construct a join where one side is a nullable string and the other side a not nullable guid.")]
	public void DLinqJoin11()
	{
		var q =
			from o in db.Orders
			join s in db.Shippers
				on o.ShippedTo equals s.Reference.ToString()
			where o.ShippedTo != null
			select o.OrderId;

		ObjectDumper.Write(q);
	}
	
	....
}

I've also attached the unit test exception.

Thx in advance. Best regards!
Join-Exception.txt

[fredericDelaporte]

May be related to #2029 (or maybe have the same root cause), and could be fixed by #2036 (which does also some changes on a path not specific to sum). @maca88, may you check that?

@micmerchant, if you can tell from which minor version the trouble has appeared (only 5.2, or 5.1 too, or 5.0 too, or 4.1 too if you were using 4.0), we would appreciate it.

[maca88]

I've checked the issue and #2036 does not solve the problem. The issue was introduced with #1856 by adding the following lines:

nhibernate-core/src/NHibernate/Linq/Functions/StringGenerator.cs

Lines 329 to 332 in 39bf753

	if (targetObject.Type == _guidType)
	{
	return treeBuilder.MethodCall("strguid", visitor.Visit(targetObject).AsExpression());
	}

I will analyze the mentioned PR and try to find a general solution.

[micmerchant]

You are right. This only happens in context of Guids.

@fredericDelaporte We are updating from NHibernate 4.0.3.

[micmerchant]

What about simply adding a custom function to the MsSql2000Dialect.cs class? e.g.

public class MsSql2000Dialect : Dialect
{
	...
	protected virtual void RegisterFunctions()
	{
		...
		// after
		RegisterFunction("str", new SQLFunctionTemplate(NHibernateUtil.String, "cast(?1 as nvarchar(50))"));
		// new one
		RegisterFunction("strguid", new SQLFunctionTemplate(NHibernateUtil.String, "cast(?1 as nvarchar(50))"));
		...
	}
	...
}

which overrides the one from the general Dialect.cs class?

RegisterFunction("strguid", new SQLFunctionTemplate(NHibernateUtil.String, "?1"));

[lillo42]

@micmerchant Just to know, Which version of MS SQL are you using?

@maca88 Probably is missing the code this code, on class MsSql2000Dialect:

RegisterFunction("strguid", new SQLFunctionTemplate(NHibernateUtil.String, "cast(?1 as nvarchar(50))"));

Like @micmerchant mention,

[micmerchant]

@lillo42
We support MsSql Server 2012 upwards.

[maca88]

Indeed, registering strguid function for MsSql2000Dialect is the correct solution. #1856 didn't register a custom function for it because the added test which uses a string parameter does not need it as Sql Server implicitly converts the parameter to uniqueidentifier. #2042 adds the missing registration and an additional test for it.

[fredericDelaporte]

Regression in 5.2.0, as ascertained by @maca88 in #2042. You may use 5.1.x instead till 5.2.5 is released, if this regression is a blocker for you.

[micmerchant]

Well, we are using a custom NHibernate build anyways, which means that I can apply this fix directly in our code base. So I'm fine with adding it in the 5.2.5 release.

[micmerchant]

@maca88
I must honestly say that i was confused about your added unit test

[Test]
public void CompareStringColumnWithGuidToString()
{
	using (var session = OpenSession())
	{
		var list = session.Query<Entity>()
						  .Where(x => x.Id.ToString() == x.Name)
						  .ToList();

		Assert.That(list, Has.Count.EqualTo(0));
	}
}

and the simplicity of reproduction of the bug. I thought this was all about using guids and strings in a join context. But i was misleaded by my own unit tests. I had a similar one, which didn't fail.

[Test(Description = "This test checks a string cast of a not nullable guid column in the where condition.")]
public void NotNullableGuidToStringWithEqualsOperatorComparisonInWhereConditionMethodSyntaxTest1()
{
	INHibernateSession session = NHibernateSessionManager.Instance.GetSession();

	string vehicleId = VehicleInitializer.SomeVehicleId.ToString();

	var result = session.Query<IVehicle>()
						.Where(v => v.ID.ToString() == vehicleId)
						.ToList();
}

It didn't fail because the sequal server of course supports this kind of comparison, because the other side is a Guid already. By changing vehicleId to a string e.g. "Test", it would fail.

So the query of the unit test above, gets translated into this:

select vehicle0_.ID                     as id1_631_,
       ....
from   Vehicle vehicle0_
where  (vehicle0_.[Mandator] & -1 /* @p0 */) != 0
       and cast(vehicle0_.ID as nvarchar(50)) = 'eca27f3a-42bb-4b72-a481-ad8f8a40ce69' /* @p1 - cle0_.ID as nvarchar(50)) */

And here I'm asking myself now, if it wouldn't be an improvement if the query translator would check the type of both sides of the comparison and then decides if a string cast is necessary at all. E.g. if the constant is already a valid Guid then a cast wouldn't be necessary.

What do you think?

[maca88]

wouldn't be an improvement if the query translator would check the type of both sides of the comparison and then decides if a string cast is necessary at all

Yes it would be an improvement, maybe by introducing a special logic that would avoid common "mistakes" made by the user like the example that you provided, which in this case would produce the same sql as the following query:

var vehicleId = VehicleInitializer.SomeVehicleId;
var result = session.Query<IVehicle>()
                    .Where(v => v.ID == vehicleId)
                    .ToList();

of course the logic would need to be dialect specific.

[micmerchant]

From that point of view, you are absolutly right :) It would only hide a mistake in the written query code, which you would never recognize e.g. via the NHibernateProfiler. And i can't come up with a better example.

Thx a lot.