From 2d76885fc6d17689287c26af9328f0e990af4335 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Thu, 25 Aug 2022 16:15:27 +0200 Subject: [PATCH] Correct behavior of NOOP deletes to match the specification in `CrudRepository`. Delete operations that receive a version attribute throw an `OptimisticFailureException` when they delete zero rows. Otherwise, the NOOP delete gets silently ignored. Note that save operations that are determined to be an update because the aggregate is not new will still throw an `IncorrectUpdateSemanticsDataAccessException` if they fail to update any row. This is somewhat asymmetric to the delete-behaviour. But with a delete the intended result is achieved: the aggregate is gone from the database. For save operations the intended result is not achieved, hence the exception. Closes #1313 Original pull request: #1314. See https://github.com/spring-projects/spring-data-commons/issues/2651 --- .../data/jdbc/core/AggregateChangeExecutor.java | 5 +++++ .../data/jdbc/core/JdbcAggregateOperations.java | 11 +++++++++++ .../core/JdbcAggregateTemplateIntegrationTests.java | 12 ++++++------ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java index 7b016e41..04a9e01b 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java @@ -15,6 +15,7 @@ */ package org.springframework.data.jdbc.core; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.jdbc.core.convert.DataAccessStrategy; import org.springframework.data.jdbc.core.convert.JdbcConverter; import org.springframework.data.relational.core.conversion.AggregateChange; @@ -87,6 +88,10 @@ class AggregateChangeExecutor { throw new RuntimeException("unexpected action"); } } catch (Exception e) { + + if (e instanceof OptimisticLockingFailureException) { + throw e; + } throw new DbActionExecutionException(action, e); } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java index 73cc7295..89504372 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java @@ -15,6 +15,7 @@ */ package org.springframework.data.jdbc.core; +import org.springframework.dao.IncorrectUpdateSemanticsDataAccessException; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; import org.springframework.data.domain.Sort; @@ -35,6 +36,8 @@ public interface JdbcAggregateOperations { * @param instance the aggregate root of the aggregate to be saved. Must not be {@code null}. * @param the type of the aggregate root. * @return the saved instance. + * @throws IncorrectUpdateSemanticsDataAccessException when the instance is determined to be not new and the resulting + * update does not update any rows. */ T save(T instance); @@ -62,6 +65,11 @@ public interface JdbcAggregateOperations { /** * Deletes a single Aggregate including all entities contained in that aggregate. + *

+ * Since no version attribute is provided this method will never throw a + * {@link org.springframework.dao.OptimisticLockingFailureException}. If no rows match the generated delete operation + * this fact will be silently ignored. + *

* * @param id the id of the aggregate root of the aggregate to be deleted. Must not be {@code null}. * @param domainType the type of the aggregate root. @@ -75,6 +83,9 @@ public interface JdbcAggregateOperations { * @param aggregateRoot to delete. Must not be {@code null}. * @param domainType the type of the aggregate root. Must not be {@code null}. * @param the type of the aggregate root. + * @throws org.springframework.dao.OptimisticLockingFailureException when {@literal T} has a version attribute and the + * version attribute of the provided entity does not match the version attribute in the database, or when + * there is no aggregate root with matching id. In other cases a NOOP delete is silently ignored. */ void delete(T aggregateRoot, Class domainType); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java index c57a66c9..b4647a4f 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java @@ -828,11 +828,11 @@ class JdbcAggregateTemplateIntegrationTests { assertThatThrownBy(() -> template.save(new AggregateWithImmutableVersion(id, 0L))) .describedAs("saving an aggregate with an outdated version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); assertThatThrownBy(() -> template.save(new AggregateWithImmutableVersion(id, 2L))) .describedAs("saving an aggregate with a future version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); } @Test // GH-1137 @@ -877,12 +877,12 @@ class JdbcAggregateTemplateIntegrationTests { assertThatThrownBy( () -> template.delete(new AggregateWithImmutableVersion(id, 0L), AggregateWithImmutableVersion.class)) .describedAs("deleting an aggregate with an outdated version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); assertThatThrownBy( () -> template.delete(new AggregateWithImmutableVersion(id, 2L), AggregateWithImmutableVersion.class)) .describedAs("deleting an aggregate with a future version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); // This should succeed template.delete(aggregate, AggregateWithImmutableVersion.class); @@ -1047,12 +1047,12 @@ class JdbcAggregateTemplateIntegrationTests { reloadedAggregate.setVersion(toConcreteNumber.apply(initialId)); assertThatThrownBy(() -> template.save(reloadedAggregate)) .withFailMessage("saving an aggregate with an outdated version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); reloadedAggregate.setVersion(toConcreteNumber.apply(initialId + 2)); assertThatThrownBy(() -> template.save(reloadedAggregate)) .withFailMessage("saving an aggregate with a future version should raise an exception") - .hasRootCauseInstanceOf(OptimisticLockingFailureException.class); + .isInstanceOf(OptimisticLockingFailureException.class); } private Long count(String tableName) {