From 58dd469e4ca59485469cf4b7c3537443faf104a9 Mon Sep 17 00:00:00 2001 From: Christian Beikov Date: Tue, 26 Nov 2019 16:25:10 +0100 Subject: [PATCH] HHH-13752 - Test and fix deletion of entities with many-to-many assocations using non-primary keys for join table --- .../hql/internal/ast/exec/DeleteExecutor.java | 41 +++++++++++++++---- .../id/AbstractTableBasedBulkIdHandler.java | 38 +++++++++++++++++ .../spi/id/TableBasedDeleteHandlerImpl.java | 2 +- .../AbstractCteValuesListBulkIdHandler.java | 26 ++++++++++++ .../cte/CteValuesListDeleteHandlerImpl.java | 2 +- .../AbstractInlineIdsDeleteHandlerImpl.java | 26 +++++++++++- .../spi/id/persistent/DeleteHandlerImpl.java | 6 +++ .../test/hql/NaturalIdDereferenceTest.java | 22 ++++++++++ 8 files changed, 152 insertions(+), 11 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/exec/DeleteExecutor.java b/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/exec/DeleteExecutor.java index a904220098..0c2aca1f22 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/internal/ast/exec/DeleteExecutor.java +++ b/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 - && notSupportingTuplesInSubqueries ) { + Type keyType = cPersister.getKeyType(); + String[] columnNames; + if ( keyType.isComponentType() ) { + ComponentType componentType = (ComponentType) keyType; + List 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 " - + String.join( ", ", persister.getIdentifierColumnNames() ) + " from " - + persister.getTableName() + idSubselectWhere + ")"; - final String where = "(" + String.join( ", ", cPersister.getKeyColumnNames() ) - + ") in " + idSubselect; + StringBuilder whereBuilder = new StringBuilder(); + whereBuilder.append( '(' ); + append( ", ", cPersister.getKeyColumnNames(), whereBuilder ); + whereBuilder.append( ") in (select " ); + 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 { diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractTableBasedBulkIdHandler.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractTableBasedBulkIdHandler.java index 71f696f255..0a59a4d3a9 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/AbstractTableBasedBulkIdHandler.java +++ b/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 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) { } diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/TableBasedDeleteHandlerImpl.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/TableBasedDeleteHandlerImpl.java index f9eee19212..6ff19ae103 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/TableBasedDeleteHandlerImpl.java +++ b/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")); } } } diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/AbstractCteValuesListBulkIdHandler.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/AbstractCteValuesListBulkIdHandler.java index bf3600ced9..2f4b75e7fe 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/AbstractCteValuesListBulkIdHandler.java +++ b/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) { diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/CteValuesListDeleteHandlerImpl.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/CteValuesListDeleteHandlerImpl.java index b587554dd7..9c0b6c059e 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/cte/CteValuesListDeleteHandlerImpl.java +++ b/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" ) ); } diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/inline/AbstractInlineIdsDeleteHandlerImpl.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/inline/AbstractInlineIdsDeleteHandlerImpl.java index 498aa24fd9..87663cddb4 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/inline/AbstractInlineIdsDeleteHandlerImpl.java +++ b/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, diff --git a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/DeleteHandlerImpl.java b/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/DeleteHandlerImpl.java index 282eb30415..7206fba012 100644 --- a/hibernate-core/src/main/java/org/hibernate/hql/spi/id/persistent/DeleteHandlerImpl.java +++ b/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 ); diff --git a/hibernate-core/src/test/java/org/hibernate/test/hql/NaturalIdDereferenceTest.java b/hibernate-core/src/test/java/org/hibernate/test/hql/NaturalIdDereferenceTest.java index 5dca52d552..bc6fee5fca 100644 --- a/hibernate-core/src/test/java/org/hibernate/test/hql/NaturalIdDereferenceTest.java +++ b/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 similarBooks; + } @Entity(name = "BookRef")