Skip to content

RowId ignored during deletes #1762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 15, 2023
Merged

RowId ignored during deletes #1762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f007b0
Use latest snapshot
DavideD Sep 13, 2023
c1e14c7
[#1756] Fix ignored RowId for delete query
DavideD Sep 13, 2023
a1c7bb9
[#1756] Add tests for delete with RowId
DavideD Sep 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
26 changes: 20 additions & 6 deletions ...ive-core/src/main/java/org/hibernate/reactive/engine/impl/ReactiveEntityDeleteAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ public CompletionStage<Void> reactiveExecute() throws HibernateException {
final boolean veto = isInstanceLoaded() && preDelete();

final Object ck = lockCacheItem();

final CompletionStage<Void> 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 );
}
Expand All @@ -84,4 +86,16 @@ public CompletionStage<Void> reactiveExecute() throws HibernateException {
}
} );
}

private CompletionStage<Void> deleteStep(
boolean veto,
ReactiveEntityPersister persister,
Object id,
Object version,
Object instance,
SharedSessionContractImplementor session) {
return !isCascadeDeleteEnabled() && !veto
? persister.deleteReactive( id, version, instance, session )
: voidFuture();
}
}
69 changes: 21 additions & 48 deletions ...main/java/org/hibernate/reactive/persister/entity/mutation/ReactiveDeleteCoordinator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -51,7 +50,7 @@ public CompletionStage<Void> 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 );
}
Expand All @@ -61,17 +60,18 @@ public CompletionStage<Void> 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 );
Expand Down Expand Up @@ -102,44 +102,49 @@ 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
? resolveNoVersionDeleteGroup( session )
: 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,
Expand All @@ -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 );
Expand Down
2 changes: 1 addition & 1 deletion ...ate-reactive-core/src/main/java/org/hibernate/reactive/pool/impl/SqlClientConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public CompletionStage<int[]> updateBatch(String sql, List<Tuple> parametersBatc

int i = 0;
RowSet<Row> resultNext = result;
if ( parametersBatch.size() > 0 ) {
if ( !parametersBatch.isEmpty() ) {
final RowIterator<Row> iterator = resultNext.iterator();
if ( iterator.hasNext() ) {
while ( iterator.hasNext() ) {
Expand Down
75 changes: 63 additions & 12 deletions ...g/hibernate/reactive/RowIdUpdateTest.java → ...te/reactive/RowIdUpdateAndDeleteTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -52,7 +52,7 @@ protected Collection<Class<?>> 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;
}

Expand All @@ -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" ) )
) ) );
}

Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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" ) )
);
}
Expand All @@ -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 )
Expand All @@ -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;
Expand All @@ -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
Expand Down