Correct behaviour of NOOP deletes to match the specification in CrudRepository. #1314
Conversation
…epository. 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 See spring-projects/spring-data-commons#2651
| * @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 |
There was a problem hiding this comment.
Would JdbcUpdateAffectedIncorrectNumberOfRowsException be the right one to use?
There was a problem hiding this comment.
We still can define IncorrectUpdateSemanticsDataAccessException here, I was just wondering that we have no code changes associed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
On a general level, why do we define |
Early design mistake by an old junior developer? I'll create a separate issue to fix that. |
|
Regarding the methods, we could clean up the interface for 3.0 and leave the methods accepting the type on the actual implementation to simplify the migration of external code. |
…epository`. 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 spring-projects/spring-data-commons#2651
…epository`. 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 spring-projects/spring-data-commons#2651
|
That's merged and backported now. |
Delete operations that receive a version attribute throw an
OptimisticFailureExceptionwhen 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
IncorrectUpdateSemanticsDataAccessExceptionif 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