Skip to content

Correct behaviour of NOOP deletes to match the specification in CrudRepository. #1314

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

Closed
wants to merge 2 commits into from
Closed

Correct behaviour of NOOP deletes to match the specification in CrudRepository. #1314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e57df8
1313-noop-deletes - Prepare branch
schauder Aug 25, 2022
de811b2
Correct behaviour of NOOP deletes to match the specification in CrudR…
schauder Aug 25, 2022
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
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>
<packaging>pom</packaging>

<name>Spring Data Relational Parent</name>
Expand Down
2 changes: 1 addition & 1 deletion spring-data-jdbc-distribution/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>
<relativePath>../pom.xml</relativePath>
</parent>

Expand Down
4 changes: 2 additions & 2 deletions spring-data-jdbc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<modelVersion>4.0.0</modelVersion>

<artifactId>spring-data-jdbc</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>

<name>Spring Data JDBC</name>
<description>Spring Data module for JDBC repositories.</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>
</parent>

<properties>
Expand Down
5 changes: 5 additions & 0 deletions ...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/AggregateChangeExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -110,6 +111,10 @@ private void execute(DbAction<?> action, JdbcAggregateChangeExecutionContext exe
throw new RuntimeException("unexpected action");
}
} catch (Exception e) {

if (e instanceof OptimisticLockingFailureException) {
throw e;
}
throw new DbActionExecutionException(action, e);
}
}
Expand Down
23 changes: 22 additions & 1 deletion ...g-data-jdbc/src/main/java/org/springframework/data/jdbc/core/JdbcAggregateOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import java.util.Optional;

import org.springframework.dao.IncorrectUpdateSemanticsDataAccessException;
import org.springframework.data.domain.Example;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
Expand All @@ -41,6 +42,8 @@ public interface JdbcAggregateOperations {
* @param instance the aggregate root of the aggregate to be saved. Must not be {@code null}.
* @param <T> 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> T save(T instance);

Expand All @@ -50,6 +53,8 @@ public interface JdbcAggregateOperations {
* @param instances the aggregate roots to be saved. Must not be {@code null}.
* @param <T> the type of the aggregate root.
* @return the saved instances.
* @throws IncorrectUpdateSemanticsDataAccessException when at least one instance is determined to be not new and the
Copy link
Member

@mp911de mp911de Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would JdbcUpdateAffectedIncorrectNumberOfRowsException be the right one to use?

Copy link
Contributor Author

@schauder schauder Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes.

Copy link
Member

@mp911de mp911de Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still can define IncorrectUpdateSemanticsDataAccessException here, I was just wondering that we have no code changes associed.

Copy link
Contributor Author

@schauder schauder Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When going through the test I noted this behaviour, wondered if it is the correct one, which I think it is and documented it to make things nice and tidy.

Copy link
Contributor Author

@schauder schauder Aug 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked into changing the actual exception thrown into a JdbcUpdateAffectedIncorrectNumberOfRowsException. It would require passing a SQL statement and an expected and actual number of updated rows to the constructor. This seems like the wrong abstraction level to me and I'd rather keep the IncorrectUpdateSemanticsDataAccessException.

* resulting update does not update any rows.
* @since 3.0
*/
<T> Iterable<T> saveAll(Iterable<T> instances);
Expand Down Expand Up @@ -78,6 +83,11 @@ public interface JdbcAggregateOperations {

/**
* Deletes a single Aggregate including all entities contained in that aggregate.
* <p>
* 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.
* </p>
*
* @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.
Expand All @@ -87,7 +97,12 @@ public interface JdbcAggregateOperations {

/**
* Deletes all aggregates identified by their aggregate root ids.
*
* <p>
* 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.
* </p>
*
* @param ids the ids of the aggregate roots of the aggregates to be deleted. Must not be {@code null}.
* @param domainType the type of the aggregate root.
* @param <T> the type of the aggregate root.
Expand All @@ -100,6 +115,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 <T> 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.
*/
<T> void delete(T aggregateRoot, Class<T> domainType);

Expand All @@ -116,6 +134,9 @@ public interface JdbcAggregateOperations {
* @param aggregateRoots to delete. Must not be {@code null}.
* @param domainType type of the aggregate roots to be deleted. Must not be {@code null}.
* @param <T> the type of the aggregate roots.
* @throws org.springframework.dao.OptimisticLockingFailureException when {@literal T} has a version attribute and for at least on entity the
* version attribute of the 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.
*/
<T> void deleteAll(Iterable<? extends T> aggregateRoots, Class<T> domainType);

Expand Down
12 changes: 6 additions & 6 deletions ...c/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,11 +866,11 @@ void saveAndUpdateAggregateWithImmutableVersion() {

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
Expand Down Expand Up @@ -915,12 +915,12 @@ void deleteAggregateWithVersion() {
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);
Expand Down Expand Up @@ -1060,12 +1060,12 @@ private <T extends Number> void saveAndUpdateAggregateWithVersion(VersionedAggre
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) {
Expand Down
4 changes: 2 additions & 2 deletions spring-data-r2dbc/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<modelVersion>4.0.0</modelVersion>

<artifactId>spring-data-r2dbc</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>

<name>Spring Data R2DBC</name>
<description>Spring Data module for R2DBC</description>
Expand All @@ -15,7 +15,7 @@
<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>
</parent>

<properties>
Expand Down
4 changes: 2 additions & 2 deletions spring-data-relational/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
<modelVersion>4.0.0</modelVersion>

<artifactId>spring-data-relational</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>

<name>Spring Data Relational</name>
<description>Spring Data Relational support</description>

<parent>
<groupId>org.springframework.data</groupId>
<artifactId>spring-data-relational-parent</artifactId>
<version>3.0.0-SNAPSHOT</version>
<version>3.0.0-1313-noop-deletes-SNAPSHOT</version>
</parent>

<properties>
Expand Down