-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support for beanRef in KafkaListener annotation #1089
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to Springwolf. Thanks a lot for creating your first pull request. Please check out our contributors guide and feel free to join us on discord.
✅ Deploy Preview for springwolf-ui canceled.
|
...-core/src/main/java/io/github/springwolf/core/asyncapi/scanners/bindings/BindingFactory.java
Outdated
Show resolved
Hide resolved
.../java/io/github/springwolf/plugins/kafka/asyncapi/scanners/bindings/KafkaBindingFactory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ruskaof for looking into this issue and providing a link to the spring-kafka code. In its basic form, the change looks good to me.
I notice that due to the BindingFactory change (public interface), a lot of files needed adjustments. Additionally, Springwolf users will need to update their implementation (breaking compile change).
I wonder if it is possible to avoid this?
If not, I like to make sure that the change is future-proof for similar feature requests (method as context instead of class?). Please understand that I/we will review this change more closely due to the interface change.
I assume that tests are missing at this point, since you wanted to get a feedback about the general direction first. Before this PR can be merged, test(s) in springwolf-plugin/springwolf-kafka-plugin are required.
Also, to demo this improvement (and have another layer of test), please update the springwolf-kafka example (probably new BeanRefConsumer file).
I plan to get a second opinion from @sam0r040 tomorrow (method vs class).
In case you have any questions, let us know.
Thank you for picking up this issue!
Thanks for your review, @timonback ! I like your suggestions and I'll try to implement them. I believe that it is not possible to implement this feature without somehow passing the annotated class (or the class that declares the annotated method) to Since one can use |
9f32935
to
3667cef
Compare
…sting ones, move bean ref logic to KafkaBeanRefHelper, implement BindingContext for passing Method and Class context to factories
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ruskaof,
Thank you for the update, sorry for the delay.
We like the change and breaking changes are avoided by the addition you added 👍
Also, the BindingContext
encapsulates the class well.
Springwolf completely operates on classes at this point, not specific Spring bean instances. The Spring ApplicationContext is used to get the (one) current instance of a class using #getBean(clazz)
[0]. The method will only return, when exactly one bean for that class is found. As a result, the following bean definition will not work:
@Bean
public Listener listener1() {
return new Listener("topic1");
}
@Bean
public Listener listener2() {
return new Listener("topic2");
}
We are willing to accept this at this point. The alternative is to create multiple classes extending the Listener
class. Just want to make sure that you are aware of this limitation and your use-case still works?
As the getBean
method may throw a NoUniqueBeanDefinitionException, can you wrap the error to indicate the class to assist the user in locating the original error? The error message may include a hint that the error is a limitation of Springwolf in combination with beanRef
.
Also, in the next step, tests are required for the KafkaBeanRefHelper
.
Great work! Looking forward to the addition.
this commit resolves #873
Hi, I'm new to this project so I'm not sure this is the best way to implement support for beanRef in KafkaListener annotation. Could you please review this MR? Most of the code comes from the spring kafka project https://github.com/spring-projects/spring-kafka/blob/main/spring-kafka/src/main/java/org/springframework/kafka/annotation/KafkaListenerAnnotationBeanPostProcessor.java