Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -407,18 +407,32 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

I think no need to do this extraction outside of the if (hasClassLevelListeners) { branch.

boolean hasMethodLevelKafkaHandlerAnnotation = !methodsWithHandler.isEmpty();
if (hasClassLevelListeners) {
Set<Method> methodsWithHandler = MethodIntrospector.selectMethods(targetClass,
(ReflectionUtils.MethodFilter) method ->
AnnotationUtils.findAnnotation(method, KafkaHandler.class) != null);
List<Method> multiMethods = new ArrayList<>(methodsWithHandler);
processMultiMethodListeners(classLevelListeners, multiMethods, targetClass, bean, beanName);
}
throwErrorIfNoListenerMethods(bean, hasMethodLevelListeners,
hasClassLevelListeners, hasMethodLevelKafkaHandlerAnnotation);
}
}
return bean;
}

private void throwErrorIfNoListenerMethods(Object bean, boolean hasMethodLevelListeners,
boolean hasClassLevelListeners, boolean hasMethodLevelKafkaHandlerAnnotation) {
if (hasMethodLevelListeners) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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());
Copy link
Member

Choose a reason for hiding this comment

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

I think just on bean is enough, without any getClass() extraction.
The bean instance might lead to the place where it declared.

}
}

/*
* AnnotationUtils.getRepeatableAnnotations does not look at interfaces
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/*
* Copyright 2017-2025 the original author or authors.
Copy link
Member

Choose a reason for hiding this comment

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

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.

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.kafka.annotation;

import org.junit.jupiter.api.Test;

import org.springframework.beans.factory.BeanCreationException;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.stereotype.Component;

import static org.assertj.core.api.Assertions.assertThatThrownBy;

/**
* @author Sanghyeok An
*
* @since 4.0.0
*/

class KafkaListenerAnnotationBeanPostProcessorTest {
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to use *Tests suffix.


@Test
void ctx_should_be_fail_to_register_bean_when_no_listener_methods_exist() {
// GIVEN
AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
ctx.register(TestConfig.class);

// GIVEN - expected
Class<?> expectedErrorType = BeanCreationException.class;
String expectedErrorMsg =
"Error creating bean with name 'org.springframework.kafka.annotation."
+ "KafkaListenerAnnotationBeanPostProcessorTest$TestConfig$BuggyListener': "
+ "No kafka listener methods found on bean type: class org.springframework.kafka"
+ ".annotation.KafkaListenerAnnotationBeanPostProcessorTest$TestConfig$BuggyListener";

// WHEN + THEN
assertThatThrownBy(ctx::refresh)
.isInstanceOf(expectedErrorType)
.hasMessage(expectedErrorMsg);
Copy link
Member

Choose a reason for hiding this comment

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

I prefer different style:

assertThatExceptionOfType()
   .isThrownBy()

I also don't see a reason in those GIVEN/WHEN/THEN

}

@Configuration
static class TestConfig {

@Bean
public KafkaListenerAnnotationBeanPostProcessor<Object, Object> kafkaListenerAnnotationBeanPostProcessor() {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to do this explicitly?
Cannot we rely on the @EnableKafka instead?

return new KafkaListenerAnnotationBeanPostProcessor<>();
}

@Component
@KafkaListener
static class BuggyListener {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that. Thanks a lot πŸ™‡β€β™‚οΈ


public void listen(String message) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need return for the void method?
Why just we cannot leave it as an empty body?

}
}

}

}