Remove DbActionExecutionException

[mipo256]

Targeting #831 issue

CC: @schauder @mp911de

[mp911de]

It seems to me that we should revisit our exception handling. DbActionExecutionException doesn't seem particularly useful to the final application whereas it might be useful for our own internals.

[mipo256]

@mp911de Yeah... But this is not a part of this issue. Maybe we can create another issue to revise the default exception to be thrown? Or what should we do?

[mp911de]

The problem is that we're throwing DbActionExecutionException and not a DAO exception, so I think that is indeed part of the issue. Looking at the code, we do not catch DbActionExecutionException. That exception was introduced as part of introducing better means for debugging (#382) while the net effect was that we changed the API experience. What we could do is add DbActionExecutionException as suppressed exception to the original one.

Paging @schauder.

[schauder]

I'm not sure what you mean by "adding as suppresed exception".

In my understanding Surpressed exceptions are ones that get lost when a finalizer throws an exception itself.

We could instantiate a new exception with the same type as the original with the additional information of the DAEE, or even the DAEE as cause. That would certainly look strange, but might actually be rather useful.

[mp911de]

Something along the lines of:

catch(RuntimeException e) {
  e.addSuppressed(new DbActionExecutionException(action));
  throw e;
}

[mipo256]

I'm a little bit confused with adding suppressed exception here. We're not really suppressing DbActionExecutionException here. I see it confusing to the end user TBH.

CC: @mp911de @schauder

[schauder]

I didn't know addSuppressed. It is a little hackish, but seems fine to me. It's not worse then the other variants I had in mind.

[mipo256]

Done

[mipo256]

Hey @schauder @mp911de!

I've done some polishing after code review, resolved both issues, can you take a look again?

[schauder]

I'm still not happy with this.

1. The JdbcExceptionTranslator is still hard wired (and has a rather non descriptive name).
2. Part of the job is done in by the JdbcExceptionTranslator, and another part is done in the catch block. Without a clear reason why this is done this way.

I see two possible ways forward:
a) drop the exception handler and simply add the DbActionExecutionException as a suppressed exception to the catched exception. I'd consider this a breaking change, so this should go into 4.0

b) Implement a ExceptionTranslator, that unwraps any DbActionExecutionException, puts the original DbActionExecutionException as the suppressed exception on the original exception.
We could offer this exception translator to users in the next release, and in 4.0 register it by default.
The catch block would stay unchanged.

@mp911de What do you think?

[mp911de]

I would honestly drop DbActionExecutionException entirely. We do not define it in our API (JdbcAggregateOperations doesn't define such a contract) so users do not expect that exception. I would even argue the sooner we remove DbActionExecutionException, the better. Our MyBatis integration doesn't care about DbActionExecutionException, it throws PersistenceException instead.

Going forward, I would even suggest introducing SQLExceptionTranslator to MyBatisDataAccessStrategy and refining the API contract at JdbcAggregateOperations to call out our delegations to NamedParameterJdbcOperations so users have an idea about exception usage.

[schauder]

Just to clarify what wasn't clear to me from Marks statement:

For this PR we want to completely drop the ExceptionTranslator and the DbActionExecutionException and just utilise the ExceptionTranslater used by the JdbcTemplate.

The MyBatis integration should eventually also get its SqlExceptionTranslater. This can happen in this PR, or we would create a separate issue for that.

[mipo256]

I can do what @schauder summarized in this PR. We just need to conclude that we agree on the solution. What are your thoughts on this @mp911de?

[schauder]

What I wrote above, was the result of a discussion with Mark, so please go ahead.

[mipo256]

Understood @schauder. Will complete shortly

[mipo256]

Done @schauder @mp911de

Because we're removing the DbActionExecutionException, I think we need to assign the breaking-change tag in order to merge the changes only into 4 major release.

Or maybe we can do better - we can just deprecate the DbActionExecutionException and keep it for now, but mark it as forRemoval = true. So the users will have a grace migration period. What are your thoughts, guys?

[schauder]

It's not just about the existence of DbActionExecutionException, but also about exception handling that relies on the current structure, so I don't think there is a reasonable way around calling it a breaking change.

[mipo256]

I agree with you @schauder. I'm just pointing out that retaining the DbActionExecutionException may be "less breaking" for some users. But in general you're right.

Maybe just removing it entirely and not retaining the legacy in the next major version is a good way to go. Thanks!

[schauder]

I see your idea, but if someone is referencing DbActionExecutionException anywhere in their code, it is actually useful to have that fail hard, because even if it would compile, it most certainly wouldn't work anymore as intended.

[schauder]

This PR has test failures in JdbcRepositoryConcurrencyIntegrationTests.
Please fix those.

[mipo256]

Fixed the concurrency tests in JdbcRepositoryConcurrencyIntegrationTests @schauder, the PR is ready.

[schauder]

Thanks for pushing this forward.

I applied some polishing and some fixes to SQL scripts and merged it into 4.0.x