From 31850409803b444734d578716077986364ec2a40 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Fri, 15 Sep 2023 11:29:11 +0200 Subject: [PATCH 1/3] Revert "[#1756] Add tests for delete with RowId" This reverts commit da5bcca1b0ecc18494c3ea30f914024c4ffebb1c. --- ...ndDeleteTest.java => RowIdUpdateTest.java} | 75 +++---------------- 1 file changed, 12 insertions(+), 63 deletions(-) rename hibernate-reactive-core/src/test/java/org/hibernate/reactive/{RowIdUpdateAndDeleteTest.java => RowIdUpdateTest.java} (69%) diff --git a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.java similarity index 69% rename from hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java rename to hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.java index af84be1e0..5abe53f35 100644 --- a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateAndDeleteTest.java +++ b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/RowIdUpdateTest.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.RowIdUpdateAndDeleteTest} + * Adapted from the test with the same name in Hibernate ORM: {@literal org.hibernate.orm.test.rowid.RowIdUpdateTest} */ -public class RowIdUpdateAndDeleteTest extends BaseReactiveTest { +public class RowIdUpdateTest 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( RowIdUpdateAndDeleteTest::isRowIdQuery, configuration.getProperties() ); + sqlTracker = new SqlStatementTracker( RowIdUpdateTest::isUsingRowId, configuration.getProperties() ); return configuration; } @@ -61,17 +61,15 @@ protected void addServices(StandardServiceRegistryBuilder builder) { sqlTracker.registerService( builder ); } - private static boolean isRowIdQuery(String s) { - return s.toLowerCase().startsWith( "update" ) || s.toLowerCase().startsWith( "delete" ); + private static boolean isUsingRowId(String s) { + return s.toLowerCase().startsWith( "update" ); } @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 SimpleEntity( 11L, "to_delete" ), - new ParentEntity( 12L, new SimpleEntity( 12L, "to_delete" ) ) + new ParentEntity( 2L, new SimpleEntity( 2L, "initial_status" ) ) ) ) ); } @@ -88,30 +86,11 @@ 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( RowIdUpdateAndDeleteTest::shouldUsePrimaryKeyForUpdate ) + .invoke( RowIdUpdateTest::shouldUsePrimaryKey ) .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(); @@ -126,26 +105,11 @@ 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( RowIdUpdateAndDeleteTest::shouldUsePrimaryKeyForUpdate ) + .invoke( RowIdUpdateTest::shouldUsePrimaryKey ) .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(); @@ -156,7 +120,7 @@ public void testSimpleUpdateDifferentTransaction(VertxTestContext context) { ) .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 1L ) ) ) - .invoke( RowIdUpdateAndDeleteTest::shouldUseRowIdForUpdate ) + .invoke( RowIdUpdateTest::shouldUseRowId ) .invoke( entity -> assertThat( entity ).hasFieldOrPropertyWithValue( "status", "new_status" ) ) ); } @@ -169,7 +133,7 @@ public void testRelatedUpdateRelatedDifferentTransaction(VertxTestContext contex .find( ParentEntity.class, 2L ) .invoke( entity -> entity.getChild().setStatus( "new_status" ) ) ) - .invoke( RowIdUpdateAndDeleteTest::shouldUseRowIdForUpdate ) + .invoke( RowIdUpdateTest::shouldUseRowId ) .chain( () -> getMutinySessionFactory() .withSession( session -> session.find( SimpleEntity.class, 2L ) ) ) .invoke( entity -> assertThat( entity ) @@ -178,19 +142,13 @@ public void testRelatedUpdateRelatedDifferentTransaction(VertxTestContext contex ); } - private static void shouldUsePrimaryKeyForUpdate() { + private static void shouldUsePrimaryKey() { assertThat( sqlTracker.getLoggedQueries() ).hasSize( 1 ); assertThat( sqlTracker.getLoggedQueries().get( 0 ) ) .matches( "update SimpleEntity set status=.+ where primary_key=.+" ); } - 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() { + private static void shouldUseRowId() { // Not all databases have a rowId column String rowId = getDialect().rowId( "" ); String column = rowId == null ? "primary_key" : rowId; @@ -199,15 +157,6 @@ private static void shouldUseRowIdForUpdate() { .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 From 1995a1877400bb273dc81c7980414ec7c4fd7710 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Fri, 15 Sep 2023 11:29:11 +0200 Subject: [PATCH 2/3] Revert "[#1756] Fix ignored RowId for delete query" This reverts commit 370df64f9bb9f0adb3b32a062e3c58d55e730f80. --- .../impl/ReactiveEntityDeleteAction.java | 26 ++----- .../mutation/ReactiveDeleteCoordinator.java | 69 +++++++++++++------ .../pool/impl/SqlClientConnection.java | 2 +- 3 files changed, 55 insertions(+), 42 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 a6b716a8c..d821ed87f 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,14 +64,12 @@ public CompletionStage reactiveExecute() throws HibernateException { final boolean veto = isInstanceLoaded() && preDelete(); final Object ck = lockCacheItem(); - return deleteStep( - veto, - (ReactiveEntityPersister) persister, - id, - version, - instance, - session - ).thenAccept( v -> { + + final CompletionStage deleteStep = !isCascadeDeleteEnabled() && !veto + ? ( (ReactiveEntityPersister) persister ).deleteReactive( id, version, instance, session ) + : voidFuture(); + + return deleteStep.thenAccept( v -> { if ( isInstanceLoaded() ) { postDeleteLoaded( id, persister, session, instance, ck ); } @@ -86,16 +84,4 @@ 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 7d4565324..6f1d4f14a 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,10 +11,12 @@ 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; @@ -23,7 +25,6 @@ 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; @@ -50,7 +51,7 @@ public CompletionStage coordinateReactiveDelete(Object entity, Object id, super.coordinateDelete( entity, id, version, session ); return stage != null ? stage : voidFuture(); } - catch (RuntimeException t) { + catch (Throwable t) { if ( stage == null ) { return failedFuture( t ); } @@ -60,18 +61,17 @@ public CompletionStage coordinateReactiveDelete(Object entity, Object id, } @Override - protected void doDynamicDelete(Object entity, Object id, Object[] loadedState, SharedSessionContractImplementor session) { + protected void doDynamicDelete(Object entity, Object id, Object rowId, Object[] loadedState, SharedSessionContractImplementor session) { stage = new CompletableFuture<>(); - final MutationOperationGroup operationGroup = generateOperationGroup( null, loadedState, true, session ); + final MutationOperationGroup operationGroup = generateOperationGroup( loadedState, true, session ); final ReactiveMutationExecutor mutationExecutor = mutationExecutor( session, operationGroup ); - for ( int i = 0; i < operationGroup.getNumberOfOperations(); i++ ) { - final MutationOperation mutation = operationGroup.getOperation( i ); + operationGroup.forEachOperation( (position, mutation) -> { if ( mutation != null ) { final String tableName = mutation.getTableDetails().getTableName(); mutationExecutor.getPreparedStatementDetails( tableName ); } - } + } ); applyLocking( null, loadedState, mutationExecutor, session ); applyId( id, null, mutationExecutor, getStaticDeleteGroup(), session ); @@ -102,29 +102,25 @@ protected void applyId( 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 ); + operationGroup.forEachOperation( (position, jdbcMutation) -> { final EntityTableMapping tableDetails = (EntityTableMapping) jdbcMutation.getTableDetails(); - breakDownKeyJdbcValues( id, rowId, session, jdbcValueBindings, tableDetails ); + breakDownIdJdbcValues( id, rowId, session, jdbcValueBindings, rowIdMapping, 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 rowId, Object[] loadedState, Object version, SharedSessionContractImplementor session) { + protected void doStaticDelete(Object entity, Object id, Object[] loadedState, Object version, SharedSessionContractImplementor session) { stage = new CompletableFuture<>(); final boolean applyVersion = entity != null; final MutationOperationGroup operationGroupToUse = entity == null @@ -132,19 +128,18 @@ protected void doStaticDelete(Object entity, Object id, Object rowId, Object[] l : getStaticDeleteGroup(); final ReactiveMutationExecutor mutationExecutor = mutationExecutor( session, operationGroupToUse ); - for ( int position = 0; position < getStaticDeleteGroup().getNumberOfOperations(); position++ ) { - final MutationOperation mutation = getStaticDeleteGroup().getOperation( position ); + getStaticDeleteGroup().forEachOperation( (position, mutation) -> { 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, rowId, mutationExecutor, getStaticDeleteGroup(), session ); + applyId( id, null, mutationExecutor, getStaticDeleteGroup(), session ); mutationExecutor.executeReactive( entity, null, @@ -163,6 +158,38 @@ protected void doStaticDelete(Object entity, Object id, Object rowId, Object[] l .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 390d81b83..adb684137 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.isEmpty() ) { + if ( parametersBatch.size() > 0 ) { final RowIterator iterator = resultNext.iterator(); if ( iterator.hasNext() ) { while ( iterator.hasNext() ) { From cbb2390259601a2103ae828058499d1a831f9284 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Fri, 15 Sep 2023 11:29:11 +0200 Subject: [PATCH 3/3] Revert "Use latest snapshot" This reverts commit 0d4b8b982eab4aab9c8365403add43e78f7b2b34. --- gradle.properties | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gradle.properties b/gradle.properties index 70d16f3b2..6d3d1f119 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.3.1-SNAPSHOT +#hibernateOrmVersion = 6.2.3.Final # 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.3.0.Final +#hibernateOrmGradlePluginVersion = 6.2.3.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