[WW-5173] - Attempt to fix DI behaviour for custom cache factories

core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java

@@ -298,8 +298,8 @@ protected Container createBootstrapContainer(List<ContainerProvider> providers)
 
         builder.factory(ObjectTypeDeterminer.class, DefaultObjectTypeDeterminer.class, Scope.SINGLETON);
         builder.factory(PropertyAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON);
-        builder.factory(ExpressionCacheFactory.class, "defaultOgnlExpressionCacheFactory", DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON);
-        builder.factory(BeanInfoCacheFactory.class, "defaultOgnlBeanInfoCacheFactory", DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON);
+        builder.factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON);
+        builder.factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON);
         builder.factory(OgnlUtil.class, Scope.SINGLETON);
 
         builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON);

core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java

@@ -217,8 +217,8 @@ public void register(ContainerBuilder builder, LocatableProperties props)
                 .factory(TextProviderFactory.class, StrutsTextProviderFactory.class, Scope.SINGLETON)
                 .factory(LocaleProviderFactory.class, DefaultLocaleProviderFactory.class, Scope.SINGLETON)
 
-                .factory(ExpressionCacheFactory.class, "defaultOgnlExpressionCacheFactory", DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON)
-                .factory(BeanInfoCacheFactory.class, "defaultOgnlBeanInfoCacheFactory", DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON)
+                .factory(ExpressionCacheFactory.class, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON)
+                .factory(BeanInfoCacheFactory.class, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON)
                 .factory(OgnlUtil.class, Scope.SINGLETON)
                 .factory(CollectionConverter.class, Scope.SINGLETON)
                 .factory(ArrayConverter.class, Scope.SINGLETON)

core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java

@@ -82,7 +82,9 @@ public class OgnlUtil {
      */
     @Deprecated
     public OgnlUtil() {
-        this(null, null);  // Instantiate default Expression and BeanInfo caches (null factories)
+        // Instantiate default Expression and BeanInfo caches (factories must be non-null).
+        this(new DefaultOgnlExpressionCacheFactory<String, Object>(),
+             new DefaultOgnlBeanInfoCacheFactory<Class<?>, BeanInfo>());
     }
 
     /**
@@ -96,10 +98,17 @@ public OgnlUtil() {
      * @param ognlExpressionCacheFactory factory for Expression cache instance.  If null, it uses a default
      * @param ognlBeanInfoCacheFactory factory for BeanInfo cache instance.  If null, it uses a default
      */
+    @Inject
     public OgnlUtil(
-            @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, required = false) ExpressionCacheFactory<String, Object> ognlExpressionCacheFactory,
-            @Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, required = false) BeanInfoCacheFactory<Class<?>, BeanInfo> ognlBeanInfoCacheFactory
+            @Inject ExpressionCacheFactory<String, Object> ognlExpressionCacheFactory,
+            @Inject BeanInfoCacheFactory<Class<?>, BeanInfo> ognlBeanInfoCacheFactory
     ) {
+        if (ognlExpressionCacheFactory == null) {
+            throw new IllegalArgumentException("ExpressionCacheFactory parameter cannot be null");
+        }
+        if (ognlBeanInfoCacheFactory == null) {
+            throw new IllegalArgumentException("BeanInfoCacheFactory parameter cannot be null");
+        }
         excludedClasses = Collections.unmodifiableSet(new HashSet<>());
         excludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>());
         excludedPackageNames = Collections.unmodifiableSet(new HashSet<>());
@@ -108,11 +117,8 @@ public OgnlUtil(
         devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>());
         devModeExcludedPackageNames = Collections.unmodifiableSet(new HashSet<>());
 
-        OgnlCacheFactory<String, Object> ognlExpressionCacheFactory1 = (ognlExpressionCacheFactory != null ? ognlExpressionCacheFactory : new DefaultOgnlExpressionCacheFactory<>());
-        OgnlCacheFactory<Class<?>, BeanInfo> ognlBeanInfoCacheFactory1 = (ognlBeanInfoCacheFactory != null ? ognlBeanInfoCacheFactory : new DefaultOgnlBeanInfoCacheFactory<>());
-
-        this.expressionCache = ognlExpressionCacheFactory1.buildOgnlCache();
-        this.beanInfoCache = ognlBeanInfoCacheFactory1.buildOgnlCache();
+        this.expressionCache = ognlExpressionCacheFactory.buildOgnlCache();
+        this.beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache();
     }
 
     @Inject

core/src/main/resources/org/apache/struts2/default.properties

@@ -231,8 +231,8 @@ struts.ognl.enableExpressionCache=true
 
 ### Specify the OGNL expression cache factory and BeanInfo cache factory to use.
 ### Currently, the default implementations are used, but can be replaced with custom ones if desired.
-struts.ognl.expressionCacheFactory=defaultOgnlExpressionCacheFactory
-struts.ognl.beanInfoCacheFactory=defaultOgnlBeanInfoCacheFactory
+# struts.ognl.expressionCacheFactory=customOgnlExpressionCacheFactory
+# struts.ognl.beanInfoCacheFactory=customOgnlBeanInfoCacheFactory
 
 ### Specify a limit to the number of entries in the OGNL expressionCache.
 ### For the standard expressionCache mode, when the limit is exceeded the entire cache's

core/src/main/resources/struts-default.xml

@@ -231,8 +231,8 @@
     <bean type="org.apache.struts2.components.date.DateFormatter" name="simpleDateFormatter" class="org.apache.struts2.components.date.SimpleDateFormatAdapter" scope="singleton"/>
     <bean type="org.apache.struts2.components.date.DateFormatter" name="dateTimeFormatter" class="org.apache.struts2.components.date.DateTimeFormatterAdapter" scope="singleton"/>
 
-    <bean type="com.opensymphony.xwork2.ognl.OgnlCacheFactory" name="ognlExpressionCacheFactory" class="com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory" scope="singleton"/>
-    <bean type="com.opensymphony.xwork2.ognl.OgnlCacheFactory" name="ognlBeanInfoCacheFactory" class="com.opensymphony.xwork2.ognl.DefaultOgnlBeanInfoCacheFactory" scope="singleton"/>
+    <bean type="com.opensymphony.xwork2.ognl.ExpressionCacheFactory" name="struts" class="com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory" scope="singleton" />
+    <bean type="com.opensymphony.xwork2.ognl.BeanInfoCacheFactory" name="struts" class="com.opensymphony.xwork2.ognl.DefaultOgnlBeanInfoCacheFactory" scope="singleton" />
 
     <package name="struts-default" abstract="true">
         <result-types>

core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java

@@ -1313,11 +1313,20 @@ public void testDefaultOgnlUtilExclusions() {
         internalTestOgnlUtilExclusionsImmutable(basicOgnlUtil);
     }
 
-    public void testDefaultOgnlUtilExclusionsAlternateConstructor() {
-        OgnlUtil basicOgnlUtil = new OgnlUtil(null, null);
-
-        internalTestInitialEmptyOgnlUtilExclusions(basicOgnlUtil);
-        internalTestOgnlUtilExclusionsImmutable(basicOgnlUtil);
+    public void testDefaultOgnlUtilAlternateConstructorArguments() {
+        // Code coverage test for the OgnlUtil alternate constructor method, and verify expected behaviour.
+        try {
+            OgnlUtil basicOgnlUtil = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<String, Object>(), null);
+            fail("null beanInfoCacheFactory should result in exception");
+        } catch (IllegalArgumentException iaex) {
+            // expected result
+        }
+        try {
+            OgnlUtil basicOgnlUtil = new OgnlUtil(null, new DefaultOgnlBeanInfoCacheFactory<Class<?>, BeanInfo>());
+            fail("null expressionCacheFactory should result in exception");
+        } catch (IllegalArgumentException iaex) {
+            // expected result
+        }
     }
 
     public void testDefaultOgnlUtilExclusionsAlternateConstructorPopulated() {
@@ -1690,20 +1699,6 @@ public void testGetExcludedPackageNames() {
         }
     }
 
-    public void testGetExcludedPackageNamesAlternateConstructor() {
-        // Getter should return an immutable collection
-        OgnlUtil util = new OgnlUtil(null, null);
-        util.setExcludedPackageNames("java.lang,java.awt");
-        assertEquals(util.getExcludedPackageNames().size(), 2);
-        try {
-            util.getExcludedPackageNames().clear();
-        } catch (Exception ex) {
-            assertTrue(ex instanceof UnsupportedOperationException);
-        } finally {
-            assertEquals(util.getExcludedPackageNames().size(), 2);
-        }
-    }
-
     public void testGetExcludedPackageNamesAlternateConstructorPopulated() {
         // Getter should return an immutable collection
         OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<String, Object>(), new DefaultOgnlBeanInfoCacheFactory<Class<?>, BeanInfo>());
@@ -1732,20 +1727,6 @@ public void testGetExcludedClasses() {
         }
     }
 
-    public void testGetExcludedClassesAlternateConstructor() {
-        // Getter should return an immutable collection
-        OgnlUtil util = new OgnlUtil(null, null);
-        util.setExcludedClasses("java.lang.Runtime,java.lang.ProcessBuilder,java.net.URL");
-        assertEquals(util.getExcludedClasses().size(), 3);
-        try {
-            util.getExcludedClasses().clear();
-        } catch (Exception ex) {
-            assertTrue(ex instanceof UnsupportedOperationException);
-        } finally {
-            assertEquals(util.getExcludedClasses().size(), 3);
-        }
-    }
-
     public void testGetExcludedClassesAlternateConstructorPopulated() {
         // Getter should return an immutable collection
         OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<String, Object>(), new DefaultOgnlBeanInfoCacheFactory<Class<?>, BeanInfo>());
@@ -1774,20 +1755,6 @@ public void testGetExcludedPackageNamePatterns() {
         }
     }
 
-    public void testGetExcludedPackageNamePatternsAlternateConstructor() {
-        // Getter should return an immutable collection
-        OgnlUtil util = new OgnlUtil(null, null);
-        util.setExcludedPackageNamePatterns("java.lang.");
-        assertEquals(util.getExcludedPackageNamePatterns().size(), 1);
-        try {
-            util.getExcludedPackageNamePatterns().clear();
-        } catch (Exception ex) {
-            assertTrue(ex instanceof UnsupportedOperationException);
-        } finally {
-            assertEquals(util.getExcludedPackageNamePatterns().size(), 1);
-        }
-    }
-
     public void testGetExcludedPackageNamePatternsAlternateConstructorPopulated() {
         // Getter should return an immutable collection
         OgnlUtil util = new OgnlUtil(new DefaultOgnlExpressionCacheFactory<String, Object>(), new DefaultOgnlBeanInfoCacheFactory<Class<?>, BeanInfo>());

core/src/test/java/org/apache/struts2/config/StrutsJavaConfigurationProviderTest.java

@@ -104,4 +104,60 @@ public Optional<BeanSelectionConfig> beanSelection() {
         Assert.assertTrue(names.contains("struts"));
         Assert.assertTrue(names.contains("struts.test.bean"));
     }
+
+    @Test
+    /**
+     * This test is purely to provide code coverage for {@link AbstractBeanSelectionProvider}.
+     * It uses an arbitrary setup to ensure a code path not followed in the registration test
+     * is traversed.
+     */
+    public void testAbstractBeanProviderCoverage() throws Exception {
+        final ConstantConfig constantConfig = new ConstantConfig();
+        final String expectedUnknownHandler = "expectedUnknownHandler";
+
+        StrutsJavaConfiguration javaConfig = new StrutsJavaConfiguration() {
+            @Override
+            public List<String> unknownHandlerStack() {
+                return Collections.singletonList(expectedUnknownHandler);
+            }
+
+            @Override
+            public List<ConstantConfig> constants() {
+                return Collections.singletonList(constantConfig);
+            }
+
+            @Override
+            public List<BeanConfig> beans() {
+                return Arrays.asList(
+                    new BeanConfig(TestBean.class, "struts")
+                );
+            }
+
+            @Override
+            public Optional<BeanSelectionConfig> beanSelection() {
+                return Optional.of(new BeanSelectionConfig(TestBeanSelectionProvider.class, "testBeans"));
+            }
+        };
+
+        StrutsJavaConfigurationProvider provider = new StrutsJavaConfigurationProvider(javaConfig);
+        Configuration configuration = new MockConfiguration();
+        ContainerBuilder builder = new ContainerBuilder();
+        LocatableProperties props = new LocatableProperties();
+
+        provider.init(configuration);
+        provider.register(builder, props);
+
+        props.put(CodeCoverageTestClass1.ALIAS_KEY, CodeCoverageTestClass1.ALIAS_VALUE);
+        TestBeanSelectionProvider testBeanSelectionProvider = new TestBeanSelectionProvider();
+        testBeanSelectionProvider.aliasCallCoverage(CodeCoverageTestClass1.class, builder, props, CodeCoverageTestClass1.ALIAS_KEY, Scope.THREAD);
+    }
+
+    final class CodeCoverageTestClass1 extends Object {
+        public static final String ALIAS_KEY = "testAliasKey";
+        public static final String ALIAS_VALUE  = "testAliasValue";
+
+        public CodeCoverageTestClass1() {
+            super();
+        }
+    }
 }

core/src/test/java/org/apache/struts2/config/TestBeanSelectionProvider.java

@@ -21,6 +21,7 @@
 import com.opensymphony.xwork2.TestBean;
 import com.opensymphony.xwork2.config.ConfigurationException;
 import com.opensymphony.xwork2.inject.ContainerBuilder;
+import com.opensymphony.xwork2.inject.Scope;
 import com.opensymphony.xwork2.util.location.LocatableProperties;
 
 public class TestBeanSelectionProvider extends AbstractBeanSelectionProvider {
@@ -30,4 +31,9 @@ public void register(ContainerBuilder builder, LocatableProperties props) throws
         alias(TestBean.class, "struts.test.bean", builder, props);
     }
 
+    public void aliasCallCoverage(Class aliasClass, ContainerBuilder builder, LocatableProperties props, String aliasKey, Scope scope) throws ConfigurationException {
+        // Allow for coverage testing of AbstractBeanSelectionProvider.
+        alias(aliasClass, aliasKey, builder, props, scope);
+    }
+
 }