diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index a66478a97c..c7e1dd916c 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -256,9 +256,15 @@ public final class StrutsConstants { public static final String STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE = "struts.objectFactory.spring.autoWire"; /** - * Whether the autowire strategy chosen by STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE is always respected. Defaults - * to false, which is the legacy behavior that tries to determine the best strategy for the situation. + * Whether the autowire strategy chosen by STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE is always respected. + * Defaults to true, which ensures the configured autowire strategy (AUTOWIRE_BY_NAME by default) is + * consistently used. This prevents issues where Spring's AUTOWIRE_CONSTRUCTOR strategy could inject + * unintended beans (e.g., String beans) into constructors. + *

+ * Set to false to restore legacy behavior that mixes injection strategies, but be aware this can + * cause issues like WW-3647 where String beans are incorrectly injected into result class constructors. * + * @see WW-3647 * @since 2.1.3 */ public static final String STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE_ALWAYS_RESPECT = "struts.objectFactory.spring.autoWire.alwaysRespect"; diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index a980196a68..59165eca3b 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -42,8 +42,8 @@ struts.objectFactory.spring.autoWire = name struts.objectFactory.spring.useClassCache = true ### ensures the autowire strategy is always respected. -### valid values are: true, false (false is the default) -struts.objectFactory.spring.autoWire.alwaysRespect = false +### valid values are: true, false (true is the default) +struts.objectFactory.spring.autoWire.alwaysRespect=true ### By default SpringObjectFactory doesn't support AOP ### This flag was added just temporally to check if nothing is broken diff --git a/plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java b/plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java index 2cfd97b667..e6cd2195bc 100644 --- a/plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java +++ b/plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java @@ -18,11 +18,11 @@ */ package org.apache.struts2.spring; -import org.apache.struts2.ObjectFactory; -import org.apache.struts2.inject.Inject; import org.apache.commons.lang3.BooleanUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.ObjectFactory; +import org.apache.struts2.inject.Inject; import org.springframework.beans.BeansException; import org.springframework.beans.factory.UnsatisfiedDependencyException; import org.springframework.beans.factory.config.AutowireCapableBeanFactory; @@ -55,9 +55,10 @@ public class SpringObjectFactory extends ObjectFactory implements ApplicationCon protected int autowireStrategy = AutowireCapableBeanFactory.AUTOWIRE_BY_NAME; private final Map classes = new HashMap<>(); private boolean useClassCache = true; - private boolean alwaysRespectAutowireStrategy = false; + private boolean alwaysRespectAutowireStrategy = true; /** * This is temporary solution, after validating can be removed + * * @since 2.3.18 */ @Deprecated @@ -66,7 +67,7 @@ public class SpringObjectFactory extends ObjectFactory implements ApplicationCon public SpringObjectFactory() { } - @Inject(value="applicationContextPath",required=false) + @Inject(value = "applicationContextPath", required = false) public void setApplicationContextPath(String ctx) { if (ctx != null) { setApplicationContext(new ClassPathXmlApplicationContext(ctx)); @@ -130,7 +131,6 @@ public int getAutowireStrategy() { * set the autoWiringFactory appropriately. * * @param context the application context - * * @return the bean factory */ protected AutowireCapableBeanFactory findAutoWiringBeanFactory(ApplicationContext context) { @@ -153,8 +153,7 @@ protected AutowireCapableBeanFactory findAutoWiringBeanFactory(ApplicationContex * * @param beanName The name of the bean to look up in the application context * @param extraContext additional context parameters - * @return A bean from Spring or the result of calling the overridden - * method. + * @return A bean from Spring or the result of calling the overridden method. * @throws Exception in case of any errors */ @Override @@ -174,7 +173,7 @@ public Object buildBean(String beanName, Map extraContext, boole } /** - * @param clazz class of bean + * @param clazz class of bean * @param extraContext additional context parameters * @return bean * @throws Exception in case of any errors @@ -212,9 +211,8 @@ public Object autoWireBean(Object bean) { } /** - * @param bean the bean to be autowired + * @param bean the bean to be autowired * @param autoWiringFactory the autowiring factory - * * @return bean */ public Object autoWireBean(Object bean, AutowireCapableBeanFactory autoWiringFactory) { @@ -235,7 +233,7 @@ private void injectApplicationContext(Object bean) { public Class getClassInstance(String className) throws ClassNotFoundException { Class clazz = null; if (useClassCache) { - synchronized(classes) { + synchronized (classes) { // this cache of classes is needed because Spring sucks at dealing with situations where the // class instance changes clazz = (Class) classes.get(className); @@ -250,7 +248,7 @@ public Class getClassInstance(String className) throws ClassNotFoundException { } if (useClassCache) { - synchronized(classes) { + synchronized (classes) { classes.put(className, clazz); } } @@ -271,7 +269,7 @@ public boolean isNoArgConstructorRequired() { } /** - * Enable / disable caching of classes loaded by Spring. + * Enable / disable caching of classes loaded by Spring. * * @param useClassCache enable / disable class cache */ diff --git a/thoughts/shared/research/2026-02-07-WW-3647-spring-constructor-autowiring.md b/thoughts/shared/research/2026-02-07-WW-3647-spring-constructor-autowiring.md new file mode 100644 index 0000000000..75c884731a --- /dev/null +++ b/thoughts/shared/research/2026-02-07-WW-3647-spring-constructor-autowiring.md @@ -0,0 +1,200 @@ +--- +date: 2026-02-07T12:00:00+01:00 +topic: "WW-3647: ServletActionRedirectResult Spring Constructor Autowiring Issue" +tags: [ research, codebase, spring-plugin, autowiring, constructor-injection ] +status: complete +--- + +# Research: WW-3647 ServletActionRedirectResult Spring Constructor Autowiring Issue + +**Date**: 2026-02-07 + +## Research Question + +Is WW-3647 (ServletActionRedirectResult injection problem with Spring JNDI lookup beans) still an issue, and what is the +root cause? + +## Summary + +**YES, this issue is still present in the current codebase.** The root cause is the default "mixed injection strategy" +in `SpringObjectFactory` that uses `AUTOWIRE_CONSTRUCTOR`, which causes Spring to inject any available String bean into +all String parameters of result class constructors. + +## Detailed Findings + +### The Problem + +When a Spring JNDI lookup bean with a String default value exists: + +```xml + + +``` + +`ServletActionRedirectResult` redirect URLs become malformed: + +``` +http://localhost:8080/XXXX/index!XXXX.action#XXXX +``` + +### Root Cause + +The issue is in `SpringObjectFactory.buildBean(Class, Map)` at line 199: + +```java +// When alwaysRespectAutowireStrategy is false (DEFAULT) +bean =autoWiringFactory. + +autowire(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false); +``` + +Spring's `AUTOWIRE_CONSTRUCTOR` strategy: + +1. Examines all constructors of `ServletActionRedirectResult` +2. Finds constructors with String parameters +3. Looks for Spring beans of type `String` to inject +4. Injects the JNDI bean's String value into **ALL** String parameters + +### ServletActionRedirectResult Constructors + +The class has 5 constructors, 4 of which take String parameters ( +`core/src/main/java/org/apache/struts2/result/ServletActionRedirectResult.java:135-156`): + +```java +public ServletActionRedirectResult() // no-arg + +public ServletActionRedirectResult(String actionName) // 1 String + +public ServletActionRedirectResult(String actionName, String method) // 2 Strings + +public ServletActionRedirectResult(String namespace, String actionName, String method) // 3 Strings + +public ServletActionRedirectResult(String namespace, String actionName, String method, String anchor) // 4 Strings +``` + +When Spring's constructor autowiring finds a String bean, it matches that bean to **every** String parameter, resulting +in: + +- `namespace = "XXXX"` +- `actionName = "XXXX"` +- `method = "XXXX"` +- `anchor = "XXXX"` + +### Default Configuration + +In `core/src/main/resources/org/apache/struts2/default.properties`: + +```properties +struts.objectFactory.spring.autoWire.alwaysRespect=false +``` + +This default value enables the legacy mixed injection strategy that causes the issue. + +### Code Flow + +1. Struts needs to create a `ServletActionRedirectResult` instance +2. `SpringObjectFactory.buildBean()` is called +3. Since `alwaysRespectAutowireStrategy = false`, it uses constructor autowiring: + ```java + bean = autoWiringFactory.autowire(clazz, AutowireCapableBeanFactory.AUTOWIRE_CONSTRUCTOR, false); + ``` +4. Spring finds a String bean (the JNDI lookup) and injects it into constructor String parameters +5. The result is instantiated with incorrect values + +### Why the Workaround Works + +Setting `struts.objectFactory.spring.autoWire.alwaysRespect = true` causes the code to take a different path ( +`SpringObjectFactory.java:188-192`): + +```java +if(alwaysRespectAutowireStrategy){ +// Leave the creation up to Spring +bean =autoWiringFactory. + +createBean(clazz, autowireStrategy, false); + +injectApplicationContext(bean); + return + +injectInternalBeans(bean); +} +``` + +The default `autowireStrategy` is `AUTOWIRE_BY_NAME`, which only injects beans when property names match bean names. +Since `ServletActionRedirectResult` doesn't have properties named after JNDI beans, no incorrect injection occurs. + +## Code References + +- `plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java:183-208` - buildBean method with + mixed injection strategy +- `plugins/spring/src/main/java/org/apache/struts2/spring/SpringObjectFactory.java:58` - alwaysRespectAutowireStrategy + field (default: false) +- `core/src/main/java/org/apache/struts2/result/ServletActionRedirectResult.java:135-156` - Constructors with String + parameters +- `core/src/main/java/org/apache/struts2/StrutsConstants.java:138` - STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE_ALWAYS_RESPECT + constant +- `core/src/main/resources/org/apache/struts2/default.properties` - Default configuration + +## Test Coverage Gap + +There are **no tests** in the Spring plugin specifically testing `ServletActionRedirectResult` autowiring behavior. +Existing tests cover: + +- Constructor injection with custom beans (`testShouldUseConstructorBasedInjectionWhenCreatingABeanFromAClassName`) +- alwaysRespect configuration switching +- Fallback behavior for ambiguous constructors + +But none test the scenario of a String bean being incorrectly injected into result class constructors. + +## Potential Fixes + +1. **Change the default**: Set `struts.objectFactory.spring.autoWire.alwaysRespect = true` as the default + - Pros: Fixes the issue for everyone + - Cons: Breaking change for existing applications relying on constructor injection + +2. **Remove String-parameter constructors**: Use only the no-arg constructor with setters + - Pros: Avoids the problem entirely + - Cons: Less convenient API, breaks existing code using these constructors + +3. **Add @Autowired(required=false) annotations**: Explicitly mark constructors + - Pros: Gives control over which constructors Spring considers + - Cons: Adds Spring-specific annotations to core classes + +4. **Use primary constructor designation**: Mark the no-arg constructor as preferred + - Pros: Spring would prefer the no-arg constructor + - Cons: Requires additional Spring configuration + +## Workaround (Still Valid) + +Configure Struts to respect the autowire strategy: + +**struts.xml:** + +```xml + + +``` + +**struts.properties:** + +```properties +struts.objectFactory.spring.autoWire.alwaysRespect=true +``` + +## Architecture Insights + +The "mixed injection strategy" was designed to provide flexibility by: + +1. First using constructor autowiring to create beans +2. Then applying property-based autowiring for additional dependencies + +However, this approach has an inherent flaw: constructor autowiring is too aggressive with simple types like String, +matching any available String bean to any String constructor parameter. This design predates the modern understanding +that constructor injection should primarily be used for required dependencies with specific types. + +## Open Questions + +1. Should the default for `alwaysRespectAutowireStrategy` be changed to `true`? +2. Are there other result classes or Struts components with similar String-parameter constructors that might be + affected? +3. Is the "mixed injection strategy" still needed, or should it be deprecated?