HHH-13752 - Test and fix deletion of entities with many-to-many assocations using non-primary keys for join table

hibernate-core/src/main/java/org/hibernate/hql/internal/ast/exec/DeleteExecutor.java

@@ -7,6 +7,7 @@
 package org.hibernate.hql.internal.ast.exec;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 import org.hibernate.HibernateException;
@@ -23,6 +24,7 @@ import org.hibernate.persister.collection.AbstractCollectionPersister;
 import org.hibernate.persister.entity.Queryable;
 import org.hibernate.sql.Delete;
 import org.hibernate.type.CollectionType;
+import org.hibernate.type.ComponentType;
 import org.hibernate.type.Type;
 
 import org.jboss.logging.Logger;
@@ -58,7 +60,8 @@ public class DeleteExecutor extends BasicExecutor {
 				final SqlGenerator gen = new SqlGenerator( factory );
 				gen.whereClause( whereClause );
 				parameterSpecifications = gen.getCollectedParameters();
-				idSubselectWhere = gen.getSQL().length() > 7 ? gen.getSQL() : "";
+				String sql = gen.getSQL();
+				idSubselectWhere = sql.length() > 7 ? sql : "";
 			}
 			else {
 				parameterSpecifications = new ArrayList<>();
@@ -74,8 +77,20 @@ public class DeleteExecutor extends BasicExecutor {
 					final CollectionType cType = (CollectionType) type;
 					final AbstractCollectionPersister cPersister = (AbstractCollectionPersister) metamodel.collectionPersister( cType.getRole() );
 					if ( cPersister.isManyToMany() ) {
-						if ( persister.getIdentifierColumnNames().length > 1
+						Type keyType = cPersister.getKeyType();
-								&& notSupportingTuplesInSubqueries ) {
+						String[] columnNames;
+						if ( keyType.isComponentType() ) {
+							ComponentType componentType = (ComponentType) keyType;
+							List<String> columns = new ArrayList<>( componentType.getPropertyNames().length );
+							for ( String propertyName : componentType.getPropertyNames() ) {
+								Collections.addAll( columns, persister.toColumns( propertyName ) );
+							}
+							columnNames = columns.toArray( new String[columns.size()] );
+						}
+						else {
+							columnNames = persister.getIdentifierColumnNames();
+						}
+						if ( columnNames.length > 1 && notSupportingTuplesInSubqueries ) {
 							LOG.warn(
 									"This dialect is unable to cascade the delete into the many-to-many join table" +
 									" when the entity has multiple primary keys.  Either properly setup cascading on" +
@@ -83,11 +98,13 @@ public class DeleteExecutor extends BasicExecutor {
 							);
 						}
 						else {
-							final String idSubselect = "(select "
+							StringBuilder whereBuilder = new StringBuilder();
-									+ String.join( ", ", persister.getIdentifierColumnNames() ) + " from "
+							whereBuilder.append( '(' );
-									+ persister.getTableName() + idSubselectWhere + ")";
+							append( ", ", cPersister.getKeyColumnNames(), whereBuilder );
-							final String where = "(" + String.join( ", ", cPersister.getKeyColumnNames() )
+							whereBuilder.append( ") in (select " );
-									+ ") in " + idSubselect;
+							append( ", ", columnNames, whereBuilder );
+							final String where = whereBuilder.append(" from ")
+								.append( persister.getTableName() ).append( idSubselectWhere ).append( ")" ).toString();
 							final Delete delete = new Delete().setTableName( cPersister.getTableName() ).setWhere( where );
 							if ( commentsEnabled ) {
 								delete.setComment( "delete FKs in join table" );
@@ -102,6 +119,14 @@ public class DeleteExecutor extends BasicExecutor {
 			throw new HibernateException( "Unable to delete the FKs in the join table!", e );
 		}
 	}
+
+	private static void append(String delimiter, String[] parts, StringBuilder sb) {
+		sb.append( parts[0] );
+		for ( int i = 1; i < parts.length; i++ ) {
+			sb.append( delimiter );
+			sb.append( parts[i] );
+		}
+	}
 	
 	@Override
 	public int execute(QueryParameters parameters, SharedSessionContractImplementor session) throws HibernateException {

hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractTableBasedBulkIdHandler.java

@@ -6,6 +6,7 @@
  */
 package org.hibernate.hql.spi.id;
 
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
@@ -16,6 +17,7 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.hql.internal.ast.HqlSqlWalker;
 import org.hibernate.hql.internal.ast.SqlGenerator;
 import org.hibernate.param.ParameterSpecification;
+import org.hibernate.persister.collection.AbstractCollectionPersister;
 import org.hibernate.persister.entity.Queryable;
 import org.hibernate.sql.InsertSelect;
 import org.hibernate.sql.Select;
@@ -23,6 +25,8 @@ import org.hibernate.sql.SelectValues;
 
 import antlr.RecognitionException;
 import antlr.collections.AST;
+import org.hibernate.type.ComponentType;
+import org.hibernate.type.Type;
 
 /**
  * Convenience base class for {@link MultiTableBulkIdStrategy.UpdateHandler}
@@ -193,6 +197,40 @@ public abstract class AbstractTableBasedBulkIdHandler {
 				" from " + idTableInfo.getQualifiedIdTableName();
 	}
 
+	protected String generateIdSubselect(Queryable persister, AbstractCollectionPersister cPersister, IdTableInfo idTableInfo) {
+		String[] columnNames = getKeyColumnNames( persister, cPersister );
+		StringBuilder selectBuilder = new StringBuilder();
+		selectBuilder.append( "select " );
+		appendJoined( ", ", columnNames, selectBuilder  );
+		return selectBuilder.append( " from " )
+			.append( idTableInfo.getQualifiedIdTableName() ).toString();
+	}
+
+	protected static String[] getKeyColumnNames(Queryable persister, AbstractCollectionPersister cPersister) {
+		Type keyType = cPersister.getKeyType();
+		String[] columnNames;
+		if ( keyType.isComponentType() ) {
+			ComponentType componentType = (ComponentType) keyType;
+			List<String> columns = new ArrayList<>(componentType.getPropertyNames().length );
+			for ( String propertyName : componentType.getPropertyNames() ) {
+				Collections.addAll( columns, persister.toColumns( propertyName ) );
+			}
+			columnNames = columns.toArray( new String[columns.size()] );
+		}
+		else {
+			columnNames = persister.getIdentifierColumnNames();
+		}
+		return columnNames;
+	}
+
+	protected static void appendJoined(String delimiter, String[] parts, StringBuilder sb) {
+		sb.append( parts[0] );
+		for ( int i = 1; i < parts.length; i++ ) {
+			sb.append( delimiter );
+			sb.append( parts[i] );
+		}
+	}
+
 	protected void prepareForUse(Queryable persister, SharedSessionContractImplementor session) {
 	}
 

hibernate-core/src/main/java/org/hibernate/hql/spi/id/TableBasedDeleteHandlerImpl.java

@@ -68,7 +68,7 @@ public class TableBasedDeleteHandlerImpl
 				AbstractCollectionPersister cPersister = (AbstractCollectionPersister) factory.getMetamodel().collectionPersister( cType.getRole() );
 				if ( cPersister.isManyToMany() ) {
 					deletes.add( generateDelete( cPersister.getTableName(),
-							cPersister.getKeyColumnNames(), idSubselect, "bulk delete - m2m join table cleanup"));
+							cPersister.getKeyColumnNames(), generateIdSubselect( targetedPersister, cPersister, idTableInfo ), "bulk delete - m2m join table cleanup"));
 				}
 			}
 		}

hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/AbstractCteValuesListBulkIdHandler.java

@@ -17,8 +17,11 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.hql.internal.ast.HqlSqlWalker;
 import org.hibernate.hql.spi.id.AbstractIdsBulkIdHandler;
 import org.hibernate.internal.util.StringHelper;
+import org.hibernate.persister.collection.AbstractCollectionPersister;
 import org.hibernate.persister.entity.Queryable;
 
+import java.util.Arrays;
+
 /**
  * Defines how identifier values are selected from the updatable/deletable tables.
  *
@@ -88,9 +91,32 @@ public abstract class AbstractCteValuesListBulkIdHandler extends
 				) )
 				.append( " from " )
 				.append( determineIdTableName( persister ) )
+				.append( " tmp2" )
 				.toString();
 	}
 
+	protected String generateIdSubselect(String idSubselect, Queryable persister, AbstractCollectionPersister cPersister) {
+		String[] columnNames = getKeyColumnNames( persister, cPersister );
+		// If the column names are equal to the identifier column names, just return the idSubselect
+		if ( Arrays.equals( getTargetedQueryable().getIdentifierColumnNames(), columnNames ) ) {
+			return idSubselect;
+		}
+
+		// Otherwise, we need to fetch the key column names from the original table
+		// Unfortunately, this is a bit more work, as only the identifiers are fetched
+		// It would be great if we could adapt #selectIds to fetch key columns as well
+		StringBuilder selectBuilder = new StringBuilder();
+		selectBuilder.append( "select " );
+		appendJoined( ", ", columnNames, selectBuilder );
+		selectBuilder.append( " from " ).append( getTargetedQueryable().getTableName() );
+		selectBuilder.append( " tmp where (" );
+		appendJoined( ", ", getTargetedQueryable().getIdentifierColumnNames(), selectBuilder );
+		selectBuilder.append( ") in (select " );
+		appendJoined( ", ", getTargetedQueryable().getIdentifierColumnNames(), selectBuilder );
+		selectBuilder.append( " from " ).append( determineIdTableName( persister ) ).append( " tmp2)" );
+		return selectBuilder.toString();
+	}
+
 	protected CteValuesListBuilder prepareCteStatement(
 			SharedSessionContractImplementor session,
 			QueryParameters queryParameters) {

hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/CteValuesListDeleteHandlerImpl.java

@@ -56,7 +56,7 @@ public class CteValuesListDeleteHandlerImpl
 					deletes.add( generateDelete(
 							cPersister.getTableName(),
 							cPersister.getKeyColumnNames(),
-							idSubselect,
+							generateIdSubselect( idSubselect, getTargetedQueryable(), cPersister ),
 							"bulk delete - m2m join table cleanup"
 					) );
 				}

hibernate-core/src/main/java/org/hibernate/hql/spi/id/inline/AbstractInlineIdsDeleteHandlerImpl.java

@@ -9,6 +9,7 @@ package org.hibernate.hql.spi.id.inline;
 import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
 import org.hibernate.engine.spi.QueryParameters;
@@ -17,6 +18,7 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.hql.internal.ast.HqlSqlWalker;
 import org.hibernate.hql.spi.id.MultiTableBulkIdStrategy;
 import org.hibernate.persister.collection.AbstractCollectionPersister;
+import org.hibernate.persister.entity.Queryable;
 import org.hibernate.sql.Delete;
 import org.hibernate.type.CollectionType;
 import org.hibernate.type.Type;
@@ -61,7 +63,7 @@ public abstract class AbstractInlineIdsDeleteHandlerImpl
 						deletes.add( generateDelete(
 								cPersister.getTableName(),
 								cPersister.getKeyColumnNames(),
-								idSubselect,
+								generateIdSubselect( idSubselect, getTargetedQueryable(), cPersister ),
 								"bulk delete - m2m join table cleanup"
 						).toStatementString() );
 					}
@@ -102,6 +104,28 @@ public abstract class AbstractInlineIdsDeleteHandlerImpl
 		return values.getIds().size();
 	}
 
+	protected String generateIdSubselect(String idSubselect, Queryable persister, AbstractCollectionPersister cPersister) {
+		String[] columnNames = getKeyColumnNames( persister, cPersister );
+		// If the column names are equal to the identifier column names, just return the idSubselect
+		if ( Arrays.equals(getTargetedQueryable().getIdentifierColumnNames(), columnNames ) ) {
+			return idSubselect;
+		}
+
+		// Otherwise, we need to fetch the key column names from the original table
+		// Unfortunately, this is a bit more work, as only the identifiers are fetched
+		// It would be great if we could adapt #selectIds to fetch key columns as well
+		StringBuilder selectBuilder = new StringBuilder();
+		selectBuilder.append( "select " );
+		appendJoined( ", ", columnNames, selectBuilder );
+		selectBuilder.append( " from " ).append( getTargetedQueryable().getTableName() );
+		selectBuilder.append( " tmp where (" );
+		appendJoined( ", ", getTargetedQueryable().getIdentifierColumnNames(), selectBuilder );
+		selectBuilder.append( ") in (" );
+		selectBuilder.append( idSubselect );
+		selectBuilder.append( ")" );
+		return selectBuilder.toString();
+	}
+
 	protected Delete generateDelete(
 			String tableName,
 			String[] columnNames,

hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/DeleteHandlerImpl.java

@@ -15,6 +15,7 @@ import org.hibernate.engine.spi.SharedSessionContractImplementor;
 import org.hibernate.hql.internal.ast.HqlSqlWalker;
 import org.hibernate.hql.spi.id.IdTableInfo;
 import org.hibernate.hql.spi.id.TableBasedDeleteHandlerImpl;
+import org.hibernate.persister.collection.AbstractCollectionPersister;
 import org.hibernate.persister.entity.Queryable;
 import org.hibernate.sql.SelectValues;
 
@@ -44,6 +45,11 @@ public class DeleteHandlerImpl extends TableBasedDeleteHandlerImpl {
 		return super.generateIdSubselect( persister, idTableInfo ) + " where " + SESSION_ID_COLUMN_NAME + "=?";
 	}
 
+	@Override
+	protected String generateIdSubselect(Queryable persister, AbstractCollectionPersister cPersister, IdTableInfo idTableInfo) {
+		return super.generateIdSubselect( persister, cPersister, idTableInfo ) + " where " + SESSION_ID_COLUMN_NAME + "=?";
+	}
+
 	@Override
 	protected int handlePrependedParametersOnIdSelection(PreparedStatement ps, SharedSessionContractImplementor session, int pos) throws SQLException {
 		Helper.INSTANCE.bindSessionIdentifier( ps, session, pos );

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

@@ -11,6 +11,7 @@ import org.hibernate.annotations.NaturalId;
 import org.hibernate.hql.spi.FilterTranslator;
 import org.hibernate.query.Query;
 
+import org.hibernate.testing.TestForIssue;
 import org.hibernate.testing.junit4.BaseCoreFunctionalTestCase;
 import org.junit.Before;
 import org.junit.Test;
@@ -21,10 +22,13 @@ import javax.persistence.GeneratedValue;
 import javax.persistence.GenerationType;
 import javax.persistence.Id;
 import javax.persistence.JoinColumn;
+import javax.persistence.JoinTable;
 import javax.persistence.ManyToOne;
+import javax.persistence.OneToMany;
 import javax.persistence.Table;
 import java.util.Collections;
 import java.util.List;
+import java.util.Set;
 
 import static org.hibernate.testing.transaction.TransactionUtil.doInHibernate;
 import static org.junit.Assert.assertEquals;
@@ -239,6 +243,16 @@ public class NaturalIdDereferenceTest extends BaseCoreFunctionalTestCase {
 		} );
 	}
 
+	@Test
+	@TestForIssue(jiraKey = "HHH-13752")
+	public void deleteWithNaturalIdBasedJoinTable() {
+		doInHibernate( this::sessionFactory, session -> {
+			Query query = session.createQuery(
+				"DELETE FROM Book b WHERE 1=0" );
+			query.executeUpdate();
+		} );
+	}
+
 	private int getSQLJoinCount(Query query) {
 		String sqlQuery = getSQLQuery( query ).toLowerCase();
 
@@ -316,6 +330,14 @@ public class NaturalIdDereferenceTest extends BaseCoreFunctionalTestCase {
 		@Column(name = "isbn", unique = true)
 		private String isbn;
 
+		@OneToMany
+		@JoinTable(
+			name = "similar_books",
+			joinColumns = @JoinColumn(name = "base_isbn", referencedColumnName = "isbn"),
+			inverseJoinColumns = @JoinColumn(name = "ref_isbn", referencedColumnName = "isbn_ref")
+		)
+		private Set<BookRef> similarBooks;
+
 	}
 
 	@Entity(name = "BookRef")