Number conversion for native query fails with nested factory expressions #626

Closed
TuomasKiviaho opened this Issue Jan 13, 2014 · 15 comments

Comments

Projects
None yet
2 participants
@TuomasKiviaho
Contributor

TuomasKiviaho commented Jan 13, 2014

Number conversion is not done for expressions such as new Tuple(bean1, bean2) will fail, because factory expressions are not fully expanded prior checking whether the type is number or enum

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 14, 2014

Member

Could you try again with the latest SNAPSHOT?

Member

timowest commented Jan 14, 2014

Could you try again with the latest SNAPSHOT?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 16, 2014

Contributor

Thanks for the fix

It was also educational because I've wondered what you can do with templates.

There's still a slight problem. I'm using JPA SQL against non entity beans. Code only works when @entity path is present.

Contributor

TuomasKiviaho commented Jan 16, 2014

Thanks for the fix

It was also educational because I've wondered what you can do with templates.

There's still a slight problem. I'm using JPA SQL against non entity beans. Code only works when @entity path is present.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 16, 2014

Member

@TuomasKiviaho Ok, I will see if I can replace the @Entity requirement.

Member

timowest commented Jan 16, 2014

@TuomasKiviaho Ok, I will see if I can replace the @Entity requirement.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 16, 2014

Contributor

I stepped through the code by skipping the check, but next problem is that there seems to be now multiple aliases. Same column name appears in both beans and hence Hibernate's validation over aliases at CustomLoader.autoDiscoverTypes fails. Could this be somehow related to template usage?

Contributor

TuomasKiviaho commented Jan 16, 2014

I stepped through the code by skipping the check, but next problem is that there seems to be now multiple aliases. Same column name appears in both beans and hence Hibernate's validation over aliases at CustomLoader.autoDiscoverTypes fails. Could this be somehow related to template usage?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 16, 2014

Contributor

What would happen if the problematic path is not inside EntityPath. Or that the entitypath is deeply nested due to some elaborate nested group by transformation. Then we'd be at square one because - if I understood correctly - only one extra level is now to be checked.

I got the case working with the following code but I might have missed something (ProjectionRole for instance).

public static <RT> Expression<RT> convertForNativeQuery(Expression<RT> expr) {
        if (Number.class.isAssignableFrom(expr.getType())) {
            return new NumberConversion<RT>(expr);
        } else if (Enum.class.isAssignableFrom(expr.getType())) {
            return new EnumConversion<RT>(expr);
        } else if (expr instanceof FactoryExpression) {
            FactoryExpression<RT> factorye = FactoryExpressionUtils.wrap((FactoryExpression<RT>)expr);
            List<Expression<?>> args = factorye.getArgs();
            for (Expression<?> e : args) {
                if (Number.class.isAssignableFrom(e.getType()) || 
                    Enum.class.isAssignableFrom(e.getType())) {
                    return new NumberConversions<RT>(factorye);
                }
            }
        }
        return expr;
    }
Contributor

TuomasKiviaho commented Jan 16, 2014

What would happen if the problematic path is not inside EntityPath. Or that the entitypath is deeply nested due to some elaborate nested group by transformation. Then we'd be at square one because - if I understood correctly - only one extra level is now to be checked.

I got the case working with the following code but I might have missed something (ProjectionRole for instance).

public static <RT> Expression<RT> convertForNativeQuery(Expression<RT> expr) {
        if (Number.class.isAssignableFrom(expr.getType())) {
            return new NumberConversion<RT>(expr);
        } else if (Enum.class.isAssignableFrom(expr.getType())) {
            return new EnumConversion<RT>(expr);
        } else if (expr instanceof FactoryExpression) {
            FactoryExpression<RT> factorye = FactoryExpressionUtils.wrap((FactoryExpression<RT>)expr);
            List<Expression<?>> args = factorye.getArgs();
            for (Expression<?> e : args) {
                if (Number.class.isAssignableFrom(e.getType()) || 
                    Enum.class.isAssignableFrom(e.getType())) {
                    return new NumberConversions<RT>(factorye);
                }
            }
        }
        return expr;
    }

timowest added a commit that referenced this issue Jan 16, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 16, 2014

Member

I stepped through the code by skipping the check, but next problem is that there seems to be now multiple aliases. Same column name appears in both beans and hence Hibernate's validation over aliases at CustomLoader.autoDiscoverTypes fails. Could this be somehow related to template usage?

Is it a case like described here http://docs.jboss.org/hibernate/orm/4.3/manual/en-US/html/ch18.html#d5e8292

For only scalar arguments it should work.

What would happen if the problematic path is not inside EntityPath. Or that the entitypath is deeply nested due to some elaborate nested group by transformation. Then we'd be at square one because - if I understood correctly - only one extra level is now to be checked.

Could you provide an example? Your version of convertForNativeQuery seems to be based on code before ff719c4

Member

timowest commented Jan 16, 2014

I stepped through the code by skipping the check, but next problem is that there seems to be now multiple aliases. Same column name appears in both beans and hence Hibernate's validation over aliases at CustomLoader.autoDiscoverTypes fails. Could this be somehow related to template usage?

Is it a case like described here http://docs.jboss.org/hibernate/orm/4.3/manual/en-US/html/ch18.html#d5e8292

For only scalar arguments it should work.

What would happen if the problematic path is not inside EntityPath. Or that the entitypath is deeply nested due to some elaborate nested group by transformation. Then we'd be at square one because - if I understood correctly - only one extra level is now to be checked.

Could you provide an example? Your version of convertForNativeQuery seems to be based on code before ff719c4

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 21, 2014

Contributor

Here's a quick n' dirty real life example that made me wonder about this.

If problematic path in this case would be the id=7605, then it would be processed because type is numeric. What if the operation id=7258 is not a numeric but a string (Coalesce.asString for instance).

expr    FactoryExpressionUtils$FactoryExpressionAdapter<T>  (id=7243)   
    'expr' referenced from: 
    args    ArrayList<E>  (id=7247) 
        'args' referenced from: 
        elementData Object[12]  (id=7249)   
            'elementData' referenced from:  
            ...
            [9] NumberPath<T>  (id=7257)    
            [10]    SimpleOperation<T>  (id=7258)   
                mixin   OperationImpl<T>  (id=7599) 
                    'mixin' referenced from:    
                    args    RegularImmutableList<E>  (id=7600)  
                        'args' referenced from: 
                        array   Object[2]  (id=7604)    
                            'array' referenced from:    
                            [0] NumberPath<T>  (id=7605)    
                            [1] ConstantImpl<T>  (id=7606)
    type    Class<T> (com.mysema.query.Tuple) (id=1359) 

Contributor

TuomasKiviaho commented Jan 21, 2014

Here's a quick n' dirty real life example that made me wonder about this.

If problematic path in this case would be the id=7605, then it would be processed because type is numeric. What if the operation id=7258 is not a numeric but a string (Coalesce.asString for instance).

expr    FactoryExpressionUtils$FactoryExpressionAdapter<T>  (id=7243)   
    'expr' referenced from: 
    args    ArrayList<E>  (id=7247) 
        'args' referenced from: 
        elementData Object[12]  (id=7249)   
            'elementData' referenced from:  
            ...
            [9] NumberPath<T>  (id=7257)    
            [10]    SimpleOperation<T>  (id=7258)   
                mixin   OperationImpl<T>  (id=7599) 
                    'mixin' referenced from:    
                    args    RegularImmutableList<E>  (id=7600)  
                        'args' referenced from: 
                        array   Object[2]  (id=7604)    
                            'array' referenced from:    
                            [0] NumberPath<T>  (id=7605)    
                            [1] ConstantImpl<T>  (id=7606)
    type    Class<T> (com.mysema.query.Tuple) (id=1359) 

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 21, 2014

Contributor

My case is more or less how the manual describes it. Two tables with identical columns.

Templated version give me somehat the following

select table1.*, table2.* from table1 inner join table2 on ...

whilst my version expands columns automatically

select table1.col1 as "col__1_1" , table2.col1 as "col__1_2" from table1 inner join table2 on ...

PS. Operations as described above are not dealt with my version either.

Contributor

TuomasKiviaho commented Jan 21, 2014

My case is more or less how the manual describes it. Two tables with identical columns.

Templated version give me somehat the following

select table1.*, table2.* from table1 inner join table2 on ...

whilst my version expands columns automatically

select table1.col1 as "col__1_1" , table2.col1 as "col__1_2" from table1 inner join table2 on ...

PS. Operations as described above are not dealt with my version either.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 21, 2014

Member

select table1., table2. from table1 inner join table2 on ...

What Querydsl query is this based on?

If problematic path in this case would be the id=7605, then it would be processed because type is numeric. What if the operation id=7258 is not a numeric but a string (Coalesce.asString for instance).

Numbers and Enums inside operations don't need to be processed, only the top level expressions.

Member

timowest commented Jan 21, 2014

select table1., table2. from table1 inner join table2 on ...

What Querydsl query is this based on?

If problematic path in this case would be the id=7605, then it would be processed because type is numeric. What if the operation id=7258 is not a numeric but a string (Coalesce.asString for instance).

Numbers and Enums inside operations don't need to be processed, only the top level expressions.

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 23, 2014

Contributor

What Querydsl query is this based on?

jpaSqlQuery.from(sTable1).where(...).innerJoin(sTable2).on(...).list(sTable1, sTable2);
Contributor

TuomasKiviaho commented Jan 23, 2014

What Querydsl query is this based on?

jpaSqlQuery.from(sTable1).where(...).innerJoin(sTable2).on(...).list(sTable1, sTable2);
@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 23, 2014

Contributor

I got it working! I seem to have botched the build somehow because it started working with the latest maven snapshots.

Contributor

TuomasKiviaho commented Jan 23, 2014

I got it working! I seem to have botched the build somehow because it started working with the latest maven snapshots.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 23, 2014

Member

So it works with a snapshot?

Member

timowest commented Jan 23, 2014

So it works with a snapshot?

@TuomasKiviaho

This comment has been minimized.

Show comment
Hide comment
@TuomasKiviaho

TuomasKiviaho Jan 23, 2014

Contributor

Yes and thanks for your patience with me.

Contributor

TuomasKiviaho commented Jan 23, 2014

Yes and thanks for your patience with me.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Jan 23, 2014

Member

Great!

Member

timowest commented Jan 23, 2014

Great!

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Feb 8, 2014

Member

Released in 3.3.1

Member

timowest commented Feb 8, 2014

Released in 3.3.1

@timowest timowest closed this Feb 8, 2014

@timowest timowest added this to the 3.3.1 milestone Apr 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment