Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4554

Spring BeanPostProcessor(s) are called twice for Struts constructed objects.

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.24
    • Fix Version/s: 2.3.28, 2.5
    • Component/s: Plugin - Spring
    • Labels:
      None

      Description

      It appears that the SpringObjectFactory in the xwork core at lines 194-197 manually yet when calling initializeBean on the autowire factory, the spring framework automatically invokes these processors too which lead to the following post processor's callbacks being invoked twice for both the before and after handlers.

      I confirmed that both Sprnig 3.0.5 and 4.2.1 have called the bean post processors when the initializeBean function is called. See a simple NullBeanPostProcessor implementation below that can be used as a simple test of post processor invocation.

      NullBeanPostProcessor.java
      import org.slf4j.Logger;
      import org.slf4j.LoggerFactory;
      import org.springframework.beans.BeansException;
      import org.springframework.beans.factory.config.BeanPostProcessor;
      
      /**
       * @since	7.0.0
       */
      public class NullBeanPostProcessor implements BeanPostProcessor {
      
      	private static final Logger LOGGER = LoggerFactory.getLogger(NullBeanPostProcessor.class);
      	
      	/**
      	 * {@inheritDoc}
      	 */
      	@Override
      	public Object postProcessBeforeInitialization(Object bean, String beanName)
      	throws BeansException {
      		LOGGER.debug("Before Initialization for {} ({})", beanName, bean);
      		return bean;
      	}
      
      	/**
      	 * {@inheritDoc}
      	 */
      	@Override
      	public Object postProcessAfterInitialization(Object bean, String beanName)
      	throws BeansException {
      		LOGGER.debug("After Initialization for {} ({})", beanName, bean);
      		return bean;
      	}
      
      }
      

        Activity

        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK7-master #381 (See https://builds.apache.org/job/Struts-JDK7-master/381/)
        WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's (lukaszlenart: rev ba361aad36528d5eb2e5ea745227e83a469024a0)

        • core/src/main/java/com/opensymphony/xwork2/spring/SpringObjectFactory.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #381 (See https://builds.apache.org/job/Struts-JDK7-master/381/ ) WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's (lukaszlenart: rev ba361aad36528d5eb2e5ea745227e83a469024a0) core/src/main/java/com/opensymphony/xwork2/spring/SpringObjectFactory.java
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ba361aad36528d5eb2e5ea745227e83a469024a0 in struts's branch refs/heads/master from Lukasz Lenart
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ba361aa ]

        WW-4554 Drops duplicated calls to initialize BeanPostProcessors
        It's already done by Spring

        (cherry picked from commit 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe)

        Show
        jira-bot ASF subversion and git services added a comment - Commit ba361aad36528d5eb2e5ea745227e83a469024a0 in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ba361aa ] WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's already done by Spring (cherry picked from commit 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe)
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Struts-JDK6-support-2.3 #925 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/925/)
        WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's (lukaszlenart: rev 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe)

        • xwork-core/src/main/java/com/opensymphony/xwork2/spring/SpringObjectFactory.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-support-2.3 #925 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/925/ ) WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's (lukaszlenart: rev 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe) xwork-core/src/main/java/com/opensymphony/xwork2/spring/SpringObjectFactory.java
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe in struts's branch refs/heads/support-2-3 from Disabled - L-u-k-a-s-z Lenart - OpenSymphony
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7848534 ]

        WW-4554 Drops duplicated calls to initialize BeanPostProcessors
        It's already done by Spring

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7848534e2f2a8cf4aa956d27b6cf672f36cfacfe in struts's branch refs/heads/support-2-3 from Disabled - L-u-k-a-s-z Lenart - OpenSymphony [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7848534 ] WW-4554 Drops duplicated calls to initialize BeanPostProcessors It's already done by Spring
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Great!

        Show
        lukaszlenart Lukasz Lenart added a comment - Great!
        Hide
        crancran Chris Cranford added a comment -

        That would be my hypothesis. I don't see any reason why they should be invoked multiple times. Spring does not seem to do it internally on beans so I see no reason why our Struts integration should be either.

        Show
        crancran Chris Cranford added a comment - That would be my hypothesis. I don't see any reason why they should be invoked multiple times. Spring does not seem to do it internally on beans so I see no reason why our Struts integration should be either.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        I think I got it, these two lines should be dropped

                        bean = autoWiringFactory.applyBeanPostProcessorsBeforeInitialization(bean, bean.getClass().getName());
                        ...
                        bean = autoWiringFactory.applyBeanPostProcessorsAfterInitialization(bean, bean.getClass().getName());
        

        as

                        bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName());
        

        does the job, right?

        Show
        lukaszlenart Lukasz Lenart added a comment - I think I got it, these two lines should be dropped bean = autoWiringFactory.applyBeanPostProcessorsBeforeInitialization(bean, bean.getClass().getName()); ... bean = autoWiringFactory.applyBeanPostProcessorsAfterInitialization(bean, bean.getClass().getName()); as bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName()); does the job, right?
        Hide
        crancran Chris Cranford added a comment - - edited

        It isn't the use of the autoWire method that is a concern but rather the use of initializeBean. Per the spring documentation:

                /**
        	 * Initialize the given raw bean, applying factory callbacks
        	 * such as {@code setBeanName} and {@code setBeanFactory},
        	 * also applying all bean post processors (including ones which
        	 * might wrap the given raw bean).
        	 * <p>Note that no bean definition of the given name has to exist
        	 * in the bean factory. The passed-in bean name will simply be used
        	 * for callbacks but not checked against the registered bean definitions.
        	 * @param existingBean the existing bean instance
        	 * @param beanName the name of the bean, to be passed to it if necessary
        	 * (only passed to {@link BeanPostProcessor BeanPostProcessors})
        	 * @return the bean instance to use, either the original or a wrapped one
        	 * @throws BeansException if the initialization failed
        	 */
        

        Inside the SpringObjectFactory you'll notice:

        @Override
            public Object buildBean(Class clazz, Map<String, Object> extraContext) throws Exception {
                Object bean;
        
                try {
                    // Decide to follow autowire strategy or use the legacy approach which mixes injection strategies
                    if (alwaysRespectAutowireStrategy) {
                        // Leave the creation up to Spring
                        bean = autoWiringFactory.createBean(clazz, autowireStrategy, false);
                        injectApplicationContext(bean);
                        return injectInternalBeans(bean);
                    } else if (enableAopSupport) {
                        bean = autoWiringFactory.createBean(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false);
                        bean = autoWireBean(bean, autoWiringFactory);
                        bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName());
                        return bean;
                    } else {
                        bean = autoWiringFactory.autowire(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false);
                        bean = autoWiringFactory.applyBeanPostProcessorsBeforeInitialization(bean, bean.getClass().getName());
                        bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName());
                        bean = autoWiringFactory.applyBeanPostProcessorsAfterInitialization(bean, bean.getClass().getName());
                        return autoWireBean(bean, autoWiringFactory);
                    }
                } catch (UnsatisfiedDependencyException e) {
                    LOG.error("Error building bean", e);
                    // Fall back
                    return autoWireBean(super.buildBean(clazz, extraContext), autoWiringFactory);
                }
            }
        

        The last else-block is the issue because the BeanPostProcessor s are being manually invoked on the autowire factory yet initializeBean specifically does that per the documentation and has since at least Spring 3.0.5.

        Show
        crancran Chris Cranford added a comment - - edited It isn't the use of the autoWire method that is a concern but rather the use of initializeBean . Per the spring documentation: /** * Initialize the given raw bean, applying factory callbacks * such as {@code setBeanName} and {@code setBeanFactory}, * also applying all bean post processors (including ones which * might wrap the given raw bean). * <p>Note that no bean definition of the given name has to exist * in the bean factory. The passed-in bean name will simply be used * for callbacks but not checked against the registered bean definitions. * @param existingBean the existing bean instance * @param beanName the name of the bean, to be passed to it if necessary * (only passed to {@link BeanPostProcessor BeanPostProcessors}) * @return the bean instance to use, either the original or a wrapped one * @throws BeansException if the initialization failed */ Inside the SpringObjectFactory you'll notice: @Override public Object buildBean(Class clazz, Map<String, Object> extraContext) throws Exception { Object bean; try { // Decide to follow autowire strategy or use the legacy approach which mixes injection strategies if (alwaysRespectAutowireStrategy) { // Leave the creation up to Spring bean = autoWiringFactory.createBean(clazz, autowireStrategy, false); injectApplicationContext(bean); return injectInternalBeans(bean); } else if (enableAopSupport) { bean = autoWiringFactory.createBean(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false); bean = autoWireBean(bean, autoWiringFactory); bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName()); return bean; } else { bean = autoWiringFactory.autowire(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false); bean = autoWiringFactory.applyBeanPostProcessorsBeforeInitialization(bean, bean.getClass().getName()); bean = autoWiringFactory.initializeBean(bean, bean.getClass().getName()); bean = autoWiringFactory.applyBeanPostProcessorsAfterInitialization(bean, bean.getClass().getName()); return autoWireBean(bean, autoWiringFactory); } } catch (UnsatisfiedDependencyException e) { LOG.error("Error building bean", e); // Fall back return autoWireBean(super.buildBean(clazz, extraContext), autoWiringFactory); } } The last else-block is the issue because the BeanPostProcessor s are being manually invoked on the autowire factory yet initializeBean specifically does that per the documentation and has since at least Spring 3.0.5.
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Not sure if I understand, the mentioned lines are using autoWiringFactory.autowire(...) which has the following JavaDoc

        	 * <p>Does <i>not</i> apply standard {@link BeanPostProcessor BeanPostProcessors}
        	 * callbacks or perform any further initialization of the bean. This interface
        	 * offers distinct, fine-grained operations for those purposes, for example
        	 * {@link #initializeBean}. However, {@link InstantiationAwareBeanPostProcessor}
        	 * callbacks are applied, if applicable to the construction of the instance.
        

        So according to this BeanPostProcessor shouldn't be applied and if it is it's a bug in Spring.

        Show
        lukaszlenart Lukasz Lenart added a comment - Not sure if I understand, the mentioned lines are using autoWiringFactory.autowire(...) which has the following JavaDoc * <p>Does <i>not</i> apply standard {@link BeanPostProcessor BeanPostProcessors} * callbacks or perform any further initialization of the bean. This interface * offers distinct, fine-grained operations for those purposes, for example * {@link #initializeBean}. However, {@link InstantiationAwareBeanPostProcessor} * callbacks are applied, if applicable to the construction of the instance. So according to this BeanPostProcessor shouldn't be applied and if it is it's a bug in Spring.
        Hide
        lukaszlenart Lukasz Lenart added a comment -
        Show
        lukaszlenart Lukasz Lenart added a comment - Discussion here http://markmail.org/message/3fq36fzlonh5dn4a

          People

          • Assignee:
            Unassigned
            Reporter:
            crancran Chris Cranford
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development