From e0e6050ba168bd8470ac8188c965298292f20e89 Mon Sep 17 00:00:00 2001 From: Andrea Boriero Date: Mon, 14 Mar 2022 10:49:31 +0100 Subject: [PATCH] HHH-15117 ConstraintViolationException is thrown using same @SecondaryTable on two entities --- .../util/collections/ArrayHelper.java | 6 +- .../entity/AbstractEntityPersister.java | 80 ++++++++++++++++++- .../entity/JoinedSubclassEntityPersister.java | 6 ++ .../entity/SingleTableEntityPersister.java | 10 +++ .../entity/UnionSubclassEntityPersister.java | 5 ++ .../internal/cte/CteDeleteHandler.java | 7 +- .../internal/cte/CteInsertHandler.java | 31 ++++++- .../internal/cte/CteUpdateHandler.java | 14 +++- .../temptable/InsertExecutionDelegate.java | 40 ++++++++-- ...ParentChildWithSameSecondaryTableTest.java | 80 +++++++++++++++++++ 10 files changed, 260 insertions(+), 19 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/ArrayHelper.java b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/ArrayHelper.java index 1e88dd3c0e..0f88594754 100644 --- a/hibernate-core/src/main/java/org/hibernate/internal/util/collections/ArrayHelper.java +++ b/hibernate-core/src/main/java/org/hibernate/internal/util/collections/ArrayHelper.java @@ -28,7 +28,11 @@ public final class ArrayHelper { } public static int indexOf(Object[] array, Object object) { - for ( int i = 0; i < array.length; i++ ) { + return indexOf( array, array.length, object ); + } + + public static int indexOf(Object[] array, int end, Object object) { + for ( int i = 0; i < end; i++ ) { if ( object.equals( array[i] ) ) { return i; } diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java index ca431c05f7..7ac80423b5 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java @@ -458,6 +458,8 @@ public abstract class AbstractEntityPersister public abstract boolean isTableCascadeDeleteEnabled(int j); + public abstract boolean hasDuplicateTables(); + public abstract String getTableName(int j); public abstract String[] getKeyColumns(int j); @@ -3891,14 +3893,84 @@ public abstract class AbstractEntityPersister if ( entityMetamodel.isDynamicInsert() ) { // For the case of dynamic-insert="true", we need to generate the INSERT SQL boolean[] notNull = getPropertiesToInsert( fields ); - for ( int j = 0; j < span; j++ ) { - insert( id, fields, notNull, j, generateInsertString( notNull, j ), object, session ); + if ( hasDuplicateTables() ) { + final String[] insertedTables = new String[span]; + for ( int j = 0; j < span; j++ ) { + if ( isInverseTable( j ) ) { + continue; + } + + //note: it is conceptually possible that a UserType could map null to + // a non-null value, so the following is arguable: + if ( isNullableTable( j ) && isAllNull( fields, j ) ) { + continue; + } + final String tableName = getTableName( j ); + insertedTables[j] = tableName; + if ( ArrayHelper.indexOf( insertedTables, j, tableName ) != -1 ) { + update( + id, + fields, + null, + null, + notNull, + j, + null, + object, + generateUpdateString( notNull, j, false ), + session + ); + } + else { + insert( id, fields, notNull, j, generateInsertString( notNull, j ), object, session ); + } + } + } + else { + for ( int j = 0; j < span; j++ ) { + insert( id, fields, notNull, j, generateInsertString( notNull, j ), object, session ); + } } } else { // For the case of dynamic-insert="false", use the static SQL - for ( int j = 0; j < span; j++ ) { - insert( id, fields, getPropertyInsertability(), j, getSQLInsertStrings()[j], object, session ); + if ( hasDuplicateTables() ) { + final String[] insertedTables = new String[span]; + for ( int j = 0; j < span; j++ ) { + if ( isInverseTable( j ) ) { + continue; + } + + //note: it is conceptually possible that a UserType could map null to + // a non-null value, so the following is arguable: + if ( isNullableTable( j ) && isAllNull( fields, j ) ) { + continue; + } + final String tableName = getTableName( j ); + insertedTables[j] = tableName; + if ( ArrayHelper.indexOf( insertedTables, j, tableName ) != -1 ) { + update( + id, + fields, + null, + null, + getPropertyInsertability(), + j, + null, + object, + getSQLUpdateStrings()[j], + session + ); + } + else { + insert( id, fields, getPropertyInsertability(), j, getSQLInsertStrings()[j], object, session ); + } + } + } + else { + for ( int j = 0; j < span; j++ ) { + insert( id, fields, getPropertyInsertability(), j, getSQLInsertStrings()[j], object, session ); + } } } } diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/JoinedSubclassEntityPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/JoinedSubclassEntityPersister.java index d6c6bbe085..f926be42fa 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/JoinedSubclassEntityPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/JoinedSubclassEntityPersister.java @@ -90,6 +90,7 @@ public class JoinedSubclassEntityPersister extends AbstractEntityPersister { // the class hierarchy structure private final int tableSpan; + private final boolean hasDuplicateTables; private final String[] tableNames; private final String[] naturalOrderTableNames; private final String[][] tableKeyColumns; @@ -301,6 +302,7 @@ public class JoinedSubclassEntityPersister extends AbstractEntityPersister { cascadeDeletes.add( key.isCascadeDeleteEnabled() && dialect.supportsCascadeDelete() ); } + hasDuplicateTables = new HashSet<>( tableNames ).size() == tableNames.size(); naturalOrderTableNames = ArrayHelper.toStringArray( tableNames ); naturalOrderTableKeyColumns = ArrayHelper.to2DStringArray( keyColumns ); String[][] naturalOrderTableKeyColumnReaders = ArrayHelper.to2DStringArray(keyColumnReaders); @@ -816,6 +818,10 @@ public class JoinedSubclassEntityPersister extends AbstractEntityPersister { return spaces; // don't need subclass tables, because they can't appear in conditions } + @Override + public boolean hasDuplicateTables() { + return hasDuplicateTables; + } @Override public String getTableName(int j) { diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/SingleTableEntityPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/SingleTableEntityPersister.java index a73a2380ec..9be900c45d 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/SingleTableEntityPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/SingleTableEntityPersister.java @@ -78,6 +78,7 @@ public class SingleTableEntityPersister extends AbstractEntityPersister { // the class hierarchy structure private final int joinSpan; + private final boolean hasDuplicateTables; private final String[] qualifiedTableNames; private final boolean[] isInverseTable; private final boolean[] isNullableTable; @@ -198,9 +199,12 @@ public class SingleTableEntityPersister extends AbstractEntityPersister { // JOINS List joinClosure = persistentClass.getJoinClosure(); + boolean hasDuplicateTableName = false; for ( int j = 1; j-1 < joinClosure.size(); j++ ) { Join join = joinClosure.get(j-1); qualifiedTableNames[j] = determineTableName( join.getTable() ); + hasDuplicateTableName = hasDuplicateTableName + || ArrayHelper.indexOf( qualifiedTableNames, j, qualifiedTableNames[j] ) != -1; isInverseTable[j] = join.isInverse(); isNullableTable[j] = join.isOptional(); cascadeDeleteEnabled[j] = join.getKey().isCascadeDeleteEnabled() && dialect.supportsCascadeDelete(); @@ -229,6 +233,7 @@ public class SingleTableEntityPersister extends AbstractEntityPersister { } } + hasDuplicateTables = hasDuplicateTableName; constraintOrderedTableNames = new String[qualifiedTableNames.length]; constraintOrderedKeyColumnNames = new String[qualifiedTableNames.length][]; for ( int i = qualifiedTableNames.length - 1, position = 0; i >= 0; i--, position++ ) { @@ -538,6 +543,11 @@ public class SingleTableEntityPersister extends AbstractEntityPersister { return discriminatorColumnName == null; } + @Override + public boolean hasDuplicateTables() { + return hasDuplicateTables; + } + @Override public String getTableName(int j) { return qualifiedTableNames[j]; diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/UnionSubclassEntityPersister.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/UnionSubclassEntityPersister.java index 8618539251..57459d2030 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/UnionSubclassEntityPersister.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/UnionSubclassEntityPersister.java @@ -324,6 +324,11 @@ public class UnionSubclassEntityPersister extends AbstractEntityPersister { return false; } + @Override + public boolean hasDuplicateTables() { + return false; + } + @Override public String getTableName(int j) { return tableName; diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteDeleteHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteDeleteHandler.java index 19a4868e55..46ffd83378 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteDeleteHandler.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteDeleteHandler.java @@ -146,8 +146,13 @@ public class CteDeleteHandler extends AbstractCteMutationHandler implements Dele getEntityDescriptor().visitConstraintOrderedTables( (tableExpression, tableColumnsVisitationSupplier) -> { + final String cteTableName = getCteTableName( tableExpression ); + if ( statement.getCteStatement( cteTableName ) != null ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + return; + } final CteTable dmlResultCte = new CteTable( - getCteTableName( tableExpression ), + cteTableName, idSelectCte.getCteTable().getCteColumns(), factory ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java index f75ebc3c9a..c88bc55436 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteInsertHandler.java @@ -303,11 +303,26 @@ public class CteInsertHandler implements InsertHandler { final QuerySpec querySpec = new QuerySpec( true ); final NavigablePath navigablePath = new NavigablePath( entityDescriptor.getRootPathName() ); final List columnNames = new ArrayList<>( targetPathColumns.size() ); + final String valuesAlias = insertingTableGroup.getPrimaryTableReference().getIdentificationVariable(); for ( Map.Entry entry : targetPathColumns ) { for ( ColumnReference columnReference : entry.getValue().getAssignable().getColumnReferences() ) { columnNames.add( columnReference.getColumnExpression() ); querySpec.getSelectClause().addSqlSelection( - new SqlSelectionImpl( 1, 0, columnReference ) + new SqlSelectionImpl( + 1, + 0, + columnReference.getQualifier().equals( valuesAlias ) + ? columnReference + : new ColumnReference( + valuesAlias, + columnReference.getColumnExpression(), + false, + null, + null, + columnReference.getJdbcMapping(), + null + ) + ) ); } } @@ -810,6 +825,11 @@ public class CteInsertHandler implements InsertHandler { final CteTable dmlResultCte; if ( i == 0 && !assignsId && identifierGenerator instanceof PostInsertIdentifierGenerator ) { // Special handling for identity generation + final String cteTableName = getCteTableName( tableExpression, "base_" ); + if ( statement.getCteStatement( cteTableName ) != null ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + continue; + } final String baseTableName = "base_" + queryCte.getCteTable().getTableExpression(); insertSelectSpec.getFromClause().addRoot( new CteTableGroup( @@ -843,7 +863,7 @@ public class CteInsertHandler implements InsertHandler { final List returningColumns = new ArrayList<>( keyCteColumns.size() + 1 ); returningColumns.addAll( keyCteColumns ); dmlResultCte = new CteTable( - getCteTableName( tableExpression, "base_" ), + cteTableName, returningColumns, factory ); @@ -1010,6 +1030,11 @@ public class CteInsertHandler implements InsertHandler { finalCteStatement = new CteStatement( finalResultCte, finalResultStatement ); } else { + final String cteTableName = getCteTableName( tableExpression ); + if ( statement.getCteStatement( cteTableName ) != null ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + continue; + } insertSelectSpec.getFromClause().addRoot( new CteTableGroup( new NamedTableReference( @@ -1021,7 +1046,7 @@ public class CteInsertHandler implements InsertHandler { ) ); dmlResultCte = new CteTable( - getCteTableName( tableExpression ), + cteTableName, keyCteColumns, factory ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteUpdateHandler.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteUpdateHandler.java index dfd764bf11..cbd3af6266 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteUpdateHandler.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/cte/CteUpdateHandler.java @@ -167,8 +167,13 @@ public class CteUpdateHandler extends AbstractCteMutationHandler implements Upda if ( assignmentList == null ) { continue; } + final String insertCteTableName = getInsertCteTableName( tableExpression ); + if ( statement.getCteStatement( insertCteTableName ) != null ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + continue; + } final CteTable dmlResultCte = new CteTable( - getInsertCteTableName( tableExpression ), + insertCteTableName, idSelectCte.getCteTable().getCteColumns(), factory ); @@ -260,8 +265,13 @@ public class CteUpdateHandler extends AbstractCteMutationHandler implements Upda getEntityDescriptor().visitConstraintOrderedTables( (tableExpression, tableColumnsVisitationSupplier) -> { + final String cteTableName = getCteTableName( tableExpression ); + if ( statement.getCteStatement( cteTableName ) != null ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + return; + } final CteTable dmlResultCte = new CteTable( - getCteTableName( tableExpression ), + cteTableName, idSelectCte.getCteTable().getCteColumns(), factory ); diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/InsertExecutionDelegate.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/InsertExecutionDelegate.java index ed8cd991ea..ce8dbc49c2 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/InsertExecutionDelegate.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/mutation/internal/temptable/InsertExecutionDelegate.java @@ -28,6 +28,7 @@ import org.hibernate.id.PostInsertIdentityPersister; import org.hibernate.id.enhanced.Optimizer; import org.hibernate.id.insert.Binder; import org.hibernate.id.insert.InsertGeneratedIdentifierDelegate; +import org.hibernate.internal.util.collections.ArrayHelper; import org.hibernate.internal.util.collections.CollectionHelper; import org.hibernate.metamodel.mapping.BasicEntityIdentifierMapping; import org.hibernate.metamodel.mapping.EntityMappingType; @@ -198,13 +199,36 @@ public class InsertExecutionDelegate implements TableBasedInsertHandler.Executio final int tableSpan = persister.getTableSpan(); insertRootTable( persister.getTableName( 0 ), rows, persister.getKeyColumns( 0 ), executionContext ); - for ( int i = 1; i < tableSpan; i++ ) { - insertTable( - persister.getTableName( i ), - persister.getKeyColumns( i ), - persister.isNullableTable( i ), - executionContext - ); + if ( persister.hasDuplicateTables() ) { + final String[] insertedTables = new String[tableSpan]; + insertedTables[0] = persister.getTableName( 0 ); + for ( int i = 1; i < tableSpan; i++ ) { + if ( persister.isInverseTable( i ) ) { + continue; + } + final String tableName = persister.getTableName( i ); + insertedTables[i] = tableName; + if ( ArrayHelper.indexOf( insertedTables, i, tableName ) != -1 ) { + // Since secondary tables could appear multiple times, we have to skip duplicates + continue; + } + insertTable( + tableName, + persister.getKeyColumns( i ), + persister.isNullableTable( i ), + executionContext + ); + } + } + else { + for ( int i = 1; i < tableSpan; i++ ) { + insertTable( + persister.getTableName( i ), + persister.getKeyColumns( i ), + persister.isNullableTable( i ), + executionContext + ); + } } } @@ -653,7 +677,7 @@ public class InsertExecutionDelegate implements TableBasedInsertHandler.Executio needsKeyInsert = optimizer != null && optimizer.getIncrementSize() > 1; } else { - needsKeyInsert = false; + needsKeyInsert = true; } if ( needsKeyInsert && insertStatement.getTargetColumnReferences() .stream() diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/secondarytable/ParentChildWithSameSecondaryTableTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/secondarytable/ParentChildWithSameSecondaryTableTest.java index 3f6143c4c5..1e50d44deb 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/secondarytable/ParentChildWithSameSecondaryTableTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/annotations/secondarytable/ParentChildWithSameSecondaryTableTest.java @@ -19,6 +19,7 @@ import jakarta.persistence.InheritanceType; import jakarta.persistence.SecondaryTable; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; @DomainModel( @@ -53,6 +54,40 @@ public class ParentChildWithSameSecondaryTableTest { ); } + @Test + public void testPersist2(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + EntityC entityC = new EntityC(); + entityC.setId( 1L ); + entityC.setAttrC( "attrC-value" ); + session.persist( entityC ); + session.flush(); + session.clear(); + + EntityC entityC1 = session.find( EntityC.class, 1L ); + assertThat( entityC1.getAttrC(), is( notNullValue() ) ); + } + ); + } + + @Test + public void testPersist3(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + EntityC entityC = new EntityC(); + entityC.setId( 1L ); + entityC.setAttrB( "attrB-value" ); + session.persist( entityC ); + session.flush(); + session.clear(); + + EntityC entityC1 = session.find( EntityC.class, 1L ); + assertThat( entityC1.getAttrB(), is( notNullValue() ) ); + } + ); + } + @Test public void testDelete(SessionFactoryScope scope) { scope.inTransaction( @@ -121,6 +156,51 @@ public class ParentChildWithSameSecondaryTableTest { ); } + @Test + public void testUpdate2(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + EntityC entityC = new EntityC(); + entityC.setId( 1L ); + entityC.setAttrB( "attrB-value" ); + entityC.setAttrC( "attrC-value" ); + session.persist( entityC ); + } + ); + scope.inTransaction( + session -> { + session.createMutationQuery( "update EntityC c set c.attrB = 'B', c.attrC = 'C'" ) + .executeUpdate(); + } + ); + scope.inTransaction( + session -> { + final EntityC entityC = session.get( EntityC.class, 1L ); + assertThat( entityC.getAttrB(), is( "B" ) ); + assertThat( entityC.getAttrC(), is( "C" ) ); + + } + ); + } + + @Test + public void testInsert(SessionFactoryScope scope) { + scope.inTransaction( + session -> { + session.createMutationQuery( "insert into EntityC(id, attrB,attrC) values (1L, 'B', 'C')" ) + .executeUpdate(); + } + ); + scope.inTransaction( + session -> { + final EntityC entityC = session.get( EntityC.class, 1L ); + assertThat( entityC.getAttrB(), is( "B" ) ); + assertThat( entityC.getAttrC(), is( "C" ) ); + + } + ); + } + @Entity(name = "EntityA") @Inheritance(strategy = InheritanceType.SINGLE_TABLE) @DiscriminatorColumn(discriminatorType = DiscriminatorType.STRING, name = "type")