Fix ambiguity in the JpaExecutor
#2640
Conversation
Since we can't mutate a provided `ParameterSourceFactory` with the provided `jpaParameters`, we have to reject such a combined configuration in favor of the advice to configure params on the provided `ParameterSourceFactory` * Throw `IllegalStateException` when `ParameterSourceFactory` and `jpaParameters` are configured together on the `JpaExecutor` * Some refactoring for `JpaExecutor` to have a consistent code style * Fix tests according new logic * Document such an ambiguity configuration to avoid confusion **Cherry-pick to 5.0.x**
| } | ||
| return parameterSource; | ||
| } | ||
|
|
There was a problem hiding this comment.
Some refactoring for JpaExecutor to have a consistent code style
Major reorganization changes like this should be done in a separate commit - it is virtually impossible to tell what code (if any) has changed). I assume the only difference is the new IllegalStateException. Has anything else changed?
There was a problem hiding this comment.
Doh... Sorry, I missed the fact that it is going to be hard to review.
The other major change is switch for the enum instead of if...else.
Also I changed the logic in the poll() to this:
else if (requestMessage != null) {
throw new MessagingException(requestMessage,
"The Jpa operation returned more than 1 result for expectSingleResult mode.");
}
else {
throw new MessagingException(
"The Jpa operation returned more than 1 result for expectSingleResult mode.");
}
Since my IDEA complains that requestMessage may be null. I believe SonarQube has the same concern in code smell section.
Other refactoring was about moving setters before afterPropertiesSet(), reordering methods which use each other, removing this. from methods.
There was a problem hiding this comment.
We should add @Nullable to that parameter - even if we haven't yet enabled NonNullApi for this package.
| assertThat(e.getMessage(), | ||
| startsWith("The 'jpaParameters' and 'parameterSourceFactory' are mutually exclusive.")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider using assertJ's assertThatThrownBy(() -> executor.afterPropertiesSet()).... for this kind of test.
Otherwise it needs a fail("Expected exception).
* Fix "No BeanFactory" warning in the `JpaExecutorTests`
| Object id = this.idExpression.getValue(this.evaluationContext, requestMessage); | ||
| Class<?> entityClass = this.entityClass; | ||
| if (entityClass == null) { | ||
| entityClass = requestMessage.getPayload().getClass(); |
There was a problem hiding this comment.
We can get an NPE here too.
Since we can't mutate a provided
ParameterSourceFactorywith theprovided
jpaParameters, we have to reject such a combinedconfiguration in favor of the advice to configure params on the
provided
ParameterSourceFactoryIllegalStateExceptionwhenParameterSourceFactoryandjpaParametersare configured together on theJpaExecutorJpaExecutorto have a consistent code styleCherry-pick to 5.0.x