HHH-8318 - Problem determining qualifier to use for column names from HQL query parser in certain circumstances

Author: sebersole

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/DotNode.java

@@ -38,6 +38,7 @@
 import org.hibernate.internal.util.StringHelper;
 import org.hibernate.persister.collection.QueryableCollection;
 import org.hibernate.persister.entity.EntityPersister;
+import org.hibernate.persister.entity.Queryable;
 import org.hibernate.sql.JoinFragment;
 import org.hibernate.sql.JoinType;
 import org.hibernate.type.CollectionType;
@@ -282,19 +283,36 @@ private void dereferenceCollection(CollectionType collectionType, boolean implic
 		String propName = getPath();
 		FromClause currentFromClause = getWalker().getCurrentFromClause();
 
-		if ( getWalker().getStatementType() != SqlTokenTypes.SELECT && indexed && classAlias == null ) {
-			// should indicate that we are processing an INSERT/UPDATE/DELETE
-			// query with a subquery implied via a collection property
-			// function. Here, we need to use the table name itself as the
-			// qualification alias.
-			// TODO : verify this works for all databases...
-			// TODO : is this also the case in non-"indexed" scenarios?
-			String alias = getLhs().getFromElement().getQueryable().getTableName();
-			columns = getFromElement().toColumns( alias, propertyPath, false, true );
+		// determine whether we should use the table name or table alias to qualify the column names...
+		// we need to use the table-name when:
+		//		1) the top-level statement is not a SELECT
+		//		2) the LHS FromElement is *the* FromElement from the top-level statement
+		//
+		// there is a caveat here.. if the update/delete statement are "multi-table" we should continue to use
+		// the alias also, even if the FromElement is the root one...
+		//
+		// in all other cases, we should use the table alias
+		final FromElement lhsFromElement = getLhs().getFromElement();
+		if ( getWalker().getStatementType() != SqlTokenTypes.SELECT ) {
+			if ( isFromElementUpdateOrDeleteRoot( lhsFromElement ) ) {
+				// at this point we know we have the 2 conditions above,
+				// lets see if we have the mentioned "multi-table" caveat...
+				boolean useAlias = false;
+				if ( getWalker().getStatementType() != SqlTokenTypes.INSERT ) {
+					final Queryable persister = lhsFromElement.getQueryable();
+					if ( persister.isMultiTable() ) {
+						useAlias = true;
+					}
+				}
+				if ( ! useAlias ) {
+					final String lhsTableName = lhsFromElement.getQueryable().getTableName();
+					columns = getFromElement().toColumns( lhsTableName, propertyPath, false, true );
+				}
+			}
 		}
 
-		//We do not look for an existing join on the same path, because
-		//it makes sense to join twice on the same collection role
+		// We do not look for an existing join on the same path, because
+		// it makes sense to join twice on the same collection role
 		FromElementFactory factory = new FromElementFactory(
 		        currentFromClause,
 		        getLhs().getFromElement(),

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/FromReferenceNode.java

@@ -27,6 +27,7 @@
 import antlr.collections.AST;
 import org.jboss.logging.Logger;
 
+import org.hibernate.hql.internal.antlr.HqlSqlTokenTypes;
 import org.hibernate.internal.CoreMessageLogger;
 
 /**
@@ -130,4 +131,15 @@ public FromElement getImpliedJoin() {
 		return null;
 	}
 
+	@SuppressWarnings("SimplifiableIfStatement")
+	protected boolean isFromElementUpdateOrDeleteRoot(FromElement element) {
+		if ( element.getFromClause().getParentFromClause() != null ) {
+			// its not even a root...
+			return false;
+		}
+
+		return getWalker().getStatementType() == HqlSqlTokenTypes.DELETE
+				|| getWalker().getStatementType() == HqlSqlTokenTypes.UPDATE;
+	}
+
 }

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/tree/IdentNode.java

@@ -150,42 +150,54 @@ else if (result == COMPONENT_REF) {
 
 	private boolean resolveAsAlias() {
 		// This is not actually a constant, but a reference to FROM element.
-		FromElement element = getWalker().getCurrentFromClause().getFromElement( getText() );
-		if ( element != null ) {
-			setType( SqlTokenTypes.ALIAS_REF );
-			setFromElement( element );
-			String[] columnExpressions = element.getIdentityColumns();
-			final boolean isInNonDistinctCount = getWalker().isInCount() && ! getWalker().isInCountDistinct();
-			final boolean isCompositeValue = columnExpressions.length > 1;
-			if ( isCompositeValue ) {
-				if ( isInNonDistinctCount && ! getWalker().getSessionFactoryHelper().getFactory().getDialect().supportsTupleCounts() ) {
-					setText( columnExpressions[0] );
-				}
-				else {
-					String joinedFragment = StringHelper.join( ", ", columnExpressions );
-					// avoid wrapping in parenthesis (explicit tuple treatment) if possible due to varied support for
-					// tuple syntax across databases..
-					final boolean shouldSkipWrappingInParenthesis =
-							getWalker().isInCount()
-							|| getWalker().getCurrentTopLevelClauseType() == HqlSqlTokenTypes.ORDER
-							|| getWalker().getCurrentTopLevelClauseType() == HqlSqlTokenTypes.GROUP;
-					if ( ! shouldSkipWrappingInParenthesis ) {
-						joinedFragment = "(" + joinedFragment + ")";
-					}
-					setText( joinedFragment );
-				}
-				return true;
+		final FromElement element = getWalker().getCurrentFromClause().getFromElement( getText() );
+		if ( element == null ) {
+			return false;
+		}
+
+		setType( SqlTokenTypes.ALIAS_REF );
+		setFromElement( element );
+
+		String[] columnExpressions = element.getIdentityColumns();
+
+		// determine whether to apply qualification (table alias) to the column(s)...
+		if ( ! isFromElementUpdateOrDeleteRoot( element ) ) {
+			if ( StringHelper.isNotEmpty( element.getTableAlias() ) ) {
+				// apparently we also need to check that they are not already qualified.  Ugh!
+				columnExpressions = StringHelper.qualifyIfNot( element.getTableAlias(), columnExpressions );
 			}
-			else if ( columnExpressions.length > 0 ) {
+		}
+
+		final boolean isInNonDistinctCount = getWalker().isInCount() && ! getWalker().isInCountDistinct();
+		final boolean isCompositeValue = columnExpressions.length > 1;
+		if ( isCompositeValue ) {
+			if ( isInNonDistinctCount && ! getWalker().getSessionFactoryHelper().getFactory().getDialect().supportsTupleCounts() ) {
 				setText( columnExpressions[0] );
-				return true;
 			}
+			else {
+				String joinedFragment = StringHelper.join( ", ", columnExpressions );
+				// avoid wrapping in parenthesis (explicit tuple treatment) if possible due to varied support for
+				// tuple syntax across databases..
+				final boolean shouldSkipWrappingInParenthesis =
+						getWalker().isInCount()
+						|| getWalker().getCurrentTopLevelClauseType() == HqlSqlTokenTypes.ORDER
+						|| getWalker().getCurrentTopLevelClauseType() == HqlSqlTokenTypes.GROUP;
+				if ( ! shouldSkipWrappingInParenthesis ) {
+					joinedFragment = "(" + joinedFragment + ")";
+				}
+				setText( joinedFragment );
+			}
+			return true;
 		}
+		else if ( columnExpressions.length > 0 ) {
+			setText( columnExpressions[0] );
+			return true;
+		}
+
 		return false;
 	}
 
-	private Type getNakedPropertyType(FromElement fromElement)
-	{
+	private Type getNakedPropertyType(FromElement fromElement) {
 		if (fromElement == null) {
 			return null;
 		}

hibernate-core/src/main/java/org/hibernate/internal/util/StringHelper.java

@@ -464,14 +464,34 @@ public static String qualify(String prefix, String name) {
 	}
 
 	public static String[] qualify(String prefix, String[] names) {
-		if ( prefix == null ) return names;
+		if ( prefix == null ) {
+			return names;
+		}
 		int len = names.length;
 		String[] qualified = new String[len];
 		for ( int i = 0; i < len; i++ ) {
 			qualified[i] = qualify( prefix, names[i] );
 		}
 		return qualified;
 	}
+
+	public static String[] qualifyIfNot(String prefix, String[] names) {
+		if ( prefix == null ) {
+			return names;
+		}
+		int len = names.length;
+		String[] qualified = new String[len];
+		for ( int i = 0; i < len; i++ ) {
+			if ( names[i].indexOf( '.' ) < 0 ) {
+				qualified[i] = qualify( prefix, names[i] );
+			}
+			else {
+				qualified[i] = names[i];
+			}
+		}
+		return qualified;
+	}
+
 	public static int firstIndexOfChar(String sqlString, BitSet keys, int startindex) {
 		for ( int i = startindex, size = sqlString.length(); i < size; i++ ) {
 			if ( keys.get( sqlString.charAt( i ) ) ) {

hibernate-core/src/test/java/org/hibernate/test/annotations/query/QueryAndSQLTest.java

@@ -453,21 +453,6 @@ public void testCollectionSQLOverriding() {
 		tx.rollback();
 		s.close();
 	}
-	
-	@Test
-	@TestForIssue( jiraKey = "HHH-8318" )
-	@FailureExpected( jiraKey = "HHH-8318" )
-	public void testDeleteMemberOf() {
-		Session s = openSession();
-		s.getTransaction().begin();
-		s.createQuery(
-				"delete Attrvalue aval where aval.id in ( "
-						+ "select val2.id from Employee e, Employeegroup eg, Attrset aset, Attrvalue val2 "
-						+ "where eg.id = e.employeegroup.id " + "and aset.id = e.attrset.id "
-						+ "and val2 member of aset.attrvalues)" ).executeUpdate();
-		s.getTransaction().commit();
-		s.close();
-	}
 
 	@Override
 	protected Class[] getAnnotatedClasses() {

hibernate-core/src/test/java/org/hibernate/test/hql/DeleteWhereMemberOfTest.java

@@ -0,0 +1,66 @@
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * Copyright (c) 2013, Red Hat Inc. or third-party contributors as
+ * indicated by the @author tags or express copyright attribution
+ * statements applied by the authors.  All third-party contributions are
+ * distributed under license by Red Hat Inc.
+ *
+ * This copyrighted material is made available to anyone wishing to use, modify,
+ * copy, or redistribute it subject to the terms and conditions of the GNU
+ * Lesser General Public License, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this distribution; if not, write to:
+ * Free Software Foundation, Inc.
+ * 51 Franklin Street, Fifth Floor
+ * Boston, MA  02110-1301  USA
+ */
+package org.hibernate.test.hql;
+
+import org.hibernate.Session;
+
+import org.junit.Test;
+
+import org.hibernate.testing.TestForIssue;
+import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
+import org.hibernate.test.annotations.query.Attrset;
+import org.hibernate.test.annotations.query.Attrvalue;
+import org.hibernate.test.annotations.query.Employee;
+import org.hibernate.test.annotations.query.Employeegroup;
+
+/**
+ * @author Steve Ebersole
+ */
+public class DeleteWhereMemberOfTest extends BaseCoreFunctionalTestCase {
+
+	@Override
+	protected Class[] getAnnotatedClasses() {
+		return new Class[] {
+				Attrset.class,
+				Attrvalue.class,
+				Employee.class,
+				Employeegroup.class
+		};
+	}
+
+	@Test
+	@TestForIssue( jiraKey = "HHH-8318" )
+//	@FailureExpected( jiraKey = "HHH-8318" )
+	public void testDeleteMemberOf() {
+		final String qry = "delete Attrvalue aval where aval.id in ( "
+				+ "select val2.id from Employee e, Employeegroup eg, Attrset aset, Attrvalue val2 "
+				+ "where eg.id = e.employeegroup.id " + "and aset.id = e.attrset.id "
+				+ "and val2 member of aset.attrvalues)";
+		Session s = openSession();
+		s.getTransaction().begin();
+		s.createQuery( qry ).executeUpdate();
+		s.getTransaction().commit();
+		s.close();
+	}
+}