diff --git a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsPreparerFactory.java b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsPreparerFactory.java index db37af2788..dcefcb312b 100644 --- a/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsPreparerFactory.java +++ b/plugins/tiles/src/main/java/org/apache/struts2/tiles/StrutsPreparerFactory.java @@ -22,9 +22,10 @@ import org.apache.struts2.ObjectFactory; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.tiles.api.preparer.ViewPreparer; import org.apache.tiles.core.prepare.factory.BasicPreparerFactory; +import org.apache.tiles.request.reflect.CannotInstantiateObjectException; +import org.apache.tiles.request.reflect.ClassUtil; /** * This is a basic ViewPreparer factory that uses {@link ObjectFactory} to create the ViewPreparer @@ -41,12 +42,33 @@ protected ViewPreparer createPreparer(String name) { return super.createPreparer(name); } + checkType(name); + + Object preparer; try { ObjectFactory factory = actionContext.getContainer().getInstance(ObjectFactory.class); - return (ViewPreparer) factory.buildBean(name, ActionContext.getContext().getContextMap()); + preparer = factory.buildBean(name, actionContext.getContextMap()); } catch (Exception e) { - LOG.error(new ParameterizedMessage("Cannot create a ViewPreparer [{}], fallback to super.createPreparer!", name), e); + LOG.warn("Cannot create a ViewPreparer [{}], fallback to super.createPreparer: {}", name, e.toString()); + LOG.debug("Cannot create a ViewPreparer [{}]", name, e); return super.createPreparer(name); } + + if (preparer instanceof ViewPreparer viewPreparer) { + return viewPreparer; + } + throw new CannotInstantiateObjectException("Preparer: '" + name + "' is not a '" + + ViewPreparer.class.getName() + "'", null); + } + + private void checkType(String name) { + try { + ClassUtil.getClass(name, ViewPreparer.class); + } catch (ClassNotFoundException e) { + LOG.debug("Preparer [{}] does not name a class, leaving it to the object factory", name); + } catch (ClassCastException e) { + throw new CannotInstantiateObjectException("Preparer: '" + name + "' is not a '" + + ViewPreparer.class.getName() + "'", e); + } } } diff --git a/plugins/tiles/src/main/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactory.java b/plugins/tiles/src/main/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactory.java index b28151da20..b97288eb0f 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactory.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactory.java @@ -24,8 +24,8 @@ import org.apache.tiles.request.Request; import org.apache.tiles.request.reflect.ClassUtil; -import java.util.HashMap; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * Default implementation of the {@link org.apache.tiles.core.prepare.factory.PreparerFactory}. @@ -50,7 +50,7 @@ public class BasicPreparerFactory implements PreparerFactory { * Constructor. */ public BasicPreparerFactory() { - this.preparers = new HashMap<>(); + this.preparers = new ConcurrentHashMap<>(); } @@ -63,11 +63,18 @@ public BasicPreparerFactory() { * @return ViewPreparer instance */ public ViewPreparer getPreparer(String name, Request context) { - if (!preparers.containsKey(name)) { - preparers.put(name, createPreparer(name)); + ViewPreparer preparer = preparers.get(name); + if (preparer != null) { + return preparer; } - return preparers.get(name); + preparer = createPreparer(name); + if (preparer == null) { + return null; + } + + ViewPreparer existing = preparers.putIfAbsent(name, preparer); + return existing != null ? existing : preparer; } /** @@ -78,7 +85,6 @@ public ViewPreparer getPreparer(String name, Request context) { */ protected ViewPreparer createPreparer(String name) { LOG.debug("Creating ViewPreparer '{}'", name); - Object instance = ClassUtil.instantiate(name, true); - return (ViewPreparer) instance; + return ClassUtil.instantiate(name, ViewPreparer.class, true); } } diff --git a/plugins/tiles/src/main/java/org/apache/tiles/request/reflect/ClassUtil.java b/plugins/tiles/src/main/java/org/apache/tiles/request/reflect/ClassUtil.java index 8211b3ac62..5433daf6de 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/request/reflect/ClassUtil.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/request/reflect/ClassUtil.java @@ -24,6 +24,7 @@ import java.beans.BeanInfo; import java.beans.Introspector; import java.beans.PropertyDescriptor; +import java.lang.reflect.InvocationTargetException; import java.util.Map; /** @@ -49,13 +50,14 @@ private ClassUtil() { * @param baseClass The base class to subclass to. * @return The loaded class. * @throws ClassNotFoundException If the class has not been found. + * @throws ClassCastException If the class is not assignable to baseClass. */ public static Class getClass(String className, Class baseClass) throws ClassNotFoundException { ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); if (classLoader == null) { classLoader = ClassUtil.class.getClassLoader(); } - return Class.forName(className, true, classLoader).asSubclass(baseClass); + return Class.forName(className, false, classLoader).asSubclass(baseClass); } /** @@ -71,6 +73,51 @@ public static Object instantiate(String className) { return instantiate(className, false); } + /** + * Returns an instance of the given class name, constrained to the given base class, by + * calling the default constructor. + *

+ * The base class is applied by {@link #getClass(String, Class)} before the constructor runs, + * so a class that does not conform to it is rejected without being initialised or constructed. + * + * @param The type the named class has to conform to. + * @param className The class name to load and to instantiate. + * @param baseClass The base class the named class has to be assignable to. + * @param returnNull If true, if the class is not found it returns + * null, otherwise it throws a + * CannotInstantiateObjectException. A class that is found but + * does not conform to baseClass is always reported as an + * exception, because it denotes a misconfigured name rather than a missing + * one. + * @return The new instance of the class name. + * @throws CannotInstantiateObjectException If something goes wrong during instantiation. + */ + public static T instantiate(String className, Class baseClass, boolean returnNull) { + Class namedClass; + try { + namedClass = getClass(className, baseClass); + } catch (ClassNotFoundException e) { + if (returnNull) { + return null; + } + throw new CannotInstantiateObjectException("Unable to resolve factory class: '" + className + "'", e); + } catch (ClassCastException e) { + throw new CannotInstantiateObjectException("Factory class: '" + className + "' is not a '" + + baseClass.getName() + "'", e); + } + try { + return namedClass.getDeclaredConstructor().newInstance(); + } catch (NoSuchMethodException | InstantiationException e) { + throw new CannotInstantiateObjectException("Unable to instantiate factory class: '" + className + + "'. Make sure that this class has a default constructor", e); + } catch (IllegalAccessException e) { + throw new CannotInstantiateObjectException("Unable to access factory class: '" + className + "'", e); + } catch (InvocationTargetException e) { + throw new CannotInstantiateObjectException("Constructor of factory class: '" + className + + "' has thrown an exception", e); + } + } + /** * Returns an instance of the given class name, by calling the default * constructor. diff --git a/plugins/tiles/src/main/java/org/apache/tiles/web/util/TilesDispatchServlet.java b/plugins/tiles/src/main/java/org/apache/tiles/web/util/TilesDispatchServlet.java index decdd607bf..4235e2fd7a 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/web/util/TilesDispatchServlet.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/web/util/TilesDispatchServlet.java @@ -78,7 +78,7 @@ public void init() throws ServletException { String temp = getInitParameter("mutator"); if (temp != null) { try { - mutator = (org.apache.tiles.web.util.AttributeContextMutator) ClassUtil.instantiate(temp); + mutator = ClassUtil.instantiate(temp, AttributeContextMutator.class, false); } catch (Exception e) { throw new ServletException("Unable to instantiate specified context mutator.", e); } diff --git a/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsPreparerFactoryTypeCheckTest.java b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsPreparerFactoryTypeCheckTest.java new file mode 100644 index 0000000000..71454b4c4c --- /dev/null +++ b/plugins/tiles/src/test/java/org/apache/struts2/tiles/StrutsPreparerFactoryTypeCheckTest.java @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.tiles; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.ObjectFactory; +import org.apache.struts2.inject.Container; +import org.apache.tiles.api.AttributeContext; +import org.apache.tiles.api.preparer.ViewPreparer; +import org.apache.tiles.request.Request; +import org.apache.tiles.request.reflect.CannotInstantiateObjectException; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.anyMap; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * The factory applies the ViewPreparer type before the bean is built, so a configured name that does + * not denote a preparer is rejected without anything being constructed, and a name that cannot be + * built is not constructed a second time through the superclass. + */ +public class StrutsPreparerFactoryTypeCheckTest { + + /** + * The evidence lives in its own class so that reading it does not initialise {@link NotAPreparer}. + */ + public static final class Evidence { + public static volatile boolean constructed; + public static volatile int preparerConstructions; + } + + public static class NotAPreparer { + public NotAPreparer() { + Evidence.constructed = true; + } + } + + public static class LegitimatePreparer implements ViewPreparer { + public LegitimatePreparer() { + Evidence.preparerConstructions++; + } + + @Override + public void execute(Request tilesContext, AttributeContext attributeContext) { + } + } + + private ObjectFactory objectFactory; + private StrutsPreparerFactory factory; + + @Before + public void setUp() { + Evidence.constructed = false; + Evidence.preparerConstructions = 0; + + objectFactory = mock(ObjectFactory.class); + Container container = mock(Container.class); + when(container.getInstance(ObjectFactory.class)).thenReturn(objectFactory); + + ActionContext.of().withContainer(container).bind(); + + factory = new StrutsPreparerFactory(); + } + + @After + public void tearDown() { + ActionContext.clear(); + Evidence.constructed = false; + } + + @Test + public void nonPreparerIsRejectedBeforeTheBeanIsBuilt() throws Exception { + // A class literal does not initialise the class it denotes. + String name = NotAPreparer.class.getName(); + + CannotInstantiateObjectException e = assertThrows(CannotInstantiateObjectException.class, + () -> factory.getPreparer(name, null)); + assertNotNull(e.getMessage()); + + verify(objectFactory, never()).buildBean(anyString(), anyMap()); + assertFalse("a class that is not a ViewPreparer must not be constructed", Evidence.constructed); + } + + @Test + public void beanThatIsNotAPreparerIsReportedAndNotBuiltTwice() throws Exception { + String name = "someContainerManagedName"; + when(objectFactory.buildBean(anyString(), anyMap())).thenReturn("not a preparer"); + + assertThrows(CannotInstantiateObjectException.class, () -> factory.getPreparer(name, null)); + + verify(objectFactory).buildBean(anyString(), anyMap()); + assertFalse(Evidence.constructed); + } + + @Test + public void buildFailureStillFallsBackToTheSuperclassAndConstructsOnce() throws Exception { + String name = LegitimatePreparer.class.getName(); + when(objectFactory.buildBean(anyString(), anyMap())).thenThrow(new IllegalStateException("boom")); + + assertNotNull("a build failure must still fall back to the superclass", factory.getPreparer(name, null)); + assertEquals("the preparer must be constructed exactly once", 1, Evidence.preparerConstructions); + } + + @Test + public void genuinePreparerIsBuiltThroughTheObjectFactory() throws Exception { + String name = LegitimatePreparer.class.getName(); + LegitimatePreparer expected = new LegitimatePreparer(); + when(objectFactory.buildBean(anyString(), anyMap())).thenReturn(expected); + + ViewPreparer preparer = factory.getPreparer(name, null); + + assertSame(expected, preparer); + assertSame("a resolved preparer must still be cached and reused", preparer, factory.getPreparer(name, null)); + } +} diff --git a/plugins/tiles/src/test/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactoryTypeCheckTest.java b/plugins/tiles/src/test/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactoryTypeCheckTest.java new file mode 100644 index 0000000000..23aa5df90d --- /dev/null +++ b/plugins/tiles/src/test/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactoryTypeCheckTest.java @@ -0,0 +1,117 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.tiles.core.prepare.factory; + +import org.apache.tiles.api.AttributeContext; +import org.apache.tiles.api.preparer.ViewPreparer; +import org.apache.tiles.request.Request; +import org.apache.tiles.request.reflect.CannotInstantiateObjectException; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.fail; + +/** + * The factory applies the ViewPreparer type before the named class is instantiated, so a class that + * does not conform is rejected without being initialised or constructed, and a lookup that did not + * produce a preparer is not retained. + */ +public class BasicPreparerFactoryTypeCheckTest { + + /** + * The evidence lives in its own class. Reading a static member of {@link NotAPreparer} would + * itself initialise that class and so invalidate the assertions below. + */ + public static final class Evidence { + public static volatile boolean classInitialised; + public static volatile boolean constructed; + } + + public static class NotAPreparer { + static { + Evidence.classInitialised = true; + } + + public NotAPreparer() { + Evidence.constructed = true; + } + } + + public static class LegitimatePreparer implements ViewPreparer { + @Override + public void execute(Request tilesContext, AttributeContext attributeContext) { + } + } + + @Test + public void nonPreparerIsRejectedBeforeInitialisationAndConstruction() { + CountingPreparerFactory factory = new CountingPreparerFactory(); + + // A class literal does not initialise the class it denotes, so naming the class this way + // does not disturb the assertions below. + String name = NotAPreparer.class.getName(); + try { + factory.getPreparer(name, null); + fail("a class that is not a ViewPreparer must be reported"); + } catch (CannotInstantiateObjectException expected) { + assertNotNull(expected.getMessage()); + } + + assertFalse("the static initialiser must not run for a class that is not a ViewPreparer", + Evidence.classInitialised); + assertFalse("the constructor must not run for a class that is not a ViewPreparer", + Evidence.constructed); + assertEquals("a rejected name must not be retained", 0, factory.size()); + } + + @Test + public void unresolvableNameIsNotRetained() { + CountingPreparerFactory factory = new CountingPreparerFactory(); + + for (int i = 0; i < 100; i++) { + assertNull(factory.getPreparer("org.doesnotexist.Preparer" + i, null)); + } + + assertEquals("a name that cannot be resolved must not be retained", 0, factory.size()); + } + + @Test + public void resolvedPreparerIsStillCachedAndReused() { + CountingPreparerFactory factory = new CountingPreparerFactory(); + String name = LegitimatePreparer.class.getName(); + + ViewPreparer first = factory.getPreparer(name, null); + assertNotNull("a genuine ViewPreparer must still be created", first); + assertSame("a resolved preparer must be cached and reused", first, factory.getPreparer(name, null)); + assertEquals(1, factory.size()); + } + + /** + * Exposes the size of the inherited cache. + */ + private static final class CountingPreparerFactory extends BasicPreparerFactory { + int size() { + return preparers.size(); + } + } +} diff --git a/plugins/tiles/src/test/java/org/apache/tiles/request/reflect/ClassUtilTest.java b/plugins/tiles/src/test/java/org/apache/tiles/request/reflect/ClassUtilTest.java index db01a2414f..3d74235aec 100644 --- a/plugins/tiles/src/test/java/org/apache/tiles/request/reflect/ClassUtilTest.java +++ b/plugins/tiles/src/test/java/org/apache/tiles/request/reflect/ClassUtilTest.java @@ -28,6 +28,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; /** * Tests {@link ClassUtil}. @@ -92,6 +93,42 @@ public void testInstantiate() { assertNull(ClassUtil.instantiate("this.class.does.not.Exist", true)); } + /** + * Test method for {@link ClassUtil#instantiate(String, Class, boolean)}. + */ + @Test + public void testInstantiateWithBaseClass() { + TestBase instance = ClassUtil.instantiate(TestSubClass.class.getName(), TestBase.class, true); + assertNotNull(instance); + assertTrue(instance instanceof TestSubClass); + } + + /** + * Test method for {@link ClassUtil#instantiate(String, Class, boolean)}. + */ + @Test + public void testInstantiateWithBaseClassNotFound() { + assertNull(ClassUtil.instantiate("this.class.does.not.Exist", TestBase.class, true)); + } + + /** + * Test method for {@link ClassUtil#instantiate(String, Class, boolean)}. + */ + @Test(expected = CannotInstantiateObjectException.class) + public void testInstantiateWithBaseClassNotFoundException() { + ClassUtil.instantiate("this.class.does.not.Exist", TestBase.class, false); + } + + /** + * Test method for {@link ClassUtil#instantiate(String, Class, boolean)}. A class that is found + * but does not conform to the base class denotes a misconfigured name rather than a missing + * one, so it is reported even when missing classes are tolerated. + */ + @Test(expected = CannotInstantiateObjectException.class) + public void testInstantiateWithBaseClassMismatch() { + ClassUtil.instantiate(TestClass.class.getName(), TestBase.class, true); + } + /** * Test method for {@link ClassUtil#instantiate(String, boolean)}. */ @@ -167,6 +204,18 @@ public interface TestInterface { public static class TestClass { } + /** + * A base class to constrain a lookup to. + */ + public static class TestBase { + } + + /** + * A class that conforms to {@link TestBase}. + */ + public static class TestSubClass extends TestBase { + } + /** * A test static private class. */