From 963b473e6c99fd1a79962bad4b16f0a6c6c82993 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Mon, 20 Jun 2022 17:07:57 -0400 Subject: [PATCH 1/3] Initial commit: - Update to use consistent name for expression and BeanInfo factories in default configuration implementation and provider. - Add inject annotation to the non-default constructor to ensure the proper constructor is called during DI for containers. - Use fully-qualified factory names for defaults in default.properties. - Ensure unique types for each cache factory in struts-default.xml --- .../opensymphony/xwork2/config/impl/DefaultConfiguration.java | 4 ++-- .../config/providers/StrutsDefaultConfigurationProvider.java | 4 ++-- core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java | 1 + core/src/main/resources/org/apache/struts2/default.properties | 4 ++-- core/src/main/resources/struts-default.xml | 4 ++-- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index 91b1b9a33..fea65cbf6 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -298,8 +298,8 @@ public class DefaultConfiguration implements Configuration { 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, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON); + builder.factory(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON); builder.factory(OgnlUtil.class, Scope.SINGLETON); builder.factory(ValueSubstitutor.class, EnvsValueSubstitutor.class, Scope.SINGLETON); diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index 1442e920f..394aaa69d 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java @@ -217,8 +217,8 @@ public class StrutsDefaultConfigurationProvider implements ConfigurationProvider .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, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON) + .factory(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, DefaultOgnlBeanInfoCacheFactory.class, Scope.SINGLETON) .factory(OgnlUtil.class, Scope.SINGLETON) .factory(CollectionConverter.class, Scope.SINGLETON) .factory(ArrayConverter.class, Scope.SINGLETON) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 5cb12f8a1..560f18968 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java @@ -96,6 +96,7 @@ public class 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 ognlExpressionCacheFactory, @Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, required = false) BeanInfoCacheFactory, BeanInfo> ognlBeanInfoCacheFactory diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 6c6e06497..edc327810 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/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=com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory +struts.ognl.beanInfoCacheFactory=com.opensymphony.xwork2.ognl.DefaultOgnlBeanInfoCacheFactory ### 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 diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index f493fceac..b6ade6a74 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -231,8 +231,8 @@ - - + + From 24e279b16556aa974352132f5b84c8bf5c0f737d Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 3 Jul 2022 17:37:58 -0400 Subject: [PATCH 2/3] Update: - Added arbitrary code coverage test. --- .../StrutsJavaConfigurationProviderTest.java | 56 +++++++++++++++++++ .../config/TestBeanSelectionProvider.java | 6 ++ 2 files changed, 62 insertions(+) diff --git a/core/src/test/java/org/apache/struts2/config/StrutsJavaConfigurationProviderTest.java b/core/src/test/java/org/apache/struts2/config/StrutsJavaConfigurationProviderTest.java index 5e08f9822..b361cf837 100644 --- a/core/src/test/java/org/apache/struts2/config/StrutsJavaConfigurationProviderTest.java +++ b/core/src/test/java/org/apache/struts2/config/StrutsJavaConfigurationProviderTest.java @@ -104,4 +104,60 @@ public class StrutsJavaConfigurationProviderTest { 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 unknownHandlerStack() { + return Collections.singletonList(expectedUnknownHandler); + } + + @Override + public List constants() { + return Collections.singletonList(constantConfig); + } + + @Override + public List beans() { + return Arrays.asList( + new BeanConfig(TestBean.class, "struts") + ); + } + + @Override + public Optional 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(); + } + } } diff --git a/core/src/test/java/org/apache/struts2/config/TestBeanSelectionProvider.java b/core/src/test/java/org/apache/struts2/config/TestBeanSelectionProvider.java index 79d6dd28c..27fa961c3 100644 --- a/core/src/test/java/org/apache/struts2/config/TestBeanSelectionProvider.java +++ b/core/src/test/java/org/apache/struts2/config/TestBeanSelectionProvider.java @@ -21,6 +21,7 @@ package org.apache.struts2.config; 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 class TestBeanSelectionProvider extends AbstractBeanSelectionProvider { 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); + } + } From 15bbf0ef1d21ae75f6eec243c613527fe9e90f79 Mon Sep 17 00:00:00 2001 From: JCgH4164838Gh792C124B5 <43964333+JCgH4164838Gh792C124B5@users.noreply.github.com> Date: Sun, 31 Jul 2022 18:23:10 -0400 Subject: [PATCH 3/3] Updated commit: - Incorporate changes from Y. Zamani's PR #581 manually, which appears to fix the previous issue that prevented customized cache factory implementations from being used (tested with sample app). Credit goes to Yasser Zamani for the fixes. - Updated unit tests and slight modifications to the changes from the PR #581. --- .../config/impl/DefaultConfiguration.java | 4 +- .../StrutsDefaultConfigurationProvider.java | 4 +- .../opensymphony/xwork2/ognl/OgnlUtil.java | 21 ++++--- .../org/apache/struts2/default.properties | 4 +- core/src/main/resources/struts-default.xml | 4 +- .../xwork2/ognl/OgnlUtilTest.java | 61 +++++-------------- 6 files changed, 35 insertions(+), 63 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java index fea65cbf6..71fdf2ff8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/impl/DefaultConfiguration.java @@ -298,8 +298,8 @@ public class DefaultConfiguration implements Configuration { builder.factory(ObjectTypeDeterminer.class, DefaultObjectTypeDeterminer.class, Scope.SINGLETON); builder.factory(PropertyAccessor.class, CompoundRoot.class.getName(), CompoundRootAccessor.class, Scope.SINGLETON); - builder.factory(ExpressionCacheFactory.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON); - builder.factory(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, 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); diff --git a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java index 394aaa69d..49308d263 100644 --- a/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java +++ b/core/src/main/java/com/opensymphony/xwork2/config/providers/StrutsDefaultConfigurationProvider.java @@ -217,8 +217,8 @@ public class StrutsDefaultConfigurationProvider implements ConfigurationProvider .factory(TextProviderFactory.class, StrutsTextProviderFactory.class, Scope.SINGLETON) .factory(LocaleProviderFactory.class, DefaultLocaleProviderFactory.class, Scope.SINGLETON) - .factory(ExpressionCacheFactory.class, StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, DefaultOgnlExpressionCacheFactory.class, Scope.SINGLETON) - .factory(BeanInfoCacheFactory.class, StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, 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) diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java b/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java index 560f18968..49be23790 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/OgnlUtil.java +++ b/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(), + new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); } /** @@ -98,9 +100,15 @@ public class OgnlUtil { */ @Inject public OgnlUtil( - @Inject(value = StrutsConstants.STRUTS_OGNL_EXPRESSION_CACHE_FACTORY, required = false) ExpressionCacheFactory ognlExpressionCacheFactory, - @Inject(value = StrutsConstants.STRUTS_OGNL_BEANINFO_CACHE_FACTORY, required = false) BeanInfoCacheFactory, BeanInfo> ognlBeanInfoCacheFactory + @Inject ExpressionCacheFactory ognlExpressionCacheFactory, + @Inject BeanInfoCacheFactory, 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<>()); @@ -109,11 +117,8 @@ public class OgnlUtil { devModeExcludedPackageNamePatterns = Collections.unmodifiableSet(new HashSet<>()); devModeExcludedPackageNames = Collections.unmodifiableSet(new HashSet<>()); - OgnlCacheFactory ognlExpressionCacheFactory1 = (ognlExpressionCacheFactory != null ? ognlExpressionCacheFactory : new DefaultOgnlExpressionCacheFactory<>()); - OgnlCacheFactory, BeanInfo> ognlBeanInfoCacheFactory1 = (ognlBeanInfoCacheFactory != null ? ognlBeanInfoCacheFactory : new DefaultOgnlBeanInfoCacheFactory<>()); - - this.expressionCache = ognlExpressionCacheFactory1.buildOgnlCache(); - this.beanInfoCache = ognlBeanInfoCacheFactory1.buildOgnlCache(); + this.expressionCache = ognlExpressionCacheFactory.buildOgnlCache(); + this.beanInfoCache = ognlBeanInfoCacheFactory.buildOgnlCache(); } @Inject diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index edc327810..753a80b8d 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/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=com.opensymphony.xwork2.ognl.DefaultOgnlExpressionCacheFactory -struts.ognl.beanInfoCacheFactory=com.opensymphony.xwork2.ognl.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 diff --git a/core/src/main/resources/struts-default.xml b/core/src/main/resources/struts-default.xml index b6ade6a74..f06a338d7 100644 --- a/core/src/main/resources/struts-default.xml +++ b/core/src/main/resources/struts-default.xml @@ -231,8 +231,8 @@ - - + + diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java index 8549fe8b2..b5795a326 100644 --- a/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/ognl/OgnlUtilTest.java @@ -1313,11 +1313,20 @@ public class OgnlUtilTest extends XWorkTestCase { 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(), null); + fail("null beanInfoCacheFactory should result in exception"); + } catch (IllegalArgumentException iaex) { + // expected result + } + try { + OgnlUtil basicOgnlUtil = new OgnlUtil(null, new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); + fail("null expressionCacheFactory should result in exception"); + } catch (IllegalArgumentException iaex) { + // expected result + } } public void testDefaultOgnlUtilExclusionsAlternateConstructorPopulated() { @@ -1690,20 +1699,6 @@ public class OgnlUtilTest extends XWorkTestCase { } } - 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(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); @@ -1732,20 +1727,6 @@ public class OgnlUtilTest extends XWorkTestCase { } } - 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(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>()); @@ -1774,20 +1755,6 @@ public class OgnlUtilTest extends XWorkTestCase { } } - 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(), new DefaultOgnlBeanInfoCacheFactory, BeanInfo>());