From 7f007b009774a7d622f859afefc7f5e77c90c888 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Wed, 13 Sep 2023 12:43:19 +0200 Subject: [PATCH 1/3] Use latest snapshot --- gradle.properties | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gradle.properties b/gradle.properties index 6d3d1f119..70d16f3b2 100644 --- a/gradle.properties +++ b/gradle.properties @@ -32,15 +32,15 @@ org.gradle.java.installations.auto-download=false #enableSonatypeOpenSourceSnapshotsRep = true # Enable the maven local repository (for local development when needed) when present (value ignored) -#enableMavenLocalRepo = true +enableMavenLocalRepo = true # Override default Hibernate ORM version -#hibernateOrmVersion = 6.2.3.Final +hibernateOrmVersion = 6.3.1-SNAPSHOT # Override default Hibernate ORM Gradle plugin version # Using the stable version because I don't know how to configure the build to download the snapshot version from # a remote repository -#hibernateOrmGradlePluginVersion = 6.2.3.Final +hibernateOrmGradlePluginVersion = 6.3.0.Final # If set to true, skip Hibernate ORM version parsing (default is true, if set to null) # this is required when using intervals or weird versions or the build will fail From c1e14c7764530bf3d6e57a2cd4c26fa2edc697cc Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Wed, 13 Sep 2023 14:22:22 +0200 Subject: [PATCH 2/3] [#1756] Fix ignored RowId for delete query --- .../impl/ReactiveEntityDeleteAction.java | 26 +++++-- .../mutation/ReactiveDeleteCoordinator.java | 69 ++++++------------- .../pool/impl/SqlClientConnection.java | 2 +- 3 files changed, 42 insertions(+), 55 deletions(-) diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/engine/impl/ReactiveEntityDeleteAction.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/engine/impl/ReactiveEntityDeleteAction.java index d821ed87f..a6b716a8c 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/engine/impl/ReactiveEntityDeleteAction.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/engine/impl/ReactiveEntityDeleteAction.java @@ -64,12 +64,14 @@ public CompletionStage reactiveExecute() throws HibernateException { final boolean veto = isInstanceLoaded() && preDelete(); final Object ck = lockCacheItem(); - - final CompletionStage deleteStep = !isCascadeDeleteEnabled() && !veto - ? ( (ReactiveEntityPersister) persister ).deleteReactive( id, version, instance, session ) - : voidFuture(); - - return deleteStep.thenAccept( v -> { + return deleteStep( + veto, + (ReactiveEntityPersister) persister, + id, + version, + instance, + session + ).thenAccept( v -> { if ( isInstanceLoaded() ) { postDeleteLoaded( id, persister, session, instance, ck ); } @@ -84,4 +86,16 @@ public CompletionStage reactiveExecute() throws HibernateException { } } ); } + + private CompletionStage deleteStep( + boolean veto, + ReactiveEntityPersister persister, + Object id, + Object version, + Object instance, + SharedSessionContractImplementor session) { + return !isCascadeDeleteEnabled() && !veto + ? persister.deleteReactive( id, version, instance, session ) + : voidFuture(); + } } diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/persister/entity/mutation/ReactiveDeleteCoordinator.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/persister/entity/mutation/ReactiveDeleteCoordinator.java index 6f1d4f14a..7d4565324 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/persister/entity/mutation/ReactiveDeleteCoordinator.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/persister/entity/mutation/ReactiveDeleteCoordinator.java @@ -11,12 +11,10 @@ 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.SessionFactoryImplementor; import org.hibernate.engine.spi.SharedSessionContractImplementor; -import org.hibernate.metamodel.mapping.EntityRowIdMapping; import org.hibernate.persister.entity.AbstractEntityPersister; import org.hibernate.persister.entity.mutation.DeleteCoordinator; import org.hibernate.persister.entity.mutation.EntityTableMapping; @@ -25,6 +23,7 @@ import org.hibernate.reactive.engine.jdbc.env.internal.ReactiveMutationExecutor; import org.hibernate.reactive.logging.impl.Log; import org.hibernate.reactive.logging.impl.LoggerFactory; +import org.hibernate.sql.model.MutationOperation; import org.hibernate.sql.model.MutationOperationGroup; import static org.hibernate.engine.jdbc.mutation.internal.ModelMutationHelper.identifiedResultsCheck; @@ -51,7 +50,7 @@ public CompletionStage coordinateReactiveDelete(Object entity, Object id, super.coordinateDelete( entity, id, version, session ); return stage != null ? stage : voidFuture(); } - catch (Throwable t) { + catch (RuntimeException t) { if ( stage == null ) { return failedFuture( t ); } @@ -61,17 +60,18 @@ public CompletionStage coordinateReactiveDelete(Object entity, Object id, } @Override - protected void doDynamicDelete(Object entity, Object id, Object rowId, Object[] loadedState, SharedSessionContractImplementor session) { + protected void doDynamicDelete(Object entity, Object id, Object[] loadedState, SharedSessionContractImplementor session) { stage = new CompletableFuture<>(); - final MutationOperationGroup operationGroup = generateOperationGroup( loadedState, true, session ); + final MutationOperationGroup operationGroup = generateOperationGroup( null, loadedState, true, session ); final ReactiveMutationExecutor mutationExecutor = mutationExecutor( session, operationGroup ); - operationGroup.forEachOperation( (position, mutation) -> { + for ( int i = 0; i < operationGroup.getNumberOfOperations(); i++ ) { + final MutationOperation mutation = operationGroup.getOperation( i ); if ( mutation != null ) { final String tableName = mutation.getTableDetails().getTableName(); mutationExecutor.getPreparedStatementDetails( tableName ); } - } ); + } applyLocking( null, loadedState, mutationExecutor, session ); applyId( id, null, mutationExecutor, getStaticDeleteGroup(), session ); @@ -102,25 +102,29 @@ protected void applyId( MutationOperationGroup operationGroup, SharedSessionContractImplementor session) { final JdbcValueBindings jdbcValueBindings = mutationExecutor.getJdbcValueBindings(); - final EntityRowIdMapping rowIdMapping = entityPersister().getRowIdMapping(); - operationGroup.forEachOperation( (position, jdbcMutation) -> { + 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 ) { PreparedStatementAdaptor.bind( statement -> { - PrepareStatementDetailsAdaptor detailsAdaptor = new PrepareStatementDetailsAdaptor( statementDetails, statement, session.getJdbcServices() ); + PrepareStatementDetailsAdaptor detailsAdaptor = new PrepareStatementDetailsAdaptor( + statementDetails, + statement, + session.getJdbcServices() + ); // force creation of the PreparedStatement //noinspection resource detailsAdaptor.resolveStatement(); } ); } - } ); + } } @Override - protected void doStaticDelete(Object entity, Object id, Object[] loadedState, Object version, SharedSessionContractImplementor session) { + protected void doStaticDelete(Object entity, Object id, Object rowId, Object[] loadedState, Object version, SharedSessionContractImplementor session) { stage = new CompletableFuture<>(); final boolean applyVersion = entity != null; final MutationOperationGroup operationGroupToUse = entity == null @@ -128,18 +132,19 @@ protected void doStaticDelete(Object entity, Object id, Object[] loadedState, Ob : getStaticDeleteGroup(); final ReactiveMutationExecutor mutationExecutor = mutationExecutor( session, operationGroupToUse ); - getStaticDeleteGroup().forEachOperation( (position, mutation) -> { + for ( int position = 0; position < getStaticDeleteGroup().getNumberOfOperations(); position++ ) { + final MutationOperation mutation = getStaticDeleteGroup().getOperation( position ); if ( mutation != null ) { mutationExecutor.getPreparedStatementDetails( mutation.getTableDetails().getTableName() ); } - } ); + } if ( applyVersion ) { applyLocking( version, null, mutationExecutor, session ); } final JdbcValueBindings jdbcValueBindings = mutationExecutor.getJdbcValueBindings(); bindPartitionColumnValueBindings( loadedState, session, jdbcValueBindings ); - applyId( id, null, mutationExecutor, getStaticDeleteGroup(), session ); + applyId( id, rowId, mutationExecutor, getStaticDeleteGroup(), session ); mutationExecutor.executeReactive( entity, null, @@ -158,38 +163,6 @@ protected void doStaticDelete(Object entity, Object id, Object[] loadedState, Ob .whenComplete( this::complete ); } - /** - * Copy and paste of the on in ORM - */ - 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 - ); - } - } - private void complete(Object o, Throwable throwable) { if ( throwable != null ) { stage.toCompletableFuture().completeExceptionally( throwable ); diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientConnection.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientConnection.java index adb684137..390d81b83 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientConnection.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientConnection.java @@ -184,7 +184,7 @@ public CompletionStage updateBatch(String sql, List parametersBatc int i = 0; RowSet resultNext = result; - if ( parametersBatch.size() > 0 ) { + if ( !parametersBatch.isEmpty() ) { final RowIterator iterator = resultNext.iterator(); if ( iterator.hasNext() ) { while ( iterator.hasNext() ) { From a1c7bb9db7cc92e6c1293e171a9be5328520fa16 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Wed, 13 Sep 2023 15:57:19 +0200 Subject: [PATCH 3/3] [#1756] Add tests for delete with RowId Adapt test for Hibernate ORM HHH-17167 --- ...est.java => RowIdUpdateAndDeleteTest.java} | 75 ++++++++++++++++--- 1 file changed, 63 insertions(+), 12 deletions(-) rename hibernate-reactive-core/src/test/java/org/hibernate/reactive/{RowIdUpdateTest.java => RowIdUpdateAndDeleteTest.java} (69%) diff --git a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.java b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java similarity index 69% rename from hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.java rename to hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java index 5abe53f35..af84be1e0 100644 --- a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.java +++ b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java @@ -33,9 +33,9 @@ import static org.hibernate.reactive.testing.DBSelectionExtension.skipTestsFor; /** - * Adapted from the test with the same name in Hibernate ORM: {@literal org.hibernate.orm.test.rowid.RowIdUpdateTest} + * Adapted from the test with the same name in Hibernate ORM: {@literal org.hibernate.orm.test.rowid.RowIdUpdateAndDeleteTest} */ -public class RowIdUpdateTest extends BaseReactiveTest { +public class RowIdUpdateAndDeleteTest extends BaseReactiveTest { // Db2: Exception: IllegalStateException: Needed to have 6 in buffer but only had 0 // Oracle: Vert.x driver doesn't support RowId type parameters @@ -52,7 +52,7 @@ protected Collection> annotatedEntities() { @Override protected Configuration constructConfiguration() { Configuration configuration = super.constructConfiguration(); - sqlTracker = new SqlStatementTracker( RowIdUpdateTest::isUsingRowId, configuration.getProperties() ); + sqlTracker = new SqlStatementTracker( RowIdUpdateAndDeleteTest::isRowIdQuery, configuration.getProperties() ); return configuration; } @@ -61,15 +61,17 @@ protected void addServices(StandardServiceRegistryBuilder builder) { sqlTracker.registerService( builder ); } - private static boolean isUsingRowId(String s) { - return s.toLowerCase().startsWith( "update" ); + private static boolean isRowIdQuery(String s) { + return s.toLowerCase().startsWith( "update" ) || s.toLowerCase().startsWith( "delete" ); } @BeforeEach public void prepareDb(VertxTestContext context) { test( context, getMutinySessionFactory().withTransaction( session -> session.persistAll( new SimpleEntity( 1L, "initial_status" ), - new ParentEntity( 2L, new SimpleEntity( 2L, "initial_status" ) ) + new ParentEntity( 2L, new SimpleEntity( 2L, "initial_status" ) ), + new SimpleEntity( 11L, "to_delete" ), + new ParentEntity( 12L, new SimpleEntity( 12L, "to_delete" ) ) ) ) ); } @@ -86,11 +88,30 @@ public void testSimpleUpdateSameTransaction(VertxTestContext context) { .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 3L ) ) ) // the update should have used the primary key, as the row-id value is not available - .invoke( RowIdUpdateTest::shouldUsePrimaryKey ) + .invoke( RowIdUpdateAndDeleteTest::shouldUsePrimaryKeyForUpdate ) .invoke( entity -> assertThat( entity ).hasFieldOrPropertyWithValue( "status", "new_status" ) ) ); } + @Test + public void testSimpleDeleteSameTransaction(VertxTestContext context) { + sqlTracker.clear(); + test( context, getMutinySessionFactory() + .withTransaction( session -> { + final SimpleEntity simpleEntity = new SimpleEntity( 13L, "to_delete" ); + return session.persist( simpleEntity ) + .call( session::flush ) + .call( () -> session.remove( simpleEntity ) ) + .invoke( () -> simpleEntity.setStatus( "new_status" ) ); + } ) + .chain( () -> getMutinySessionFactory() + .withSession( session -> session.find( SimpleEntity.class, 13L ) ) ) + // the update should have used the primary key, as the row-id value is not available + .invoke( RowIdUpdateAndDeleteTest::shouldUsePrimaryKeyForDelete ) + .invoke( entity -> assertThat( entity ).isNull() ) + ); + } + @Test public void testRelatedUpdateSameTransaction(VertxTestContext context) { sqlTracker.clear(); @@ -105,11 +126,26 @@ public void testRelatedUpdateSameTransaction(VertxTestContext context) { .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 4L ) ) ) // the update should have used the primary key, as the row-id value is not available - .invoke( RowIdUpdateTest::shouldUsePrimaryKey ) + .invoke( RowIdUpdateAndDeleteTest::shouldUsePrimaryKeyForUpdate ) .invoke( entity -> assertThat( entity ).hasFieldOrPropertyWithValue( "status", "new_status" ) ) ); } + @Test + public void testSimpleDeleteDifferentTransaction(VertxTestContext context) { + sqlTracker.clear(); + test( context, getMutinySessionFactory() + .withTransaction( session -> session + .find( SimpleEntity.class, 11L ) + .call( session::remove ) + ) + .chain( () -> getMutinySessionFactory() + .withSession( session -> session.find( SimpleEntity.class, 11L ) ) ) + .invoke( RowIdUpdateAndDeleteTest::shouldUseRowIdForDelete ) + .invoke( entity -> assertThat( entity ).isNull() ) + ); + } + @Test public void testSimpleUpdateDifferentTransaction(VertxTestContext context) { sqlTracker.clear(); @@ -120,7 +156,7 @@ public void testSimpleUpdateDifferentTransaction(VertxTestContext context) { ) .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 1L ) ) ) - .invoke( RowIdUpdateTest::shouldUseRowId ) + .invoke( RowIdUpdateAndDeleteTest::shouldUseRowIdForUpdate ) .invoke( entity -> assertThat( entity ).hasFieldOrPropertyWithValue( "status", "new_status" ) ) ); } @@ -133,7 +169,7 @@ public void testRelatedUpdateRelatedDifferentTransaction(VertxTestContext contex .find( ParentEntity.class, 2L ) .invoke( entity -> entity.getChild().setStatus( "new_status" ) ) ) - .invoke( RowIdUpdateTest::shouldUseRowId ) + .invoke( RowIdUpdateAndDeleteTest::shouldUseRowIdForUpdate ) .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 2L ) ) ) .invoke( entity -> assertThat( entity ) @@ -142,13 +178,19 @@ public void testRelatedUpdateRelatedDifferentTransaction(VertxTestContext contex ); } - private static void shouldUsePrimaryKey() { + private static void shouldUsePrimaryKeyForUpdate() { assertThat( sqlTracker.getLoggedQueries() ).hasSize( 1 ); assertThat( sqlTracker.getLoggedQueries().get( 0 ) ) .matches( "update SimpleEntity set status=.+ where primary_key=.+" ); } - private static void shouldUseRowId() { + private static void shouldUsePrimaryKeyForDelete() { + assertThat( sqlTracker.getLoggedQueries() ).hasSize( 1 ); + assertThat( sqlTracker.getLoggedQueries().get( 0 ) ) + .matches( "delete from SimpleEntity where primary_key=.+" ); + } + + private static void shouldUseRowIdForUpdate() { // Not all databases have a rowId column String rowId = getDialect().rowId( "" ); String column = rowId == null ? "primary_key" : rowId; @@ -157,6 +199,15 @@ private static void shouldUseRowId() { .matches( "update SimpleEntity set status=.+ where " + column + "=.+" ); } + private static void shouldUseRowIdForDelete() { + // Not all databases have a rowId column + String rowId = getDialect().rowId( "" ); + String column = rowId == null ? "primary_key" : rowId; + assertThat( sqlTracker.getLoggedQueries() ).hasSize( 1 ); + assertThat( sqlTracker.getLoggedQueries().get( 0 ) ) + .matches( "delete from SimpleEntity where " + column + "=.+" ); + } + @Entity(name = "SimpleEntity") @Table(name = "SimpleEntity") @RowId