fix(spring): WW-3647 change autowire alwaysRespect default to true (#1571)

Change the default value of struts.objectFactory.spring.autoWire.alwaysRespect from false to true to fix the Spring constructor autowiring issue. When a Spring String bean exists (e.g., JNDI lookup with default-value), Spring's AUTOWIRE_CONSTRUCTOR strategy incorrectly injects that value into ALL String parameters of ServletActionRedirectResult constructors, causing malformed redirect URLs. Setting alwaysRespect to true by default ensures the configured autowire strategy (AUTOWIRE_BY_NAME) is consistently used, preventing unintended bean injection. Users who rely on the legacy constructor autowiring behavior can restore it by setting: <constant name="struts.objectFactory.spring.autoWire.alwaysRespect" value="false" /> Fixes https://issues.apache.org/jira/browse/WW-3647 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
2026-08-05 14:47:09 +00:00 · 2026-02-09 10:08:33 +02:00
parent 2bff83282d
commit fd87425863
4 changed files with 221 additions and 17 deletions
@@ -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.
+     * <p>
+     * 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 <a href="https://issues.apache.org/jira/browse/WW-3647">WW-3647</a>
     * @since 2.1.3
     */
    public static final String STRUTS_OBJECTFACTORY_SPRING_AUTOWIRE_ALWAYS_RESPECT = "struts.objectFactory.spring.autoWire.alwaysRespect";
@@ -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
@@ -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<String, Object> 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 class SpringObjectFactory extends ObjectFactory implements ApplicationCon
     * set the autoWiringFactory appropriately.
     *
     * @param context the application context
-     *
     * @return the bean factory
     */
    protected AutowireCapableBeanFactory findAutoWiringBeanFactory(ApplicationContext context) {
@@ -153,8 +153,7 @@ public class SpringObjectFactory extends ObjectFactory implements ApplicationCon
     *
     * @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 class SpringObjectFactory extends ObjectFactory implements ApplicationCon
    }

    /**
-     * @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 class SpringObjectFactory extends ObjectFactory implements ApplicationCon
    }

    /**
-     * @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 @@ public class SpringObjectFactory extends ObjectFactory implements ApplicationCon
    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 SpringObjectFactory extends ObjectFactory implements ApplicationCon
            }

            if (useClassCache) {
-                synchronized(classes) {
+                synchronized (classes) {
                    classes.put(className, clazz);
                }
            }
@@ -271,7 +269,7 @@ public class SpringObjectFactory extends ObjectFactory implements ApplicationCon
    }

    /**
-     *  Enable / disable caching of classes loaded by Spring.
+     * Enable / disable caching of classes loaded by Spring.
     *
     * @param useClassCache enable / disable class cache
     */
@@ -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
+
+<jee:jndi-lookup jndi-name="someName" id="currentEnvironment" default-value="XXXX"/>
+```
+
+`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
+
+<constant name="struts.objectFactory.spring.autoWire.alwaysRespect" value="true"/>
+```
+
+**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?