Skip to content
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

MultiRabbit containerFactory SpEL Resolution Bug #2809

Closed
BenEfrati opened this issue Sep 5, 2024 · 2 comments · Fixed by #2936
Closed

MultiRabbit containerFactory SpEL Resolution Bug #2809

BenEfrati opened this issue Sep 5, 2024 · 2 comments · Fixed by #2936

Comments

@BenEfrati
Copy link
Contributor

In what version(s) of Spring AMQP are you seeing this issue?

3.1.6

Describe the bug

The resolveMultiRabbitAdminName method in MultiRabbitListenerAnnotationBeanPostProcessor does not properly resolve SpEL expressions for the containerFactory attribute of @RabbitListener annotations. According to the JavaDoc, the containerFactory attribute should support SpEL expressions, but the current implementation does not resolve these expressions, potentially leading to incorrect admin name resolution.

Additional context
JavaDoc for containerFactory() method in RabbitListener:

 * The bean name of the
 * {@link org.springframework.amqp.rabbit.listener.RabbitListenerContainerFactory} to
 * use to create the message listener container responsible to serve this endpoint.
 * <p>
 * If not specified, the default container factory is used, if any. If a SpEL
 * expression is provided ({@code #{...}}), the expression can either evaluate to a
 * container factory instance or a bean name.
 * @return the
 * {@link org.springframework.amqp.rabbit.listener.RabbitListenerContainerFactory}
 * bean name.
 */
String containerFactory() default "";

* expression is provided ({@code #{...}}), the expression can either evaluate to a

To Reproduce

  1. Define a @RabbitListener annotation with a SpEL expression for the containerFactory attribute.
  2. Observe that the resolveMultiRabbitAdminName method does not resolve the SpEL expression.

Expected behavior

The method should resolve SpEL expressions for both admin and containerFactory attributes. A proposed fix would be:

protected String resolveMultiRabbitAdminName(RabbitListener rabbitListener) {
    String admin = super.resolveExpressionAsString(rabbitListener.admin(), "admin");
    if (!StringUtils.hasText(admin) && StringUtils.hasText(rabbitListener.containerFactory())) {
        String containerFactory = super.resolveExpressionAsString(rabbitListener.containerFactory(), "containerFactory");
        admin = containerFactory + RabbitListenerConfigUtils.MULTI_RABBIT_ADMIN_SUFFIX;
    }
    if (!StringUtils.hasText(admin)) {
        admin = RabbitListenerConfigUtils.RABBIT_ADMIN_BEAN_NAME;
    }
    return admin;
}
@artembilan
Copy link
Member

Confirmed.
And I think the fix you propose is OK, but it would only work if those expressions are evaluated to bean names.
See resolveExpressionAsString() impl:

	protected String resolveExpressionAsString(String value, String attribute) {
		Object resolved = resolveExpression(value);
		if (resolved instanceof String str) {
			return str;
		}
		else {
			throw new IllegalStateException("The [" + attribute + "] must resolve to a String. "
					+ "Resolved to [" + resolved.getClass() + "] for [" + value + "]");
		}
	}

So, if that is resolved to the RabbitAdmin instead it would fail.
Same for containerFactory() attribute.
We probably can workaround for the RabbitAdmin because it goes with a BeanNameAware contract implementation.
But RabbitListenerContainerFactory does not have one.

However that must not be hard to do that as well, although this is going to be more involved fix.
But yeah, more robust.

There is a workaround for the problem. Use explicit admin attribute on the annotation.
Therefore I am not going to back-port the fix to the previous version.

Sounds like a plan?

@BenEfrati
Copy link
Contributor Author

Hi,
The issue not fully resolved, open PR:
#2936

artembilan pushed a commit that referenced this issue Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants