-
Notifications
You must be signed in to change notification settings - Fork 632
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
MultiRabbitListenerAnnotationBeanPostProcessor resolveMultiRabbitAdminName #2810
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.
Thank you for proposed fix!
Unfortunately we cannot go via simple expression parsing where it would simply be just a plain bean name without an expression wrapping. The point of the expression is really let to do more complex logic for that resolution.
See my comment in the respective issue to make it more robust: #2809 (comment)
...g/springframework/amqp/rabbit/annotation/MultiRabbitListenerAnnotationBeanPostProcessor.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.
And your name to the @author
section of all the affected classes.
...src/main/java/org/springframework/amqp/rabbit/config/BaseRabbitListenerContainerFactory.java
Show resolved
Hide resolved
...g/springframework/amqp/rabbit/annotation/MultiRabbitListenerAnnotationBeanPostProcessor.java
Outdated
Show resolved
Hide resolved
...g/springframework/amqp/rabbit/annotation/MultiRabbitListenerAnnotationBeanPostProcessor.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/springframework/amqp/rabbit/listener/RabbitListenerContainerFactory.java
Outdated
Show resolved
Hide resolved
...t/src/main/java/org/springframework/amqp/rabbit/listener/RabbitListenerContainerFactory.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/springframework/amqp/rabbit/config/RabbitListenerContainerTestFactory.java
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.
Oh! I see now what you mean:
EnableRabbitTests > sampleConfiguration() FAILED
org.opentest4j.AssertionFailedError:
expected: "rabbitListenerContainerFactory"
but was: null
at app//org.springframework.amqp.rabbit.annotation.AbstractRabbitAnnotationDrivenTests.testSampleConfiguration(AbstractRabbitAnnotationDrivenTests.java:91)
at app//org.springframework.amqp.rabbit.annotation.EnableRabbitTests.sampleConfiguration(EnableRabbitTests.java:71)
Apparently those tests are based on some test entity.
So, yeah: we have to have these new methods implemented in that RabbitListenerContainerTestFactory
as well.
Why do you push commits for every line of code you change?
Why just don't combine all the requested changes and send a single commit with a proper message?
Thanks
...t/src/main/java/org/springframework/amqp/rabbit/listener/RabbitListenerContainerFactory.java
Show resolved
Hide resolved
Sorry, my network is so lag. After reloading github page, I have seen your newest comment |
thank you for contribution; looking forward for more! |
Thanks for your review. I have learn a lot. |
issue #2809
Condition in
MultiRabbitListenerAnnotationBeanPostProcessor#resolveMultiRabbitAdminName
RabbitListener#admin
if it's not blank elseRabbitListener#containerFactory
returnRabbitListener#containerFactory
+-admin
if it's not blank elseRabbitListenerConfigUtils#RABBIT_ADMIN_BEAN_NAME
in case 2, I think just get bean name from SpEL
#{@...}
and concate string, no need callsuper#resolveExpressionAsString
. Wherther the given name is valid or not, function chain in classRabbitListenerAnnotationBeanPostProcessor
will attempt to create RabbitAmin with it.Please give me more advice if I miss something