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

Queues property is not correctly resolved on the @org.springframework.amqp.rabbit.annotation.RabbitListener class #672

Closed
novtor opened this issue Oct 2, 2017 · 4 comments

Comments

@novtor
Copy link

novtor commented Oct 2, 2017

When the "queues" value is configured via application.yml with conditionals inside, it is not correctly resolved
RabbitListenerIssue.zip

@novtor
Copy link
Author

novtor commented Oct 2, 2017

I think, the issue is in this file:

should be replaced by
if (!(resolvedValue.**contains**("#{") && value.**contains**("}"))) {

@artembilan
Copy link
Member

I think you have a point here!

Indeed the TemplateAwareExpressionParser used from the StandardBeanExpressionResolver parses the first aaa.bbb. part as LiteralExpression and the next part with the #{...} as a SpelExpression to evaluation.

I've just tested and confirmed that we are good to go with your suggestion!

@RabbitListener(queues = "test.#{'simple'}", group = "testGroup")

But that's only the part of the solution for your. Since your expression is very complex there, you should be careful with all the required symbols to honor SpEL syntax.

For now I'd suggest you as workaround as:

amqp:
  queue:
    suffix: 001
    name: #{'aaa.bbb.' + '${amqp.queue.suffix}' == '001' ? 'default': '${amqp.queue.suffix}'}

Pay attention to single quotes everywhere. Since after resolving the property placeholder, the result becomes something valuable for the SpEL and tries to be resolved against application context. As far as they all are literals they have to be quoted. Not sure though if a single quote ' will be enough: https://docs.spring.io/spring-cloud-dataflow/docs/1.3.0.M2/reference/htmlsingle/#shell-white-space. But it worth to try!

Feel free to come back to us with the contribution on the matter!

@garyrussell
Copy link
Contributor

garyrussell commented Oct 3, 2017

I don't think changing it contains(() is the right solution.

I think we should simply document that the SpEL evaluation is done after the property placeholder resolution and must match the regex pattern ^#{...}$.

My 2c.

@artembilan
Copy link
Member

Gary, one more time.

The TemplateAwareExpressionParser is able to parse an expression like foo.#{myBean.x}.bar to the CompositeStringExpression: LiteralExpression, SpelExpression, LiteralExpression. Just because it has a logic like:

	while (startIdx < expressionString.length()) {
			int prefixIndex = expressionString.indexOf(prefix, startIdx);
			if (prefixIndex >= startIdx) {
				// an inner expression was found - this is a composite
				if (prefixIndex > startIdx) {
					expressions.add(new LiteralExpression(expressionString.substring(startIdx, prefixIndex)));
				}

That CompositeStringExpression does exactly string concatenation under hood:

for (Expression expression : this.expressions) {
	String value = expression.getValue(String.class);
	if (value != null) {
		sb.append(value);
	}
}

I would even say that we don't need that condition to check if that contains expression tokens or not.
We always can rely on the TemplateAwareExpressionParser and get a simple LiteralExpression for non-templated string. Well, that is actually what is done by Spring per se. See AbstractBeanFactory.doResolveBeanClass() for example:

String className = mbd.getBeanClassName();
		if (className != null) {
			Object evaluated = evaluateBeanDefinitionString(className, mbd);
			if (!className.equals(evaluated)) {

So, I would even say that with the current logic we even restrict the user in possible expression variants.

Well, I may see your concern to make such a change in the current GA release, but we definitely should reconsider it in the next 2.1.0 or even in the point 2.0.1 version.

FYI. I have just commented out the code:

if (!(resolvedValue.startsWith("#{") && value.endsWith("}"))) {
	return resolvedValue;
}

And all our tests are passing.

artembilan added a commit to artembilan/spring-amqp that referenced this issue Oct 3, 2017
Fixes spring-projects#672

The `StandardBeanExpressionResolver` is based on the
`TemplateAwareExpressionParser` which can parse any string to the
`LiteralExpression` or to the `CompositeStringExpression` if
template tokens (`#{` & `}`) are present in the source expression.
This way we don't need to deal with literal concatenations within
templated expression.
The expression like `#{'foo.' + myBean.bar}` can be replaced
with the `foo.#{myBean.bar}`
garyrussell pushed a commit that referenced this issue Oct 9, 2017
Fixes #672

The `StandardBeanExpressionResolver` is based on the
`TemplateAwareExpressionParser` which can parse any string to the
`LiteralExpression` or to the `CompositeStringExpression` if
template tokens (`#{` & `}`) are present in the source expression.
This way we don't need to deal with literal concatenations within
templated expression.
The expression like `#{'foo.' + myBean.bar}` can be replaced
with the `foo.#{myBean.bar}`
artembilan added a commit to artembilan/spring-kafka that referenced this issue Oct 9, 2017
The `StandardBeanExpressionResolver` is based on the
`TemplateAwareExpressionParser` which can parse any string to the
`LiteralExpression` or to the `CompositeStringExpression` if
template tokens (`#{` & `}`) are present in the source expression.
This way we don't need to deal with literal concatenations within
templated expression.
The expression like `#{'foo.' + myBean.bar}` can be replaced
with the `foo.#{myBean.bar}`

See: spring-projects/spring-amqp#672
garyrussell pushed a commit to spring-projects/spring-kafka that referenced this issue Oct 9, 2017
The `StandardBeanExpressionResolver` is based on the
`TemplateAwareExpressionParser` which can parse any string to the
`LiteralExpression` or to the `CompositeStringExpression` if
template tokens (`#{` & `}`) are present in the source expression.
This way we don't need to deal with literal concatenations within
templated expression.
The expression like `#{'foo.' + myBean.bar}` can be replaced
with the `foo.#{myBean.bar}`

See: spring-projects/spring-amqp#672
denis554 added a commit to denis554/spring-kafka that referenced this issue Mar 27, 2019
The `StandardBeanExpressionResolver` is based on the
`TemplateAwareExpressionParser` which can parse any string to the
`LiteralExpression` or to the `CompositeStringExpression` if
template tokens (`#{` & `}`) are present in the source expression.
This way we don't need to deal with literal concatenations within
templated expression.
The expression like `#{'foo.' + myBean.bar}` can be replaced
with the `foo.#{myBean.bar}`

See: spring-projects/spring-amqp#672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants