Skip to content
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.

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

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

Closed
TuomasKiviaho opened this issue Jan 13, 2014 · 15 comments
Closed
Milestone

Comments

@TuomasKiviaho
Copy link
Contributor

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
Copy link
Member

Could you try again with the latest SNAPSHOT?

@TuomasKiviaho
Copy link
Contributor Author

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
Copy link
Member

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

@TuomasKiviaho
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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

@TuomasKiviaho
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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.

@TuomasKiviaho
Copy link
Contributor Author

What Querydsl query is this based on?

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

@TuomasKiviaho
Copy link
Contributor Author

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

@timowest
Copy link
Member

So it works with a snapshot?

@TuomasKiviaho
Copy link
Contributor Author

Yes and thanks for your patience with me.

@timowest
Copy link
Member

Great!

@timowest
Copy link
Member

timowest commented Feb 8, 2014

Released in 3.3.1

@timowest timowest closed this as completed 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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants