diff --git a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java index 6185552c1d..d0bc09d85e 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -56,6 +56,8 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { private static final String TOMCAT_WEBAPP_CLASSLOADER = "org.apache.catalina.loader.WebappClassLoader"; private static final String TOMCAT_WEBAPP_CLASSLOADER_BASE = "org.apache.catalina.loader.WebappClassLoaderBase"; private static final String RELOADED = "org.apache.struts2.util.LocalizedTextProvider.reloaded"; + @SuppressWarnings("java:S2129") // deliberate: a non-interned instance is required for an identity (==) sentinel + private static final String NOT_FOUND = new String("__STRUTS_TEXT_NOT_FOUND__"); // unique identity sentinel; compared with == protected final ConcurrentMap bundlesMap = new ConcurrentHashMap<>(); protected boolean devMode = false; @@ -66,6 +68,8 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { private final ConcurrentMap> classLoaderMap = new ConcurrentHashMap<>(); private final Set missingBundles = ConcurrentHashMap.newKeySet(); private final ConcurrentMap delegatedClassLoaderMap = new ConcurrentHashMap<>(); + private final ConcurrentMap classHierarchyCache = new ConcurrentHashMap<>(); + private final ConcurrentMap packageHierarchyCache = new ConcurrentHashMap<>(); @Override public void addDefaultResourceBundle(String bundleName) { @@ -90,6 +94,22 @@ protected ClassLoader getCurrentThreadContextClassLoader() { return Thread.currentThread().getContextClassLoader(); } + private int currentLoaderHashCode() { + // Identity-based on purpose: a custom ClassLoader overriding hashCode() must not be able to + // collapse (or collide) the per-classloader cache partitions. + return System.identityHashCode(getCurrentThreadContextClassLoader()); + } + + /** Test-support accessor: current number of cached class-hierarchy resolutions. */ + protected int classHierarchyCacheSize() { + return classHierarchyCache.size(); + } + + /** Test-support accessor: current number of cached package-hierarchy resolutions. */ + protected int packageHierarchyCacheSize() { + return packageHierarchyCache.size(); + } + @Inject(value = StrutsConstants.STRUTS_CUSTOM_I18N_RESOURCES, required = false) public void setCustomI18NResources(String bundles) { if (bundles == null || bundles.isEmpty()) { @@ -187,6 +207,8 @@ public void setDelegatedClassLoader(final ClassLoader classLoader) { protected void clearBundle(final String bundleName, Locale locale) { final String key = createMissesKey(String.valueOf(getCurrentThreadContextClassLoader().hashCode()), bundleName, locale); final ResourceBundle removedBundle = bundlesMap.remove(key); + classHierarchyCache.clear(); + packageHierarchyCache.clear(); LOG.debug("Clearing resource bundle [{}], locale [{}], result: [{}].", bundleName, locale, removedBundle != null); } @@ -204,6 +226,8 @@ protected void clearBundle(final String bundleName, Locale locale) { */ protected void clearMissingBundlesCache() { missingBundles.clear(); + classHierarchyCache.clear(); + packageHierarchyCache.clear(); LOG.debug("Cleared the missing bundles cache."); } @@ -222,6 +246,8 @@ protected void reloadBundles(Map context) { } if (!reloaded) { bundlesMap.clear(); + classHierarchyCache.clear(); + packageHierarchyCache.clear(); clearResourceBundleClassloaderCaches(); // now, for the true and utter hack, if we're running in tomcat, clear @@ -507,9 +533,48 @@ protected GetDefaultMessageReturnArg getDefaultMessageWithAlternateKey(String ke return result; } + /** + * Resolves the raw (untranslated, unformatted) message pattern for a key within a single bundle. + * Returns {@code null} when the bundle or key is absent. This is the cacheable unit relied upon by + * the hierarchy-resolution caches; translation and formatting are applied separately by + * {@link #formatMessage(String, Locale, ValueStack, Object[])}. + */ + private String getRawMessage(String bundleName, Locale locale, String key) { + ResourceBundle bundle = findResourceBundle(bundleName, locale); + if (bundle == null) { + return null; + } + try { + return bundle.getString(key); + } catch (MissingResourceException e) { + LOG.debug("Missing key [{}] in bundle [{}]!", key, bundleName); + return null; + } + } + + /** + * Applies value stack variable translation (when a stack is available) and {@link MessageFormat} + * argument substitution to a raw message pattern. Mirrors the rendering previously performed inline + * by {@link #getMessage(String, Locale, String, ValueStack, Object[])}. + */ + protected String formatMessage(String rawPattern, Locale locale, ValueStack valueStack, Object[] args) { + String message = (valueStack != null) + ? TextParseUtil.translateVariables(rawPattern, valueStack) + : rawPattern; + MessageFormat mf = buildMessageFormat(message, locale); + return formatWithNullDetection(mf, args); + } + /** * @return the message from the named resource bundle. + * @deprecated since 7.3.0 — superseded by the internal raw-resolution + caching path + * ({@link #formatMessage(String, Locale, ValueStack, Object[])} over a raw lookup). Retained for + * backward compatibility with descendant classes that call it directly. No longer invoked + * by {@code findText}: overriding this method does not affect framework message lookup + * anymore; override {@link #formatMessage(String, Locale, ValueStack, Object[])} to customize + * rendering instead. */ + @Deprecated(since = "7.3.0", forRemoval = true) protected String getMessage(String bundleName, Locale locale, String key, ValueStack valueStack, Object[] args) { ResourceBundle bundle = findResourceBundle(bundleName, locale); if (bundle == null) { @@ -519,12 +584,8 @@ protected String getMessage(String bundleName, Locale locale, String key, ValueS reloadBundles(valueStack.getContext()); } try { - String message = bundle.getString(key); - if (valueStack != null) { - message = TextParseUtil.translateVariables(bundle.getString(key), valueStack); - } - MessageFormat mf = buildMessageFormat(message, locale); - return formatWithNullDetection(mf, args); + String rawPattern = bundle.getString(key); + return formatMessage(rawPattern, locale, valueStack, args); } catch (MissingResourceException e) { LOG.debug("Missing key [{}] in bundle [{}]!", key, bundleName); return null; @@ -532,13 +593,11 @@ protected String getMessage(String bundleName, Locale locale, String key, ValueS } /** - * Traverse up class hierarchy looking for message. Looks at class, then implemented interface, - * before going up hierarchy. - * - * @return the message + * Raw-pattern twin of {@link #findMessage}. Walks class, implemented interfaces, then up the + * hierarchy, returning the first raw message pattern found (via {@link #getRawMessage}) without + * translation or formatting. Used by the cached class-hierarchy resolver. */ - protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, - ValueStack valueStack) { + private String findMessageRaw(Class clazz, String key, String indexedKey, Locale locale, Set checked) { if (checked == null) { checked = new TreeSet<>(); } else if (checked.contains(clazz.getName())) { @@ -546,59 +605,144 @@ protected String findMessage(Class clazz, String key, String indexedKey, Loca } // look in properties of this class - String msg = getMessage(clazz.getName(), locale, key, valueStack, args); - + String msg = getRawMessageWithAlternate(clazz.getName(), locale, key, indexedKey); if (msg != null) { return msg; } - if (indexedKey != null) { - msg = getMessage(clazz.getName(), locale, indexedKey, valueStack, args); - - if (msg != null) { - return msg; - } - } - // look in properties of implemented interfaces - Class[] interfaces = clazz.getInterfaces(); - - for (Class anInterface : interfaces) { - msg = getMessage(anInterface.getName(), locale, key, valueStack, args); - + for (Class anInterface : clazz.getInterfaces()) { + msg = getRawMessageWithAlternate(anInterface.getName(), locale, key, indexedKey); if (msg != null) { return msg; } + } - if (indexedKey != null) { - msg = getMessage(anInterface.getName(), locale, indexedKey, valueStack, args); - + // traverse up hierarchy + if (clazz.isInterface()) { + for (Class anInterface : clazz.getInterfaces()) { + msg = findMessageRaw(anInterface, key, indexedKey, locale, checked); if (msg != null) { return msg; } } + } else if (!clazz.equals(Object.class) && !clazz.isPrimitive()) { + return findMessageRaw(clazz.getSuperclass(), key, indexedKey, locale, checked); } - // traverse up hierarchy - if (clazz.isInterface()) { - interfaces = clazz.getInterfaces(); + return null; + } - for (Class anInterface : interfaces) { - msg = findMessage(anInterface, key, indexedKey, locale, args, checked, valueStack); + /** + * Resolves the raw message pattern for a key within a single bundle, falling back to the + * indexed (general-form) key when the primary key is absent. + */ + private String getRawMessageWithAlternate(String bundleName, Locale locale, String key, String indexedKey) { + String msg = getRawMessage(bundleName, locale, key); + if (msg == null && indexedKey != null) { + msg = getRawMessage(bundleName, locale, indexedKey); + } + return msg; + } + + /** + * Cached resolution of the class/interface/superclass hierarchy for a key. Returns the raw pattern + * found, or {@link #NOT_FOUND} when the key is absent from the entire hierarchy. Keyed on the + * context classloader hash + class name + key + locale, so no {@link Class} reference is retained. + * Uses get + putIfAbsent (never computeIfAbsent) because the child-property path recurses into findText. + */ + String resolveClassHierarchyRaw(Class clazz, String textKey, Locale locale) { + TextCacheKey cacheKey = new TextCacheKey(currentLoaderHashCode(), clazz.getName(), textKey, locale); + String cached = classHierarchyCache.get(cacheKey); + if (cached != null) { + return cached; + } + // Derived here (miss-only) rather than accepted as a parameter, so the cache key trivially + // covers every input that influences the resolution result. + String raw = findMessageRaw(clazz, textKey, extractIndexedName(textKey), locale, null); + String toStore = (raw != null) ? raw : NOT_FOUND; + classHierarchyCache.putIfAbsent(cacheKey, toStore); + return toStore; + } + /** @return true when a cached raw-resolution result represents "not found". */ + @SuppressWarnings("java:S4973") // deliberate identity comparison against the non-interned NOT_FOUND sentinel + protected boolean isNotFound(String cachedRawResult) { + return cachedRawResult == NOT_FOUND; + } + + /** + * Raw-pattern walk of the {@code *.package} bundles up the class hierarchy of {@code startClazz}. + * Returns the first raw pattern found (via {@link #getRawMessage}) for the key or its indexed form, + * or {@code null} when none match. + */ + private String findPackageMessageRaw(Class startClazz, String textKey, String indexedTextName, Locale locale) { + for (Class clazz = startClazz; + (clazz != null) && !clazz.equals(Object.class); + clazz = clazz.getSuperclass()) { + + String basePackageName = clazz.getName(); + while (basePackageName.lastIndexOf('.') != -1) { + basePackageName = basePackageName.substring(0, basePackageName.lastIndexOf('.')); + String packageName = basePackageName + ".package"; + String msg = getRawMessageWithAlternate(packageName, locale, textKey, indexedTextName); if (msg != null) { return msg; } } - } else { - if (!clazz.equals(Object.class) && !clazz.isPrimitive()) { - return findMessage(clazz.getSuperclass(), key, indexedKey, locale, args, checked, valueStack); - } } - return null; } + /** + * Cached resolution of the {@code *.package} hierarchy for a key. Returns the raw pattern found, or + * {@link #NOT_FOUND} when absent. Same keying and get + putIfAbsent discipline as + * {@link #resolveClassHierarchyRaw}. + */ + String resolvePackageHierarchyRaw(Class startClazz, String textKey, Locale locale) { + TextCacheKey cacheKey = new TextCacheKey(currentLoaderHashCode(), startClazz.getName(), textKey, locale); + String cached = packageHierarchyCache.get(cacheKey); + if (cached != null) { + return cached; + } + // Derived here (miss-only) rather than accepted as a parameter, so the cache key trivially + // covers every input that influences the resolution result. + String raw = findPackageMessageRaw(startClazz, textKey, extractIndexedName(textKey), locale); + String toStore = (raw != null) ? raw : NOT_FOUND; + packageHierarchyCache.putIfAbsent(cacheKey, toStore); + return toStore; + } + + /** + * Traverse up class hierarchy looking for message. Looks at class, then implemented interface, + * before going up hierarchy. + * + * @return the message + * @deprecated since 7.3.0 — superseded by the internal raw-resolution + caching path + * ({@link #findMessageRaw} + {@link #formatMessage(String, Locale, ValueStack, Object[])}). Retained + * for backward compatibility with descendant classes that call it directly. No longer + * invoked by {@code findText}: overriding this method does not affect framework message + * lookup anymore; override {@link #formatMessage(String, Locale, ValueStack, Object[])} to + * customize rendering instead. Note: unlike the pre-7.3.0 implementation, a + * candidate whose formatted value is the literal {@code "null"} no longer causes the search to + * continue deeper in the same hierarchy; this affects only the pathological case of the same key + * redefined at multiple hierarchy levels with the shallow value formatting to {@code "null"}. + * The bundle-reload check is now triggered once on entry (when reload mode is enabled) rather than + * lazily per bundle probe, preserving the reload side effect that the previous getMessage-per-probe + * walk provided. + */ + @Deprecated(since = "7.3.0", forRemoval = true) + protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, + ValueStack valueStack) { + if (valueStack != null) { + reloadBundles(valueStack.getContext()); + } else { + reloadBundles(); + } + String rawPattern = findMessageRaw(clazz, key, indexedKey, locale, checked); + return rawPattern != null ? formatMessage(rawPattern, locale, valueStack, args) : null; + } + protected String extractIndexedName(String textKey) { String indexedTextName = null; // calculate indexedTextName (collection[*]) if applicable @@ -658,6 +802,40 @@ public int hashCode() { } } + static class TextCacheKey { + private final int classLoaderHash; + private final String className; + private final String textKey; + private final Locale locale; + + TextCacheKey(int classLoaderHash, String className, String textKey, Locale locale) { + this.classLoaderHash = classLoaderHash; + this.className = className; + this.textKey = textKey; + this.locale = locale; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + TextCacheKey that = (TextCacheKey) o; + return classLoaderHash == that.classLoaderHash + && Objects.equals(className, that.className) + && Objects.equals(textKey, that.textKey) + && Objects.equals(locale, that.locale); + } + + @Override + public int hashCode() { + int result = classLoaderHash; + result = 31 * result + (className != null ? className.hashCode() : 0); + result = 31 * result + (textKey != null ? textKey.hashCode() : 0); + result = 31 * result + (locale != null ? locale.hashCode() : 0); + return result; + } + } + static class GetDefaultMessageReturnArg { String message; boolean foundInBundle; diff --git a/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java b/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java index bfdfe22fb4..71340ffe82 100644 --- a/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java @@ -65,6 +65,17 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin LOG.debug("Key is null, short-circuit to default message"); return defaultMessage; } + + // Trigger bundle reload (and cache invalidation) once, before any cached hierarchy lookup, + // so that in reload/devMode the hierarchy caches are cleared before they are read. With no + // value stack, fall back to the ActionContext-based overload so the RELOADED flag is still + // tracked and the caches can warm on that path too. + if (valueStack != null) { + reloadBundles(valueStack.getContext()); + } else { + reloadBundles(); + } + String indexedTextName = extractIndexedName(textKey); // Allow for and track an early lookup for the message in the default resource bundles first, before searching the class hierarchy. @@ -81,11 +92,14 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin } } - // search up class hierarchy - String msg = findMessage(startClazz, textKey, indexedTextName, locale, args, null, valueStack); - - if (msg != null) { - return msg; + // search up class hierarchy (cached raw resolution; format per call) + String classHierarchyRaw = resolveClassHierarchyRaw(startClazz, textKey, locale); + String msg = null; + if (!isNotFound(classHierarchyRaw)) { + msg = formatMessage(classHierarchyRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } } if (ModelDriven.class.isAssignableFrom(startClazz)) { @@ -99,37 +113,24 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin if (action instanceof ModelDriven) { Object model = ((ModelDriven) action).getModel(); if (model != null) { - msg = findMessage(model.getClass(), textKey, indexedTextName, locale, args, null, valueStack); - if (msg != null) { - return msg; + String modelRaw = resolveClassHierarchyRaw(model.getClass(), textKey, locale); + if (!isNotFound(modelRaw)) { + msg = formatMessage(modelRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } } } } } } - // nothing still? alright, search the package hierarchy now - for (Class clazz = startClazz; - (clazz != null) && !clazz.equals(Object.class); - clazz = clazz.getSuperclass()) { - - String basePackageName = clazz.getName(); - while (basePackageName.lastIndexOf('.') != -1) { - basePackageName = basePackageName.substring(0, basePackageName.lastIndexOf('.')); - String packageName = basePackageName + ".package"; - msg = getMessage(packageName, locale, textKey, valueStack, args); - - if (msg != null) { - return msg; - } - - if (indexedTextName != null) { - msg = getMessage(packageName, locale, indexedTextName, valueStack, args); - - if (msg != null) { - return msg; - } - } + // search the package hierarchy (cached raw resolution; format per call) + String packageRaw = resolvePackageHierarchyRaw(startClazz, textKey, locale); + if (!isNotFound(packageRaw)) { + msg = formatMessage(packageRaw, locale, valueStack, args); + if (msg != null) { + return msg; } } diff --git a/core/src/test/java/org/apache/struts2/text/CacheFixture.java b/core/src/test/java/org/apache/struts2/text/CacheFixture.java new file mode 100644 index 0000000000..4e072a5706 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/text/CacheFixture.java @@ -0,0 +1,37 @@ +/* + * 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.text; + +/** + * Simple fixture whose class-associated bundle ({@code CacheFixture.properties}) backs the + * localized-text caching tests. The {@code name} property is exposed so OGNL expressions such as + * {@code ${name}} can be resolved against a value stack. + */ +public class CacheFixture { + + private final String name; + + public CacheFixture(String name) { + this.name = name; + } + + public String getName() { + return name; + } +} diff --git a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java index 06c84f4e79..d7157c5520 100644 --- a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java @@ -547,6 +547,207 @@ public void testFindText_FullParameterSet_FirstParameterIsClass() { assertEquals("Result of bean2.name lookup not as expected ?", "Okay! You found Me!", messageResult); } + public void testClassHierarchyCacheReusesFoundPattern() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + assertEquals("Cache not empty before first lookup ?", 0, provider.classHierarchyCacheSize()); + String first = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Static cached value", first); + assertEquals("Cache not populated after found lookup ?", 1, provider.classHierarchyCacheSize()); + + String second = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Second lookup differs from first ?", first, second); + assertEquals("Cache grew on repeated lookup ?", 1, provider.classHierarchyCacheSize()); + } + + public void testClassHierarchyCacheStoresMisses() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + String first = provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack); + assertEquals("Fallback", first); + assertEquals("Miss not cached ?", 1, provider.classHierarchyCacheSize()); + + String second = provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack); + assertEquals("Fallback", second); + assertEquals("Miss cache grew on repeat ?", 1, provider.classHierarchyCacheSize()); + } + + public void testFormattingIsPerCallNotCached() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + String x = provider.findText(CacheFixture.class, "cache.withparam", Locale.ENGLISH, null, new Object[]{"X"}, valueStack); + String y = provider.findText(CacheFixture.class, "cache.withparam", Locale.ENGLISH, null, new Object[]{"Y"}, valueStack); + assertEquals("Value with param X", x); + assertEquals("Value with param Y", y); + assertEquals("Raw pattern should be cached once, not per format ?", 1, provider.classHierarchyCacheSize()); + } + + public void testOgnlTranslationIsPerCall() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + valueStack.push(new CacheFixture("World")); + String world = provider.findText(CacheFixture.class, "cache.withognl", Locale.ENGLISH, null, null, valueStack); + valueStack.pop(); + valueStack.push(new CacheFixture("Mars")); + String mars = provider.findText(CacheFixture.class, "cache.withognl", Locale.ENGLISH, null, null, valueStack); + valueStack.pop(); + + assertEquals("Hello World", world); + assertEquals("Hello Mars", mars); + assertEquals("Raw pattern should be cached once across value stacks ?", 1, provider.classHierarchyCacheSize()); + } + + public void testNullFormattingFallsThroughToDefault() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // "{0}" with a null arg formats to the literal "null"; findText must fall through to the default. + String first = provider.findText(CacheFixture.class, "cache.nullformat", Locale.ENGLISH, "Fallback", new Object[]{null}, valueStack); + assertEquals("Fallback", first); + // Repeat after the pattern is cached — still falls through. + String second = provider.findText(CacheFixture.class, "cache.nullformat", Locale.ENGLISH, "Fallback", new Object[]{null}, valueStack); + assertEquals("Fallback", second); + } + + public void testReloadClearsClassHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not populated ?", 1, provider.classHierarchyCacheSize()); + + provider.callReloadBundlesForceReload(); + assertEquals("Reload did not clear class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); + } + + public void testClearBundleAndClearMissingCacheEmptyClassHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not populated ?", 1, provider.classHierarchyCacheSize()); + provider.callClearBundleWithLocale("org/apache/struts2/text/CacheFixture", Locale.ENGLISH); + assertEquals("clearBundle did not empty class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not repopulated ?", 1, provider.classHierarchyCacheSize()); + provider.callClearMissingBundlesCache(); + assertEquals("clearMissingBundlesCache did not empty class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); + } + + public void testPackageHierarchyCacheReusesFoundPattern() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // ModelDrivenAction2 lives in a package that provides "package.properties" = "It works!". + assertEquals("Package cache not empty before lookup ?", 0, provider.packageHierarchyCacheSize()); + String first = provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("It works!", first); + assertEquals("Package cache not populated after found lookup ?", 1, provider.packageHierarchyCacheSize()); + + String second = provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Second package lookup differs ?", first, second); + assertEquals("Package cache grew on repeat ?", 1, provider.packageHierarchyCacheSize()); + } + + public void testReloadClearsPackageHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Package cache not populated ?", 1, provider.packageHierarchyCacheSize()); + + provider.callReloadBundlesForceReload(); + assertEquals("Reload did not clear package hierarchy cache ?", 0, provider.packageHierarchyCacheSize()); + } + + public void testClearBundleAndClearMissingCacheEmptyPackageHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Package cache not populated ?", 1, provider.packageHierarchyCacheSize()); + provider.callClearBundleWithLocale("org/apache/struts2/test/package", Locale.getDefault()); + assertEquals("clearBundle did not empty package hierarchy cache ?", 0, provider.packageHierarchyCacheSize()); + + provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Package cache not repopulated ?", 1, provider.packageHierarchyCacheSize()); + provider.callClearMissingBundlesCache(); + assertEquals("clearMissingBundlesCache did not empty package hierarchy cache ?", 0, provider.packageHierarchyCacheSize()); + } + + public void testDeprecatedFindMessageStillDelegates() { + // findMessage leaves findText's hot path in this task; this locks the deprecated delegator. + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + assertEquals("Static cached value", provider.callFindMessage(CacheFixture.class, "cache.static", Locale.ENGLISH, valueStack)); + assertNull(provider.callFindMessage(CacheFixture.class, "cache.missing", Locale.ENGLISH, valueStack)); + } + + public void testModelDrivenTierUsesClassHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + + ModelDrivenAction2 action = new ModelDrivenAction2(); + Mock mockActionInvocation = new Mock(ActionInvocation.class); + mockActionInvocation.matchAndReturn("getAction", action); + ActionContext.getContext().withActionInvocation((ActionInvocation) mockActionInvocation.proxy()); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // "invalid.count" resolves only via the model's hierarchy (TestBean2 -> TestBean.properties), + // not via the action class hierarchy, so it exercises the ModelDriven tier. + String first = provider.findText(ModelDrivenAction2.class, "invalid.count", Locale.ENGLISH, null, null, valueStack); + assertNotNull("Model-tier lookup found nothing ?", first); + assertTrue("Model-tier lookup did not resolve via the TestBean bundle ?", first.startsWith("TestBean model:")); + // Two entries: a miss for the action class hierarchy plus a hit for the model class hierarchy. + assertEquals("Class-hierarchy cache should hold action miss + model hit ?", 2, provider.classHierarchyCacheSize()); + + String second = provider.findText(ModelDrivenAction2.class, "invalid.count", Locale.ENGLISH, null, null, valueStack); + assertEquals("Warm model-tier lookup differs from cold ?", first, second); + assertEquals("Cache grew on repeated model-tier lookup ?", 2, provider.classHierarchyCacheSize()); + } + + public void testLocaleIsPartOfCacheKey() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + String english = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + String german = provider.findText(CacheFixture.class, "cache.static", Locale.GERMAN, null, null, valueStack); + assertEquals("Static cached value", english); + assertEquals("Statischer Wert", german); + assertEquals("Each locale should have its own cache entry ?", 2, provider.classHierarchyCacheSize()); + + assertEquals("Warm English lookup differs ?", english, + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack)); + assertEquals("Warm German lookup differs ?", german, + provider.findText(CacheFixture.class, "cache.static", Locale.GERMAN, null, null, valueStack)); + assertEquals("Cache grew on warm per-locale lookups ?", 2, provider.classHierarchyCacheSize()); + } + + public void testIndexedKeyResolvesThroughCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // "cache.indexed[20]" falls back to the general form "cache.indexed[*]" during raw resolution. + String first = provider.findText(CacheFixture.class, "cache.indexed[20]", Locale.ENGLISH, null, null, valueStack); + assertEquals("Indexed cached value", first); + assertEquals("Indexed lookup not cached ?", 1, provider.classHierarchyCacheSize()); + + String second = provider.findText(CacheFixture.class, "cache.indexed[20]", Locale.ENGLISH, null, null, valueStack); + assertEquals("Warm indexed lookup differs from cold ?", first, second); + assertEquals("Cache grew on warm indexed lookup ?", 1, provider.classHierarchyCacheSize()); + + // A different index is a distinct cache key (the cache is keyed on the full textKey), + // resolving to the same general form. + String other = provider.findText(CacheFixture.class, "cache.indexed[7]", Locale.ENGLISH, null, null, valueStack); + assertEquals("Indexed cached value", other); + assertEquals("A different index should create its own cache entry ?", 2, provider.classHierarchyCacheSize()); + } + @Override protected void setUp() throws Exception { super.setUp(); @@ -616,5 +817,20 @@ public boolean getBundlesReloadedIndicatorValue() { final Object reloadedObject = ActionContext.getContext().get(RELOADED); return reloadedObject instanceof Boolean && (Boolean) reloadedObject; } + + @Override + public int classHierarchyCacheSize() { + return super.classHierarchyCacheSize(); + } + + @Override + public int packageHierarchyCacheSize() { + return super.packageHierarchyCacheSize(); + } + + @SuppressWarnings("removal") // deliberately exercises the deprecated delegator + public String callFindMessage(Class clazz, String key, Locale locale, ValueStack valueStack) { + return super.findMessage(clazz, key, null, locale, null, null, valueStack); + } } } diff --git a/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties new file mode 100644 index 0000000000..7d19337b3a --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties @@ -0,0 +1,23 @@ +# +# 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. +# +cache.static=Static cached value +cache.withparam=Value with param {0} +cache.withognl=Hello ${name} +cache.nullformat={0} +cache.indexed[*]=Indexed cached value diff --git a/core/src/test/resources/org/apache/struts2/text/CacheFixture_de.properties b/core/src/test/resources/org/apache/struts2/text/CacheFixture_de.properties new file mode 100644 index 0000000000..a9f49ac84b --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/text/CacheFixture_de.properties @@ -0,0 +1,19 @@ +# +# 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. +# +cache.static=Statischer Wert diff --git a/docs/superpowers/plans/2026-07-23-WW-5540-localized-text-provider-caching.md b/docs/superpowers/plans/2026-07-23-WW-5540-localized-text-provider-caching.md new file mode 100644 index 0000000000..8d524519ed --- /dev/null +++ b/docs/superpowers/plans/2026-07-23-WW-5540-localized-text-provider-caching.md @@ -0,0 +1,871 @@ +# WW-5540: LocalizedTextProvider Traversal Caching — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Cache the class- and package-hierarchy traversal in `StrutsLocalizedTextProvider.findText` so repeated lookups for the same `(classloader, class, key, locale)` collapse to a single map lookup, without changing observable behavior. + +**Architecture:** Split raw-pattern resolution (cacheable) from translation/formatting (per-call). Add two `ConcurrentHashMap` caches in `AbstractLocalizedTextProvider` — one for the class/interface/superclass walk, one for the `*.package` walk — each storing a raw pattern or a shared `NOT_FOUND` sentinel. `findText` reads the caches, then formats per call, and falls through to the next tier when a cached pattern formats to `null` (preserving `formatWithNullDetection` semantics). + +**Tech Stack:** Java, Maven, JUnit 3/4 via `XWorkTestCase` (junit.framework), Mockito/mockobjects (existing test deps). + +## Global Constraints + +- Commit messages MUST be prefixed with the Jira ticket: `WW-5540 [scope]: `. +- Cache key stores the class **name (String)**, never a `Class` object — no classloader pinning. +- Caches are unbounded `ConcurrentHashMap` (consistent with existing `bundlesMap`/`missingBundles`). +- `NOT_FOUND` is a unique `String` instance compared by `==` (identity), never by content. +- Use `get` + `putIfAbsent`, never `computeIfAbsent`, on the new caches (the child-property path recurses back into `findText`). +- Only the raw pattern is cached; `TextParseUtil.translateVariables` + `MessageFormat` always run per call. +- Core tests here extend `XWorkTestCase` — use `testXxx()` methods and `junit.framework` `assert*`; **no `@Test`, no AssertJ** (an `@Test` here silently never runs). +- Build/test: `mvn test -DskipAssembly -pl core -Dtest=#` (single) or `-Dtest=` (whole class). +- Work on branch `WW-5540-localized-text-provider-caching` (already created off `main`). + +--- + +### Task 1: Raw / format split (behavior-preserving refactor) + +Extract the "find the raw pattern" and "render a pattern" steps from `getMessage`, and add a raw twin of `findMessage`. No caching yet. Behavior must be byte-identical; the existing test suite is the safety net. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java` +- Test (regression only): `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java` + +**Interfaces:** +- Consumes: existing `findResourceBundle`, `buildMessageFormat`, `formatWithNullDetection`, `reloadBundles`. +- Produces (used by Task 2 & 3): + - `private String getRawMessage(String bundleName, Locale locale, String key)` → raw pattern or `null`. + - `protected String formatMessage(String rawPattern, Locale locale, ValueStack valueStack, Object[] args)` → translated+formatted string (or `null` via null-detection). + - `private String findMessageRaw(Class clazz, String key, String indexedKey, Locale locale, Set checked)` → first raw pattern found walking class/interface/superclass, or `null`. +- Deprecates (retained as legacy `protected` extension points, superseded by the raw path): + - `getMessage(...)` — now `@Deprecated`, delegates its formatting to `formatMessage` (behavior identical). + - `findMessage(...)` — now `@Deprecated`, delegates to `findMessageRaw` + `formatMessage`. + +- [ ] **Step 1: Add `getRawMessage` and `formatMessage`, and re-express `getMessage` via them (deprecating it)** + +In `AbstractLocalizedTextProvider`, add these two methods (place them just above the existing `getMessage`): + +```java +/** + * Resolves the raw (untranslated, unformatted) message pattern for a key within a single bundle. + * Returns {@code null} when the bundle or key is absent. This is the cacheable unit relied upon by + * the hierarchy-resolution caches; translation and formatting are applied separately by + * {@link #formatMessage(String, Locale, ValueStack, Object[])}. + */ +private String getRawMessage(String bundleName, Locale locale, String key) { + ResourceBundle bundle = findResourceBundle(bundleName, locale); + if (bundle == null) { + return null; + } + try { + return bundle.getString(key); + } catch (MissingResourceException e) { + LOG.debug("Missing key [{}] in bundle [{}]!", key, bundleName); + return null; + } +} + +/** + * Applies value stack variable translation (when a stack is available) and {@link MessageFormat} + * argument substitution to a raw message pattern. Mirrors the rendering previously performed inline + * by {@link #getMessage(String, Locale, String, ValueStack, Object[])}. + */ +protected String formatMessage(String rawPattern, Locale locale, ValueStack valueStack, Object[] args) { + String message = (valueStack != null) + ? TextParseUtil.translateVariables(rawPattern, valueStack) + : rawPattern; + MessageFormat mf = buildMessageFormat(message, locale); + return formatWithNullDetection(mf, args); +} +``` + +Then replace the body of the existing `getMessage` (currently at `AbstractLocalizedTextProvider.java:513-532`) with the version below, and mark it `@Deprecated` (it is superseded internally by the raw-resolution path and retained only as a legacy extension point): + +```java +/** + * @return the message from the named resource bundle. + * @deprecated since 7.3.0 — superseded by the internal raw-resolution + caching path + * ({@link #formatMessage(String, Locale, ValueStack, Object[])} over a raw lookup). Retained for + * backward compatibility with descendant classes. + */ +@Deprecated +protected String getMessage(String bundleName, Locale locale, String key, ValueStack valueStack, Object[] args) { + ResourceBundle bundle = findResourceBundle(bundleName, locale); + if (bundle == null) { + return null; + } + if (valueStack != null) { + reloadBundles(valueStack.getContext()); + } + try { + String rawPattern = bundle.getString(key); + return formatMessage(rawPattern, locale, valueStack, args); + } catch (MissingResourceException e) { + LOG.debug("Missing key [{}] in bundle [{}]!", key, bundleName); + return null; + } +} +``` + +(This keeps `getMessage`'s order — `findResourceBundle` → `reloadBundles` → `getString` — identical; only the trailing translate/format is now delegated to `formatMessage`, so behavior is unchanged.) + +- [ ] **Step 2: Add `findMessageRaw`** + +Add next to the existing `findMessage`. It mirrors `findMessage` exactly (including the pre-existing, never-populated `checked` cycle guard) but returns the raw pattern via `getRawMessage` with no translation/formatting/args: + +```java +/** + * Raw-pattern twin of {@link #findMessage}. Walks class, implemented interfaces, then up the + * hierarchy, returning the first raw message pattern found (via {@link #getRawMessage}) without + * translation or formatting. Used by the cached class-hierarchy resolver. + */ +private String findMessageRaw(Class clazz, String key, String indexedKey, Locale locale, Set checked) { + if (checked == null) { + checked = new TreeSet<>(); + } else if (checked.contains(clazz.getName())) { + return null; + } + + // look in properties of this class + String msg = getRawMessage(clazz.getName(), locale, key); + if (msg != null) { + return msg; + } + if (indexedKey != null) { + msg = getRawMessage(clazz.getName(), locale, indexedKey); + if (msg != null) { + return msg; + } + } + + // look in properties of implemented interfaces + Class[] interfaces = clazz.getInterfaces(); + for (Class anInterface : interfaces) { + msg = getRawMessage(anInterface.getName(), locale, key); + if (msg != null) { + return msg; + } + if (indexedKey != null) { + msg = getRawMessage(anInterface.getName(), locale, indexedKey); + if (msg != null) { + return msg; + } + } + } + + // traverse up hierarchy + if (clazz.isInterface()) { + interfaces = clazz.getInterfaces(); + for (Class anInterface : interfaces) { + msg = findMessageRaw(anInterface, key, indexedKey, locale, checked); + if (msg != null) { + return msg; + } + } + } else { + if (!clazz.equals(Object.class) && !clazz.isPrimitive()) { + return findMessageRaw(clazz.getSuperclass(), key, indexedKey, locale, checked); + } + } + + return null; +} +``` + +- [ ] **Step 3: Replace `findMessage`'s body with delegation and deprecate it** + +Replace the entire body of the existing `findMessage` (currently at `AbstractLocalizedTextProvider.java:540-600`) with a thin delegation to `findMessageRaw` + `formatMessage`, and mark it `@Deprecated`. This removes the duplicated traversal (the walk now lives only in `findMessageRaw`): + +```java +/** + * Traverse up class hierarchy looking for message. Looks at class, then implemented interface, + * before going up hierarchy. + * + * @return the message + * @deprecated since 7.3.0 — superseded by the internal raw-resolution + caching path + * ({@link #findMessageRaw} + {@link #formatMessage(String, Locale, ValueStack, Object[])}). Retained + * for backward compatibility with descendant classes. Note: unlike the pre-7.3.0 implementation, a + * candidate whose formatted value is the literal {@code "null"} no longer causes the search to + * continue deeper in the same hierarchy; this affects only the pathological case of the same key + * redefined at multiple hierarchy levels with the shallow value formatting to {@code "null"}. + * The bundle-reload check is now triggered once on entry (when reload mode is enabled) rather than + * lazily per bundle probe, preserving the reload side effect that the previous {@code getMessage}-per-probe + * walk provided. + */ +@Deprecated +protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, + ValueStack valueStack) { + reloadBundles(valueStack != null ? valueStack.getContext() : null); + String rawPattern = findMessageRaw(clazz, key, indexedKey, locale, checked); + return rawPattern != null ? formatMessage(rawPattern, locale, valueStack, args) : null; +} +``` + +(The reload-on-entry preserves the side effect that the old `getMessage`-per-probe walk carried, so both the deprecated external-caller path and the Task-1 intermediate state — where `findText` still calls `findMessage` — keep triggering reload. The final cached path added in Task 2 relies instead on the reload hoisted to the top of `findText`.) + +- [ ] **Step 4: Compile** + +Run: `mvn -q test-compile -DskipAssembly -pl core` +Expected: BUILD SUCCESS (new methods compile; `getRawMessage`/`findMessageRaw` may be flagged unused by the IDE but not by the compiler). + +- [ ] **Step 5: Run the full regression suite for this class** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest` +Expected: PASS — all existing tests green. `getMessage` is behavior-identical; `findMessage` is behavior-identical for all single-definition keys (the only tests here), so the suite proves the split. (`findMessage`'s deprecated delegation differs from before only in the pathological multi-level null-format case, which no existing test exercises.) + +- [ ] **Step 6: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +git commit -m "WW-5540 refactor(core): split raw message resolution from formatting + +Add getRawMessage/formatMessage and a raw twin findMessageRaw. Re-express +getMessage via formatMessage and make findMessage delegate to +findMessageRaw + formatMessage; deprecate both as legacy extension points +superseded by the raw-resolution path. Groundwork for the traversal caches. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 2: Class-hierarchy cache (with invalidation, reload hoist, and correctness tests) + +Introduce the first cache end-to-end: key type, sentinel, map, resolver, `findText` wiring for the class + ModelDriven tiers, invalidation at all three clear sites, and the reload hoist. Add a test fixture and correctness tests first. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java` +- Modify: `core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java` +- Create: `core/src/test/java/org/apache/struts2/text/CacheFixture.java` +- Create: `core/src/test/resources/org/apache/struts2/text/CacheFixture.properties` +- Modify: `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java` + +**Interfaces:** +- Consumes (from Task 1): `getRawMessage`, `formatMessage`, `findMessageRaw`. +- Produces (used by Task 3 and tests): + - `static class TextCacheKey` with fields `int classLoaderHash, String className, String textKey, Locale locale` and `equals`/`hashCode`. + - `private static final String NOT_FOUND` — identity sentinel. + - `private final ConcurrentMap classHierarchyCache`. + - `private int currentLoaderHashCode()` → `getCurrentThreadContextClassLoader().hashCode()`. + - `protected String resolveClassHierarchyRaw(Class clazz, String textKey, String indexedKey, Locale locale)` → raw pattern or `NOT_FOUND`. + - `protected int classHierarchyCacheSize()` → entry count (test support). + +- [ ] **Step 1: Create the test fixture class** + +Create `core/src/test/java/org/apache/struts2/text/CacheFixture.java`: + +```java +/* + * 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.text; + +/** + * Simple fixture whose class-associated bundle ({@code CacheFixture.properties}) backs the + * localized-text caching tests. The {@code name} property is exposed so OGNL expressions such as + * {@code ${name}} can be resolved against a value stack. + */ +public class CacheFixture { + + private final String name; + + public CacheFixture(String name) { + this.name = name; + } + + public String getName() { + return name; + } +} +``` + +- [ ] **Step 2: Create the fixture bundle** + +Create `core/src/test/resources/org/apache/struts2/text/CacheFixture.properties`: + +```properties +cache.static=Static cached value +cache.withparam=Value with param {0} +cache.withognl=Hello ${name} +cache.nullformat={0} +``` + +- [ ] **Step 3: Add public cache-size accessor to the test helper** + +In `StrutsLocalizedTextProviderTest.java`, inside the nested `TestStrutsLocalizedTextProvider` class (after `getBundlesReloadedIndicatorValue`, before its closing brace), add: + +```java +public int classHierarchyCacheSize() { + return super.classHierarchyCacheSize(); +} + +public String callFindMessage(Class clazz, String key, Locale locale, ValueStack valueStack) { + return super.findMessage(clazz, key, null, locale, null, null, valueStack); +} +``` + +- [ ] **Step 4: Write the failing tests** + +Add these methods to `StrutsLocalizedTextProviderTest` (anywhere among the other `testXxx` methods). They reference `resolveClassHierarchyRaw`/`classHierarchyCacheSize`/`NOT_FOUND` behavior that does not exist yet, so they fail to compile/pass until Steps 5–9: + +```java +public void testClassHierarchyCacheReusesFoundPattern() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + assertEquals("Cache not empty before first lookup ?", 0, provider.classHierarchyCacheSize()); + String first = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Static cached value", first); + assertEquals("Cache not populated after found lookup ?", 1, provider.classHierarchyCacheSize()); + + String second = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Second lookup differs from first ?", first, second); + assertEquals("Cache grew on repeated lookup ?", 1, provider.classHierarchyCacheSize()); +} + +public void testClassHierarchyCacheStoresMisses() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + String first = provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack); + assertEquals("Fallback", first); + assertEquals("Miss not cached ?", 1, provider.classHierarchyCacheSize()); + + String second = provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack); + assertEquals("Fallback", second); + assertEquals("Miss cache grew on repeat ?", 1, provider.classHierarchyCacheSize()); +} + +public void testFormattingIsPerCallNotCached() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + String x = provider.findText(CacheFixture.class, "cache.withparam", Locale.ENGLISH, null, new Object[]{"X"}, valueStack); + String y = provider.findText(CacheFixture.class, "cache.withparam", Locale.ENGLISH, null, new Object[]{"Y"}, valueStack); + assertEquals("Value with param X", x); + assertEquals("Value with param Y", y); +} + +public void testOgnlTranslationIsPerCall() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + valueStack.push(new CacheFixture("World")); + String world = provider.findText(CacheFixture.class, "cache.withognl", Locale.ENGLISH, null, null, valueStack); + valueStack.pop(); + valueStack.push(new CacheFixture("Mars")); + String mars = provider.findText(CacheFixture.class, "cache.withognl", Locale.ENGLISH, null, null, valueStack); + valueStack.pop(); + + assertEquals("Hello World", world); + assertEquals("Hello Mars", mars); +} + +public void testNullFormattingFallsThroughToDefault() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // "{0}" with a null arg formats to the literal "null"; findText must fall through to the default. + String first = provider.findText(CacheFixture.class, "cache.nullformat", Locale.ENGLISH, "Fallback", new Object[]{null}, valueStack); + assertEquals("Fallback", first); + // Repeat after the pattern is cached — still falls through. + String second = provider.findText(CacheFixture.class, "cache.nullformat", Locale.ENGLISH, "Fallback", new Object[]{null}, valueStack); + assertEquals("Fallback", second); +} + +public void testReloadClearsClassHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not populated ?", 1, provider.classHierarchyCacheSize()); + + provider.callReloadBundlesForceReload(); + assertEquals("Reload did not clear class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); +} + +public void testClearBundleAndClearMissingCacheEmptyClassHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not populated ?", 1, provider.classHierarchyCacheSize()); + provider.callClearBundleWithLocale("org/apache/struts2/text/CacheFixture", Locale.ENGLISH); + assertEquals("clearBundle did not empty class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); + + provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack); + assertEquals("Cache not repopulated ?", 1, provider.classHierarchyCacheSize()); + provider.callClearMissingBundlesCache(); + assertEquals("clearMissingBundlesCache did not empty class hierarchy cache ?", 0, provider.classHierarchyCacheSize()); +} + +public void testDeprecatedFindMessageStillDelegates() { + // findMessage leaves findText's hot path in this task; this locks the deprecated delegator. + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + assertEquals("Static cached value", provider.callFindMessage(CacheFixture.class, "cache.static", Locale.ENGLISH, valueStack)); + assertNull(provider.callFindMessage(CacheFixture.class, "cache.missing", Locale.ENGLISH, valueStack)); +} +``` + +- [ ] **Step 5: Run the new tests to confirm they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testClassHierarchyCacheReusesFoundPattern+testReloadClearsClassHierarchyCache` +Expected: compilation failure (`classHierarchyCacheSize()` undefined) — this is the red state. + +- [ ] **Step 6: Add the cache field, sentinel, key type, loader-hash helper, and size accessor** + +In `AbstractLocalizedTextProvider`, add the sentinel near the other constants (after `RELOADED` at line ~58): + +```java +private static final String NOT_FOUND = new String("__STRUTS_TEXT_NOT_FOUND__"); // unique identity sentinel; compared with == +``` + +Add the field next to the other caches (after `delegatedClassLoaderMap` at line ~68): + +```java +private final ConcurrentMap classHierarchyCache = new ConcurrentHashMap<>(); +``` + +Add the helper and the size accessor (place near `getCurrentThreadContextClassLoader`): + +```java +private int currentLoaderHashCode() { + return getCurrentThreadContextClassLoader().hashCode(); +} + +/** Test-support accessor: current number of cached class-hierarchy resolutions. */ +protected int classHierarchyCacheSize() { + return classHierarchyCache.size(); +} +``` + +Add the key class next to the existing `MessageFormatKey` static class: + +```java +static class TextCacheKey { + private final int classLoaderHash; + private final String className; + private final String textKey; + private final Locale locale; + + TextCacheKey(int classLoaderHash, String className, String textKey, Locale locale) { + this.classLoaderHash = classLoaderHash; + this.className = className; + this.textKey = textKey; + this.locale = locale; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + TextCacheKey that = (TextCacheKey) o; + return classLoaderHash == that.classLoaderHash + && Objects.equals(className, that.className) + && Objects.equals(textKey, that.textKey) + && Objects.equals(locale, that.locale); + } + + @Override + public int hashCode() { + int result = classLoaderHash; + result = 31 * result + (className != null ? className.hashCode() : 0); + result = 31 * result + (textKey != null ? textKey.hashCode() : 0); + result = 31 * result + (locale != null ? locale.hashCode() : 0); + return result; + } +} +``` + +(`Objects`, `ConcurrentMap`, `ConcurrentHashMap`, `Locale` are already imported.) + +- [ ] **Step 7: Add the cached class-hierarchy resolver** + +Add to `AbstractLocalizedTextProvider` (near `findMessageRaw`): + +```java +/** + * Cached resolution of the class/interface/superclass hierarchy for a key. Returns the raw pattern + * found, or {@link #NOT_FOUND} when the key is absent from the entire hierarchy. Keyed on the + * context classloader hash + class name + key + locale, so no {@link Class} reference is retained. + * Uses get + putIfAbsent (never computeIfAbsent) because the child-property path recurses into findText. + */ +protected String resolveClassHierarchyRaw(Class clazz, String textKey, String indexedKey, Locale locale) { + TextCacheKey cacheKey = new TextCacheKey(currentLoaderHashCode(), clazz.getName(), textKey, locale); + String cached = classHierarchyCache.get(cacheKey); + if (cached != null) { + return cached; + } + String raw = findMessageRaw(clazz, textKey, indexedKey, locale, null); + String toStore = (raw != null) ? raw : NOT_FOUND; + classHierarchyCache.putIfAbsent(cacheKey, toStore); + return toStore; +} + +/** @return true when a cached raw-resolution result represents "not found". */ +protected boolean isNotFound(String cachedRawResult) { + return cachedRawResult == NOT_FOUND; +} +``` + +(`NOT_FOUND` is `private` and not visible to the `StrutsLocalizedTextProvider` subclass, so the subclass tests "found?" via `isNotFound(...)` rather than referencing the sentinel directly.) + +- [ ] **Step 8: Wire invalidation into the three clear sites** + +In `reloadBundles(Map context)`, inside the `if (!reloaded)` block, add the clear immediately after `bundlesMap.clear();` (line ~224): + +```java +bundlesMap.clear(); +classHierarchyCache.clear(); +``` + +In `clearBundle(String bundleName, Locale locale)` (line ~187), add after the `bundlesMap.remove(key)` line: + +```java +final ResourceBundle removedBundle = bundlesMap.remove(key); +classHierarchyCache.clear(); +``` + +In `clearMissingBundlesCache()` (line ~205), add after `missingBundles.clear();`: + +```java +missingBundles.clear(); +classHierarchyCache.clear(); +``` + +- [ ] **Step 9: Rewire `findText` (class + ModelDriven tiers) and hoist the reload** + +In `StrutsLocalizedTextProvider.findText(Class, String, Locale, String, Object[], ValueStack)` (lines 62-195), make these edits. + +Immediately after the `textKey == null` guard (after line 67), add the reload hoist: + +```java + // Trigger bundle reload (and cache invalidation) once, before any cached hierarchy lookup, + // so that in reload/devMode the hierarchy caches are cleared before they are read. + reloadBundles(valueStack != null ? valueStack.getContext() : null); +``` + +Replace the class-hierarchy block (current lines 84-89): + +```java + // search up class hierarchy + String msg = findMessage(startClazz, textKey, indexedTextName, locale, args, null, valueStack); + + if (msg != null) { + return msg; + } +``` + +with: + +```java + // search up class hierarchy (cached raw resolution; format per call) + String classHierarchyRaw = resolveClassHierarchyRaw(startClazz, textKey, indexedTextName, locale); + String msg = null; + if (!isNotFound(classHierarchyRaw)) { + msg = formatMessage(classHierarchyRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } + } +``` + +Replace the ModelDriven inner lookup (current lines 102-105): + +```java + msg = findMessage(model.getClass(), textKey, indexedTextName, locale, args, null, valueStack); + if (msg != null) { + return msg; + } +``` + +with: + +```java + String modelRaw = resolveClassHierarchyRaw(model.getClass(), textKey, indexedTextName, locale); + if (!isNotFound(modelRaw)) { + msg = formatMessage(modelRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } + } +``` + +(`isNotFound` was added in Step 7; the sentinel itself stays private to `AbstractLocalizedTextProvider`.) + +Leave the package loop (lines 111-134), child-property block, and default-message block unchanged in this task. + +- [ ] **Step 10: Compile and run the new tests** + +Run: `mvn -q test-compile -DskipAssembly -pl core` +Expected: BUILD SUCCESS. + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testClassHierarchyCacheReusesFoundPattern+testClassHierarchyCacheStoresMisses+testFormattingIsPerCallNotCached+testOgnlTranslationIsPerCall+testNullFormattingFallsThroughToDefault+testReloadClearsClassHierarchyCache+testClearBundleAndClearMissingCacheEmptyClassHierarchyCache+testDeprecatedFindMessageStillDelegates` +Expected: PASS (8 tests green). + +- [ ] **Step 11: Run the full class to confirm no regression** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest` +Expected: PASS — all tests (existing + 6 new) green. + +- [ ] **Step 12: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java \ + core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java \ + core/src/test/java/org/apache/struts2/text/CacheFixture.java \ + core/src/test/resources/org/apache/struts2/text/CacheFixture.properties \ + core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +git commit -m "WW-5540 perf(core): cache class-hierarchy text resolution + +Cache the class/interface/superclass traversal in findText keyed on +(classloader, class name, key, locale), storing the raw pattern or a +NOT_FOUND marker. Formatting stays per call and falls through to the +next tier when a cached pattern formats to null. Invalidated on +reloadBundles/clearBundle/clearMissingBundlesCache; reload is hoisted +to the top of findText so caches are cleared before they are read. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +### Task 3: Package-hierarchy cache + +Cache the `*.package` traversal the same way, wire it into `findText`, and invalidate it at the same three sites. + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java` +- Modify: `core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java` +- Modify: `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java` + +**Interfaces:** +- Consumes (from Task 1/2): `getRawMessage`, `formatMessage`, `isNotFound`, `TextCacheKey`, `NOT_FOUND`, `currentLoaderHashCode`. +- Produces: + - `private final ConcurrentMap packageHierarchyCache`. + - `private String findPackageMessageRaw(Class startClazz, String textKey, String indexedTextName, Locale locale)` → first raw `*.package` match or `null`. + - `protected String resolvePackageHierarchyRaw(Class startClazz, String textKey, String indexedTextName, Locale locale)` → raw pattern or `NOT_FOUND`. + - `protected int packageHierarchyCacheSize()` → entry count (test support). + +- [ ] **Step 1: Add public accessor to the test helper** + +In `StrutsLocalizedTextProviderTest.TestStrutsLocalizedTextProvider`, add: + +```java +public int packageHierarchyCacheSize() { + return super.packageHierarchyCacheSize(); +} +``` + +- [ ] **Step 2: Write the failing tests** + +Add to `StrutsLocalizedTextProviderTest`: + +```java +public void testPackageHierarchyCacheReusesFoundPattern() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + // ModelDrivenAction2 lives in a package that provides "package.properties" = "It works!". + assertEquals("Package cache not empty before lookup ?", 0, provider.packageHierarchyCacheSize()); + String first = provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("It works!", first); + assertEquals("Package cache not populated after found lookup ?", 1, provider.packageHierarchyCacheSize()); + + String second = provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Second package lookup differs ?", first, second); + assertEquals("Package cache grew on repeat ?", 1, provider.packageHierarchyCacheSize()); +} + +public void testReloadClearsPackageHierarchyCache() { + TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); + ValueStack valueStack = ActionContext.getContext().getValueStack(); + + provider.findText(org.apache.struts2.test.ModelDrivenAction2.class, "package.properties", Locale.getDefault(), null, null, valueStack); + assertEquals("Package cache not populated ?", 1, provider.packageHierarchyCacheSize()); + + provider.callReloadBundlesForceReload(); + assertEquals("Reload did not clear package hierarchy cache ?", 0, provider.packageHierarchyCacheSize()); +} +``` + +- [ ] **Step 3: Run the new tests to confirm they fail** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testPackageHierarchyCacheReusesFoundPattern+testReloadClearsPackageHierarchyCache` +Expected: compilation failure (`packageHierarchyCacheSize()` undefined) — red state. + +- [ ] **Step 4: Add the package cache field, raw walk, resolver, and size accessor** + +In `AbstractLocalizedTextProvider`, add the field next to `classHierarchyCache`: + +```java +private final ConcurrentMap packageHierarchyCache = new ConcurrentHashMap<>(); +``` + +Add the size accessor next to `classHierarchyCacheSize`: + +```java +/** Test-support accessor: current number of cached package-hierarchy resolutions. */ +protected int packageHierarchyCacheSize() { + return packageHierarchyCache.size(); +} +``` + +Add the raw package walk (mirrors the current package loop in `StrutsLocalizedTextProvider.findText`, using `getRawMessage`) and its cached resolver, next to `resolveClassHierarchyRaw`: + +```java +/** + * Raw-pattern walk of the {@code *.package} bundles up the class hierarchy of {@code startClazz}. + * Returns the first raw pattern found (via {@link #getRawMessage}) for the key or its indexed form, + * or {@code null} when none match. + */ +private String findPackageMessageRaw(Class startClazz, String textKey, String indexedTextName, Locale locale) { + for (Class clazz = startClazz; + (clazz != null) && !clazz.equals(Object.class); + clazz = clazz.getSuperclass()) { + + String basePackageName = clazz.getName(); + while (basePackageName.lastIndexOf('.') != -1) { + basePackageName = basePackageName.substring(0, basePackageName.lastIndexOf('.')); + String packageName = basePackageName + ".package"; + String msg = getRawMessage(packageName, locale, textKey); + if (msg != null) { + return msg; + } + if (indexedTextName != null) { + msg = getRawMessage(packageName, locale, indexedTextName); + if (msg != null) { + return msg; + } + } + } + } + return null; +} + +/** + * Cached resolution of the {@code *.package} hierarchy for a key. Returns the raw pattern found, or + * {@link #NOT_FOUND} when absent. Same keying and get + putIfAbsent discipline as + * {@link #resolveClassHierarchyRaw}. + */ +protected String resolvePackageHierarchyRaw(Class startClazz, String textKey, String indexedTextName, Locale locale) { + TextCacheKey cacheKey = new TextCacheKey(currentLoaderHashCode(), startClazz.getName(), textKey, locale); + String cached = packageHierarchyCache.get(cacheKey); + if (cached != null) { + return cached; + } + String raw = findPackageMessageRaw(startClazz, textKey, indexedTextName, locale); + String toStore = (raw != null) ? raw : NOT_FOUND; + packageHierarchyCache.putIfAbsent(cacheKey, toStore); + return toStore; +} +``` + +- [ ] **Step 5: Invalidate the package cache at the three clear sites** + +Add `packageHierarchyCache.clear();` immediately after each `classHierarchyCache.clear();` added in Task 2 — in `reloadBundles` (the `if (!reloaded)` block), `clearBundle`, and `clearMissingBundlesCache`. + +- [ ] **Step 6: Replace the package loop in `findText` with the cached resolver** + +In `StrutsLocalizedTextProvider.findText`, replace the entire package-hierarchy loop (current lines 111-134): + +```java + // nothing still? alright, search the package hierarchy now + for (Class clazz = startClazz; + (clazz != null) && !clazz.equals(Object.class); + clazz = clazz.getSuperclass()) { + + String basePackageName = clazz.getName(); + while (basePackageName.lastIndexOf('.') != -1) { + basePackageName = basePackageName.substring(0, basePackageName.lastIndexOf('.')); + String packageName = basePackageName + ".package"; + msg = getMessage(packageName, locale, textKey, valueStack, args); + + if (msg != null) { + return msg; + } + + if (indexedTextName != null) { + msg = getMessage(packageName, locale, indexedTextName, valueStack, args); + + if (msg != null) { + return msg; + } + } + } + } +``` + +with: + +```java + // search the package hierarchy (cached raw resolution; format per call) + String packageRaw = resolvePackageHierarchyRaw(startClazz, textKey, indexedTextName, locale); + if (!isNotFound(packageRaw)) { + msg = formatMessage(packageRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } + } +``` + +- [ ] **Step 7: Compile and run the new tests** + +Run: `mvn -q test-compile -DskipAssembly -pl core` +Expected: BUILD SUCCESS. + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testPackageHierarchyCacheReusesFoundPattern+testReloadClearsPackageHierarchyCache` +Expected: PASS (2 tests green). In particular `testFindTextInPackage` (existing) must still pass. + +- [ ] **Step 8: Run the full class and the sibling suite** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest` +Expected: PASS — all tests green. + +Run: `mvn test -DskipAssembly -pl core -Dtest=GlobalLocalizedTextProviderTest,LocalizedTextUtilTest` +Expected: PASS (or "No tests matching" for any class that doesn't exist — confirm the ones that do exist pass). This guards the other `AbstractLocalizedTextProvider` subclass and legacy util. + +- [ ] **Step 9: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java \ + core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java \ + core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +git commit -m "WW-5540 perf(core): cache package-hierarchy text resolution + +Cache the *.package traversal in findText the same way as the class +hierarchy, with the same keying, fall-through, and invalidation. + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Final verification + +- [ ] Run the whole core text package and a broad i18n-touching slice: + `mvn test -DskipAssembly -pl core -Dtest='*LocalizedText*,*TextProvider*,TextProviderSupportTest'` + Expected: PASS. +- [ ] Confirm no `computeIfAbsent` was used on the new caches and no `Class` object is stored in any key. +- [ ] Confirm `NOT_FOUND` is only ever compared via `isNotFound(...)` / `==`, never `.equals`. + +## Notes / residual behavior (documented in the spec) + +- A message that formats to the literal `"null"` and is redefined deeper in the *same* class hierarchy may resolve differently than before (shallow `null` short-circuits the cached path). Accepted as pathological — see the spec's "formatWithNullDetection fall-through" section. +- Caches are unbounded, consistent with `bundlesMap`/`missingBundles`; see the spec's "Known limitation". diff --git a/docs/superpowers/specs/2026-07-23-WW-5540-localized-text-provider-caching-design.md b/docs/superpowers/specs/2026-07-23-WW-5540-localized-text-provider-caching-design.md new file mode 100644 index 0000000000..9b01de70a0 --- /dev/null +++ b/docs/superpowers/specs/2026-07-23-WW-5540-localized-text-provider-caching-design.md @@ -0,0 +1,263 @@ +# WW-5540: Add caching to AbstractLocalizedTextProvider + +- **Jira:** [WW-5540](https://issues.apache.org/jira/browse/WW-5540) — Improvement (Minor) +- **Fix version:** 7.3.0 +- **Component:** Core +- **Affected classes:** `org.apache.struts2.text.AbstractLocalizedTextProvider`, `org.apache.struts2.text.StrutsLocalizedTextProvider` + +## Problem + +`StrutsLocalizedTextProvider.findText(Class, textKey, locale, defaultMessage, args, valueStack)` +runs a full **class → interface → superclass → `*.package` hierarchy traversal on every +call**, plus an optional child-property recursion. This method is on the hottest path in the +framework: it backs every UI tag label and every validation message rendered during a request. + +Three caches already exist in `AbstractLocalizedTextProvider`: + +- `bundlesMap` — resolved `ResourceBundle` by `(classloader, bundle, locale)` +- `messageFormats` — `MessageFormat` by `(pattern, locale)` +- `missingBundles` — negative cache for missing *bundles* + +None of them caches the **outcome of the hierarchy traversal** keyed on +`(classloader, class, textKey, locale)`. Missing keys are especially costly: they walk the +entire class hierarchy *and* the entire package hierarchy, each iteration performing a bundle +lookup and (for a miss) triggering a swallowed `MissingResourceException`. This work repeats +identically on every request. + +## Goal + +Cache the result of the traversal so repeated lookups for the same `(class, key, locale)` +collapse to a single map lookup, for both found and not-found keys — **without changing any +observable behavior** (lookup order, dynamic-message evaluation, argument formatting, +devMode/reload semantics). + +## Non-goals + +- No caching of the final rendered string (would break OGNL/`${...}` and per-call `args`). +- No new `StrutsConstant` toggle (consistent with the other, un-toggled caches). +- No bounded/LRU eviction — the caches remain unbounded `ConcurrentHashMap`s, matching + `bundlesMap` / `missingBundles`. See *Known limitation* below. +- No JMH microbenchmark added to the repo. + +## Core invariant + +The return value of `findText` is **not** a pure function of `(class, key, locale)`: + +- The raw message may contain `${...}` OGNL expressions, evaluated per call via + `TextParseUtil.translateVariables(pattern, valueStack)`. +- `MessageFormat` is applied with per-call `args`. +- The `defaultMessage`, the `ModelDriven` model instance, and the child-property recursion + all depend on runtime `ActionContext` / `ValueStack` state. + +Therefore **only the raw resolved pattern (or a `NOT_FOUND` marker) is cached.** Translation +and formatting always run per call. + +### `formatWithNullDetection` fall-through + +`formatWithNullDetection` returns `null` when a message formats to the literal string `"null"`, +and the current `findText` treats that as "not found in this tier" and **keeps searching** the +remaining tiers (ModelDriven → package → default). Because formatting depends on per-call `args` +/ ValueStack, the raw pattern the hierarchy "resolves to" is not a pure function of +`(class, key, locale)` in that case (e.g. a `"{0}"` pattern with a `null` argument formats to +`"null"`). + +**Decision:** keep positive + negative caching, and preserve the fall-through: after a positive +cache hit, `findText` formats the cached raw pattern and, **only if the formatted result is +non-null, returns it** — otherwise it falls through to the next tier exactly as today. This makes +the common case (shallow match formats to `null`, fall through to package/default) behave +identically. + +**Documented residual difference:** if the *same* key is defined at multiple levels of a single +class hierarchy and the shallowest match formats to `"null"` for the given args while a deeper +match would format to a real value, the current code returns the deeper match, whereas the cached +path returns the shallow `null` and falls through past the deeper match. This requires the same +key redefined within one hierarchy with the shallow value formatting to exactly `"null"` — treated +as pathological and accepted. + +## Design (Approach B — hierarchy-result cache) + +### 1. Raw / format split + +Add a private helper to `AbstractLocalizedTextProvider` that renders a raw pattern exactly as +`getMessage` does inline today: + +```java +private String formatMessage(String rawPattern, Locale locale, ValueStack valueStack, Object[] args) { + String message = (valueStack != null) + ? TextParseUtil.translateVariables(rawPattern, valueStack) + : rawPattern; + MessageFormat mf = buildMessageFormat(message, locale); // already cached via messageFormats + return formatWithNullDetection(mf, args); +} +``` + +Add a raw resolver that does only lookup, no rendering: + +```java +private String getRawMessage(String bundleName, Locale locale, String key) { + ResourceBundle bundle = findResourceBundle(bundleName, locale); // already cached via bundlesMap + if (bundle == null) return null; + try { + return bundle.getString(key); + } catch (MissingResourceException e) { + LOG.debug("Missing key [{}] in bundle [{}]!", key, bundleName); + return null; + } +} +``` + +`getMessage(bundleName, locale, key, valueStack, args)` keeps its current signature and +behavior, re-expressed as `getRawMessage(...)` followed by `formatMessage(...)` so every +existing caller is byte-for-byte equivalent. + +### 2. Two caches + +```java +private final ConcurrentMap classHierarchyCache = new ConcurrentHashMap<>(); +private final ConcurrentMap packageHierarchyCache = new ConcurrentHashMap<>(); + +private static final String NOT_FOUND = new String("__STRUTS_TEXT_NOT_FOUND__"); // identity sentinel +``` + +- Values are either the raw pattern or the shared `NOT_FOUND` sentinel (a distinct `String` + instance compared by `==`). A sentinel is required because `ConcurrentHashMap` forbids null + values. +- `TextCacheKey` is a small static class modelled on the existing `MessageFormatKey`, holding + **`int classLoaderHash`, `String className`, `String textKey`, `Locale locale`** with + `equals`/`hashCode`. +- The key stores the **class name (String), not the `Class` object**, so the cache never pins + a `Class`/classloader (consistent with recent classloader-leak remediation). `classLoaderHash` + uses `getCurrentThreadContextClassLoader().hashCode()`, matching the keying scheme already + used by `bundlesMap` / `missingBundles` / `classLoaderMap`. + +### 3. Cached resolution methods + +```java +private String resolveClassHierarchyRaw(Class clazz, String textKey, String indexedKey, Locale locale) { + TextCacheKey key = new TextCacheKey(loaderHash(), clazz.getName(), textKey, locale); + String cached = classHierarchyCache.get(key); + if (cached != null) return cached; // hit: pattern or NOT_FOUND + String raw = findMessageRaw(clazz, textKey, indexedKey, locale, null); + String toStore = (raw != null) ? raw : NOT_FOUND; + classHierarchyCache.putIfAbsent(key, toStore); + return toStore; +} +``` + +- `resolvePackageHierarchyRaw(...)` mirrors this around the `*.package` loop. +- `findMessageRaw` is the raw twin of `findMessage`: walks class → interfaces → superclass, + tries `textKey` then `indexedKey` per class/interface, returns the raw pattern via + `getRawMessage` (no translate/format/args). The internal recursion stays uncached; only the + top-level entry per `(class, key, locale)` is cached. +- Uses **`get` + `putIfAbsent`, not `computeIfAbsent`** — the child-property path recurses back + into `findText`, and a nested `computeIfAbsent` on the same map during its own mapping + function is unsafe. + +### 4. `findText` flow (StrutsLocalizedTextProvider) + +Order is **identical to the current implementation**; only the traversal steps become cached: + +``` +if (textKey == null) return defaultMessage; +indexedTextName = extractIndexedName(textKey); + +if (searchDefaultBundlesFirst) { ... unchanged early lookup ... } + +// class/interface/superclass hierarchy — CACHED +raw = resolveClassHierarchyRaw(startClazz, textKey, indexedTextName, locale); +if (raw != NOT_FOUND) { msg = formatMessage(raw, locale, valueStack, args); if (msg != null) return msg; } + +// ModelDriven model hierarchy — CACHED (reuses classHierarchyCache) +if (ModelDriven.class.isAssignableFrom(startClazz)) { + ... resolve model ... + raw = resolveClassHierarchyRaw(model.getClass(), textKey, indexedTextName, locale); + if (raw != NOT_FOUND) { msg = formatMessage(raw, locale, valueStack, args); if (msg != null) return msg; } +} + +// *.package hierarchy — CACHED +raw = resolvePackageHierarchyRaw(startClazz, textKey, indexedTextName, locale); +if (raw != NOT_FOUND) { msg = formatMessage(raw, locale, valueStack, args); if (msg != null) return msg; } + +// child-property recursion — UNCACHED (valueStack-dependent) — unchanged +// default message — unchanged +``` + +Keeping `classHierarchyCache` and `packageHierarchyCache` separate preserves the exact +ordering (class → ModelDriven → package → child-property → default). The `ModelDriven` model +lookup reuses the class-hierarchy cache for free. + +## Cache invalidation + +The two new caches are cleared at every site that already clears the bundle caches: + +1. **`reloadBundles(context)`** — inside the guarded block that calls `bundlesMap.clear()`, + also `classHierarchyCache.clear()` and `packageHierarchyCache.clear()`. This preserves + devMode / `struts.i18n.reload=true` semantics: the caches are wiped once per request + (guarded by the `RELOADED` context flag), so edited properties are always seen. In + production (`reloadBundles=false`) the caches persist — the intended win. When `findText` + is called with a null ValueStack, the hoisted reload now passes a null context; in + reload/devMode this triggers an eager cache/bundle clear that the previous per-probe path + skipped for null-stack lookups — strictly more eager (never staler), and inert in + production where reload is disabled. +2. **`clearBundle(bundleName, locale)`** — clear both new caches fully (a cached pattern may + originate from the cleared bundle; rare admin operation, full clear acceptable). +3. **`clearMissingBundlesCache()`** — clear both new caches fully, honoring its documented + intent that previously-missing lookups may now resolve (our `NOT_FOUND` entries are + analogous to the missing-bundle cache). + +**Not invalidated by `addDefaultResourceBundle`:** the two caches only ever hold class-named +and `*.package` bundle results. Default bundles are consulted solely in the uncached +`getDefaultMessage` path, so adding a default bundle cannot stale these caches. + +## Correctness invariants preserved + +- **Dynamic `${...}` messages** — raw pattern cached; `translateVariables` runs per call against + the live `ValueStack`, so different stacks yield different outputs. +- **Parameterized messages** — `MessageFormat.format(args)` runs per call. +- **`defaultMessage`, child-property, `ModelDriven` model resolution** — runtime-dependent, + never cached at the result level. +- **Lookup order** — unchanged. +- **No classloader pinning** — keys hold class names, not `Class` objects. + +## Testing + +Tests live in `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java`, +which extends `XWorkTestCase` — **JUnit 3/4 style: `testXxx()` methods with plain `assert*` +(junit.framework), no `@Test`, no AssertJ.** (A `@Test`-annotated method here would silently +never run.) + +1. **Found key, repeated lookup** — same message on both calls; second served from cache. +2. **Missing key, repeated lookup** — default/null on both; cached as `NOT_FOUND` (verified + indirectly: after caching a miss, adding the property + `reloadBundles` makes it resolve). +3. **Dynamic `${...}` message after a cache hit** — same `(class, key, locale)` rendered against + two different `ValueStack`s yields two different results. +4. **Parameterized message** — different `args` across calls format correctly. +5. **`reloadBundles` invalidation** — with reload enabled, a changed bundle value is picked up + on the next request. +6. **`clearBundle` / `clearMissingBundlesCache`** — both drop cached entries. +7. **`ModelDriven` path** — model-class messages still resolve, reusing the class-hierarchy cache. +8. **Package-hierarchy resolution** — a `*.package` key resolves, caches, and the class-before-package + order is unchanged. +9. **Regression** — the existing `StrutsLocalizedTextProviderTest` suite and related + localized-text tests stay green. + +## Known limitation + +Per decision during design, the new caches are **unbounded** `ConcurrentHashMap`s, matching the +existing `bundlesMap` / `missingBundles`. Because `getText`-style tags can accept dynamic / +user-influenced keys, an unbounded `(class, key, locale)` key space is a theoretical +memory-exhaustion vector. This is consistent with the pre-existing exposure of the other caches +and is accepted here for consistency and simplicity; bounding all localized-text caches could be +revisited as a separate, cross-cutting change. + +## Files touched + +- `core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java` + — `formatMessage`, `getRawMessage`, `findMessageRaw`, `resolveClassHierarchyRaw`, + `resolvePackageHierarchyRaw`, `TextCacheKey`, `NOT_FOUND`, the two cache maps, and + invalidation hooks in `reloadBundles` / `clearBundle` / `clearMissingBundlesCache`; + `getMessage` re-expressed via `getRawMessage` + `formatMessage`. +- `core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java` + — `findText(Class, ...)` rewired to the cached resolvers. +- `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java` + — new tests.