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

[AbstractJPASQLQuery] Using the same expression multiple times in a query projection will crash when using Hibernate #910

Closed
yrodiere opened this Issue Aug 25, 2014 · 7 comments

Comments

Projects
None yet
3 participants
@yrodiere

Hibernate doesn't support calling the SQLQuery#addScalar or SQLQuery#addEntity multiple times with the same alias. The call itself will work, but further pre-processing will throw an exception :

javax.persistence.PersistenceException: org.hibernate.loader.custom.NonUniqueDiscoveredSqlAliasException: Encountered a duplicated sql alias [col__1_9] during auto-discovery of a native-sql query
    at org.hibernate.jpa.spi.AbstractEntityManagerImpl.convert(AbstractEntityManagerImpl.java:1763)
    at org.hibernate.jpa.spi.AbstractEntityManagerImpl.convert(AbstractEntityManagerImpl.java:1677)
    at org.hibernate.jpa.internal.QueryImpl.getSingleResult(QueryImpl.java:524)
    at com.mysema.query.jpa.sql.AbstractJPASQLQuery.getSingleResult(AbstractJPASQLQuery.java:223)
    at com.mysema.query.jpa.sql.AbstractJPASQLQuery.uniqueResult(AbstractJPASQLQuery.java:279)
    at com.mysema.query.jpa.sql.AbstractJPASQLQuery.uniqueResult(AbstractJPASQLQuery.java:273)
    at com.mysema.query.support.ProjectableQuery.singleResult(ProjectableQuery.java:75)
    at  [...]
Caused by: org.hibernate.loader.custom.NonUniqueDiscoveredSqlAliasException: Encountered a duplicated sql alias [col__1_9] during auto-discovery of a native-sql query
    at org.hibernate.loader.custom.CustomLoader.validateAliases(CustomLoader.java:524)
    at org.hibernate.loader.custom.CustomLoader.autoDiscoverTypes(CustomLoader.java:501)
    at org.hibernate.loader.Loader.getResultSet(Loader.java:2073)
    at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:1862)
    at org.hibernate.loader.Loader.executeQueryStatement(Loader.java:1838)
    at org.hibernate.loader.Loader.doQuery(Loader.java:909)
    at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:354)
    at org.hibernate.loader.Loader.doList(Loader.java:2553)
    at org.hibernate.loader.Loader.doList(Loader.java:2539)
    at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2369)
    at org.hibernate.loader.Loader.list(Loader.java:2364)
    at org.hibernate.loader.custom.CustomLoader.list(CustomLoader.java:353)
    at org.hibernate.internal.SessionImpl.listCustomQuery(SessionImpl.java:1873)
    at org.hibernate.internal.AbstractSessionImpl.list(AbstractSessionImpl.java:311)
    at org.hibernate.internal.SQLQueryImpl.list(SQLQueryImpl.java:141)
    at org.hibernate.jpa.internal.QueryImpl.list(QueryImpl.java:573)
    at org.hibernate.jpa.internal.QueryImpl.getSingleResult(QueryImpl.java:495)
    ... 116 more

In QueryDSL itself, the problem lies in AbstractJPASQLQuery.
It seems that if we use two Expressions that are equal according to the java definition, they will be given the same alias. Things are handled properly in the select clause generation (only one expression is rendered), but the call to addScalar/addEntity (in AbstractJPASQLQuery, line 122) are made multiple times, which (ultimately) crashes the whole thing.

One way to fix this would be to make this call only once per alias. But this would mean that the result rows (Object[]) returned by Hibernate would miss these values, and we should then pre-process the result rows before passing them as arguments to the factory expression.

Here is the (quick and dirty) fix we applied in our project. I did not provide this as a pull request since it suffers an unacceptable drawback : it works around the com.mysema.query.jpa.QueryHandler and will only work on Hibernate. This is essentially a copy of the AbstractJPASQLQuery#createQuery method, where I ensured to call the SQLQuery#add* methods only once per query and to duplicate the columns in the result rows accordingly. Modified lines/blocks are commented with "// FIX" or "// -------------- FIX".

import java.util.List;
import java.util.Map;
import java.util.Set;

import javax.persistence.EntityManager;
import javax.persistence.Query;

import org.hibernate.jpa.HibernateQuery;
import org.hibernate.transform.ResultTransformer;

import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.mysema.query.QueryMetadata;
import com.mysema.query.jpa.NativeSQLSerializer;
import com.mysema.query.jpa.QueryHandler;
import com.mysema.query.jpa.impl.JPAUtil;
import com.mysema.query.jpa.sql.AbstractJPASQLQuery;
import com.mysema.query.sql.Configuration;
import com.mysema.query.sql.SQLTemplates;
import com.mysema.query.types.Expression;
import com.mysema.query.types.FactoryExpression;
import com.mysema.query.types.FactoryExpressionUtils;

public class FixedJPASQLQuery extends AbstractJPASQLQuery<FixedJPASQLQuery> {

    private final EntityManager entityManager;

    public FixedJPASQLQuery(EntityManager entityManager, SQLTemplates sqlTemplates) {
        super(entityManager, new Configuration(sqlTemplates));
        this.entityManager = entityManager;
    }

    public FixedJPASQLQuery(EntityManager entityManager, Configuration conf) {
        super(entityManager, conf);
        this.entityManager = entityManager;
    }

    public FixedJPASQLQuery(EntityManager entityManager, Configuration conf, QueryHandler queryHandler) {
        super(entityManager, conf, queryHandler);
        this.entityManager = entityManager;
    }

    public FixedJPASQLQuery(EntityManager entityManager, SQLTemplates sqlTemplates, QueryMetadata metadata) {
        super(entityManager, new Configuration(sqlTemplates), metadata);
        this.entityManager = entityManager;
    }

    public FixedJPASQLQuery(EntityManager entityManager, Configuration conf, QueryMetadata metadata) {
        super(entityManager, conf, metadata);
        this.entityManager = entityManager;
    }

    public FixedJPASQLQuery(EntityManager entityManager, Configuration conf, QueryHandler queryHandler, QueryMetadata metadata) {
        super(entityManager, conf, queryHandler, metadata);
        this.entityManager = entityManager;
    }

    @Override
    public FixedJPASQLQuery clone(EntityManager entityManager) {
        FixedJPASQLQuery q = new FixedJPASQLQuery(entityManager, configuration, queryHandler, getMetadata().clone());
        q.clone(this);
        return q;
    }

    @Override
    public Query createQuery(Expression<?>... args) {
        queryMixin.getMetadata().setValidate(false);
        queryMixin.addProjection(args);
        return createQuery(false);
    }

    private Query createQuery(boolean forCount) {
        NativeSQLSerializer serializer = (NativeSQLSerializer) serialize(forCount);
        String queryString = serializer.toString();
        logQuery(queryString);
        List<? extends Expression<?>> projection = queryMixin.getMetadata().getProjection();
        Query query;

        Expression<?> proj = projection.get(0);
        if (!FactoryExpression.class.isAssignableFrom(proj.getClass()) && isEntityExpression(proj)) {
            if (projection.size() == 1) {
                if (queryHandler.createNativeQueryTyped()) {
                    query = entityManager.createNativeQuery(queryString, proj.getType());
                } else {
                    query = entityManager.createNativeQuery(queryString);
                }
            } else {
                throw new IllegalArgumentException("Only single element entity projections are supported");
            }

        } else {
            query = entityManager.createNativeQuery(queryString);
        }
        if (!forCount) {
            Map<Expression<?>, String> aliases = serializer.getAliases();
            if (proj instanceof FactoryExpression) {
                Set<Expression<?>> handledExpressions = Sets.newHashSet(); // FIX
                for (Expression<?> expr : ((FactoryExpression<?>)proj).getArgs()) {
                    if (!handledExpressions.contains(expr)) { // FIX
                        if (isEntityExpression(expr)) {
                            handledExpressions.add(expr); // FIX
                            queryHandler.addEntity(query, extractEntityExpression(expr).toString(), expr.getType());
                        } else if (aliases.containsKey(expr)) {
                            handledExpressions.add(expr); // FIX
                            queryHandler.addScalar(query, aliases.get(expr), expr.getType());
                        }
                    } // FIX
                }
            } else if (isEntityExpression(proj)) {
                queryHandler.addEntity(query, extractEntityExpression(proj).toString(), proj.getType());
            } else if (aliases.containsKey(proj)) {
                queryHandler.addScalar(query, aliases.get(proj), proj.getType());
            }
        }

        if (lockMode != null) {
            query.setLockMode(lockMode);
        }
        if (flushMode != null) {
            query.setFlushMode(flushMode);
        }

        for (Map.Entry<String, Object> entry : hints.entries()) {
            query.setHint(entry.getKey(), entry.getValue());
        }


        // set constants
        JPAUtil.setConstants(query, serializer.getConstantToLabel(), queryMixin.getMetadata().getParams());

        FactoryExpression<?> wrapped = projection.size() > 1 ? FactoryExpressionUtils.wrap(projection) : null;
        if ((projection.size() == 1 && projection.get(0) instanceof FactoryExpression) || wrapped != null) {
            Expression<?> expr = wrapped != null ? wrapped : projection.get(0);

            // ---------------------------------------------- FIX
            // (copied and adapted from HibernateHandler#transform)
            if (query instanceof HibernateQuery) {
                final FactoryExpression<?> factoryExpr = (FactoryExpression<?>)expr;
                ResultTransformer transformer = new FixedFactoryResultTransformer(factoryExpr, serializer.getAliases());
                ((HibernateQuery)query).getHibernateQuery().setResultTransformer(transformer);
            } else {
                this.projection = (FactoryExpression<?>)projection.get(0);
                if (wrapped != null) {
                    this.projection = wrapped;
                    getMetadata().clearProjection();
                    getMetadata().addProjection(wrapped);
                }
            }
            // ---------------------------------------------- END OF FIX
        }

        return query;
    }

// ---------------------------------------------- FIX
    /*
     * A FactoryResultTransformer handling duplication of result columns for expressions that are used multiple times.
     */
    private static class FixedFactoryResultTransformer implements ResultTransformer {
        private static final long serialVersionUID = -3625957233853100239L;

        private final transient FactoryExpression<?> projection;

        private final transient Map<Expression<?>, String> expressionToAlias;

        public FixedFactoryResultTransformer(FactoryExpression<?> projection, Map<Expression<?>, String> aliases) {
            super();
            this.projection = projection;
            this.expressionToAlias = aliases;
        }

        @SuppressWarnings("rawtypes")
        @Override
        public List transformList(List collection) {
            return collection;
        }

        @Override
        public Object transformTuple(Object[] tuple, String[] aliases) {
            if (projection.getArgs().size() < tuple.length) {
                Object[] shortened = new Object[projection.getArgs().size()];
                System.arraycopy(tuple, 0, shortened, 0, shortened.length);
                tuple = shortened;
            }
            Map<String, Object> aliasToValue = Maps.newHashMap();
            for (int i = 0 ; i < tuple.length ; ++i) {
                aliasToValue.put(aliases[i], tuple[i]);
            }
            List<Object> actualTuple = Lists.newArrayList();
            for (Expression<?> expression : projection.getArgs()) {
                String alias = expressionToAlias.get(expression);
                actualTuple.add(aliasToValue.get(alias));
            }
            return projection.newInstance(Iterables.toArray(actualTuple, Object.class));
        }

    }
// ---------------------------------------------- END OF FIX

}
@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 27, 2014

Member

Thanks for the bug report. Could you provide a few example queries that reproduce the problem?

Member

timowest commented Aug 27, 2014

Thanks for the bug report. Could you provide a few example queries that reproduce the problem?

@yrodiere

This comment has been minimized.

Show comment
Hide comment
@yrodiere

yrodiere Aug 28, 2014

I'll submit a pull request with a test case (no fix, I'm affraid) next week.

I'll submit a pull request with a test case (no fix, I'm affraid) next week.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Aug 29, 2014

Member

Thanks

Member

timowest commented Aug 29, 2014

Thanks

yrodiere added a commit to openwide-java/querydsl that referenced this issue Sep 4, 2014

@yrodiere

This comment has been minimized.

Show comment
Hide comment
@yrodiere

yrodiere Sep 4, 2014

Here it is. Thanks a lot for your support !

yrodiere commented Sep 4, 2014

Here it is. Thanks a lot for your support !

@Shredder121

This comment has been minimized.

Show comment
Hide comment
@Shredder121

Shredder121 Sep 4, 2014

Member

For reference, the generated SQL:
select count(cat.id) as col__1_1, count(cat.id) as col__1_2 from animal_ cat

while preprocessing in NativeSQLSerializer#serialize at line 102, aliases that have the same hashcode are overwritten, so only the last alias (col__1_2) will remain.

Member

Shredder121 commented Sep 4, 2014

For reference, the generated SQL:
select count(cat.id) as col__1_1, count(cat.id) as col__1_2 from animal_ cat

while preprocessing in NativeSQLSerializer#serialize at line 102, aliases that have the same hashcode are overwritten, so only the last alias (col__1_2) will remain.

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 4, 2014

Member

@Shredder121 You are right. I think if create the alias mapping in a different way, this could be fixed.

Member

timowest commented Sep 4, 2014

@Shredder121 You are right. I think if create the alias mapping in a different way, this could be fixed.

@timowest timowest added the bug label Sep 6, 2014

@timowest timowest added this to the 3.5.0 milestone Sep 7, 2014

@timowest timowest closed this in #927 Sep 8, 2014

@timowest

This comment has been minimized.

Show comment
Hide comment
@timowest

timowest Sep 30, 2014

Member

Released in 3.5.0

Member

timowest commented Sep 30, 2014

Released in 3.5.0

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