Skip to content

Fix QueryOver Coalesce projection in Select #2246

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 20, 2019
Merged

Fix QueryOver Coalesce projection in Select #2246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f223574
Fix QueryOver Coalesce projection in Select
bahusoid Oct 14, 2019
6fb0a62
Fix test
bahusoid Oct 14, 2019
da83e28
Handle null return type
bahusoid Oct 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/NHibernate.Test/Async/Criteria/Lambda/ProjectIntegrationFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,5 +134,43 @@ var actual
Assert.That((int) actual[1], Is.EqualTo(2), "distinct count by name");
}
}

[Test]
public async Task ProjectionCanCoalesceInSelectAsync()
{
using (var s = OpenSession())
using (s.BeginTransaction())
{
var actual
= (await (s.QueryOver<Person>()
.Select(x => x.Age.Coalesce(0))
.Where(x => x.Age == 20)
.ListAsync<int>())).FirstOrDefault();

Assert.That(actual, Is.EqualTo(20));
}
}

//NH-2983
[Test]
public async Task ProjectionSelectSumOnCoalesceAsync()
{
using (var s = OpenSession())
using (s.BeginTransaction())
{
var actual
= (await (s.QueryOver<Person>()
.SelectList(
l =>
l
.SelectSum(xx => xx.Age.Coalesce(0))
.SelectSum(xx => xx.Age.Coalesce(1)))
.Where(x => x.Age == 20)
.ListAsync<object[]>())).FirstOrDefault();

Assert.That(actual[0], Is.EqualTo(20));
Assert.That(actual[1], Is.EqualTo(20));
}
}
}
}
38 changes: 38 additions & 0 deletions src/NHibernate.Test/Criteria/Lambda/ProjectIntegrationFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,5 +123,43 @@ var actual
Assert.That((int) actual[1], Is.EqualTo(2), "distinct count by name");
}
}

[Test]
public void ProjectionCanCoalesceInSelect()
{
using (var s = OpenSession())
using (s.BeginTransaction())
{
var actual
= s.QueryOver<Person>()
.Select(x => x.Age.Coalesce(0))
.Where(x => x.Age == 20)
.List<int>().FirstOrDefault();

Assert.That(actual, Is.EqualTo(20));
}
}

//NH-2983
[Test]
public void ProjectionSelectSumOnCoalesce()
{
using (var s = OpenSession())
using (s.BeginTransaction())
{
var actual
= s.QueryOver<Person>()
.SelectList(
l =>
l
.SelectSum(xx => xx.Age.Coalesce(0))
.SelectSum(xx => xx.Age.Coalesce(1)))
.Where(x => x.Age == 20)
.List<object[]>().FirstOrDefault();

Assert.That(actual[0], Is.EqualTo(20));
Assert.That(actual[1], Is.EqualTo(20));
}
}
}
}
6 changes: 3 additions & 3 deletions src/NHibernate.Test/Criteria/Lambda/RestrictionsFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,9 @@ public void FunctionExtensions()
.Add(Restrictions.Eq(Projections.SqlFunction("substring", NHibernateUtil.String, Projections.Property("Name"), Projections.Property("Age"), Projections.Constant(2)), "te"))
.Add(Restrictions.Eq(Projections.SqlFunction("locate", NHibernateUtil.String, Projections.Constant("e"), Projections.Property("Name"), Projections.Constant(1)), 2))
.Add(Restrictions.Eq(Projections.SqlFunction("locate", NHibernateUtil.String, Projections.Constant("e"), Projections.Property("Name"), Projections.Property("Age")), 2))
.Add(Restrictions.Eq(Projections.SqlFunction("coalesce", NHibernateUtil.Object, Projections.Property("Name"), Projections.Constant("not-null-val")), "test"))
.Add(Restrictions.Eq(Projections.SqlFunction("coalesce", NHibernateUtil.Object, Projections.Property("Name"), Projections.Property("Nickname")), "test"))
.Add(Restrictions.Eq(Projections.SqlFunction("coalesce", NHibernateUtil.Object, Projections.Property("NullableIsParent"), Projections.Constant(true)), true))
.Add(Restrictions.Eq(new SqlFunctionProjection("coalesce", Projections.Property("Name"), Projections.Property("Name"), Projections.Constant("not-null-val")), "test"))
.Add(Restrictions.Eq(new SqlFunctionProjection("coalesce", Projections.Property("Name"), Projections.Property("Name"), Projections.Property("Nickname")), "test"))
.Add(Restrictions.Eq(new SqlFunctionProjection("coalesce", Projections.Property("NullableIsParent"), Projections.Property("NullableIsParent"), Projections.Constant(true)), true))
.Add(Restrictions.Eq(Projections.SqlFunction("concat", NHibernateUtil.String, Projections.Property("Name"), Projections.Constant(", "), Projections.Property("Name")), "test, test"))
.Add(Restrictions.Eq(Projections.SqlFunction("mod", NHibernateUtil.Int32, Projections.Property("Height"), Projections.Constant(10)), 0))
.Add(Restrictions.Eq(Projections.SqlFunction("mod", NHibernateUtil.Int32, Projections.Property("Height"), Projections.Property("Age")), 0));
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Criterion/ProjectionsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// <summary>
Expand Down
23 changes: 19 additions & 4 deletions src/NHibernate/Criterion/SqlFunctionProjection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using NHibernate.Dialect.Function;
using NHibernate.Engine;
using NHibernate.SqlCommand;
using NHibernate.Type;
using NHibernate.Util;

namespace NHibernate.Criterion
{
Expand All @@ -16,6 +15,7 @@ public class SqlFunctionProjection : SimpleProjection
private readonly ISQLFunction function;
private readonly string functionName;
private readonly IType returnType;
private readonly IProjection returnTypeProjection;

public SqlFunctionProjection(string functionName, IType returnType, params IProjection[] args)
{
Expand All @@ -31,6 +31,13 @@ public SqlFunctionProjection(ISQLFunction function, IType returnType, params IPr
this.args = args;
}

public SqlFunctionProjection(string functionName, IProjection returnTypeProjection, params IProjection[] args)
{
this.functionName = functionName;
this.returnTypeProjection = returnTypeProjection;
this.args = args;
}

public override bool IsAggregate
{
get { return false; }
Expand Down Expand Up @@ -107,10 +114,18 @@ private static SqlString GetProjectionArgument(ICriteriaQuery criteriaQuery, ICr
}

public override IType[] GetTypes(ICriteria criteria, ICriteriaQuery criteriaQuery)
{
var type = GetReturnType(criteria, criteriaQuery);
return type != null ? new[] {type} : Array.Empty<IType>();
}

private IType GetReturnType(ICriteria criteria, ICriteriaQuery criteriaQuery)
{
ISQLFunction sqlFunction = GetFunction(criteriaQuery);
IType type = sqlFunction.ReturnType(returnType, criteriaQuery.Factory);
return new IType[] {type};

var resultType = returnType ?? returnTypeProjection?.GetTypes(criteria, criteriaQuery).FirstOrDefault();
Copy link
Member

@fredericDelaporte fredericDelaporte Oct 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@fredericDelaporte fredericDelaporte Oct 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return sqlFunction.ReturnType(resultType, criteriaQuery.Factory);
}

public override TypedValue[] GetTypedValues(ICriteria criteria, ICriteriaQuery criteriaQuery)
Expand Down