ApplicationListener callbacks to be supported for inner beans as well [SPR-6049]

[spring-projects-issues]

aaron gaalswyk opened SPR-6049 and commented

I have a bean definition like this:

<bean id="mybean" class="com.mycompany.MyBeanImpl">

<security:intercept-methods access-decision-manager-ref="defaultAccessDecisionManager">
<security:protect method="com.mycompany.MyBean.*" access="ROLE_USER"/>
</security:intercept-methods>

</bean>

The MyBeanImpl bean implements both the MyBean interface and the Spring ApplicationListener interface, because it needs the ContextRefreshedEvent to complete some configuration. With the definition done as above, security gets applied correctly, but the ApplicationListener.onApplicationEvent callback fails to ever get invoked. If I comment out the intercept-methods decorator, the callback works again. If I delete the intercept-methods decorator and replace it with a stand-alone global-method-security element, I'm in good shape with both security and the onApplicationEvent callback:

<security:global-method-security access-decision-manager-ref="defaultAccessDecisionManager">
    <security:protect-pointcut access="ROLE_USER"
        expression="execution(* com.mycompany.MyBean.*(..))" />
</security:global-method-security>

---

Affects: 3.0 M4

Issue Links:

• Inner bean behind BeanFactoryPostProcessor should be able to receive application events [SPR-14783] #19349 Inner bean behind BeanFactoryPostProcessor should be able to receive application events

Referenced from: commits 0e1b04d

[spring-projects-issues]

Luke Taylor commented

Closed wrong issue. Reopening.

[spring-projects-issues]

Luke Taylor commented

This seems to be something to do with the way Spring is setting up the list of ApplicationListeners internally whe the application context starts up. The decorated bean still retains its ApplicationListener interface, but doesn't receive events. It needs further investigation so postponing till after the M2 release.

[spring-projects-issues]

Luke Taylor commented

Moved to Spring Framework after discussion with Juergen.

Additional info:

Stepping into the AbstractApplicationContext.registerListeners method in some more detail...

1. The method calls getBeanNamesForType with ApplicationListener as
   the type.

2. The bean "target" is of type ProxyFactoryBean

3. It gets to the line

boolean matchFound = (allowEagerInit || !isFactoryBean ||
containsSingleton(beanName))&& (includeNonSingletons ||
isSingleton(beanName))&& isTypeMatch(beanName, type);

which evaluates to "false" as containsSingleton(beanName) is false.

4. It then goes into the block beginning

if (!matchFound&& isFactoryBean) {

which also fails to match as isTypeMatch("&target",
"ApplicationListener") returns false for the ProxyFactoryBean type.

In the "isTypeMatch" method, the code

// Check bean class whether we're dealing with a FactoryBean. if
(FactoryBean.class.isAssignableFrom(beanClass)) { if
(!BeanFactoryUtils.isFactoryDereference(name)) { // If it's a
FactoryBean, we want to look at what it creates, not the factory
class. Class type = getTypeForFactoryBean(beanName, mbd); return
(type != null&& typeToMatch.isAssignableFrom(type)); } else { return
typeToMatch.isAssignableFrom(beanClass); } }

evaluates the second part, using ProxyFactoryBean as the class,
rather than the value for the getTypeForFactoryBean(). If the latter
was used the bean would be recognised as an ApplicationListener.

[spring-projects-issues]

Mark Fisher commented

The problem I'm seeing after making the change above (using getTypeForFactoryBean() instead of beanClass) is that the actual instance registered is still the FactoryBean. Therefore an error like the following occurs:

org.springframework.beans.factory.BeanNotOfRequiredTypeException:
 Bean named '&testBean' must be of type [org.springframework.context.ApplicationListener],
 but was actually of type [org.springframework.context.support.ApplicationListenerFactoryBeanTests$TestFactoryBean]
	at 
org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:347)
	at
org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:192)
	at 
org.springframework.context.event.AbstractApplicationEventMulticaster.getApplicationListeners(AbstractApplicationEventMulticaster.java:132)
	at 
org.springframework.context.event.SimpleApplicationEventMulticaster.multicastEvent(SimpleApplicationEventMulticaster.java:86)
	at org.springframework.context.support.AbstractApplicationContext.publishEvent(AbstractApplicationContext.java:280)
	at 
org.springframework.context.support.AbstractApplicationContext.finishRefresh(AbstractApplicationContext.java:739)
	at 
org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:390)
	at 
org.springframework.context.support.ApplicationListenerFactoryBeanTests.checkFactoryBean(ApplicationListenerFactoryBeanTests.java:24)
...

[spring-projects-issues]

Mark Fisher commented

The following change within AbstractApplicationEventMulticaster.getApplicationListeners() does "fix" this issue, but it might not be the best place to address it:

BEFORE:

listener = beanFactory.getBean(listenerBeanName, ApplicationListener.class);

AFTER:

try {
    listener = beanFactory.getBean(listenerBeanName, ApplicationListener.class);
}
catch (BeanNotOfRequiredTypeException e) {
    if (BeanFactoryUtils.isFactoryDereference(listenerBeanName)) {
        listener = beanFactory.getBean(
                listenerBeanName.substring(BeanFactory.FACTORY_BEAN_PREFIX.length()),
                ApplicationListener.class);					}
}

[spring-projects-issues]

Mark Fisher commented

Since the change listed in the above comment is really at the wrong level (we should be finding the correct match up-front if possible rather than relying on an "expected error"), I continued to explore other options. The following change does the trick for this particular scenario, but of course the implications of this change are fairly significant (causing other tests to fail).

This code excerpt is from: DefaultListableBeanFactory.getBeanNamesForType(Class type, boolean includeNonSingletons, boolean allowEagerInit)

BEFORE:

if (!matchFound && isFactoryBean) {
    // In case of FactoryBean, try to match FactoryBean instance itself next.
    beanName = FACTORY_BEAN_PREFIX + beanName;
    matchFound = (includeNonSingletons || mbd.isSingleton()) && isTypeMatch(beanName, type);
}

AFTER:

if (isTypeMatch(beanName, type)) { // checking target first
    result.add(beanName);
}
// then check the FactoryBean as well
if (!matchFound) {
    // In case of FactoryBean, try to match FactoryBean instance itself next.
    beanName = FACTORY_BEAN_PREFIX + beanName;
    matchFound = (includeNonSingletons || mbd.isSingleton()) && isTypeMatch(beanName, type);
}

OR... if the target should take precedence (hence bypassing the FactoryBean check), we could do the following:

if (isTypeMatch(beanName, type)) { // checking target first
    matchFound = true;
}
else { // next check the FactoryBean
    beanName = FACTORY_BEAN_PREFIX + beanName;
    if ((includeNonSingletons || mbd.isSingleton()) && isTypeMatch(beanName, type)) {
        matchFound = true;
    }
}

However, this still breaks the expected contract for other tests (e.g. BeanFactoryUtilsTests)

[spring-projects-issues]

Mark Fisher commented

Re-assigning back to Juergen after discussing a plan to implement this as an extension to the ApplicationContextAwareProcessor.

[spring-projects-issues]

Juergen Hoeller commented

Inner beans are now detected as ApplicationListeners as well. Note that this is only supported for inner singletons, since we can't reliably obtain the current inner instance otherwise.

Juergen