WW-5827 check the preparer type before instantiating it and do not cache failed preparer lookups

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);
+        }
     }
 }

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);
     }
 }

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 <code>baseClass</code>.
      */
     public static <T> Class<? extends T> getClass(String className, Class<T> 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.
+     * <p>
+     * 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 <T>        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 <code>true</code>, if the class is not found it returns
+     *                   <code>null</code>, otherwise it throws a
+     *                   <code>CannotInstantiateObjectException</code>. A class that is found but
+     *                   does not conform to <code>baseClass</code> 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> T instantiate(String className, Class<T> baseClass, boolean returnNull) {
+        Class<? extends T> 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.

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);
             }

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));
+    }
+}