[
https://issues.apache.org/jira/browse/WW-5827?focusedWorklogId=1051757&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051757
]
ASF GitHub Bot logged work on WW-5827:
--------------------------------------
Author: ASF GitHub Bot
Created on: 01/Nov/26 17:58
Start Date: 01/Nov/26 17:58
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #2084:
URL: https://github.com/apache/struts/pull/2084#discussion_r4436739244
##########
plugins/tiles/src/main/java/org/apache/tiles/request/reflect/ClassUtil.java:
##########
@@ -71,9 +80,60 @@ 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) {
Review Comment:
Optional: the untyped `instantiate(String, boolean)` could now delegate to
this one with `Object.class`, which would remove the second copy of the catch
ladder. The exception messages for the untyped path would change slightly, so
I'm fine either way.
##########
plugins/tiles/src/main/java/org/apache/tiles/core/prepare/factory/BasicPreparerFactory.java:
##########
@@ -63,11 +63,23 @@ 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) {
+ // A lookup that did not produce a preparer is not recorded.
Caching it would make a
Review Comment:
Nit: could you drop the inline comments? These files have none on `main`,
and the reasoning (why failures aren't cached, why not `computeIfAbsent`, why
`initialize=false`, why resolution and construction are caught separately) is
already in the commit message and the PR description, where it won't drift.
Same for the two in `ClassUtil` and the extra "Prefer `instantiate(String,
Class, boolean)`" paragraphs added to the untyped methods' javadoc.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051757)
Time Spent: 0.5h (was: 20m)
> Tiles plugin: check the preparer type before instantiating it and do not
> cache failed preparer lookups
> ------------------------------------------------------------------------------------------------------
>
> Key: WW-5827
> URL: https://issues.apache.org/jira/browse/WW-5827
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - Tiles
> Reporter: Lukasz Lenart
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Two issues in how the Tiles plugin resolves view preparers.
> *ClassUtil instantiates before checking the type*
> {{ClassUtil.getClass(String, Class)}} loads the class with
> {{Class.forName(className, true, classLoader)}}, so static initialisation
> runs on lookup. {{ClassUtil.instantiate(String, boolean)}} passes
> {{Object.class}} as the base type and calls {{newInstance()}}; the
> {{ViewPreparer}} check happens only afterwards, as a cast in
> {{BasicPreparerFactory.createPreparer}}. A configured preparer name that is
> not a {{ViewPreparer}} is therefore loaded, initialised and constructed
> before it is rejected.
> Expected: resolve the class without initialising it, apply the expected base
> type ({{ViewPreparer}}) before instantiation, and fail without constructing
> anything when it does not match.
> *BasicPreparerFactory caches failed lookups in a non-thread-safe map*
> {{BasicPreparerFactory.getPreparer}} stores the result of {{createPreparer}}
> in a plain {{HashMap}}, including the {{null}} returned when the class cannot
> be found. A failed lookup stays cached for the lifetime of the factory, and
> the map is read and written concurrently by request threads.
> Expected: do not cache failed lookups, and use a thread-safe map (e.g.
> {{ConcurrentHashMap}}).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
