Are there actual tests for this besides the full zip on the issue? How did
you guys investigate this?
On Mon, Oct 15, 2018 at 7:50 AM Guillaume Smet <guillaume.s...@gmail.com>
wrote:
> OK, ping me when you're ready!
>
> On Mon, Oct 15, 2018 at 1:39 PM Steve Ebersole <st...@hibernate.org>
> wrote:
>
>> Let's discuss on Hip Chat in a few after I have woken up and had some
>> coffee :)
>>
>>
>> On Mon, Oct 15, 2018, 6:02 AM Guillaume Smet <guillaume.s...@gmail.com>
>> wrote:
>>
>>> Hi,
>>>
>>> We discussed a bit more with Fabio on Friday and we ended up discovering
>>> that we have an issue with most Expressions that contain nested
>>> Expressions.
>>>
>>> The Renderable contract is defined with these 3 methods:
>>> String render(RenderingContext renderingContext);
>>>
>>> default String renderProjection(RenderingContext renderingContext) {
>>> return render( renderingContext );
>>> }
>>>
>>> default String renderGroupBy(RenderingContext renderingContext) {
>>> return render( renderingContext );
>>> }
>>>
>>> The issue is that most of the Expressions containting nested Expressions
>>> only implement render().
>>>
>>> This means that you basically do the following:
>>> call renderProjection() -> expression1 only implement render() -> we call
>>> render() on the nested expressions instead of renderProjection().
>>>
>>> One possible workaround is to do what was done in SearchedCaseExpression
>>> for all the Expressions containing nested expressions (e.g. implement
>>> the 3
>>> methods and have a private method taking a lambda to propagate the
>>> variation).
>>>
>>> You can see an example of this change here:
>>>
>>> https://github.com/hibernate/hibernate-orm/pull/2568/files#diff-8fcc837a76b1af31f11e16a01332fd1dR96
>>> .
>>>
>>> Note that it has to be done for *all* the Expressions containing nested
>>> expressions. Either that or we simplify the Renderable contract to have
>>> only one render() method and a parameter defining the context. That would
>>> allow to avoid all these changes.
>>>
>>> Thoughts?
>>>
>>> --
>>> Guillaume
>>>
>>> On Thu, Oct 11, 2018 at 4:01 PM Guillaume Smet <guillaume.s...@gmail.com
>>> >
>>> wrote:
>>>
>>> > Hi,
>>> >
>>> > We have an interesting test case here
>>> > https://hibernate.atlassian.net/browse/HHH-13001 about the use of
>>> > LiteralHandlingMode with the criteria builder. It turns out that the
>>> > problem is a bit more general than that.
>>> >
>>> > Let's take this (not so useful) example:
>>> > query.multiselect(
>>> > document.get( "id" ),
>>> > cb.sum( cb.parameter( Long.class ) ) )
>>> > .groupBy( document.get( "id" ) );
>>> >
>>> > It will lead to a NPE when trying to determine the return type of the
>>> sum
>>> > in:
>>> >
>>> >
>>> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/dialect/function/StandardAnsiSqlAggregationFunctions.java#L157
>>> >
>>> > The fact is that we totally lose the type information along the way.
>>> >
>>> > I don't know if it's something addressed in 6 and if we should try to
>>> fix
>>> > it in 5.4. In any case, I think having a workaround would be nice.
>>> >
>>> > There are a few paths we could follow:
>>> > 1/ at least throw a more meaningful error and provide a workaround. I
>>> > thought about forcing the use of the cast function but it doesn't work
>>> as
>>> > we have an optimization not adding the cast function is the type is
>>> > corresponding (see
>>> >
>>> https://github.com/hibernate/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/query/criteria/internal/expression/ExpressionImpl.java#L35
>>> ).
>>> > If we remove this one and provide a helpful error message, that could
>>> help.
>>> > Note that I don't know if it's just an optimization or something
>>> mandated
>>> > by the spec itself.
>>> >
>>> > 2/ we could try to add a cast automatically for these functions
>>> needing a
>>> > proper type only when strictly necessary e.g. when you have a
>>> parameter (or
>>> > a LiteralExpression transformed to a parameter). That would mean
>>> adding a
>>> > method ExpressionImplementor (and change all our code to use
>>> > ExpressionImplementor as it currently uses the JPA version mostly
>>> > everywhere). That would be more transparent for the user but clearly a
>>> > significant amount of (dumb) work/changes. And probably a nightmare to
>>> > merge in 6 if this area has changed.
>>> >
>>> > Thoughts?
>>> >
>>> > --
>>> > Guillaume
>>> >
>>> _______________________________________________
>>> hibernate-dev mailing list
>>> hibernate-dev@lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>>
>>
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
