Skip to content

Conversation

@garyrussell
Copy link
Contributor

@garyrussell garyrussell commented Jan 14, 2018

In the @KafkaListener bean post processor, allow property SpEL expressions to
access properties and methods on the bean being post-processed.

See Stack Overflow.

@garyrussell garyrussell added this to the 2.1.2 milestone Jan 14, 2018
@garyrussell garyrussell added in pr and removed in pr labels Jan 14, 2018
@garyrussell garyrussell force-pushed the kl_beanRef branch 2 times, most recently from 3e8813b to 66c7dad Compare January 14, 2018 18:44
In the `@KafkaListener` bean post processor, allow property SpEL expressions to
access properties and methods on the bean being post-processed.
this.topic = topic;
}

@KafkaListener(topics = "#{__listenerBean__.topic}", groupId = "#{__listenerBean__.topic}.group")
Copy link
Member

Choose a reason for hiding this comment

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

Can't advertise the feature like SpEL evaluation context variable and therefore register the outer object under a #listener name? More convenient to use and familiar syntax for SpEL users: https://docs.spring.io/spring/docs/5.0.2.RELEASE/spring-framework-reference/core.html#expressions-ref-variables ?

What I feel fragile for is that funky beanRef on the annotation...

Just my feeling from the end-user perspective.
If you are not agree with me, I'll merge it as you did.
Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial plan, but we don't have access to the SpEL evaluation context - it is a single instance that's cached in the StandardBeanExpressionResolver - it would require replacing all that resolution infrastructure with our own.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I checked the code there, bu I mean pretend like we have it as a SpEL variable , by the name of the object in scope you use already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works already, out of the box...

@Bean
public Listener listener1() {
    return new Listener("topic1");
}

with

    @KafkaListener(topics = "#{listener1.topic}",
        groupId = "#{listener1.topic}.group")
    public void listen(...) {
        ...
    }

But we're trying to solve a different problem, where we don't know the bean name while writing the code...

@Bean
public Listener listener1() {
    return new Listener("topic1");
}

@Bean
public Listener listener2() {
    return new Listener("topic2");
}

We somehow need a reference to the current bean, but not by the bean name. Yes, ideally, a variable #currentBean would be best but, for the reason I specified above, we can't do that without copying a lot of code.

Copy link
Member

Choose a reason for hiding this comment

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

OK. Let's go simpler discussion way!
I don't like the name for the object in scope as __listenerBean__. I would prefer #listener instead. This way it mimics the well-known SpEL variable syntax.

Also I don't like that new beanRef annotation attribute. Don't know yet why, but my feeling like I would stay away of it. Although I agree about the argument "unlikely event that you have an actual bean called" especially in this point release.

So, having all of this thinking in my head I only suggest to rename the default __listenerBean__ key to the #listener. And that's all.

Let me know if my point is still unclear!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't use #listener because SpEL will think it's a real variable and fail to find it.

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'd be ok with _listener or _listener_ as the default.

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Good point!

OK. One more attempt. The StandardBeanExpressionResolver has this:

/**
 * Template method for customizing the expression evaluation context.
 * <p>The default implementation is empty.
 */
protected void customizeEvaluationContext(StandardEvaluationContext evalContext) {
}

Looks like this is the place where we could inject a SpEL variable for the bean in the method:

protected void processListener(MethodKafkaListenerEndpoint<?, ?> endpoint, KafkaListener kafkaListener, Object bean,
			Object adminTarget, String beanName) {

But I agree that is more coding, ThreadLocal, param propagation etc. And in the end technically we end up with the same logic for end user.

So, let me know if you would like to play with this or let's leave as is and merge!

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with _listener or _listener_ as the default.

Yeah... That really might be two _ in the beginning, but I'm some lost to read dot after two _...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok; I will change it; and do no more; I think we've spent enough time on this 😄

@artembilan artembilan merged commit eb4eca2 into spring-projects:master Jan 17, 2018
@garyrussell garyrussell deleted the kl_beanRef branch November 7, 2018 18:10
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

Successfully merging this pull request may close these issues.

2 participants