From ee00217733018075ccade7e1145f45ff9acae0c2 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sun, 8 Sep 2024 12:42:52 +0200 Subject: [PATCH] HHH-18586 report StaleObjectStateExceptions when batch update fails and some minor cleanups to the Coordinators Signed-off-by: Gavin King --- .../hibernate/StaleObjectStateException.java | 13 ++ .../org/hibernate/StaleStateException.java | 11 ++ .../engine/jdbc/batch/internal/BatchImpl.java | 28 +++- .../engine/jdbc/batch/spi/Batch.java | 13 ++ .../jdbc/mutation/MutationExecutor.java | 9 ++ .../internal/AbstractMutationExecutor.java | 22 ++- .../internal/ModelMutationHelper.java | 2 +- .../MutationExecutorSingleBatched.java | 7 +- .../internal/MutationExecutorStandard.java | 5 +- .../java/org/hibernate/jdbc/Expectations.java | 24 +-- .../entity/AbstractEntityPersister.java | 2 +- .../mutation/AbstractDeleteCoordinator.java | 99 ++++++------ .../mutation/AbstractMutationCoordinator.java | 2 +- .../mutation/InsertCoordinatorStandard.java | 38 +++-- .../mutation/UpdateCoordinatorStandard.java | 143 +++++++++--------- .../batch/BatchOptimisticLockingTest.java | 6 +- .../test/batch/BatchUpdateAndVersionTest.java | 60 +++++++- .../batch/AbstractBatchingTest.java | 5 + 18 files changed, 325 insertions(+), 164 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/StaleObjectStateException.java b/hibernate-core/src/main/java/org/hibernate/StaleObjectStateException.java index 3fe0a875bc..315d285a65 100644 --- a/hibernate-core/src/main/java/org/hibernate/StaleObjectStateException.java +++ b/hibernate-core/src/main/java/org/hibernate/StaleObjectStateException.java @@ -40,6 +40,19 @@ public class StaleObjectStateException extends StaleStateException { this.identifier = identifier; } + /** + * Constructs a {@code StaleObjectStateException} using the supplied information + * and cause. + * + * @param entityName The name of the entity + * @param identifier The identifier of the entity + */ + public StaleObjectStateException(String entityName, Object identifier, StaleStateException cause) { + super( cause.getMessage(), cause ); + this.entityName = entityName; + this.identifier = identifier; + } + public String getEntityName() { return entityName; } diff --git a/hibernate-core/src/main/java/org/hibernate/StaleStateException.java b/hibernate-core/src/main/java/org/hibernate/StaleStateException.java index a56dabe26a..406d5dbde0 100644 --- a/hibernate-core/src/main/java/org/hibernate/StaleStateException.java +++ b/hibernate-core/src/main/java/org/hibernate/StaleStateException.java @@ -23,4 +23,15 @@ public class StaleStateException extends HibernateException { public StaleStateException(String message) { super( message ); } + + /** + * Constructs a {@code StaleStateException} using the supplied message + * and cause. + * + * @param message The message explaining the exception condition + * @param cause An exception to wrap + */ + public StaleStateException(String message, Exception cause) { + super( message, cause ); + } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchImpl.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchImpl.java index 083fd0d2f8..37a6e71ad1 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/internal/BatchImpl.java @@ -11,6 +11,7 @@ import java.sql.SQLException; import java.util.LinkedHashSet; import org.hibernate.HibernateException; +import org.hibernate.StaleStateException; import org.hibernate.engine.jdbc.batch.spi.Batch; import org.hibernate.engine.jdbc.batch.spi.BatchKey; import org.hibernate.engine.jdbc.batch.spi.BatchObserver; @@ -50,6 +51,7 @@ public class BatchImpl implements Batch { private int batchPosition; private boolean batchExecuted; + private StaleStateMapper[] staleStateMappers; public BatchImpl( BatchKey key, @@ -97,6 +99,19 @@ public class BatchImpl implements Batch { observers.add( observer ); } + @Override + public void addToBatch( + JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker, + StaleStateMapper staleStateMapper) { + if ( staleStateMapper != null ) { + if ( staleStateMappers == null ) { + staleStateMappers = new StaleStateMapper[batchSizeToUse]; + } + staleStateMappers[batchPosition] = staleStateMapper; + } + addToBatch( jdbcValueBindings, inclusionChecker ); + } + @Override public void addToBatch(JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker) { final boolean loggerTraceEnabled = BATCH_LOGGER.isTraceEnabled(); @@ -304,7 +319,8 @@ public class BatchImpl implements Batch { } } - private void checkRowCounts(int[] rowCounts, PreparedStatementDetails statementDetails) throws SQLException, HibernateException { + private void checkRowCounts(int[] rowCounts, PreparedStatementDetails statementDetails) + throws SQLException, HibernateException { final int numberOfRowCounts = rowCounts.length; if ( batchPosition != 0 ) { if ( numberOfRowCounts != batchPosition ) { @@ -317,7 +333,15 @@ public class BatchImpl implements Batch { } for ( int i = 0; i < numberOfRowCounts; i++ ) { - statementDetails.getExpectation().verifyOutcome( rowCounts[i], statementDetails.getStatement(), i, statementDetails.getSqlString() ); + try { + statementDetails.getExpectation() + .verifyOutcome( rowCounts[i], statementDetails.getStatement(), i, statementDetails.getSqlString() ); + } + catch ( StaleStateException staleStateException ) { + if ( staleStateMappers != null ) { + throw staleStateMappers[i].map( staleStateException ); + } + } } } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/spi/Batch.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/spi/Batch.java index f2a90d197c..607d5b17ed 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/spi/Batch.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/batch/spi/Batch.java @@ -9,7 +9,9 @@ package org.hibernate.engine.jdbc.batch.spi; import java.sql.PreparedStatement; import java.util.function.Supplier; +import org.hibernate.HibernateException; import org.hibernate.Incubating; +import org.hibernate.StaleStateException; import org.hibernate.engine.jdbc.mutation.JdbcValueBindings; import org.hibernate.engine.jdbc.mutation.TableInclusionChecker; import org.hibernate.engine.jdbc.mutation.group.PreparedStatementGroup; @@ -51,6 +53,17 @@ public interface Batch { */ void addToBatch(JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker); + /** + * Apply the value bindings to the batch JDBC statements and indicates completion + * of the current part of the batch. + */ + void addToBatch(JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker, StaleStateMapper staleStateMapper); + + @FunctionalInterface + interface StaleStateMapper { + HibernateException map(StaleStateException staleStateException); + } + /** * Execute this batch. */ diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/MutationExecutor.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/MutationExecutor.java index ad26b54490..ec941aadf2 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/MutationExecutor.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/MutationExecutor.java @@ -7,6 +7,7 @@ package org.hibernate.engine.jdbc.mutation; import org.hibernate.Incubating; +import org.hibernate.engine.jdbc.batch.spi.Batch; import org.hibernate.engine.jdbc.mutation.group.PreparedStatementDetails; import org.hibernate.engine.spi.SharedSessionContractImplementor; import org.hibernate.generator.values.GeneratedValues; @@ -50,5 +51,13 @@ public interface MutationExecutor { OperationResultChecker resultChecker, SharedSessionContractImplementor session); + GeneratedValues execute( + Object modelReference, + ValuesAnalysis valuesAnalysis, + TableInclusionChecker inclusionChecker, + OperationResultChecker resultChecker, + SharedSessionContractImplementor session, + Batch.StaleStateMapper staleStateMapper); + void release(); } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/AbstractMutationExecutor.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/AbstractMutationExecutor.java index 1698c1f08e..42b9c792a4 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/AbstractMutationExecutor.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/AbstractMutationExecutor.java @@ -8,6 +8,7 @@ package org.hibernate.engine.jdbc.mutation.internal; import java.sql.SQLException; +import org.hibernate.engine.jdbc.batch.spi.Batch; import org.hibernate.engine.jdbc.batch.spi.BatchKey; import org.hibernate.engine.jdbc.mutation.JdbcValueBindings; import org.hibernate.engine.jdbc.mutation.MutationExecutor; @@ -21,6 +22,7 @@ import org.hibernate.persister.entity.mutation.EntityTableMapping; import org.hibernate.sql.model.TableMapping; import org.hibernate.sql.model.ValuesAnalysis; +import static org.hibernate.engine.jdbc.mutation.internal.ModelMutationHelper.checkResults; import static org.hibernate.sql.model.ModelMutationLogging.MODEL_MUTATION_LOGGER; /** @@ -52,6 +54,17 @@ public abstract class AbstractMutationExecutor implements MutationExecutor { TableInclusionChecker inclusionChecker, OperationResultChecker resultChecker, SharedSessionContractImplementor session) { + return execute( modelReference, valuesAnalysis, inclusionChecker, resultChecker, session, null ); + } + + @Override + public final GeneratedValues execute( + Object modelReference, + ValuesAnalysis valuesAnalysis, + TableInclusionChecker inclusionChecker, + OperationResultChecker resultChecker, + SharedSessionContractImplementor session, + Batch.StaleStateMapper staleStateMapper) { final GeneratedValues generatedValues = performNonBatchedOperations( modelReference, valuesAnalysis, @@ -60,10 +73,12 @@ public abstract class AbstractMutationExecutor implements MutationExecutor { session ); performSelfExecutingOperations( valuesAnalysis, inclusionChecker, session ); - performBatchedOperations( valuesAnalysis, inclusionChecker ); + performBatchedOperations( valuesAnalysis, inclusionChecker, staleStateMapper ); return generatedValues; } + + protected GeneratedValues performNonBatchedOperations( Object modelReference, ValuesAnalysis valuesAnalysis, @@ -81,7 +96,8 @@ public abstract class AbstractMutationExecutor implements MutationExecutor { protected void performBatchedOperations( ValuesAnalysis valuesAnalysis, - TableInclusionChecker inclusionChecker) { + TableInclusionChecker inclusionChecker, + Batch.StaleStateMapper staleStateMapper) { } /** @@ -138,7 +154,7 @@ public abstract class AbstractMutationExecutor implements MutationExecutor { return; } - ModelMutationHelper.checkResults( resultChecker, statementDetails, affectedRowCount, -1 ); + checkResults( resultChecker, statementDetails, affectedRowCount, -1 ); } catch (SQLException e) { throw session.getJdbcServices().getSqlExceptionHelper().convert( diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/ModelMutationHelper.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/ModelMutationHelper.java index 4bb44617e3..f75701d84a 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/ModelMutationHelper.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/ModelMutationHelper.java @@ -72,7 +72,7 @@ public class ModelMutationHelper { if ( statistics.isStatisticsEnabled() ) { statistics.optimisticFailure( mutationTarget.getNavigableRole().getFullPath() ); } - throw new StaleObjectStateException( mutationTarget.getNavigableRole().getFullPath(), id ); + throw new StaleObjectStateException( mutationTarget.getNavigableRole().getFullPath(), id, e ); } return false; } diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorSingleBatched.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorSingleBatched.java index eff7daf09b..09c0f9de18 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorSingleBatched.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorSingleBatched.java @@ -56,8 +56,11 @@ public class MutationExecutorSingleBatched extends AbstractSingleMutationExecuto } @Override - protected void performBatchedOperations(ValuesAnalysis valuesAnalysis, TableInclusionChecker inclusionChecker) { - resolveBatch().addToBatch( getJdbcValueBindings(), inclusionChecker ); + protected void performBatchedOperations( + ValuesAnalysis valuesAnalysis, + TableInclusionChecker inclusionChecker, + Batch.StaleStateMapper staleStateMapper) { + resolveBatch().addToBatch( getJdbcValueBindings(), inclusionChecker, staleStateMapper ); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorStandard.java b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorStandard.java index 937a7e72de..927306f3ec 100644 --- a/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorStandard.java +++ b/hibernate-core/src/main/java/org/hibernate/engine/jdbc/mutation/internal/MutationExecutorStandard.java @@ -285,11 +285,12 @@ public class MutationExecutorStandard extends AbstractMutationExecutor implement @Override protected void performBatchedOperations( ValuesAnalysis valuesAnalysis, - TableInclusionChecker inclusionChecker) { + TableInclusionChecker inclusionChecker, + Batch.StaleStateMapper staleStateMapper) { if ( batch == null ) { return; } - batch.addToBatch( valueBindings, inclusionChecker ); + batch.addToBatch( valueBindings, inclusionChecker, staleStateMapper ); } @Override diff --git a/hibernate-core/src/main/java/org/hibernate/jdbc/Expectations.java b/hibernate-core/src/main/java/org/hibernate/jdbc/Expectations.java index a90daed36d..7bb463549f 100644 --- a/hibernate-core/src/main/java/org/hibernate/jdbc/Expectations.java +++ b/hibernate-core/src/main/java/org/hibernate/jdbc/Expectations.java @@ -76,17 +76,15 @@ public class Expectations { default: if ( expectedRowCount > rowCount ) { throw new StaleStateException( - "Batch update returned unexpected row count from update [" - + batchPosition + "]; actual row count: " + rowCount - + "; expected: " + expectedRowCount + "; statement executed: " - + sql + "Batch update returned unexpected row count from update " + batchPosition + + actualVsExpected( expectedRowCount, rowCount ) + + " [" + sql + "]" ); } else if ( expectedRowCount < rowCount ) { throw new BatchedTooManyRowsAffectedException( - "Batch update returned unexpected row count from update [" + - batchPosition + "]; actual row count: " + rowCount + - "; expected: " + expectedRowCount, + "Batch update returned unexpected row count from update " + batchPosition + + actualVsExpected( expectedRowCount, rowCount ), expectedRowCount, rowCount, batchPosition ); } } @@ -95,18 +93,24 @@ public class Expectations { static void checkNonBatched(int expectedRowCount, int rowCount, String sql) { if ( expectedRowCount > rowCount ) { throw new StaleStateException( - "Unexpected row count: " + rowCount + "; expected: " + expectedRowCount - + "; statement executed: " + sql + "Unexpected row count" + + actualVsExpected( expectedRowCount, rowCount ) + + " [" + sql + "]" ); } if ( expectedRowCount < rowCount ) { throw new TooManyRowsAffectedException( - "Unexpected row count: " + rowCount + "; expected: " + expectedRowCount, + "Unexpected row count" + + actualVsExpected( expectedRowCount, rowCount ), 1, rowCount ); } } + private static String actualVsExpected(int expectedRowCount, int rowCount) { + return " (expected row count " + expectedRowCount + " but was " + rowCount + ")"; + } + // Various Expectation instances ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /** 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 c900c60857..bc28b31202 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 @@ -2549,7 +2549,7 @@ public abstract class AbstractEntityPersister if ( statistics.isStatisticsEnabled() ) { statistics.optimisticFailure( getEntityName() ); } - throw new StaleObjectStateException( getEntityName(), id ); + throw new StaleObjectStateException( getEntityName(), id, e ); } return false; } diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractDeleteCoordinator.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractDeleteCoordinator.java index 0f613940d7..6dadee89f4 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractDeleteCoordinator.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/AbstractDeleteCoordinator.java @@ -6,6 +6,8 @@ */ package org.hibernate.persister.entity.mutation; +import org.hibernate.StaleObjectStateException; +import org.hibernate.StaleStateException; import org.hibernate.engine.OptimisticLockStyle; import org.hibernate.engine.jdbc.batch.internal.BasicBatchKey; import org.hibernate.engine.jdbc.mutation.JdbcValueBindings; @@ -93,7 +95,8 @@ public abstract class AbstractDeleteCoordinator Object rowId, Object[] loadedState, SharedSessionContractImplementor session) { - final MutationOperationGroup operationGroup = generateOperationGroup( null, loadedState, true, session ); + final MutationOperationGroup operationGroup = + generateOperationGroup( null, loadedState, true, session ); final MutationExecutor mutationExecutor = executor( session, operationGroup ); for ( int i = 0; i < operationGroup.getNumberOfOperations(); i++ ) { @@ -118,15 +121,10 @@ public abstract class AbstractDeleteCoordinator entity, null, null, - (statementDetails, affectedRowCount, batchPosition) -> identifiedResultsCheck( - statementDetails, - affectedRowCount, - batchPosition, - entityPersister(), - id, - factory() - ), - session + (statementDetails, affectedRowCount, batchPosition) -> + resultCheck( id, statementDetails, affectedRowCount, batchPosition ), + session, + staleStateException -> staleObjectState( id, staleStateException ) ); } finally { @@ -171,31 +169,32 @@ public abstract class AbstractDeleteCoordinator final EntityPersister persister = entityPersister(); final boolean[] versionability = persister.getPropertyVersionability(); for ( int attributeIndex = 0; attributeIndex < versionability.length; attributeIndex++ ) { - final AttributeMapping attribute; // only makes sense to lock on singular attributes which are not excluded from optimistic locking - if ( versionability[attributeIndex] && !( attribute = persister.getAttributeMapping( attributeIndex ) ).isPluralAttributeMapping() ) { - final Object loadedValue = loadedState[attributeIndex]; - if ( loadedValue != null ) { - final String mutationTableName = persister.getAttributeMutationTableName( attributeIndex ); - attribute.breakDownJdbcValues( - loadedValue, - 0, - jdbcValueBindings, - mutationTableName, - (valueIndex, bindings, tableName, jdbcValue, jdbcValueMapping) -> { - if ( jdbcValue == null ) { - // presumably the SQL was generated with `is null` - return; - } - bindings.bindValue( - jdbcValue, - tableName, - jdbcValueMapping.getSelectionExpression(), - ParameterUsage.RESTRICT - ); - }, - session - ); + if ( versionability[attributeIndex] ) { + final AttributeMapping attribute = persister.getAttributeMapping( attributeIndex ); + if ( !attribute.isPluralAttributeMapping() ) { + final Object loadedValue = loadedState[attributeIndex]; + if ( loadedValue != null ) { + attribute.breakDownJdbcValues( + loadedValue, + 0, + jdbcValueBindings, + persister.getAttributeMutationTableName( attributeIndex ), + (valueIndex, bindings, tableName, jdbcValue, jdbcValueMapping) -> { + if ( jdbcValue == null ) { + // presumably the SQL was generated with `is null` + return; + } + bindings.bindValue( + jdbcValue, + tableName, + jdbcValueMapping.getSelectionExpression(), + ParameterUsage.RESTRICT + ); + }, + session + ); + } } } } @@ -230,7 +229,8 @@ public abstract class AbstractDeleteCoordinator final MutationOperation jdbcMutation = operationGroup.getOperation( position ); final EntityTableMapping tableDetails = (EntityTableMapping) jdbcMutation.getTableDetails(); breakDownKeyJdbcValues( id, rowId, session, jdbcValueBindings, tableDetails ); - final PreparedStatementDetails statementDetails = mutationExecutor.getPreparedStatementDetails( tableDetails.getTableName() ); + final PreparedStatementDetails statementDetails = + mutationExecutor.getPreparedStatementDetails( tableDetails.getTableName() ); if ( statementDetails != null ) { // force creation of the PreparedStatement //noinspection resource @@ -279,20 +279,31 @@ public abstract class AbstractDeleteCoordinator entity, null, null, - (statementDetails, affectedRowCount, batchPosition) -> identifiedResultsCheck( - statementDetails, - affectedRowCount, - batchPosition, - entityPersister(), - id, - factory() - ), - session + (statementDetails, affectedRowCount, batchPosition) -> + resultCheck( id, statementDetails, affectedRowCount, batchPosition ), + session, + staleStateException -> staleObjectState( id, staleStateException ) ); mutationExecutor.release(); } + private StaleObjectStateException staleObjectState(Object id, StaleStateException staleStateException) { + return new StaleObjectStateException( entityPersister.getEntityName(), id, staleStateException ); + } + + private boolean resultCheck( + Object id, PreparedStatementDetails statementDetails, int affectedRowCount, int batchPosition) { + return identifiedResultsCheck( + statementDetails, + affectedRowCount, + batchPosition, + entityPersister, + id, + factory + ); + } + protected void applyStaticDeleteTableDetails( Object id, Object rowId, 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 ae299d12cd..d5335d2d5c 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 @@ -95,7 +95,7 @@ public abstract class AbstractMutationCoordinator { int outputIndex = 0; int skipped = 0; for ( int i = 0; i < mutationGroup.getNumberOfTableMutations(); i++ ) { - final TableMutation tableMutation = mutationGroup.getTableMutation( i ); + final TableMutation tableMutation = mutationGroup.getTableMutation( i ); final MutationOperation operation = tableMutation.createMutationOperation( valuesAnalysis, factory ); if ( operation != null ) { operations[outputIndex++] = operation; diff --git a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/InsertCoordinatorStandard.java b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/InsertCoordinatorStandard.java index 9baa75830e..8df56a036e 100644 --- a/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/InsertCoordinatorStandard.java +++ b/hibernate-core/src/main/java/org/hibernate/persister/entity/mutation/InsertCoordinatorStandard.java @@ -6,6 +6,7 @@ */ package org.hibernate.persister.entity.mutation; +import java.sql.SQLException; import java.util.ArrayList; import java.util.List; @@ -17,6 +18,7 @@ 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.TableInclusionChecker; +import org.hibernate.engine.jdbc.mutation.group.PreparedStatementDetails; import org.hibernate.engine.spi.SessionFactoryImplementor; import org.hibernate.engine.spi.SharedSessionContractImplementor; import org.hibernate.generator.BeforeExecutionGenerator; @@ -192,15 +194,7 @@ public class InsertCoordinatorStandard extends AbstractMutationCoordinator imple object, insertValuesAnalysis, tableInclusionChecker, - (statementDetails, affectedRowCount, batchPosition) -> { - statementDetails.getExpectation().verifyOutcome( - affectedRowCount, - statementDetails.getStatement(), - batchPosition, - statementDetails.getSqlString() - ); - return true; - }, + InsertCoordinatorStandard::verifyOutcome, session ); } @@ -300,7 +294,8 @@ public class InsertCoordinatorStandard extends AbstractMutationCoordinator imple SharedSessionContractImplementor session, boolean forceIdentifierBinding) { final boolean[] insertability = getPropertiesToInsert( values ); - final MutationOperationGroup insertGroup = generateDynamicInsertSqlGroup( insertability, object, session, forceIdentifierBinding ); + final MutationOperationGroup insertGroup = + generateDynamicInsertSqlGroup( insertability, object, session, forceIdentifierBinding ); final MutationExecutor mutationExecutor = executor( session, insertGroup, true ); @@ -315,15 +310,7 @@ public class InsertCoordinatorStandard extends AbstractMutationCoordinator imple object, insertValuesAnalysis, tableInclusionChecker, - (statementDetails, affectedRowCount, batchPosition) -> { - statementDetails.getExpectation().verifyOutcome( - affectedRowCount, - statementDetails.getStatement(), - batchPosition, - statementDetails.getSqlString() - ); - return true; - }, + InsertCoordinatorStandard::verifyOutcome, session ); } @@ -332,6 +319,17 @@ public class InsertCoordinatorStandard extends AbstractMutationCoordinator imple } } + private static boolean verifyOutcome(PreparedStatementDetails statementDetails, int affectedRowCount, int batchPosition) + throws SQLException { + statementDetails.getExpectation().verifyOutcome( + affectedRowCount, + statementDetails.getStatement(), + batchPosition, + statementDetails.getSqlString() + ); + return true; + } + private MutationExecutor executor(SharedSessionContractImplementor session, MutationOperationGroup group, boolean dynamicUpdate) { return mutationExecutorService .createExecutor( resolveBatchKeyAccess( dynamicUpdate, session ), group, session ); @@ -415,7 +413,7 @@ public class InsertCoordinatorStandard extends AbstractMutationCoordinator imple attributeInclusions[attributeIndex] = true; attributeMapping.forEachInsertable( insertGroupBuilder ); } - else if ( isValueGenerationInSql( generator, factory().getJdbcServices().getDialect() ) ) { + else if ( isValueGenerationInSql( generator, factory.getJdbcServices().getDialect() ) ) { handleValueGeneration( attributeMapping, insertGroupBuilder, (OnExecutionGenerator) generator ); } } 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 799e96b190..8e72e45a96 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 @@ -14,6 +14,8 @@ import java.util.function.Supplier; import org.hibernate.AssertionFailure; import org.hibernate.HibernateException; import org.hibernate.Internal; +import org.hibernate.StaleObjectStateException; +import org.hibernate.StaleStateException; import org.hibernate.dialect.Dialect; import org.hibernate.engine.OptimisticLockStyle; import org.hibernate.engine.jdbc.batch.internal.BasicBatchKey; @@ -21,6 +23,7 @@ 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.engine.jdbc.mutation.group.PreparedStatementDetails; import org.hibernate.engine.jdbc.mutation.internal.MutationQueryOptions; import org.hibernate.engine.jdbc.mutation.internal.NoBatchKeyAccess; import org.hibernate.engine.jdbc.mutation.spi.BatchKeyAccess; @@ -99,6 +102,7 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple } //Used by Hibernate Reactive to efficiently create new instances of this same class + @SuppressWarnings("unused") protected UpdateCoordinatorStandard( EntityPersister entityPersister, SessionFactoryImplementor factory, @@ -296,7 +300,6 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple session ); - //noinspection StatementWithEmptyBody if ( valuesAnalysis.tablesNeedingUpdate.isEmpty() && valuesAnalysis.tablesNeedingDynamicUpdate.isEmpty() ) { // nothing to do return null; @@ -477,9 +480,11 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple SharedSessionContractImplementor session) { assert versionUpdateGroup != null; - final EntityTableMapping mutatingTableDetails = (EntityTableMapping) versionUpdateGroup.getSingleOperation().getTableDetails(); + final EntityTableMapping mutatingTableDetails = + (EntityTableMapping) versionUpdateGroup.getSingleOperation().getTableDetails(); - final MutationExecutor mutationExecutor = updateVersionExecutor( session, versionUpdateGroup, false, batching ); + final MutationExecutor mutationExecutor = + updateVersionExecutor( session, versionUpdateGroup, false, batching ); final EntityVersionMapping versionMapping = entityPersister().getVersionMapping(); @@ -494,14 +499,13 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple // restrict the key mutatingTableDetails.getKeyMapping().breakDownKeyJdbcValues( id, - (jdbcValue, columnMapping) -> { - mutationExecutor.getJdbcValueBindings().bindValue( - jdbcValue, - mutatingTableDetails.getTableName(), - columnMapping.getColumnName(), - ParameterUsage.RESTRICT - ); - }, + (jdbcValue, columnMapping) -> + mutationExecutor.getJdbcValueBindings().bindValue( + jdbcValue, + mutatingTableDetails.getTableName(), + columnMapping.getColumnName(), + ParameterUsage.RESTRICT + ), session ); @@ -517,16 +521,11 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple return mutationExecutor.execute( entity, null, - (tableMapping) -> tableMapping.getTableName().equals( entityPersister().getIdentifierTableName() ), - (statementDetails, affectedRowCount, batchPosition) -> identifiedResultsCheck( - statementDetails, - affectedRowCount, - batchPosition, - entityPersister(), - id, - factory() - ), - session + tableMapping -> tableMapping.getTableName().equals( entityPersister.getIdentifierTableName() ), + (statementDetails, affectedRowCount, batchPosition) -> + resultCheck( id, statementDetails, affectedRowCount, batchPosition ), + session, + staleStateException -> staleObjectStateException( id, staleStateException ) ); } finally { @@ -636,8 +635,7 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple try { if ( attributeMapping.getJdbcTypeCount() > 0 - && attributeMapping instanceof SingularAttributeMapping ) { - SingularAttributeMapping asSingularAttributeMapping = (SingularAttributeMapping) attributeMapping; + && attributeMapping instanceof SingularAttributeMapping asSingularAttributeMapping ) { processAttribute( entity, analysis, @@ -771,15 +769,10 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple entity, valuesAnalysis, valuesAnalysis.tablesNeedingUpdate::contains, - (statementDetails, affectedRowCount, batchPosition) -> identifiedResultsCheck( - statementDetails, - affectedRowCount, - batchPosition, - entityPersister(), - id, - factory() - ), - session + (statementDetails, affectedRowCount, batchPosition) -> + resultCheck( id, statementDetails, affectedRowCount, batchPosition ), + session, + staleStateException -> staleObjectStateException( id, staleStateException ) ); } finally { @@ -911,7 +904,8 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple return true; } else if ( entityPersister().getEntityMetamodel().isDynamicUpdate() && dirtinessChecker != null ) { - return attributeAnalysis.includeInSet() && dirtinessChecker.isDirty( attributeIndex, attributeMapping ).isDirty(); + return attributeAnalysis.includeInSet() + && dirtinessChecker.isDirty( attributeIndex, attributeMapping ).isDirty(); } else { return true; @@ -948,7 +942,10 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple valuesAnalysis, mutationExecutor, dynamicUpdateGroup, - (attributeIndex, attribute) -> dirtinessChecker.include( attributeIndex, (SingularAttributeMapping) attribute ) ? AttributeAnalysis.DirtynessStatus.CONSIDER_LIKE_DIRTY : AttributeAnalysis.DirtynessStatus.NOT_DIRTY, + (attributeIndex, attribute) -> + dirtinessChecker.include( attributeIndex, (SingularAttributeMapping) attribute ) + ? AttributeAnalysis.DirtynessStatus.CONSIDER_LIKE_DIRTY + : AttributeAnalysis.DirtynessStatus.NOT_DIRTY, session ); bindPartitionColumnValueBindings( oldValues, session, mutationExecutor.getJdbcValueBindings() ); @@ -957,28 +954,15 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple return mutationExecutor.execute( entity, valuesAnalysis, - (tableMapping) -> { - if ( tableMapping.isOptional() - && !valuesAnalysis.tablesWithNonNullValues.contains( tableMapping ) ) { - // the table is optional, and we have null values for all of its columns - if ( valuesAnalysis.dirtyAttributeIndexes.length > 0 ) { - return true; - } - return false; - } - else { - return valuesAnalysis.tablesNeedingUpdate.contains( tableMapping ); - } - }, - (statementDetails, affectedRowCount, batchPosition) -> identifiedResultsCheck( - statementDetails, - affectedRowCount, - batchPosition, - entityPersister(), - id, - factory() - ), - session + tableMapping -> + tableMapping.isOptional() && !valuesAnalysis.tablesWithNonNullValues.contains( tableMapping ) + // the table is optional, and we have null values for all of its columns + ? valuesAnalysis.dirtyAttributeIndexes.length > 0 + : valuesAnalysis.tablesNeedingUpdate.contains( tableMapping ), + (statementDetails, affectedRowCount, batchPosition) -> + resultCheck( id, statementDetails, affectedRowCount, batchPosition ), + session, + staleStateException -> staleObjectStateException( id, staleStateException ) ); } finally { @@ -986,12 +970,14 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple } } - private MutationExecutor executor(SharedSessionContractImplementor session, MutationOperationGroup group, boolean dynamicUpdate) { + private MutationExecutor executor( + SharedSessionContractImplementor session, MutationOperationGroup group, boolean dynamicUpdate) { return mutationExecutorService .createExecutor( resolveBatchKeyAccess( dynamicUpdate, session ), group, session ); } - private MutationExecutor updateVersionExecutor(SharedSessionContractImplementor session, MutationOperationGroup group, boolean dynamicUpdate) { + private MutationExecutor updateVersionExecutor( + SharedSessionContractImplementor session, MutationOperationGroup group, boolean dynamicUpdate) { return mutationExecutorService .createExecutor( resolveUpdateVersionBatchKeyAccess( dynamicUpdate, session ), group, session ); } @@ -1014,8 +1000,9 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple && session.getTransactionCoordinator().isTransactionActive() ) { return this::getVersionUpdateBatchkey; } - - return NoBatchKeyAccess.INSTANCE; + else { + return NoBatchKeyAccess.INSTANCE; + } } //Used by Hibernate Reactive @@ -1023,6 +1010,22 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple return versionUpdateBatchkey; } + private boolean resultCheck( + Object id, PreparedStatementDetails statementDetails, int affectedRowCount, int batchPosition) { + return identifiedResultsCheck( + statementDetails, + affectedRowCount, + batchPosition, + entityPersister, + id, + factory + ); + } + + private StaleObjectStateException staleObjectStateException(Object id, StaleStateException staleStateException) { + return new StaleObjectStateException( entityPersister.getEntityName(), id, staleStateException ); + } + protected MutationOperationGroup generateDynamicUpdateGroup( Object entity, Object id, @@ -1035,7 +1038,6 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple entityPersister().forEachMutableTable( (tableMapping) -> { final MutatingTableReference tableReference = new MutatingTableReference( tableMapping ); final TableMutationBuilder tableUpdateBuilder; - //noinspection SuspiciousMethodCalls if ( ! valuesAnalysis.tablesNeedingUpdate.contains( tableReference.getTableMapping() ) ) { // this table does not need updating tableUpdateBuilder = new TableUpdateBuilderSkipped( tableReference ); @@ -1060,15 +1062,13 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple } private TableMutationBuilder createTableUpdateBuilder(EntityTableMapping tableMapping) { - final GeneratedValuesMutationDelegate delegate = tableMapping.isIdentifierTable() ? - entityPersister().getUpdateDelegate() : - null; - if ( delegate != null ) { - return delegate.createTableMutationBuilder( tableMapping.getInsertExpectation(), factory() ); - } - else { - return newTableUpdateBuilder( tableMapping ); - } + final GeneratedValuesMutationDelegate delegate = + tableMapping.isIdentifierTable() + ? entityPersister().getUpdateDelegate() + : null; + return delegate != null + ? delegate.createTableMutationBuilder( tableMapping.getInsertExpectation(), factory() ) + : newTableUpdateBuilder( tableMapping ); } protected AbstractTableUpdateBuilder newTableUpdateBuilder(EntityTableMapping tableMapping) { @@ -1099,7 +1099,8 @@ public class UpdateCoordinatorStandard extends AbstractMutationCoordinator imple final AttributeAnalysis attributeAnalysis = updateValuesAnalysis.attributeAnalyses.get( attributeIndex ); if ( attributeAnalysis.includeInSet() ) { - assert updateValuesAnalysis.tablesNeedingUpdate.contains( tableMapping ) || updateValuesAnalysis.tablesNeedingDynamicUpdate.contains( tableMapping ); + assert updateValuesAnalysis.tablesNeedingUpdate.contains( tableMapping ) + || updateValuesAnalysis.tablesNeedingDynamicUpdate.contains( tableMapping ); applyAttributeUpdateDetails( entity, updateGroupBuilder, diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchOptimisticLockingTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchOptimisticLockingTest.java index 17708e7471..de01110f14 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchOptimisticLockingTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchOptimisticLockingTest.java @@ -12,10 +12,8 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import org.hibernate.StaleObjectStateException; import org.hibernate.cfg.AvailableSettings; import org.hibernate.dialect.CockroachDialect; -import org.hibernate.dialect.OracleDialect; import org.hibernate.testing.junit4.BaseNonConfigCoreFunctionalTestCase; import org.junit.Test; @@ -109,9 +107,9 @@ public class BatchOptimisticLockingTest extends ); } else { - assertEquals( - "Batch update returned unexpected row count from update [1]; actual row count: 0; expected: 1; statement executed: update Person set name=?,version=? where id=? and version=?", + assertTrue( expected.getMessage() + .startsWith("Batch update returned unexpected row count from update 1 (expected row count 1 but was 0) [update Person set name=?,version=? where id=? and version=?]") ); } } diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchUpdateAndVersionTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchUpdateAndVersionTest.java index 6ac973105e..c9d58c5f06 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchUpdateAndVersionTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/batch/BatchUpdateAndVersionTest.java @@ -4,6 +4,8 @@ package org.hibernate.orm.test.batch; import java.util.ArrayList; import java.util.List; +import jakarta.persistence.OptimisticLockException; +import org.hibernate.StaleObjectStateException; import org.hibernate.cfg.AvailableSettings; import org.hibernate.testing.TestForIssue; @@ -24,6 +26,8 @@ import jakarta.persistence.Table; import jakarta.persistence.Version; import static org.assertj.core.api.AssertionsForClassTypes.assertThat; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; @DomainModel( annotatedClasses = { @@ -34,17 +38,16 @@ import static org.assertj.core.api.AssertionsForClassTypes.assertThat; @ServiceRegistry( settings = @Setting(name = AvailableSettings.STATEMENT_BATCH_SIZE, value = "2") ) -@TestForIssue(jiraKey = "HHH-16394") public class BatchUpdateAndVersionTest { @Test + @TestForIssue(jiraKey = "HHH-16394") public void testUpdate(SessionFactoryScope scope) { + scope.getSessionFactory().getSchemaManager().truncate(); scope.inTransaction( session -> { EntityA entityA1 = new EntityA( 1 ); - EntityA entityA2 = new EntityA( 2 ); - EntityA ownerA2 = new EntityA( 3 ); session.persist( ownerA2 ); @@ -80,6 +83,57 @@ public class BatchUpdateAndVersionTest { ); } + @Test + public void testFailedUpdate(SessionFactoryScope scope) { + scope.getSessionFactory().getSchemaManager().truncate(); + scope.inTransaction( + session -> { + EntityA entityA1 = new EntityA( 1 ); + EntityA entityA2 = new EntityA( 2 ); + EntityA ownerA2 = new EntityA( 3 ); + session.persist( ownerA2 ); + session.persist( entityA1 ); + session.persist( entityA2 ); + } + ); + + try { + scope.inTransaction( + session1 -> { + EntityA entityA1_1 = session1.get( EntityA.class, 1 ); + assertThat( entityA1_1.getVersion() ).isEqualTo( 0 ); + assertThat( entityA1_1.getPropertyA() ).isEqualTo( 0 ); + + EntityA entityA2_1 = session1.get( EntityA.class, 2 ); + assertThat( entityA2_1.getVersion() ).isEqualTo( 0 ); + assertThat( entityA2_1.getPropertyA() ).isEqualTo( 0 ); + + scope.inTransaction( + session2 -> { + EntityA entityA1_2 = session2.get( EntityA.class, 1 ); + assertThat( entityA1_2.getVersion() ).isEqualTo( 0 ); + assertThat( entityA1_2.getPropertyA() ).isEqualTo( 0 ); + + EntityA entityA2_2 = session2.get( EntityA.class, 2 ); + assertThat( entityA2_2.getVersion() ).isEqualTo( 0 ); + assertThat( entityA2_2.getPropertyA() ).isEqualTo( 0 ); + + entityA1_2.setPropertyA( 5 ); + entityA2_2.setPropertyA( 5 ); + } + ); + + entityA1_1.setPropertyA( 3 ); + entityA2_1.setPropertyA( 3 ); + } + ); + fail(); + } + catch (OptimisticLockException ole) { + assertTrue( ole.getCause() instanceof StaleObjectStateException ); + } + } + @Entity(name = "EntityA") @Table(name = "ENTITY_A") public static class EntityA { diff --git a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/transaction/batch/AbstractBatchingTest.java b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/transaction/batch/AbstractBatchingTest.java index 0f1da2420c..e923d13978 100644 --- a/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/transaction/batch/AbstractBatchingTest.java +++ b/hibernate-core/src/test/java/org/hibernate/orm/test/jpa/transaction/batch/AbstractBatchingTest.java @@ -115,6 +115,11 @@ public abstract class AbstractBatchingTest { return wrapped.getStatementGroup(); } + @Override + public void addToBatch(JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker, StaleStateMapper staleStateMapper) { + addToBatch( jdbcValueBindings, inclusionChecker ); + } + @Override public void addToBatch(JdbcValueBindings jdbcValueBindings, TableInclusionChecker inclusionChecker) { numberOfBatches++;