HHH-17167 Also use `@RowId` for deletes when available

hibernate-core/src/main/java/org/hibernate/annotations/RowId.java

@@ -14,7 +14,7 @@ import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 /**
  * 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.
  * <p>
  * If the {@linkplain org.hibernate.dialect.Dialect SQL dialect} does

hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractMutationCoordinator.java

@@ -28,6 +28,7 @@ import org.hibernate.sql.model.ast.MutationGroup;
 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 @@ public abstract class AbstractMutationCoordinator {
 			}
 		}
 	}
+
+	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
+			);
+		}
+	}
 }

hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/DeleteCoordinator.java

@@ -12,21 +12,18 @@ import org.hibernate.engine.jdbc.mutation.JdbcValueBindings;
 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 class DeleteCoordinator extends AbstractMutationCoordinator {
 
 		final PersistenceContext persistenceContext = session.getPersistenceContextInternal();
 		final EntityEntry entry = persistenceContext.getEntry( entity );
-		final Object[] loadedState = entry != null && isImpliedOptimisticLocking ? entry.getLoadedState() : null;
+		final Object[] loadedState = entry != null ? entry.getLoadedState() : null;
-		final Object rowId = entry != null && entityPersister().hasRowId() ? entry.getRowId() : null;
+		final Object rowId = entry != null ? entry.getRowId() : null;
 
-		if ( ( isImpliedOptimisticLocking && loadedState != null ) || rowId != null ) {
+		if ( ( isImpliedOptimisticLocking && loadedState != null ) || ( rowId == null && entityPersister().hasRowId() ) ) {
-			doDynamicDelete( entity, id, rowId, loadedState, session );
+			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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 
 		applyLocking( null, loadedState, mutationExecutor, session );
 
-		applyId( id, rowId, mutationExecutor, operationGroup, session );
+		applyId( id, null, mutationExecutor, operationGroup, session );
 
 		try {
 			mutationExecutor.execute(
@@ -217,14 +213,11 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 			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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 		}
 	}
 
-	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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 
 		bindPartitionColumnValueBindings( loadedState, session, jdbcValueBindings );
 
-		applyId( id, null, mutationExecutor, staticOperationGroup, session );
+		applyId( id, rowId, mutationExecutor, staticOperationGroup, session );
 
 		mutationExecutor.execute(
 				entity,
@@ -321,13 +284,14 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 
 	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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 			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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 		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 @@ public class DeleteCoordinator extends AbstractMutationCoordinator {
 		// 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,

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.batch.spi.BatchKey;
 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.CoreMessageLogger;
 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.RestrictedTableMutationBuilder;
 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 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple
 		} );
 	}
 
-	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 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple
 			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 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple
 		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 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple
 		}
 	}
 
-	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 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple
 								|| 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 );
 						}
 					}