Fix QueryOver Coalesce projection in Select #2246
Conversation
| @@ -316,7 +316,7 @@ internal static IProjection ProcessCoalesce(MethodCallExpression methodCallExpre | |||
| { | |||
| IProjection property = ExpressionProcessor.FindMemberProjection(methodCallExpression.Arguments[0]).AsProjection(); | |||
| var replaceValueIfIsNull = ExpressionProcessor.FindMemberProjection(methodCallExpression.Arguments[1]); | |||
| return Projections.SqlFunction("coalesce", NHibernateUtil.Object, property, replaceValueIfIsNull.AsProjection()); | |||
| return new SqlFunctionProjection("coalesce", returnTypeProjection: property, property, replaceValueIfIsNull.AsProjection()); | |||
There was a problem hiding this comment.
Other option is to use GuessType here but current fix looks more proper to me (though it requires changes in SqlFunctionProjection). Let me know if you think GuessType is enough here.
| IType type = sqlFunction.ReturnType(returnType, criteriaQuery.Factory); | ||
| return new IType[] {type}; | ||
|
|
||
| var resultType = returnType ?? returnTypeProjection?.GetTypes(criteria, criteriaQuery).FirstOrDefault(); |
There was a problem hiding this comment.
If there are more than one type returned, should it be silently ignored? (Shouldn't SingleOrDefault() be called here?)
And all the null coalesce, OrDefault stuff combined with yielding an IType[] of one element which may as a result be null does not look good. Digging on the GetTypes usage, it seems it ends up being used by calling methods on the elements it contains without checking if they are null or not. (See CriteriaLoader.GetResultRow.)
So either it should yield an empty type array when no type are determined if such an empty array can be a valid return value, or throw for the sake of failing earlier.
This defect was somehow already there, since nothing was checking the sqlFunction return type was not null. But it looks worsened by the change.
The function may have a fixed return type, in which case its argument being null changes nothing, it will return a non-null return type. But otherwise it may return null if we give it null.
There was a problem hiding this comment.
If there are more than one type returned, should it be silently ignored? (Shouldn't SingleOrDefault() be called here?)
Well I don't know. I made it the way I saw somewhere else in Criteria handling. IMHO it's not important until multi-column support is added to ISQLFunction.ReturnType
And all the null coalesce, OrDefault stuff combined with yielding an IType[] of one element which may as a result be null does not look good. ... it seems it ends up being used by calling methods
Not sure I properly understand it. But this logic ends up in sqlFunction.ReturnType which expects single IType and returns single IType. And I see no restrictions why null should be forbidden for sqlFunction.ReturnType - function return type is known beforehand in most cases . So both returnType and returnTypeProjection should be nullable.
There was a problem hiding this comment.
The trouble is, GetTypes returned array is used as if its content could never be null, without null checks, see CriteriaLoader.GetResultRow. So potentially yielding new IType [] { null } looks plain wrong.
The PR introduces more possibilities for this to happen.
I think is should either yield an empty array, or throw. It should not yield an array containing null. Or is there something I have missed which would allow this returned value to be actually supported?
There was a problem hiding this comment.
No but empty type collection also doesn't make much sense for using in Select projection... Ok - I've added null handling just in case.
Fixes #1107
In fact simple
Select(x => x.Value.Coalesce(0))was broken in QueryOver due to usingNHibernateUtil.Objectas return type forcoalescefunction which is something completely unrelated: