GH-3869: Should fail bean registration when no method … #3870
Conversation
…listeners are registered. Signed-off-by: chickenchickenlove <[email protected]>
Signed-off-by: chickenchickenlove <[email protected]>
|
@artembilan thanks for comments even if holiday! I added new test code to cover worst case. When you have time, Please take a look 🙇♂️ |
| * @since 4.0.0 | ||
| */ | ||
|
|
||
| class KafkaListenerAnnotationBeanPostProcessorTest { |
There was a problem hiding this comment.
We prefer to use *Tests suffix.
| static class TestConfig { | ||
|
|
||
| @Bean | ||
| public KafkaListenerAnnotationBeanPostProcessor<Object, Object> kafkaListenerAnnotationBeanPostProcessor() { |
There was a problem hiding this comment.
Why do we need to do this explicitly?
Cannot we rely on the @EnableKafka instead?
| // WHEN + THEN | ||
| assertThatThrownBy(ctx::refresh) | ||
| .isInstanceOf(expectedErrorType) | ||
| .hasMessage(expectedErrorMsg); |
There was a problem hiding this comment.
I prefer different style:
assertThatExceptionOfType()
.isThrownBy()
I also don't see a reason in those GIVEN/WHEN/THEN
|
|
||
| @Component | ||
| @KafkaListener | ||
| static class BuggyListener { |
There was a problem hiding this comment.
I don't think buggy is the correct word to express the problem.
For me this word mean "the comfortable cart for infants".
Perhaps something like NoHandlerMethodListener to be as explicit for the problem as possible?
There was a problem hiding this comment.
I didn't know that. Thanks a lot 🙇♂️
| static class BuggyListener { | ||
|
|
||
| public void listen(String message) { | ||
| return; |
There was a problem hiding this comment.
Why do we need return for the void method?
Why just we cannot leave it as an empty body?
| @@ -0,0 +1,76 @@ | |||
| /* | |||
| * Copyright 2017-2025 the original author or authors. | |||
There was a problem hiding this comment.
This might be a copy/paste from other classes, but what is the point of that 2017?
The goal of the copyright dates is to show when the entity was created and when modified.
Was this class created in 2017? No. So, why use that pattern?
Something like just 2025 is enough. However the pattern 2025-2025 would still work if you are afraid some parser would expect two years.
| private void throwErrorIfNoListenerMethods(Object bean, boolean hasMethodLevelListeners, | ||
| boolean hasClassLevelListeners, boolean hasMethodLevelKafkaHandlerAnnotation) { | ||
| if (hasMethodLevelListeners) { | ||
| return; |
There was a problem hiding this comment.
This is a bit awkward style.
Why do we need to go to this method at all, if we meet this condition?
I fully don't see a reason in this method.
The if (!hasMethodLevelListeners && hasClassLevelListeners && !hasMethodLevelKafkaHandlerAnnotation) and throw new IllegalStateException is enough to have in place.
There was a problem hiding this comment.
I agree with you and addressed it!
I anticipated that the conditions could become more complex because of #3865 .
So, I extracted them into a separate method to clearly separate and handle the problematic conditions.
| } | ||
|
|
||
| if (hasClassLevelListeners && !hasMethodLevelKafkaHandlerAnnotation) { | ||
| throw new IllegalStateException("No kafka listener methods found on bean type: " + bean.getClass()); |
There was a problem hiding this comment.
I think just on bean is enough, without any getClass() extraction.
The bean instance might lead to the place where it declared.
Signed-off-by: chickenchickenlove <[email protected]>
|
I added two |
...main/java/org/springframework/kafka/annotation/KafkaListenerAnnotationBeanPostProcessor.java
Outdated
Show resolved
Hide resolved
...main/java/org/springframework/kafka/annotation/KafkaListenerAnnotationBeanPostProcessor.java
Outdated
Show resolved
Hide resolved
| @@ -407,13 +407,19 @@ public Object postProcessAfterInitialization(final Object bean, final String bea | |||
| this.logger.debug(() -> annotatedMethods.size() + " @KafkaListener methods processed on bean '" | |||
| + beanName + "': " + annotatedMethods); | |||
| } | |||
| Set<Method> methodsWithHandler = MethodIntrospector.selectMethods(targetClass, | |||
| (ReflectionUtils.MethodFilter) method -> | |||
| AnnotationUtils.findAnnotation(method, KafkaHandler.class) != null); | |||
There was a problem hiding this comment.
I think no need to do this extraction outside of the if (hasClassLevelListeners) { branch.
|
|
||
| assertThatExceptionOfType(BeanCreationException.class) | ||
| .isThrownBy(ctx::refresh) | ||
| .withMessageContaining("No kafka listener methods found on bean type.") |
There was a problem hiding this comment.
We also can check for our IllegalStateException as a cause.
…n/KafkaListenerAnnotationBeanPostProcessor.java Co-authored-by: Artem Bilan <[email protected]> Signed-off-by: ChickenchickenLove <[email protected]>
Signed-off-by: chickenchickenlove <[email protected]>
I created an issue and a PR regarding an improvement I thought of from a fast-fail perspective.
Please feel free to let me know if you think the changes are unnecessary.
Thanks for your time 🙇♂️