From 9d36c13b3507db3b9aa52f5cfd3b1ceea88a21f5 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Thu, 7 Sep 2023 13:51:07 +0200 Subject: [PATCH 1/2] HHH-17167 Add test for issue --- ...est.java => RowIdUpdateAndDeleteTest.java} | 39 ++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) rename hibernate-core/src/test/java/org/hibernate/orm/test/rowid/{RowIdUpdateTest.java => RowIdUpdateAndDeleteTest.java} (76%) diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateAndDeleteTest.java similarity index 76% rename from hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateTest.java rename to hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateAndDeleteTest.java index e472badee51b..20cee8e18585 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/rowid/RowIdUpdateAndDeleteTest.java @@ -30,15 +30,21 @@ /** * @author Marco Belladelli */ -@DomainModel( annotatedClasses = { RowIdUpdateTest.SimpleEntity.class, RowIdUpdateTest.ParentEntity.class } ) +@DomainModel( annotatedClasses = { + RowIdUpdateAndDeleteTest.SimpleEntity.class, + RowIdUpdateAndDeleteTest.ParentEntity.class +} ) @SessionFactory( useCollectingStatementInspector = true ) @Jira( "https://hibernate.atlassian.net/browse/HHH-17045" ) -public class RowIdUpdateTest { +@Jira( "https://hibernate.atlassian.net/browse/HHH-17167" ) +public class RowIdUpdateAndDeleteTest { @BeforeAll public void setUp(SessionFactoryScope scope) { scope.inTransaction( session -> { session.persist( new SimpleEntity( 1L, "initial_status" ) ); session.persist( new ParentEntity( 2L, new SimpleEntity( 2L, "initial_status" ) ) ); + session.persist( new SimpleEntity( 11L, "to_delete" ) ); + session.persist( new ParentEntity( 12L, new SimpleEntity( 12L, "to_delete" ) ) ); } ); } @@ -68,6 +74,22 @@ public void testSimpleUpdateSameTransaction(SessionFactoryScope scope) { ).isEqualTo( "new_status" ) ); } + @Test + public void testSimpleDeleteSameTransaction(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + inspector.clear(); + scope.inTransaction( session -> { + final SimpleEntity simpleEntity = new SimpleEntity( 13L, "to_delete" ); + session.persist( simpleEntity ); + session.flush(); + session.remove( simpleEntity ); + inspector.clear(); + } ); + // the update should have used the primary key, as the row-id value is not available + checkUpdateQuery( inspector, true ); + scope.inTransaction( session -> assertThat( session.find( SimpleEntity.class, 13L ) ).isNull() ); + } + @Test public void testRelatedUpdateSameTransaction(SessionFactoryScope scope) { final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); @@ -107,6 +129,19 @@ public void testSimpleUpdateDifferentTransaction(SessionFactoryScope scope) { ).isEqualTo( "new_status" ) ); } + @Test + public void testSimpleDeleteDifferentTransaction(SessionFactoryScope scope) { + final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); + scope.inTransaction( session -> { + final SimpleEntity simpleEntity = session.find( SimpleEntity.class, 11L ); + session.remove( simpleEntity ); + inspector.clear(); + } ); + final Dialect dialect = scope.getSessionFactory().getJdbcServices().getDialect(); + checkUpdateQuery( inspector, dialect.rowId( "" ) == null ); + scope.inTransaction( session -> assertThat( session.find( SimpleEntity.class, 11L ) ).isNull() ); + } + @Test public void testRelatedUpdateRelatedDifferentTransaction(SessionFactoryScope scope) { final SQLStatementInspector inspector = scope.getCollectingStatementInspector(); From 2b73799287f1b9c83cb06e15fe0fdd6db984a178 Mon Sep 17 00:00:00 2001 From: Marco Belladelli Date: Thu, 7 Sep 2023 13:51:17 +0200 Subject: [PATCH 2/2] HHH-17167 Also use `@RowId` for deletes when available --- .../java/org/hibernate/annotations/RowId.java | 2 +- .../mutation/AbstractMutationCoordinator.java | 48 +++++++++++++ .../entity/mutation/DeleteCoordinator.java | 71 +++++-------------- .../mutation/UpdateCoordinatorStandard.java | 55 +------------- 4 files changed, 68 insertions(+), 108 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/annotations/RowId.java b/hibernate-core/src/main/java/org/hibernate/annotations/RowId.java index 3fd7a9fbb16a..e276e36537e3 100644 --- a/hibernate-core/src/main/java/org/hibernate/annotations/RowId.java +++ b/hibernate-core/src/main/java/org/hibernate/annotations/RowId.java @@ -14,7 +14,7 @@ /** * Specifies that a {@code rowid}-like column or pseudo-column should be - * used as the row locator in SQL {@code update} statements for an entity, + * used as the row locator in CRUD operations for an entity, * instead of the primary key of the table. *

* If the {@linkplain org.hibernate.dialect.Dialect SQL dialect} does diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractMutationCoordinator.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractMutationCoordinator.java index a8b3a9e87e34..fc2cad69f9a4 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractMutationCoordinator.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractMutationCoordinator.java @@ -28,6 +28,7 @@ import org.hibernate.sql.model.ast.TableMutation; import org.hibernate.sql.model.ast.builder.ColumnValuesTableMutationBuilder; import org.hibernate.sql.model.ast.builder.MutationGroupBuilder; +import org.hibernate.sql.model.ast.builder.RestrictedTableMutationBuilder; import org.hibernate.sql.model.internal.MutationOperationGroupFactory; /** @@ -165,4 +166,51 @@ protected void bindPartitionColumnValueBindings( } } } + + protected static boolean needsRowId(AbstractEntityPersister entityPersister, EntityTableMapping tableMapping) { + return entityPersister.getRowIdMapping() != null && tableMapping.isIdentifierTable(); + } + + protected static void applyKeyRestriction( + Object rowId, + AbstractEntityPersister entityPersister, + RestrictedTableMutationBuilder tableMutationBuilder, + EntityTableMapping tableMapping) { + if ( rowId != null && needsRowId( entityPersister, tableMapping ) ) { + tableMutationBuilder.addKeyRestrictionLeniently( entityPersister.getRowIdMapping() ); + } + else { + tableMutationBuilder.addKeyRestrictions( tableMapping.getKeyMapping() ); + } + } + + protected void breakDownKeyJdbcValues( + Object id, + Object rowId, + SharedSessionContractImplementor session, + JdbcValueBindings jdbcValueBindings, + EntityTableMapping tableMapping) { + if ( rowId != null && needsRowId( entityPersister(), tableMapping ) ) { + jdbcValueBindings.bindValue( + rowId, + tableMapping.getTableName(), + entityPersister().getRowIdMapping().getRowIdName(), + ParameterUsage.RESTRICT + ); + } + else { + tableMapping.getKeyMapping().breakDownKeyJdbcValues( + id, + (jdbcValue, columnMapping) -> { + jdbcValueBindings.bindValue( + jdbcValue, + tableMapping.getTableName(), + columnMapping.getColumnName(), + ParameterUsage.RESTRICT + ); + }, + session + ); + } + } } diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/DeleteCoordinator.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/DeleteCoordinator.java index f1a48ab96f6d..487a4d4ffb11 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/DeleteCoordinator.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/DeleteCoordinator.java @@ -12,21 +12,18 @@ import org.hibernate.engine.jdbc.mutation.MutationExecutor; import org.hibernate.engine.jdbc.mutation.ParameterUsage; import org.hibernate.engine.jdbc.mutation.group.PreparedStatementDetails; -import org.hibernate.engine.jdbc.mutation.spi.MutationExecutorService; import org.hibernate.engine.spi.EntityEntry; import org.hibernate.engine.spi.PersistenceContext; import org.hibernate.engine.spi.SessionFactoryImplementor; import org.hibernate.engine.spi.SharedSessionContractImplementor; import org.hibernate.metamodel.mapping.AttributeMapping; import org.hibernate.metamodel.mapping.AttributeMappingsList; -import org.hibernate.metamodel.mapping.EntityRowIdMapping; import org.hibernate.metamodel.mapping.EntityVersionMapping; import org.hibernate.metamodel.mapping.SelectableMapping; import org.hibernate.persister.entity.AbstractEntityPersister; import org.hibernate.sql.model.MutationOperation; import org.hibernate.sql.model.MutationOperationGroup; import org.hibernate.sql.model.MutationType; -import org.hibernate.sql.model.ast.ColumnValueBinding; import org.hibernate.sql.model.ast.ColumnValueBindingList; import org.hibernate.sql.model.ast.builder.MutationGroupBuilder; import org.hibernate.sql.model.ast.builder.RestrictedTableMutationBuilder; @@ -52,7 +49,7 @@ public class DeleteCoordinator extends AbstractMutationCoordinator { public DeleteCoordinator(AbstractEntityPersister entityPersister, SessionFactoryImplementor factory) { super( entityPersister, factory ); - this.staticOperationGroup = generateOperationGroup( null, true, null ); + this.staticOperationGroup = generateOperationGroup( "", null, true, null ); this.batchKey = new BasicBatchKey( entityPersister.getEntityName() + "#DELETE" ); if ( !entityPersister.isVersioned() ) { @@ -79,24 +76,23 @@ public void coordinateDelete( final PersistenceContext persistenceContext = session.getPersistenceContextInternal(); final EntityEntry entry = persistenceContext.getEntry( entity ); - final Object[] loadedState = entry != null && isImpliedOptimisticLocking ? entry.getLoadedState() : null; - final Object rowId = entry != null && entityPersister().hasRowId() ? entry.getRowId() : null; + final Object[] loadedState = entry != null ? entry.getLoadedState() : null; + final Object rowId = entry != null ? entry.getRowId() : null; - if ( ( isImpliedOptimisticLocking && loadedState != null ) || rowId != null ) { - doDynamicDelete( entity, id, rowId, loadedState, session ); + if ( ( isImpliedOptimisticLocking && loadedState != null ) || ( rowId == null && entityPersister().hasRowId() ) ) { + doDynamicDelete( entity, id, loadedState, session ); } else { - doStaticDelete( entity, id, entry == null ? null : entry.getLoadedState(), version, session ); + doStaticDelete( entity, id, rowId, loadedState, version, session ); } } protected void doDynamicDelete( Object entity, Object id, - Object rowId, Object[] loadedState, SharedSessionContractImplementor session) { - final MutationOperationGroup operationGroup = generateOperationGroup( loadedState, true, session ); + final MutationOperationGroup operationGroup = generateOperationGroup( null, loadedState, true, session ); final MutationExecutor mutationExecutor = executor( session, operationGroup ); @@ -110,7 +106,7 @@ protected void doDynamicDelete( applyLocking( null, loadedState, mutationExecutor, session ); - applyId( id, rowId, mutationExecutor, operationGroup, session ); + applyId( id, null, mutationExecutor, operationGroup, session ); try { mutationExecutor.execute( @@ -217,14 +213,11 @@ protected void applyId( MutationExecutor mutationExecutor, MutationOperationGroup operationGroup, SharedSessionContractImplementor session) { - final JdbcValueBindings jdbcValueBindings = mutationExecutor.getJdbcValueBindings(); - final EntityRowIdMapping rowIdMapping = entityPersister().getRowIdMapping(); - for ( int position = 0; position < operationGroup.getNumberOfOperations(); position++ ) { final MutationOperation jdbcMutation = operationGroup.getOperation( position ); final EntityTableMapping tableDetails = (EntityTableMapping) jdbcMutation.getTableDetails(); - breakDownIdJdbcValues( id, rowId, session, jdbcValueBindings, rowIdMapping, tableDetails ); + breakDownKeyJdbcValues( id, rowId, session, jdbcValueBindings, tableDetails ); final PreparedStatementDetails statementDetails = mutationExecutor.getPreparedStatementDetails( tableDetails.getTableName() ); if ( statementDetails != null ) { // force creation of the PreparedStatement @@ -234,40 +227,10 @@ protected void applyId( } } - private static void breakDownIdJdbcValues( - Object id, - Object rowId, - SharedSessionContractImplementor session, - JdbcValueBindings jdbcValueBindings, - EntityRowIdMapping rowIdMapping, - EntityTableMapping tableDetails) { - if ( rowId != null && rowIdMapping != null && tableDetails.isIdentifierTable() ) { - jdbcValueBindings.bindValue( - rowId, - tableDetails.getTableName(), - rowIdMapping.getRowIdName(), - ParameterUsage.RESTRICT - ); - } - else { - tableDetails.getKeyMapping().breakDownKeyJdbcValues( - id, - (jdbcValue, columnMapping) -> { - jdbcValueBindings.bindValue( - jdbcValue, - tableDetails.getTableName(), - columnMapping.getColumnName(), - ParameterUsage.RESTRICT - ); - }, - session - ); - } - } - protected void doStaticDelete( Object entity, Object id, + Object rowId, Object[] loadedState, Object version, SharedSessionContractImplementor session) { @@ -299,7 +262,7 @@ protected void doStaticDelete( bindPartitionColumnValueBindings( loadedState, session, jdbcValueBindings ); - applyId( id, null, mutationExecutor, staticOperationGroup, session ); + applyId( id, rowId, mutationExecutor, staticOperationGroup, session ); mutationExecutor.execute( entity, @@ -321,13 +284,14 @@ protected void doStaticDelete( protected MutationOperationGroup resolveNoVersionDeleteGroup(SharedSessionContractImplementor session) { if ( noVersionDeleteGroup == null ) { - noVersionDeleteGroup = generateOperationGroup( null, false, session ); + noVersionDeleteGroup = generateOperationGroup( "", null, false, session ); } return noVersionDeleteGroup; } protected MutationOperationGroup generateOperationGroup( + Object rowId, Object[] loadedState, boolean applyVersion, SharedSessionContractImplementor session) { @@ -340,13 +304,14 @@ protected MutationOperationGroup generateOperationGroup( deleteGroupBuilder.addTableDetailsBuilder( tableDeleteBuilder ); } ); - applyTableDeleteDetails( deleteGroupBuilder, loadedState, applyVersion, session ); + applyTableDeleteDetails( deleteGroupBuilder, rowId, loadedState, applyVersion, session ); return createOperationGroup( null, deleteGroupBuilder.buildMutationGroup() ); } private void applyTableDeleteDetails( MutationGroupBuilder deleteGroupBuilder, + Object rowId, Object[] loadedState, boolean applyVersion, SharedSessionContractImplementor session) { @@ -354,7 +319,7 @@ private void applyTableDeleteDetails( deleteGroupBuilder.forEachTableMutationBuilder( (builder) -> { final EntityTableMapping tableMapping = (EntityTableMapping) builder.getMutatingTable().getTableMapping(); final TableDeleteBuilder tableDeleteBuilder = (TableDeleteBuilder) builder; - applyKeyDetails( tableDeleteBuilder, tableMapping ); + applyKeyRestriction( rowId, entityPersister(), tableDeleteBuilder, tableMapping ); } ); if ( applyVersion ) { @@ -382,10 +347,6 @@ private void applyTableDeleteDetails( // todo (6.2) : apply where + where-fragments } - private static void applyKeyDetails(TableDeleteBuilder tableDeleteBuilder, EntityTableMapping tableMapping) { - tableDeleteBuilder.addKeyRestrictions( tableMapping.getKeyMapping() ); - } - protected void applyOptimisticLocking( MutationGroupBuilder mutationGroupBuilder, Object[] loadedState, diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/UpdateCoordinatorStandard.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/UpdateCoordinatorStandard.java index f0831451785a..86d45e4fa738 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/UpdateCoordinatorStandard.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/UpdateCoordinatorStandard.java @@ -7,10 +7,8 @@ package org.hibernate.persister.entity.mutation; import java.util.ArrayList; -import java.util.HashSet; import java.util.List; import java.util.Locale; -import java.util.Set; import org.hibernate.AssertionFailure; import org.hibernate.HibernateException; @@ -22,7 +20,6 @@ import org.hibernate.engine.jdbc.mutation.JdbcValueBindings; import org.hibernate.engine.jdbc.mutation.MutationExecutor; import org.hibernate.engine.jdbc.mutation.ParameterUsage; -import org.hibernate.sql.model.internal.MutationOperationGroupFactory; import org.hibernate.engine.jdbc.mutation.internal.MutationQueryOptions; import org.hibernate.engine.jdbc.mutation.internal.NoBatchKeyAccess; import org.hibernate.engine.jdbc.mutation.spi.BatchKeyAccess; @@ -37,7 +34,6 @@ import org.hibernate.internal.util.collections.CollectionHelper; import org.hibernate.metamodel.mapping.AttributeMapping; import org.hibernate.metamodel.mapping.AttributeMappingsList; -import org.hibernate.metamodel.mapping.EntityRowIdMapping; import org.hibernate.metamodel.mapping.EntityVersionMapping; import org.hibernate.metamodel.mapping.SelectableMapping; import org.hibernate.metamodel.mapping.SingularAttributeMapping; @@ -53,6 +49,7 @@ import org.hibernate.sql.model.ast.builder.TableUpdateBuilder; import org.hibernate.sql.model.ast.builder.TableUpdateBuilderSkipped; import org.hibernate.sql.model.ast.builder.TableUpdateBuilderStandard; +import org.hibernate.sql.model.internal.MutationOperationGroupFactory; import org.hibernate.sql.model.jdbc.JdbcMutationOperation; import org.hibernate.tuple.entity.EntityMetamodel; @@ -874,38 +871,6 @@ private static void optimisticLock( } ); } - private void breakDownKeyJdbcValues( - Object id, - Object rowId, - SharedSessionContractImplementor session, - JdbcValueBindings jdbcValueBindings, - EntityTableMapping tableMapping) { - // if the mutation is against the identifier table and we need to use row-id... - if ( tableMapping.isIdentifierTable() && entityPersister().hasRowId() && rowId != null ) { - // todo (mutation) : make sure the SQL uses row-id in this case - jdbcValueBindings.bindValue( - rowId, - tableMapping.getTableName(), - entityPersister().getRowIdMapping().getRowIdName(), - ParameterUsage.RESTRICT - ); - } - else { - tableMapping.getKeyMapping().breakDownKeyJdbcValues( - id, - (jdbcValue, columnMapping) -> { - jdbcValueBindings.bindValue( - jdbcValue, - tableMapping.getTableName(), - columnMapping.getColumnName(), - ParameterUsage.RESTRICT - ); - }, - session - ); - } - } - private static void decomposeAttributeMapping( SharedSessionContractImplementor session, JdbcValueBindings jdbcValueBindings, @@ -1100,7 +1065,6 @@ private void applyTableUpdateDetails( DirtinessChecker dirtinessChecker, SharedSessionContractImplementor session) { final EntityVersionMapping versionMapping = entityPersister().getVersionMapping(); - final EntityRowIdMapping rowIdMapping = entityPersister().getRowIdMapping(); final AttributeMappingsList attributeMappings = entityPersister().getAttributeMappings(); final boolean[] versionability = entityPersister().getPropertyVersionability(); final OptimisticLockStyle optimisticLockStyle = entityPersister().optimisticLockStyle(); @@ -1155,7 +1119,7 @@ private void applyTableUpdateDetails( updateGroupBuilder.forEachTableMutationBuilder( (builder) -> { final EntityTableMapping tableMapping = (EntityTableMapping) builder.getMutatingTable().getTableMapping(); final TableUpdateBuilder tableUpdateBuilder = (TableUpdateBuilder) builder; - applyKeyRestriction( rowId, rowIdMapping, tableUpdateBuilder, tableMapping ); + applyKeyRestriction( rowId, entityPersister(), tableUpdateBuilder, tableMapping ); applyPartitionKeyRestriction( tableUpdateBuilder ); } ); } @@ -1177,19 +1141,6 @@ private void applyPartitionKeyRestriction(TableUpdateBuilder tableUpdateBuild } } - private static void applyKeyRestriction( - Object rowId, - EntityRowIdMapping rowIdMapping, - TableUpdateBuilder tableUpdateBuilder, - EntityTableMapping tableMapping) { - if ( rowIdMapping != null && rowId != null && tableMapping.isIdentifierTable() ) { - tableUpdateBuilder.addKeyRestrictionLeniently( rowIdMapping ); - } - else { - tableUpdateBuilder.addKeyRestrictions( tableMapping.getKeyMapping() ); - } - } - private static void applyAttributeLockingDetails( Object[] oldValues, SharedSessionContractImplementor session, @@ -1344,7 +1295,7 @@ else if ( dirtyAttributeIndexes != null ) { || entityPersister().optimisticLockStyle() == DIRTY ) { tablesNeedingDynamicUpdate.add( tableMapping ); } - else if ( rowId == null && entityPersister().getRowIdMapping() != null && tableMapping.isIdentifierTable() ) { + else if ( rowId == null && needsRowId( entityPersister(), tableMapping ) ) { tablesNeedingDynamicUpdate.add( tableMapping ); } }