Skip to content
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

Revert "RowId ignored during deletes" #1763

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Revert "RowId ignored during deletes" #1763

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
3185040
Revert "[#1756] Add tests for delete with RowId"
DavideD Sep 15, 2023
1995a18
Revert "[#1756] Fix ignored RowId for delete query"
DavideD Sep 15, 2023
cbb2390
Revert "Use latest snapshot"
DavideD Sep 15, 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.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
Expand Down
26 changes: 6 additions & 20 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,14 +64,12 @@ public CompletionStage<Void> 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<Void> deleteStep = !isCascadeDeleteEnabled() && !veto
? ( (ReactiveEntityPersister) persister ).deleteReactive( id, version, instance, session )
: voidFuture();

return deleteStep.thenAccept( v -> {
if ( isInstanceLoaded() ) {
postDeleteLoaded( id, persister, session, instance, ck );
}
Expand All @@ -86,16 +84,4 @@ 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: 48 additions & 21 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,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;
Expand All @@ -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;
Expand All @@ -50,7 +51,7 @@ public CompletionStage<Void> 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 );
}
Expand All @@ -60,18 +61,17 @@ public CompletionStage<Void> 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 );
}
}
} );
Comment on lines +69 to +74

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation

Invoking [MutationOperationGroup.forEachOperation](1) should be avoided because it has been deprecated.

applyLocking( null, loadedState, mutationExecutor, session );
applyId( id, null, mutationExecutor, getStaticDeleteGroup(), session );
Expand Down Expand Up @@ -102,49 +102,44 @@ 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();
} );
}
}
} );
Comment on lines +107 to +119

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation

Invoking [MutationOperationGroup.forEachOperation](1) should be avoided because it has been deprecated.
}

@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
? resolveNoVersionDeleteGroup( session )
: 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() );
}
}
} );
Comment on lines +131 to +135

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation

Invoking [MutationOperationGroup.forEachOperation](1) should be avoided because it has been deprecated.

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,
Expand All @@ -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 );
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.isEmpty() ) {
if ( parametersBatch.size() > 0 ) {
final RowIterator<Row> iterator = resultNext.iterator();
if ( iterator.hasNext() ) {
while ( iterator.hasNext() ) {
Expand Down
75 changes: 12 additions & 63 deletions ...te/reactive/RowIdUpdateAndDeleteTest.java → ...g/hibernate/reactive/RowIdUpdateTest.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.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
Expand All @@ -52,7 +52,7 @@ protected Collection<Class<?>> 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;
}

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

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