[arunmanni-ai]

Fixes WW-5827

Addresses the three items in the ticket.

ClassUtil instantiates before checking the type

getClass(String, Class) resolved with initialize=true, so the named class ran its static initialiser on lookup, and instantiate(String, boolean) passed Object.class as the base type, which left the asSubclass check unable to fail. BasicPreparerFactory cast to ViewPreparer only afterwards, so a configured name that does not denote a ViewPreparer was loaded, initialised and constructed before it was rejected.

The class is now resolved without being initialised, and a new instantiate(String, Class<T>, boolean) applies the caller's base type before the constructor runs, so a name that does not conform is rejected without anything being constructed.

A name that resolves but does not conform is reported as CannotInstantiateObjectException rather than returned as null. Returning null here would be swallowed by the ignoreMissing path in BasicTilesContainer.prepare, turning a misconfigured preparer into a silent no-op; the exception keeps the failure as visible as the ClassCastException it replaces. returnNull continues to mean "class not found" only. Resolution and construction are caught separately, so an exception thrown by the constructor itself is never reported as a type mismatch.

The untyped instantiate methods are unchanged; TilesDispatchServlet is the only other caller and is migrated to the typed one.

BasicPreparerFactory caches failed lookups in a non-thread-safe map

getPreparer stored whatever createPreparer returned, including null, which containsKey then reported as resolved. A single failure became permanent for the lifetime of the factory even when its cause was transient. Failed lookups are no longer recorded, and the map is a ConcurrentHashMap, since it is read and written by request threads. get/putIfAbsent rather than computeIfAbsent, so class loading does not run while a bin of the map is locked.

StrutsPreparerFactory constructs before checking the type, twice on failure, and logs every failure at ERROR

createPreparer built the preparer through ObjectFactory.buildBean and cast to ViewPreparer afterwards, so the check added above did not apply on this path. When the cast failed, the catch (Exception) fell back to super.createPreparer, which constructed the class a second time. That fallback logged at ERROR with a full stack trace; with failed lookups no longer cached, a misconfigured name would log that on every request instead of once.

The type is now checked before the bean is built, so a name that resolves to a class that is not a ViewPreparer is rejected without anything being constructed. checkType resolves the name without initialising it and lets ClassNotFoundException pass, since a preparer name need not be a class name — bean names resolved by the object factory reach buildBean too and are left to it. A name that does resolve but does not conform throws CannotInstantiateObjectException, as in the ClassUtil path.

A bean that is built and turns out not to be a ViewPreparer is reported rather than passed to the fallback, so it is not constructed a second time. The fallback is kept for a genuine buildBean failure, and now logs at WARN with the exception message; the stack trace moves to DEBUG.

Tests

mvn test -DskipAssembly -pl plugins/tiles — 576 tests, 0 failures.

New BasicPreparerFactoryTypeCheckTest covers rejection before initialisation and construction, non-retention of a name that cannot be resolved, and that a genuine ViewPreparer is still created and cached. The evidence flags live in a separate holder class, because reading a static member of the marker class would itself initialise it. Four cases added to ClassUtilTest for the typed overload.

New StrutsPreparerFactoryTypeCheckTest covers the third section: a name that resolves to a non-ViewPreparer is rejected before buildBean is called, a bean that is built but does not conform is reported without a second construction, and a genuine build failure still falls back to the superclass and constructs once. The ObjectFactory is a Mockito mock bound into a real ActionContext.

PatternUtilTest is unchanged.

[arunmanni-ai]

Hi @lukaszlenart ,

One scope question. In a Struts deployment StrutsPreparerFactory.createPreparer builds the preparer through the ObjectFactory and casts to ViewPreparer afterwards, so the type check in this PR is not reached there — and when the cast fails, catch (Exception) falls back to super.createPreparer, which constructs the class a second time. That is the double construction you mentioned; it is not in the ticket description.

Two things would fold into the same change. The fallback logs at ERROR with a full stack trace on every call, and now that failed lookups are no longer cached, a misconfigured preparer name logs that per request rather than once.

Should both go in this PR, or would you rather have a separate ticket? Happy either way — I just didn't want to widen this one without asking.

[lukaszlenart]

Thanks, this looks good: the scope matches WW-5827 and the tests cover both items. Two small things inline, neither blocking.

[arunmanni-ai]

Dropped the inline comments in both files and the two “Prefer instantiate(String, Class, boolean)” paragraphs from the untyped methods’ javadoc.

[lukaszlenart]

Thanks for the cleanup, and sorry for the slow answer on the scope question.

Yes, please fold both into this PR. You're right about the logging: with failed lookups no longer cached, the ERROR plus stack trace from the StrutsPreparerFactory fallback would repeat on every request instead of appearing once, so it belongs with this change. I've added a third section to WW-5827 covering StrutsPreparerFactory: check the type before building the bean, construct at most once, and don't log a full ERROR stack trace per request for a failed lookup.

One small thing from the last push: BasicPreparerFactory.getPreparer now has trailing whitespace after if (preparer == null) { and on the blank line before putIfAbsent.

[arunmanni-ai]

Both folded in, and the trailing whitespace is gone.

Three judgement calls in the StrutsPreparerFactory section, cheap to change now and awkward after merge:

The super.createPreparer fallback stays, but only for a genuine build failure. I read the ticket as scoping the double construction to the cast path, so that is the path I closed: the type is checked before buildBean, and a bean that is built but does not conform throws instead of falling back. Nothing is constructed twice on either route. A real buildBean failure still falls back, which is what an application gets today when the preparer is a plain class the object factory cannot build. Say the word if you wanted the fallback gone entirely and I will drop it.

checkType lets ClassNotFoundException pass. A preparer name is not always a class name — bean names resolved by the object factory reach buildBean as well, and failing those at the type check would break configurations that work today. A name that does not resolve is logged at DEBUG and left to the object factory; the final instanceof still catches it if what comes back is not a ViewPreparer.

The non-conforming case throws rather than returning null. Same reasoning as the ClassUtil section: null here would be swallowed by ignoreMissing in BasicTilesContainer.prepare and turn a misconfigured preparer into a silent no-op.

On the logging: the fallback is now WARN with the exception message and the stack trace at DEBUG, so a misconfigured name no longer produces an ERROR stack trace per request.

mvn test -DskipAssembly -pl plugins/tiles — 576 tests, 0 failures. Description updated for the third section.