mirror of
https://github.com/apache/struts.git
synced 2026-08-05 14:47:09 +00:00
WW-5668 Bound the localized-text provider caches and align request-locale resolution (#1821)
* WW-5668 docs: design spec for bounded i18n caches and request-locale resolution Follow-up to WW-5540. Bound the AbstractLocalizedTextProvider caches via the existing OgnlCache abstraction (configurable struts.i18n.cacheType/cacheMaxSize), and add opt-in request-locale resolution consistency between Dispatcher and I18nInterceptor (struts.locale.validateRequestLocale, default off). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 docs: implementation plan for bounded i18n caches and request-locale resolution Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add remove(key) to the OgnlCache abstraction Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Bound the localized-text provider caches with configurable size Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Fix reassignable-lock hazard and add volatile to i18n cache fields synchronized (bundlesMap) locked on a monitor that rebuildI18nCaches() can reassign; introduce a dedicated bundlesMapLock and lock on that instead. Mark the five i18n cache fields volatile for safe publication across the reassignment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add opt-in request-locale resolution consistency to Dispatcher Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Keep the localized-text caches transient so the provider stays serializable Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Add missing Javadoc and cache-rebuild coverage for i18n provider Add the one-line Javadoc that sibling fields/setters carry to validateRequestLocale and its @Inject setter in Dispatcher, and add two tests covering StrutsLocalizedTextProvider's serialize/deserialize cache rebuild and cacheType selection behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Pin explicit serialVersionUID on the localized-text providers Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Suppress false-positive Sonar S3077 on the thread-safe i18n caches Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-5668 Drop the unused throws Exception from the new Dispatcher locale tests Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -135,6 +135,14 @@ public final class StrutsConstants {
|
||||
*/
|
||||
public static final String STRUTS_LOCALE = "struts.locale";
|
||||
|
||||
/**
|
||||
* When enabled, request-derived locales (from {@code Accept-Language}, used when {@code struts.locale} is
|
||||
* unset) are restricted to the JVM's available-locale set; unavailable values fall back to the default.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_LOCALE_VALIDATE_REQUEST = "struts.locale.validateRequestLocale";
|
||||
|
||||
/**
|
||||
* Whether to use a Servlet request parameter workaround necessary for some versions of WebLogic
|
||||
*/
|
||||
@@ -544,6 +552,22 @@ public final class StrutsConstants {
|
||||
*/
|
||||
public static final String STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE = "struts.ognl.expressionCacheMaxSize";
|
||||
|
||||
/**
|
||||
* Specifies the type of cache to use for the localized-text provider caches. Valid values defined in
|
||||
* {@link org.apache.struts2.ognl.OgnlCacheFactory.CacheType}.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_I18N_CACHE_TYPE = "struts.i18n.cacheType";
|
||||
|
||||
/**
|
||||
* Specifies the maximum size of each localized-text provider cache. Configure based on the cache type
|
||||
* chosen and application-specific needs.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_I18N_CACHE_MAXSIZE = "struts.i18n.cacheMaxSize";
|
||||
|
||||
/**
|
||||
* Specifies the type of cache to use for proxy detection. Valid values defined in
|
||||
* {@link org.apache.struts2.ognl.OgnlCacheFactory.CacheType}.
|
||||
|
||||
@@ -151,6 +151,11 @@ public class Dispatcher {
|
||||
*/
|
||||
private String defaultLocale;
|
||||
|
||||
/**
|
||||
* Store state of {@link StrutsConstants#STRUTS_LOCALE_VALIDATE_REQUEST} setting.
|
||||
*/
|
||||
private boolean validateRequestLocale = false;
|
||||
|
||||
/**
|
||||
* Store state of {@link StrutsConstants#STRUTS_MULTIPART_SAVE_DIR} setting.
|
||||
*/
|
||||
@@ -310,6 +315,16 @@ public class Dispatcher {
|
||||
defaultLocale = val;
|
||||
}
|
||||
|
||||
/**
|
||||
* Modify state of {@link StrutsConstants#STRUTS_LOCALE_VALIDATE_REQUEST} setting.
|
||||
*
|
||||
* @param val New setting
|
||||
*/
|
||||
@Inject(value = StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, required = false)
|
||||
public void setValidateRequestLocale(String val) {
|
||||
validateRequestLocale = Boolean.parseBoolean(val);
|
||||
}
|
||||
|
||||
/**
|
||||
* Modify state of StrutsConstants.STRUTS_I18N_ENCODING setting.
|
||||
*
|
||||
@@ -929,7 +944,7 @@ public class Dispatcher {
|
||||
locale = LocaleUtils.toLocale(defaultLocale);
|
||||
} catch (IllegalArgumentException e) {
|
||||
try {
|
||||
locale = request.getLocale();
|
||||
locale = resolveRequestLocale(request);
|
||||
LOG.warn(new ParameterizedMessage("Cannot convert 'struts.locale' = [{}] to proper locale, defaulting to request locale [{}]",
|
||||
defaultLocale, locale), e);
|
||||
} catch (RuntimeException rex) {
|
||||
@@ -940,7 +955,7 @@ public class Dispatcher {
|
||||
}
|
||||
} else {
|
||||
try {
|
||||
locale = request.getLocale();
|
||||
locale = resolveRequestLocale(request);
|
||||
} catch (RuntimeException rex) {
|
||||
LOG.warn("Cannot get locale from HTTP Request, falling back to system default locale", rex);
|
||||
locale = Locale.getDefault();
|
||||
@@ -949,6 +964,28 @@ public class Dispatcher {
|
||||
return locale;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the request locale. When {@code struts.locale.validateRequestLocale} is enabled and the
|
||||
* request locale is not part of the JVM's available-locale set, falls back to the configured
|
||||
* {@code struts.locale} when set and parseable, otherwise the JVM default. When disabled (default),
|
||||
* returns the request locale unchanged.
|
||||
*/
|
||||
protected Locale resolveRequestLocale(HttpServletRequest request) {
|
||||
Locale locale = request.getLocale();
|
||||
if (!validateRequestLocale || LocaleUtils.isAvailableLocale(locale)) {
|
||||
return locale;
|
||||
}
|
||||
if (defaultLocale != null) {
|
||||
try {
|
||||
return LocaleUtils.toLocale(defaultLocale);
|
||||
} catch (IllegalArgumentException e) {
|
||||
LOG.debug("Configured 'struts.locale' = [{}] is not parseable; falling back to system default", defaultLocale);
|
||||
}
|
||||
}
|
||||
LOG.debug("Request locale [{}] is not available; falling back to system default locale", locale);
|
||||
return Locale.getDefault();
|
||||
}
|
||||
|
||||
/**
|
||||
* Return the path to save uploaded files to (this is configurable).
|
||||
*
|
||||
|
||||
@@ -47,6 +47,15 @@ public interface OgnlCache<K, V> {
|
||||
|
||||
void putIfAbsent(K key, V value);
|
||||
|
||||
/**
|
||||
* Removes the mapping for the given key, if present.
|
||||
*
|
||||
* @param key the key to remove
|
||||
* @return the previous value associated with the key, or {@code null} if none
|
||||
* @since 7.3.0
|
||||
*/
|
||||
V remove(K key);
|
||||
|
||||
int size();
|
||||
|
||||
void clear();
|
||||
|
||||
@@ -63,6 +63,11 @@ public class OgnlCaffeineCache<K, V> implements OgnlCache<K, V> {
|
||||
cache.asMap().putIfAbsent(key, value);
|
||||
}
|
||||
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return cache.asMap().remove(key);
|
||||
}
|
||||
|
||||
@Override
|
||||
public int size() {
|
||||
return cache.asMap().size();
|
||||
|
||||
@@ -63,6 +63,11 @@ public class OgnlDefaultCache<K, V> implements OgnlCache<K, V> {
|
||||
clearIfEvictionLimitExceeded();
|
||||
}
|
||||
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return ognlCache.remove(key);
|
||||
}
|
||||
|
||||
@Override
|
||||
public int size() {
|
||||
return ognlCache.size();
|
||||
|
||||
@@ -70,6 +70,11 @@ public class OgnlLRUCache<K, V> implements OgnlCache<K, V> {
|
||||
ognlLRUCache.putIfAbsent(key, value);
|
||||
}
|
||||
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return ognlLRUCache.remove(key);
|
||||
}
|
||||
|
||||
@Override
|
||||
public int size() {
|
||||
return ognlLRUCache.size();
|
||||
|
||||
@@ -18,12 +18,16 @@
|
||||
*/
|
||||
package org.apache.struts2.text;
|
||||
|
||||
import org.apache.commons.lang3.EnumUtils;
|
||||
import org.apache.commons.lang3.ObjectUtils;
|
||||
import org.apache.logging.log4j.LogManager;
|
||||
import org.apache.logging.log4j.Logger;
|
||||
import org.apache.struts2.ActionContext;
|
||||
import org.apache.struts2.StrutsConstants;
|
||||
import org.apache.struts2.inject.Inject;
|
||||
import org.apache.struts2.ognl.DefaultOgnlCacheFactory;
|
||||
import org.apache.struts2.ognl.OgnlCache;
|
||||
import org.apache.struts2.ognl.OgnlCacheFactory.CacheType;
|
||||
import org.apache.struts2.util.TextParseUtil;
|
||||
import org.apache.struts2.util.ValueStack;
|
||||
|
||||
@@ -46,6 +50,8 @@ import java.util.concurrent.CopyOnWriteArrayList;
|
||||
|
||||
abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider {
|
||||
|
||||
private static final long serialVersionUID = 1L;
|
||||
|
||||
private static final Logger LOG = LogManager.getLogger(AbstractLocalizedTextProvider.class);
|
||||
|
||||
public static final String XWORK_MESSAGES_BUNDLE = "org/apache/struts2/xwork-messages";
|
||||
@@ -59,17 +65,38 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider {
|
||||
@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<String, ResourceBundle> bundlesMap = new ConcurrentHashMap<>();
|
||||
protected boolean devMode = false;
|
||||
protected boolean reloadBundles = false;
|
||||
protected boolean searchDefaultBundlesFirst = false; // Search default resource bundles first. Note: This flag may not be meaningful to all implementations.
|
||||
|
||||
private final ConcurrentMap<MessageFormatKey, MessageFormat> messageFormats = new ConcurrentHashMap<>();
|
||||
private final ConcurrentMap<Integer, List<String>> classLoaderMap = new ConcurrentHashMap<>();
|
||||
private final Set<String> missingBundles = ConcurrentHashMap.newKeySet();
|
||||
private final ConcurrentMap<Integer, ClassLoader> delegatedClassLoaderMap = new ConcurrentHashMap<>();
|
||||
private final ConcurrentMap<TextCacheKey, String> classHierarchyCache = new ConcurrentHashMap<>();
|
||||
private final ConcurrentMap<TextCacheKey, String> packageHierarchyCache = new ConcurrentHashMap<>();
|
||||
|
||||
// Dedicated monitor for bundlesMap-related synchronization: bundlesMap is reassigned by
|
||||
// rebuildI18nCaches(), so locking on it directly would lock on a monitor that can change identity.
|
||||
// transient + reinitialised in readObject: a bare Object is not Serializable.
|
||||
private transient Object bundlesMapLock = new Object();
|
||||
|
||||
private volatile CacheType i18nCacheType = CacheType.WTLFU;
|
||||
private volatile int i18nCacheMaxSize = 10000;
|
||||
|
||||
private <K, V> OgnlCache<K, V> buildI18nCache() {
|
||||
return new DefaultOgnlCacheFactory<K, V>(i18nCacheMaxSize, i18nCacheType).buildOgnlCache();
|
||||
}
|
||||
|
||||
// The OgnlCache implementations are themselves thread-safe; volatile only safely publishes the
|
||||
// reference when rebuildI18nCaches() replaces a cache (during injection / readObject), so S3077
|
||||
// ("volatile is not enough") does not apply here.
|
||||
@SuppressWarnings("java:S3077")
|
||||
protected transient volatile OgnlCache<String, ResourceBundle> bundlesMap = buildI18nCache();
|
||||
@SuppressWarnings("java:S3077")
|
||||
private transient volatile OgnlCache<MessageFormatKey, MessageFormat> messageFormats = buildI18nCache();
|
||||
@SuppressWarnings("java:S3077")
|
||||
private transient volatile OgnlCache<String, Boolean> missingBundles = buildI18nCache();
|
||||
@SuppressWarnings("java:S3077")
|
||||
private transient volatile OgnlCache<TextCacheKey, String> classHierarchyCache = buildI18nCache();
|
||||
@SuppressWarnings("java:S3077")
|
||||
private transient volatile OgnlCache<TextCacheKey, String> packageHierarchyCache = buildI18nCache();
|
||||
|
||||
@Override
|
||||
public void addDefaultResourceBundle(String bundleName) {
|
||||
@@ -110,6 +137,21 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider {
|
||||
return packageHierarchyCache.size();
|
||||
}
|
||||
|
||||
/** Test-support accessor: current number of cached resource bundles. */
|
||||
protected int bundlesMapSize() {
|
||||
return bundlesMap.size();
|
||||
}
|
||||
|
||||
/** Test-support accessor: current number of cached missing-bundle markers. */
|
||||
protected int missingBundlesSize() {
|
||||
return missingBundles.size();
|
||||
}
|
||||
|
||||
/** Test-support accessor: current number of cached message formats. */
|
||||
protected int messageFormatsSize() {
|
||||
return messageFormats.size();
|
||||
}
|
||||
|
||||
@Inject(value = StrutsConstants.STRUTS_CUSTOM_I18N_RESOURCES, required = false)
|
||||
public void setCustomI18NResources(String bundles) {
|
||||
if (bundles == null || bundles.isEmpty()) {
|
||||
@@ -191,7 +233,7 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider {
|
||||
* @param classLoader a {@link ClassLoader} to look up the bundle from if none can be found on the current thread's classloader
|
||||
*/
|
||||
public void setDelegatedClassLoader(final ClassLoader classLoader) {
|
||||
synchronized (bundlesMap) {
|
||||
synchronized (bundlesMapLock) {
|
||||
delegatedClassLoaderMap.put(getCurrentThreadContextClassLoader().hashCode(), classLoader);
|
||||
}
|
||||
}
|
||||
@@ -404,39 +446,67 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider {
|
||||
this.searchDefaultBundlesFirst = Boolean.parseBoolean(searchDefaultBundlesFirst);
|
||||
}
|
||||
|
||||
@Inject(value = StrutsConstants.STRUTS_I18N_CACHE_TYPE, required = false)
|
||||
public void setI18nCacheType(String cacheType) {
|
||||
this.i18nCacheType = EnumUtils.getEnumIgnoreCase(CacheType.class, cacheType, CacheType.WTLFU);
|
||||
rebuildI18nCaches();
|
||||
}
|
||||
|
||||
@Inject(value = StrutsConstants.STRUTS_I18N_CACHE_MAXSIZE, required = false)
|
||||
public void setI18nCacheMaxSize(String cacheMaxSize) {
|
||||
this.i18nCacheMaxSize = Integer.parseInt(cacheMaxSize);
|
||||
rebuildI18nCaches();
|
||||
}
|
||||
|
||||
/**
|
||||
* Rebuilds the localized-text caches from the current type/size. Called during dependency injection
|
||||
* (single-threaded startup, before the provider serves lookups); discards any warm-up entries.
|
||||
*/
|
||||
private void rebuildI18nCaches() {
|
||||
bundlesMap = buildI18nCache();
|
||||
messageFormats = buildI18nCache();
|
||||
missingBundles = buildI18nCache();
|
||||
classHierarchyCache = buildI18nCache();
|
||||
packageHierarchyCache = buildI18nCache();
|
||||
}
|
||||
|
||||
private void readObject(java.io.ObjectInputStream in) throws java.io.IOException, ClassNotFoundException {
|
||||
in.defaultReadObject();
|
||||
bundlesMapLock = new Object();
|
||||
rebuildI18nCaches();
|
||||
}
|
||||
|
||||
@Override
|
||||
public ResourceBundle findResourceBundle(String bundleName, Locale locale) {
|
||||
ClassLoader classLoader = getCurrentThreadContextClassLoader();
|
||||
String key = createMissesKey(String.valueOf(classLoader.hashCode()), bundleName, locale);
|
||||
|
||||
if (missingBundles.contains(key)) {
|
||||
if (missingBundles.get(key) != null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
ResourceBundle bundle = null;
|
||||
try {
|
||||
if (bundlesMap.containsKey(key)) {
|
||||
bundle = bundlesMap.get(key);
|
||||
} else {
|
||||
bundle = bundlesMap.get(key);
|
||||
if (bundle == null) {
|
||||
bundle = ResourceBundle.getBundle(bundleName, locale, classLoader);
|
||||
bundlesMap.putIfAbsent(key, bundle);
|
||||
}
|
||||
} catch (MissingResourceException ex) {
|
||||
if (delegatedClassLoaderMap.containsKey(classLoader.hashCode())) {
|
||||
try {
|
||||
if (bundlesMap.containsKey(key)) {
|
||||
bundle = bundlesMap.get(key);
|
||||
} else {
|
||||
bundle = bundlesMap.get(key);
|
||||
if (bundle == null) {
|
||||
bundle = ResourceBundle.getBundle(bundleName, locale, delegatedClassLoaderMap.get(classLoader.hashCode()));
|
||||
bundlesMap.putIfAbsent(key, bundle);
|
||||
}
|
||||
} catch (MissingResourceException e) {
|
||||
LOG.debug("Missing resource bundle [{}]!", bundleName, e);
|
||||
missingBundles.add(key);
|
||||
missingBundles.put(key, Boolean.TRUE);
|
||||
}
|
||||
} else {
|
||||
LOG.debug("Missing resource bundle [{}]!", bundleName);
|
||||
missingBundles.add(key);
|
||||
missingBundles.put(key, Boolean.TRUE);
|
||||
}
|
||||
}
|
||||
return bundle;
|
||||
|
||||
@@ -34,6 +34,8 @@ import java.util.ResourceBundle;
|
||||
*/
|
||||
public class GlobalLocalizedTextProvider extends AbstractLocalizedTextProvider {
|
||||
|
||||
private static final long serialVersionUID = 1L;
|
||||
|
||||
private static final Logger LOG = LogManager.getLogger(GlobalLocalizedTextProvider.class);
|
||||
|
||||
public GlobalLocalizedTextProvider() {
|
||||
|
||||
@@ -38,6 +38,8 @@ import java.util.ResourceBundle;
|
||||
*/
|
||||
public class StrutsLocalizedTextProvider extends AbstractLocalizedTextProvider {
|
||||
|
||||
private static final long serialVersionUID = 1L;
|
||||
|
||||
private static final Logger LOG = LogManager.getLogger(StrutsLocalizedTextProvider.class);
|
||||
private transient ReflectionProvider reflectionProvider;
|
||||
|
||||
|
||||
@@ -24,6 +24,9 @@
|
||||
|
||||
### This can be used to set your default locale and encoding scheme
|
||||
# struts.locale=en_US
|
||||
### When true, restrict request-derived locales (Accept-Language, used when struts.locale is unset) to the
|
||||
### JVM's available-locale set; unavailable values fall back to the default locale. Defaults to false.
|
||||
struts.locale.validateRequestLocale=false
|
||||
struts.i18n.encoding=UTF-8
|
||||
|
||||
### if specified, the default object factory can be overridden here
|
||||
@@ -296,6 +299,13 @@ struts.ognl.expressionCacheType=wtlfu
|
||||
### chosen and application-specific needs.
|
||||
struts.ognl.expressionCacheMaxSize=10000
|
||||
|
||||
### Specifies the type of cache to use for the localized-text provider caches. See StrutsConstants for details.
|
||||
struts.i18n.cacheType=wtlfu
|
||||
|
||||
### Specifies the maximum size of each localized-text provider cache. This should be configured based on the
|
||||
### cache type chosen and application-specific needs.
|
||||
struts.i18n.cacheMaxSize=10000
|
||||
|
||||
### Specifies the type of cache to use for BeanInfo objects. See StrutsConstants class for further information.
|
||||
struts.ognl.beanInfoCacheType=wtlfu
|
||||
|
||||
|
||||
@@ -571,6 +571,46 @@ public class DispatcherTest extends StrutsJUnit4InternalTestCase {
|
||||
assertEquals(Locale.getDefault(), context.getLocale()); // Expect the system default value when Mock request access fails.
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testValidateRequestLocaleOffPassesThrough() {
|
||||
initDispatcher(new HashMap<>());
|
||||
dispatcher.setDefaultLocale(null); // Force struts.locale unset; the test-config default would otherwise mask the request locale.
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
// A syntactically valid but not JVM-available locale.
|
||||
Locale exotic = new Locale("en", "US", "xzz99");
|
||||
when(request.getLocale()).thenReturn(exotic);
|
||||
|
||||
assertEquals("Default off must pass the request locale through unchanged",
|
||||
exotic, dispatcher.getLocale(request));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testValidateRequestLocaleOnKeepsAvailableLocale() {
|
||||
Map<String, String> params = new HashMap<>();
|
||||
params.put(StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, "true");
|
||||
initDispatcher(params);
|
||||
dispatcher.setDefaultLocale(null); // Force struts.locale unset; the test-config default would otherwise mask the request locale.
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
when(request.getLocale()).thenReturn(Locale.UK);
|
||||
|
||||
assertEquals("Available request locale must be kept", Locale.UK, dispatcher.getLocale(request));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testValidateRequestLocaleOnFallsBackForUnavailableLocale() {
|
||||
Map<String, String> params = new HashMap<>();
|
||||
params.put(StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, "true");
|
||||
initDispatcher(params);
|
||||
dispatcher.setDefaultLocale(null); // Force struts.locale unset; the test-config default would otherwise mask the request locale.
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
Locale exotic = new Locale("en", "US", "xzz99");
|
||||
when(request.getLocale()).thenReturn(exotic);
|
||||
|
||||
// struts.locale unset in this dispatcher -> fall back to the JVM default.
|
||||
assertEquals("Unavailable request locale must fall back to system default",
|
||||
Locale.getDefault(), dispatcher.getLocale(request));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void dispatcherReinjectedAfterReload() {
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
|
||||
@@ -0,0 +1,50 @@
|
||||
/*
|
||||
* 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.ognl;
|
||||
|
||||
import org.junit.Test;
|
||||
|
||||
import static org.junit.Assert.assertEquals;
|
||||
import static org.junit.Assert.assertNull;
|
||||
|
||||
public class OgnlCacheRemoveTest {
|
||||
|
||||
private void assertRemoveContract(OgnlCache<String, String> cache) {
|
||||
cache.put("k", "v");
|
||||
assertEquals("v", cache.get("k"));
|
||||
assertEquals("remove returns previous value", "v", cache.remove("k"));
|
||||
assertNull("entry gone after remove", cache.get("k"));
|
||||
assertNull("remove of absent key returns null", cache.remove("absent"));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void caffeineCacheRemove() {
|
||||
assertRemoveContract(new OgnlCaffeineCache<>(10, 16));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void defaultCacheRemove() {
|
||||
assertRemoveContract(new OgnlDefaultCache<>(10, 16, 0.75f));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void lruCacheRemove() {
|
||||
assertRemoveContract(new OgnlLRUCache<>(10, 16, 0.75f));
|
||||
}
|
||||
}
|
||||
@@ -748,6 +748,79 @@ public class StrutsLocalizedTextProviderTest extends XWorkTestCase {
|
||||
assertEquals("A different index should create its own cache entry ?", 2, provider.classHierarchyCacheSize());
|
||||
}
|
||||
|
||||
public void testCachesAreBoundedByConfiguredMaxSize() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
provider.setI18nCacheMaxSize("100");
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
for (int i = 0; i < 20000; i++) {
|
||||
Locale locale = Locale.forLanguageTag("en-US-x" + String.format("%05d", i));
|
||||
provider.findText(CacheFixture.class, "cache.missing", locale, "Fallback", null, valueStack);
|
||||
}
|
||||
|
||||
assertTrue("classHierarchyCache not bounded ?", provider.classHierarchyCacheSize() <= 2000);
|
||||
assertTrue("packageHierarchyCache not bounded ?", provider.packageHierarchyCacheSize() <= 2000);
|
||||
assertTrue("bundlesMap not bounded ?", provider.bundlesMapSize() <= 2000);
|
||||
assertTrue("missingBundles not bounded ?", provider.missingBundlesSize() <= 2000);
|
||||
assertTrue("messageFormats not bounded ?", provider.messageFormatsSize() <= 2000);
|
||||
}
|
||||
|
||||
public void testCorrectTextStillReturnedUnderEviction() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
provider.setI18nCacheMaxSize("50");
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
// Force heavy eviction with many distinct locales.
|
||||
for (int i = 0; i < 5000; i++) {
|
||||
Locale locale = Locale.forLanguageTag("en-US-x" + String.format("%05d", i));
|
||||
provider.findText(CacheFixture.class, "cache.missing", locale, "Fallback", null, valueStack);
|
||||
}
|
||||
|
||||
// A real key in a real locale still resolves correctly after eviction pressure.
|
||||
String result = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack);
|
||||
assertEquals("Static cached value", result);
|
||||
}
|
||||
|
||||
public void testReloadClearsBoundedCaches() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack);
|
||||
assertTrue("missingBundles not populated ?", provider.missingBundlesSize() > 0);
|
||||
|
||||
provider.callReloadBundlesForceReload();
|
||||
assertEquals("reload did not clear bundlesMap ?", 0, provider.bundlesMapSize());
|
||||
}
|
||||
|
||||
public void testProviderIsUsableAfterDeserialization() throws Exception {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack);
|
||||
|
||||
java.io.ByteArrayOutputStream baos = new java.io.ByteArrayOutputStream();
|
||||
try (java.io.ObjectOutputStream oos = new java.io.ObjectOutputStream(baos)) {
|
||||
oos.writeObject(provider);
|
||||
}
|
||||
Object restored;
|
||||
try (java.io.ObjectInputStream ois = new java.io.ObjectInputStream(new java.io.ByteArrayInputStream(baos.toByteArray()))) {
|
||||
restored = ois.readObject();
|
||||
}
|
||||
TestStrutsLocalizedTextProvider deserialized = (TestStrutsLocalizedTextProvider) restored;
|
||||
// Caches were transient (null right after defaultReadObject) but readObject rebuilds them:
|
||||
assertEquals("Deserialized caches not rebuilt empty", 0, deserialized.classHierarchyCacheSize());
|
||||
String result = deserialized.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack);
|
||||
assertEquals("Static cached value", result);
|
||||
}
|
||||
|
||||
public void testCacheTypeSelectionKeepsProviderWorking() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
provider.setI18nCacheType("basic");
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
String result = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack);
|
||||
assertEquals("Static cached value", result);
|
||||
assertTrue("class hierarchy cache should populate", provider.classHierarchyCacheSize() >= 1);
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void setUp() throws Exception {
|
||||
super.setUp();
|
||||
|
||||
@@ -0,0 +1,617 @@
|
||||
# WW-5668 — Bounded i18n caches and consistent request-locale resolution — 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:** Bound the five `AbstractLocalizedTextProvider` caches with configurable size/eviction via the existing `OgnlCache` abstraction, and add an opt-in consistency check that resolves request-derived locales against the JVM available-locale set in `Dispatcher`.
|
||||
|
||||
**Architecture:** Reuse `OgnlCache` + `DefaultOgnlCacheFactory` (add an additive `remove`) for the i18n caches; drive them from two new `@Inject` constants. Add an opt-in `Dispatcher` helper mirroring `I18nInterceptor`'s availability check. The two parts are independent; Part 1 is always on, Part 2 defaults off.
|
||||
|
||||
**Tech Stack:** Java 17, Struts core, `org.apache.struts2.ognl` cache classes (Caffeine/WTLFU), commons-lang3 `LocaleUtils`/`EnumUtils`, JUnit 4 / `XWorkTestCase`, Mockito.
|
||||
|
||||
**Spec:** `docs/superpowers/specs/2026-07-30-WW-5668-i18n-cache-bounds-locale-resolution-design.md`
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Ticket prefix on every commit: `WW-5668`. Neutral framing only — no security/DoS/attacker/exhaustion language in code, comments, tests, or commit messages.
|
||||
- Core tests are JUnit 3/4. Tests extending `XWorkTestCase` use `public void testXxx()` methods (JUnit 3 style); standalone tests use JUnit 4 (`org.junit.Test`, `org.junit.Assert`). Never JUnit 5 `@Test` — it silently does not run.
|
||||
- Build/test command: `mvn test -DskipAssembly -pl core -Dtest=<ClassName>` (append `#<method>` for a single method).
|
||||
- New config constant defaults must match existing cache conventions: `wtlfu` / `10000`.
|
||||
- `struts.locale.validateRequestLocale` default `false` — current behaviour must be preserved byte-for-byte when unset.
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Add `remove` to the `OgnlCache` abstraction
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/OgnlCache.java`
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/OgnlCaffeineCache.java`
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/OgnlDefaultCache.java`
|
||||
- Modify: `core/src/main/java/org/apache/struts2/ognl/OgnlLRUCache.java`
|
||||
- Test (create): `core/src/test/java/org/apache/struts2/ognl/OgnlCacheRemoveTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: `V OgnlCache.remove(K key)` — removes the mapping for `key`, returning the previous value or `null`. Implemented by all three cache classes.
|
||||
|
||||
- [ ] **Step 1: Write the failing test**
|
||||
|
||||
Create `core/src/test/java/org/apache/struts2/ognl/OgnlCacheRemoveTest.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.ognl;
|
||||
|
||||
import org.junit.Test;
|
||||
|
||||
import static org.junit.Assert.assertEquals;
|
||||
import static org.junit.Assert.assertNull;
|
||||
|
||||
public class OgnlCacheRemoveTest {
|
||||
|
||||
private void assertRemoveContract(OgnlCache<String, String> cache) {
|
||||
cache.put("k", "v");
|
||||
assertEquals("v", cache.get("k"));
|
||||
assertEquals("remove returns previous value", "v", cache.remove("k"));
|
||||
assertNull("entry gone after remove", cache.get("k"));
|
||||
assertNull("remove of absent key returns null", cache.remove("absent"));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void caffeineCacheRemove() {
|
||||
assertRemoveContract(new OgnlCaffeineCache<>(10, 16));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void defaultCacheRemove() {
|
||||
assertRemoveContract(new OgnlDefaultCache<>(10, 16, 0.75f));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void lruCacheRemove() {
|
||||
assertRemoveContract(new OgnlLRUCache<>(10, 16, 0.75f));
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run test to verify it fails**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=OgnlCacheRemoveTest`
|
||||
Expected: COMPILE FAILURE — `OgnlCache` has no `remove` method.
|
||||
|
||||
- [ ] **Step 3: Add `remove` to the interface**
|
||||
|
||||
In `OgnlCache.java`, after the `void put(K key, V value);` / `void putIfAbsent(...)` declarations, add:
|
||||
|
||||
```java
|
||||
/**
|
||||
* Removes the mapping for the given key, if present.
|
||||
*
|
||||
* @param key the key to remove
|
||||
* @return the previous value associated with the key, or {@code null} if none
|
||||
* @since 7.3.0
|
||||
*/
|
||||
V remove(K key);
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Implement in all three cache classes**
|
||||
|
||||
`OgnlCaffeineCache.java` — add:
|
||||
|
||||
```java
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return cache.asMap().remove(key);
|
||||
}
|
||||
```
|
||||
|
||||
`OgnlDefaultCache.java` — add:
|
||||
|
||||
```java
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return ognlCache.remove(key);
|
||||
}
|
||||
```
|
||||
|
||||
`OgnlLRUCache.java` — add:
|
||||
|
||||
```java
|
||||
@Override
|
||||
public V remove(K key) {
|
||||
return ognlLRUCache.remove(key);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Run test to verify it passes**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=OgnlCacheRemoveTest`
|
||||
Expected: PASS (3 tests).
|
||||
|
||||
- [ ] **Step 6: Verify no other implementors broke**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=OgnlUtilTest`
|
||||
Expected: PASS — the only `OgnlCache` implementors are the three modified classes (confirmed by design), so nothing else needed the new method.
|
||||
|
||||
- [ ] **Step 7: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/ognl/OgnlCache.java \
|
||||
core/src/main/java/org/apache/struts2/ognl/OgnlCaffeineCache.java \
|
||||
core/src/main/java/org/apache/struts2/ognl/OgnlDefaultCache.java \
|
||||
core/src/main/java/org/apache/struts2/ognl/OgnlLRUCache.java \
|
||||
core/src/test/java/org/apache/struts2/ognl/OgnlCacheRemoveTest.java
|
||||
git commit -m "WW-5668 Add remove(key) to the OgnlCache abstraction
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Bound the localized-text provider caches (configurable)
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/StrutsConstants.java` (add two constants)
|
||||
- Modify: `core/src/main/resources/org/apache/struts2/default.properties` (document them)
|
||||
- Modify: `core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java` (cache fields, setters, call sites, size accessors)
|
||||
- Test: `core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java` (add tests; existing 33 must stay green)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `OgnlCache<K,V>` with `get`/`put`/`putIfAbsent`/`remove`/`clear`/`size` (Task 1); `DefaultOgnlCacheFactory<K,V>(int cacheMaxSize, CacheType type)` with `buildOgnlCache()`; `OgnlCacheFactory.CacheType` enum (`BASIC`/`LRU`/`WTLFU`).
|
||||
- Produces: constants `STRUTS_I18N_CACHE_TYPE = "struts.i18n.cacheType"`, `STRUTS_I18N_CACHE_MAXSIZE = "struts.i18n.cacheMaxSize"`; provider setters `setI18nCacheType(String)`, `setI18nCacheMaxSize(String)`; `protected int` size accessors `bundlesMapSize()`, `missingBundlesSize()`, `messageFormatsSize()` (alongside the existing `classHierarchyCacheSize()`/`packageHierarchyCacheSize()`).
|
||||
|
||||
- [ ] **Step 1: Add the constants**
|
||||
|
||||
In `StrutsConstants.java`, after `STRUTS_OGNL_EXPRESSION_CACHE_MAXSIZE` (line ~545), add:
|
||||
|
||||
```java
|
||||
/**
|
||||
* Specifies the type of cache to use for the localized-text provider caches. Valid values defined in
|
||||
* {@link org.apache.struts2.ognl.OgnlCacheFactory.CacheType}.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_I18N_CACHE_TYPE = "struts.i18n.cacheType";
|
||||
|
||||
/**
|
||||
* Specifies the maximum size of each localized-text provider cache. Configure based on the cache type
|
||||
* chosen and application-specific needs.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_I18N_CACHE_MAXSIZE = "struts.i18n.cacheMaxSize";
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Document them in default.properties**
|
||||
|
||||
In `core/src/main/resources/org/apache/struts2/default.properties`, after the `struts.ognl.expressionCacheMaxSize=10000` block (line ~297), add:
|
||||
|
||||
```properties
|
||||
### Specifies the type of cache to use for the localized-text provider caches. See StrutsConstants for details.
|
||||
struts.i18n.cacheType=wtlfu
|
||||
|
||||
### Specifies the maximum size of each localized-text provider cache. This should be configured based on the
|
||||
### cache type chosen and application-specific needs.
|
||||
struts.i18n.cacheMaxSize=10000
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Write the failing tests**
|
||||
|
||||
In `StrutsLocalizedTextProviderTest.java`, add these methods (JUnit 3 style, matching the file). They use the existing `TestStrutsLocalizedTextProvider` subclass and `CacheFixture` fixture already present in this test class:
|
||||
|
||||
```java
|
||||
public void testCachesAreBoundedByConfiguredMaxSize() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
provider.setI18nCacheMaxSize("100");
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
for (int i = 0; i < 20000; i++) {
|
||||
Locale locale = Locale.forLanguageTag("en-US-x" + String.format("%05d", i));
|
||||
provider.findText(CacheFixture.class, "cache.missing", locale, "Fallback", null, valueStack);
|
||||
}
|
||||
|
||||
assertTrue("classHierarchyCache not bounded ?", provider.classHierarchyCacheSize() <= 2000);
|
||||
assertTrue("packageHierarchyCache not bounded ?", provider.packageHierarchyCacheSize() <= 2000);
|
||||
assertTrue("bundlesMap not bounded ?", provider.bundlesMapSize() <= 2000);
|
||||
assertTrue("missingBundles not bounded ?", provider.missingBundlesSize() <= 2000);
|
||||
assertTrue("messageFormats not bounded ?", provider.messageFormatsSize() <= 2000);
|
||||
}
|
||||
|
||||
public void testCorrectTextStillReturnedUnderEviction() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
provider.setI18nCacheMaxSize("50");
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
// Force heavy eviction with many distinct locales.
|
||||
for (int i = 0; i < 5000; i++) {
|
||||
Locale locale = Locale.forLanguageTag("en-US-x" + String.format("%05d", i));
|
||||
provider.findText(CacheFixture.class, "cache.missing", locale, "Fallback", null, valueStack);
|
||||
}
|
||||
|
||||
// A real key in a real locale still resolves correctly after eviction pressure.
|
||||
String result = provider.findText(CacheFixture.class, "cache.static", Locale.ENGLISH, null, null, valueStack);
|
||||
assertEquals("Static cached value", result);
|
||||
}
|
||||
|
||||
public void testReloadClearsBoundedCaches() {
|
||||
TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider();
|
||||
ValueStack valueStack = ActionContext.getContext().getValueStack();
|
||||
|
||||
provider.findText(CacheFixture.class, "cache.missing", Locale.ENGLISH, "Fallback", null, valueStack);
|
||||
assertTrue("missingBundles not populated ?", provider.missingBundlesSize() > 0);
|
||||
|
||||
provider.reloadBundles(ActionContext.getContext().getContextMap());
|
||||
assertEquals("reload did not clear bundlesMap ?", 0, provider.bundlesMapSize());
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the new tests to verify they fail**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testCachesAreBoundedByConfiguredMaxSize+testCorrectTextStillReturnedUnderEviction+testReloadClearsBoundedCaches`
|
||||
Expected: COMPILE FAILURE — `setI18nCacheMaxSize`, `bundlesMapSize`, `missingBundlesSize`, `messageFormatsSize` do not exist yet.
|
||||
|
||||
- [ ] **Step 5: Add imports and replace the cache fields**
|
||||
|
||||
In `AbstractLocalizedTextProvider.java`, add imports:
|
||||
|
||||
```java
|
||||
import org.apache.commons.lang3.EnumUtils;
|
||||
import org.apache.struts2.ognl.DefaultOgnlCacheFactory;
|
||||
import org.apache.struts2.ognl.OgnlCache;
|
||||
import org.apache.struts2.ognl.OgnlCacheFactory.CacheType;
|
||||
```
|
||||
|
||||
Replace the five cache field declarations (currently lines ~62, 67, 69, 71, 72):
|
||||
|
||||
```java
|
||||
protected final ConcurrentMap<String, ResourceBundle> bundlesMap = new ConcurrentHashMap<>();
|
||||
...
|
||||
private final ConcurrentMap<MessageFormatKey, MessageFormat> messageFormats = new ConcurrentHashMap<>();
|
||||
...
|
||||
private final Set<String> missingBundles = ConcurrentHashMap.newKeySet();
|
||||
...
|
||||
private final ConcurrentMap<TextCacheKey, String> classHierarchyCache = new ConcurrentHashMap<>();
|
||||
private final ConcurrentMap<TextCacheKey, String> packageHierarchyCache = new ConcurrentHashMap<>();
|
||||
```
|
||||
|
||||
with (keeping `classLoaderMap` and `delegatedClassLoaderMap` as they are — they still use `ConcurrentMap`):
|
||||
|
||||
```java
|
||||
private volatile CacheType i18nCacheType = CacheType.WTLFU;
|
||||
private volatile int i18nCacheMaxSize = 10000;
|
||||
|
||||
private <K, V> OgnlCache<K, V> buildI18nCache() {
|
||||
return new DefaultOgnlCacheFactory<K, V>(i18nCacheMaxSize, i18nCacheType).buildOgnlCache();
|
||||
}
|
||||
|
||||
protected OgnlCache<String, ResourceBundle> bundlesMap = buildI18nCache();
|
||||
private OgnlCache<MessageFormatKey, MessageFormat> messageFormats = buildI18nCache();
|
||||
private OgnlCache<String, Boolean> missingBundles = buildI18nCache();
|
||||
private OgnlCache<TextCacheKey, String> classHierarchyCache = buildI18nCache();
|
||||
private OgnlCache<TextCacheKey, String> packageHierarchyCache = buildI18nCache();
|
||||
```
|
||||
|
||||
Keep the field ordering so `i18nCacheType`/`i18nCacheMaxSize` and `buildI18nCache()` are declared before the five cache fields (field initializers run top-to-bottom). Leave `boolean devMode`/`reloadBundles`/`searchDefaultBundlesFirst` where they are.
|
||||
|
||||
- [ ] **Step 6: Add the injectable setters and rebuild helper**
|
||||
|
||||
Near the other `@Inject` setters (e.g. after `setSearchDefaultBundlesFirst`, line ~405), add:
|
||||
|
||||
```java
|
||||
@Inject(value = StrutsConstants.STRUTS_I18N_CACHE_TYPE, required = false)
|
||||
public void setI18nCacheType(String cacheType) {
|
||||
this.i18nCacheType = EnumUtils.getEnumIgnoreCase(CacheType.class, cacheType, CacheType.WTLFU);
|
||||
rebuildI18nCaches();
|
||||
}
|
||||
|
||||
@Inject(value = StrutsConstants.STRUTS_I18N_CACHE_MAXSIZE, required = false)
|
||||
public void setI18nCacheMaxSize(String cacheMaxSize) {
|
||||
this.i18nCacheMaxSize = Integer.parseInt(cacheMaxSize);
|
||||
rebuildI18nCaches();
|
||||
}
|
||||
|
||||
/**
|
||||
* Rebuilds the localized-text caches from the current type/size. Called during dependency injection
|
||||
* (single-threaded startup, before the provider serves lookups); discards any warm-up entries.
|
||||
*/
|
||||
private void rebuildI18nCaches() {
|
||||
bundlesMap = buildI18nCache();
|
||||
messageFormats = buildI18nCache();
|
||||
missingBundles = buildI18nCache();
|
||||
classHierarchyCache = buildI18nCache();
|
||||
packageHierarchyCache = buildI18nCache();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 7: Update the `bundlesMap` / `missingBundles` call sites in `findResourceBundle`**
|
||||
|
||||
Replace the body of `findResourceBundle` (lines ~408-442) that uses `missingBundles.contains` / `bundlesMap.containsKey` / `bundlesMap.get` / `putIfAbsent` / `missingBundles.add`. New body:
|
||||
|
||||
```java
|
||||
@Override
|
||||
public ResourceBundle findResourceBundle(String bundleName, Locale locale) {
|
||||
ClassLoader classLoader = getCurrentThreadContextClassLoader();
|
||||
String key = createMissesKey(String.valueOf(classLoader.hashCode()), bundleName, locale);
|
||||
|
||||
if (missingBundles.get(key) != null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
ResourceBundle bundle = null;
|
||||
try {
|
||||
bundle = bundlesMap.get(key);
|
||||
if (bundle == null) {
|
||||
bundle = ResourceBundle.getBundle(bundleName, locale, classLoader);
|
||||
bundlesMap.putIfAbsent(key, bundle);
|
||||
}
|
||||
} catch (MissingResourceException ex) {
|
||||
if (delegatedClassLoaderMap.containsKey(classLoader.hashCode())) {
|
||||
try {
|
||||
bundle = bundlesMap.get(key);
|
||||
if (bundle == null) {
|
||||
bundle = ResourceBundle.getBundle(bundleName, locale, delegatedClassLoaderMap.get(classLoader.hashCode()));
|
||||
bundlesMap.putIfAbsent(key, bundle);
|
||||
}
|
||||
} catch (MissingResourceException e) {
|
||||
LOG.debug("Missing resource bundle [{}]!", bundleName, e);
|
||||
missingBundles.put(key, Boolean.TRUE);
|
||||
}
|
||||
} else {
|
||||
LOG.debug("Missing resource bundle [{}]!", bundleName);
|
||||
missingBundles.put(key, Boolean.TRUE);
|
||||
}
|
||||
}
|
||||
return bundle;
|
||||
}
|
||||
```
|
||||
|
||||
`clearBundle` (line ~209) needs no change: `bundlesMap.remove(key)` now calls `OgnlCache.remove` and still returns the removed `ResourceBundle`. `clearMissingBundlesCache` (`missingBundles.clear()`), and `reloadBundles` (`bundlesMap.clear()` etc.) also need no change — `clear()` is unchanged on `OgnlCache`. `buildMessageFormat` (`messageFormats.get`/`put`), `resolveClassHierarchyRaw`/`resolvePackageHierarchyRaw` (`get`/`putIfAbsent`) are signature-compatible and need no change.
|
||||
|
||||
- [ ] **Step 8: Add the three size accessors**
|
||||
|
||||
Next to the existing `classHierarchyCacheSize()`/`packageHierarchyCacheSize()` (lines ~103-111), add:
|
||||
|
||||
```java
|
||||
/** Test-support accessor: current number of cached resource bundles. */
|
||||
protected int bundlesMapSize() {
|
||||
return bundlesMap.size();
|
||||
}
|
||||
|
||||
/** Test-support accessor: current number of cached missing-bundle markers. */
|
||||
protected int missingBundlesSize() {
|
||||
return missingBundles.size();
|
||||
}
|
||||
|
||||
/** Test-support accessor: current number of cached message formats. */
|
||||
protected int messageFormatsSize() {
|
||||
return messageFormats.size();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 9: Run the new tests to verify they pass**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest#testCachesAreBoundedByConfiguredMaxSize+testCorrectTextStillReturnedUnderEviction+testReloadClearsBoundedCaches`
|
||||
Expected: PASS (3 tests).
|
||||
|
||||
- [ ] **Step 10: Run the full provider suite to confirm no regression**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest`
|
||||
Expected: PASS (all existing tests + 3 new).
|
||||
|
||||
- [ ] **Step 11: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/StrutsConstants.java \
|
||||
core/src/main/resources/org/apache/struts2/default.properties \
|
||||
core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java \
|
||||
core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java
|
||||
git commit -m "WW-5668 Bound the localized-text provider caches with configurable size
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Opt-in request-locale resolution consistency in Dispatcher
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/StrutsConstants.java` (add one constant)
|
||||
- Modify: `core/src/main/resources/org/apache/struts2/default.properties` (document it)
|
||||
- Modify: `core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java` (flag field, setter, helper, `getLocale` call sites)
|
||||
- Test: `core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java` (add tests)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `org.apache.commons.lang3.LocaleUtils.isAvailableLocale(Locale)` (LocaleUtils already imported in `Dispatcher`).
|
||||
- Produces: constant `STRUTS_LOCALE_VALIDATE_REQUEST = "struts.locale.validateRequestLocale"`; `Dispatcher.setValidateRequestLocale(String)`; private `Locale resolveRequestLocale(HttpServletRequest)`.
|
||||
|
||||
- [ ] **Step 1: Add the constant**
|
||||
|
||||
In `StrutsConstants.java`, after `STRUTS_LOCALE` (line 136), add:
|
||||
|
||||
```java
|
||||
/**
|
||||
* When enabled, request-derived locales (from {@code Accept-Language}, used when {@code struts.locale} is
|
||||
* unset) are restricted to the JVM's available-locale set; unavailable values fall back to the default.
|
||||
*
|
||||
* @since 7.3.0
|
||||
*/
|
||||
public static final String STRUTS_LOCALE_VALIDATE_REQUEST = "struts.locale.validateRequestLocale";
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Document it in default.properties**
|
||||
|
||||
In `default.properties`, after the `# struts.locale=en_US` line (line ~26), add:
|
||||
|
||||
```properties
|
||||
### When true, restrict request-derived locales (Accept-Language, used when struts.locale is unset) to the
|
||||
### JVM's available-locale set; unavailable values fall back to the default locale. Defaults to false.
|
||||
struts.locale.validateRequestLocale=false
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Write the failing tests**
|
||||
|
||||
In `DispatcherTest.java` (JUnit 4, uses Mockito), add — modelled on the existing `getLocale` tests around lines 481-532:
|
||||
|
||||
```java
|
||||
@Test
|
||||
public void testValidateRequestLocaleOffPassesThrough() throws Exception {
|
||||
Dispatcher du = initDispatcher(new HashMap<>());
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
// A syntactically valid but not JVM-available locale.
|
||||
Locale exotic = new Locale("en", "US", "xzz99");
|
||||
when(request.getLocale()).thenReturn(exotic);
|
||||
|
||||
assertEquals("Default off must pass the request locale through unchanged",
|
||||
exotic, du.getLocale(request));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testValidateRequestLocaleOnKeepsAvailableLocale() throws Exception {
|
||||
Map<String, String> params = new HashMap<>();
|
||||
params.put(StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, "true");
|
||||
Dispatcher du = initDispatcher(params);
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
when(request.getLocale()).thenReturn(Locale.UK);
|
||||
|
||||
assertEquals("Available request locale must be kept", Locale.UK, du.getLocale(request));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testValidateRequestLocaleOnFallsBackForUnavailableLocale() throws Exception {
|
||||
Map<String, String> params = new HashMap<>();
|
||||
params.put(StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, "true");
|
||||
Dispatcher du = initDispatcher(params);
|
||||
HttpServletRequest request = mock(HttpServletRequest.class);
|
||||
Locale exotic = new Locale("en", "US", "xzz99");
|
||||
when(request.getLocale()).thenReturn(exotic);
|
||||
|
||||
// struts.locale unset in this dispatcher -> fall back to the JVM default.
|
||||
assertEquals("Unavailable request locale must fall back to system default",
|
||||
Locale.getDefault(), du.getLocale(request));
|
||||
}
|
||||
```
|
||||
|
||||
Note: reuse the test's existing helper for building a `Dispatcher` with init params. If the existing tests use a different constructor pattern than `initDispatcher(Map)`, match whatever those `getLocale` tests already use to build `du` (check the top of the nearest existing `getLocale` test and copy its setup verbatim).
|
||||
|
||||
- [ ] **Step 4: Run the new tests to verify they fail**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=DispatcherTest#testValidateRequestLocaleOffPassesThrough+testValidateRequestLocaleOnKeepsAvailableLocale+testValidateRequestLocaleOnFallsBackForUnavailableLocale`
|
||||
Expected: COMPILE FAILURE — `STRUTS_LOCALE_VALIDATE_REQUEST` / setter not present.
|
||||
|
||||
- [ ] **Step 5: Add the flag field and setter**
|
||||
|
||||
In `Dispatcher.java`, near `defaultLocale` (line ~152) add a field:
|
||||
|
||||
```java
|
||||
private boolean validateRequestLocale = false;
|
||||
```
|
||||
|
||||
Near `setDefaultLocale` (line ~308) add:
|
||||
|
||||
```java
|
||||
@Inject(value = StrutsConstants.STRUTS_LOCALE_VALIDATE_REQUEST, required = false)
|
||||
public void setValidateRequestLocale(String val) {
|
||||
validateRequestLocale = Boolean.parseBoolean(val);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 6: Add the resolution helper and route both call sites through it**
|
||||
|
||||
In `Dispatcher.getLocale(HttpServletRequest)` (lines ~925-950), replace the two `locale = request.getLocale();` calls (lines ~932 and ~943) with `locale = resolveRequestLocale(request);`. Then add the helper directly below `getLocale`:
|
||||
|
||||
```java
|
||||
/**
|
||||
* Resolves the request locale. When {@code struts.locale.validateRequestLocale} is enabled and the
|
||||
* request locale is not part of the JVM's available-locale set, falls back to the configured
|
||||
* {@code struts.locale} when set and parseable, otherwise the JVM default. When disabled (default),
|
||||
* returns the request locale unchanged.
|
||||
*/
|
||||
protected Locale resolveRequestLocale(HttpServletRequest request) {
|
||||
Locale locale = request.getLocale();
|
||||
if (!validateRequestLocale || LocaleUtils.isAvailableLocale(locale)) {
|
||||
return locale;
|
||||
}
|
||||
if (defaultLocale != null) {
|
||||
try {
|
||||
return LocaleUtils.toLocale(defaultLocale);
|
||||
} catch (IllegalArgumentException e) {
|
||||
LOG.debug("Configured 'struts.locale' = [{}] is not parseable; falling back to system default", defaultLocale);
|
||||
}
|
||||
}
|
||||
LOG.debug("Request locale [{}] is not available; falling back to system default locale", locale);
|
||||
return Locale.getDefault();
|
||||
}
|
||||
```
|
||||
|
||||
(The `defaultLocale`-set-and-parseable branch is inert at the two current call sites — see the spec's fallback note — but is retained as the helper's general contract. Do not add logic assuming it fires.)
|
||||
|
||||
- [ ] **Step 7: Run the new tests to verify they pass**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=DispatcherTest#testValidateRequestLocaleOffPassesThrough+testValidateRequestLocaleOnKeepsAvailableLocale+testValidateRequestLocaleOnFallsBackForUnavailableLocale`
|
||||
Expected: PASS (3 tests).
|
||||
|
||||
- [ ] **Step 8: Run the full Dispatcher suite to confirm no regression**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=DispatcherTest`
|
||||
Expected: PASS — existing `getLocale` tests unaffected (flag defaults off).
|
||||
|
||||
- [ ] **Step 9: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/StrutsConstants.java \
|
||||
core/src/main/resources/org/apache/struts2/default.properties \
|
||||
core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java \
|
||||
core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java
|
||||
git commit -m "WW-5668 Add opt-in request-locale resolution consistency to Dispatcher
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Whole-change verification
|
||||
|
||||
- [ ] **Step 1: Run both affected suites together**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=OgnlCacheRemoveTest,StrutsLocalizedTextProviderTest,DispatcherTest,OgnlUtilTest`
|
||||
Expected: all PASS.
|
||||
|
||||
- [ ] **Step 2: Run the broader i18n/dispatcher regression set**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest="*LocalizedText*,*Dispatcher*,*Ognl*"`
|
||||
Expected: all PASS. If any pre-existing unrelated flake appears (e.g. jasperreports temp-file), note it and re-run the specific class.
|
||||
|
||||
- [ ] **Step 3: Confirm the working tree is clean and the branch is ready**
|
||||
|
||||
Run: `git status --short && git log --oneline main..HEAD`
|
||||
Expected: no uncommitted changes; three `WW-5668` commits (Task 1-3).
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage:**
|
||||
- Part 1 bounded caches (reuse OgnlCache, add `remove`, five caches, config constants, default.properties, correctness-safe eviction) → Tasks 1 + 2. ✓
|
||||
- Part 1 sizing default `wtlfu`/`10000` single shared size → Task 2 Steps 1-2, 5. ✓
|
||||
- Part 2 opt-in `struts.locale.validateRequestLocale` default false, helper mirroring `I18nInterceptor`, fallback to configured `struts.locale` else `Locale.getDefault()`, inertness caveat → Task 3. ✓
|
||||
- Testing: bound invariant, correctness under eviction, clear/remove, flag off/on/fallback → Task 2 Step 3, Task 3 Step 3. ✓
|
||||
- Out-of-scope (rename, per-cache sizes, path consolidation) → not implemented, correctly deferred. ✓
|
||||
|
||||
**Placeholder scan:** No TBD/TODO; all steps contain concrete code or exact commands. The one soft spot — Task 3 Step 3's note to match the existing `getLocale` test's dispatcher-construction helper — is a real instruction to copy verified local code, not a placeholder, because the surrounding `getLocale` tests already build `du` that way.
|
||||
|
||||
**Type consistency:** `OgnlCache.remove(K)→V` used identically in Task 1 (definition) and Task 2 (clearBundle relies on it). `CacheType`, `DefaultOgnlCacheFactory(int, CacheType)`, `buildOgnlCache()` match the real signatures. Size accessors `bundlesMapSize`/`missingBundlesSize`/`messageFormatsSize` defined in Task 2 Step 8, used in Task 2 Step 3. `setI18nCacheMaxSize(String)` defined Step 6, used Step 3. `STRUTS_I18N_CACHE_*` and `STRUTS_LOCALE_VALIDATE_REQUEST` defined and consumed consistently.
|
||||
+215
@@ -0,0 +1,215 @@
|
||||
# WW-5668 — Bounded localized-text caches and consistent request-locale resolution
|
||||
|
||||
- **Ticket:** [WW-5668](https://issues.apache.org/jira/browse/WW-5668)
|
||||
- **Type:** Improvement
|
||||
- **Fix version:** 7.3.0
|
||||
- **Date:** 2026-07-30
|
||||
|
||||
> Framing note: this is a follow-up to WW-5540. Keep all wording — spec, code,
|
||||
> commits, tests — in terms of cache bounds, eviction, configurability, and
|
||||
> resolution consistency. No security/DoS/attacker language.
|
||||
|
||||
## Summary
|
||||
|
||||
Two related changes to the localized-text subsystem:
|
||||
|
||||
1. Give the `AbstractLocalizedTextProvider` caches a configurable maximum size and
|
||||
eviction, using the framework's existing cache abstraction, so their footprint
|
||||
is bounded like the OGNL expression, BeanInfo, and proxy caches already are.
|
||||
2. Make request-derived locale resolution consistent between `Dispatcher` and
|
||||
`I18nInterceptor`, behind an opt-in flag, so an operator can restrict
|
||||
request-derived locales to the JVM's available-locale set.
|
||||
|
||||
The two parts are independent. Part 1 is always on. Part 2 defaults off.
|
||||
|
||||
## Part 1 — Bounded localized-text caches
|
||||
|
||||
### Current state
|
||||
|
||||
`AbstractLocalizedTextProvider` holds five internal caches as plain
|
||||
`ConcurrentHashMap` / `ConcurrentHashMap.newKeySet`:
|
||||
|
||||
| Field | Type | Key |
|
||||
|---|---|---|
|
||||
| `bundlesMap` | `ConcurrentMap<String, ResourceBundle>` | classloader + bundleName + locale |
|
||||
| `missingBundles` | `Set<String>` | classloader + bundleName + locale |
|
||||
| `messageFormats` | `ConcurrentMap<MessageFormatKey, MessageFormat>` | pattern + locale |
|
||||
| `classHierarchyCache` | `ConcurrentMap<TextCacheKey, String>` | classloader + className + key + locale |
|
||||
| `packageHierarchyCache` | `ConcurrentMap<TextCacheKey, String>` | classloader + className + key + locale |
|
||||
|
||||
None has a configurable upper bound or eviction. Elsewhere the framework
|
||||
standardises on bounded caches via `OgnlCacheFactory` / `DefaultOgnlCacheFactory`
|
||||
(`struts.ognl.expressionCacheMaxSize=10000`, `struts.proxy.cacheMaxSize=10000`,
|
||||
both `wtlfu`). These five are the outlier.
|
||||
|
||||
### Approach
|
||||
|
||||
Reuse the existing cache abstraction (`OgnlCache` + `DefaultOgnlCacheFactory`)
|
||||
rather than introducing a new one or calling Caffeine directly.
|
||||
|
||||
**Interface change (additive):** add `V remove(K key)` to `OgnlCache<K, V>`.
|
||||
- `OgnlCaffeineCache`: `return cache.asMap().remove(key);`
|
||||
- `OgnlDefaultCache`, `OgnlLRUCache`: delegate to the backing map's `remove`.
|
||||
- A `default` implementation is acceptable if it keeps existing impls compiling,
|
||||
but each impl should override with the native map removal so `clearBundle`
|
||||
keeps returning the removed value for its debug log.
|
||||
|
||||
This is the only change to shared OGNL code, and it is purely additive.
|
||||
|
||||
**Provider change:** replace the five fields with `OgnlCache` instances built from a
|
||||
`DefaultOgnlCacheFactory`, mapping call sites:
|
||||
|
||||
| Field | New type | Call-site mapping |
|
||||
|---|---|---|
|
||||
| `bundlesMap` | `OgnlCache<String, ResourceBundle>` | `containsKey` → `get(k) != null`; keep `putIfAbsent`; `remove` |
|
||||
| `missingBundles` | `OgnlCache<String, Boolean>` | `contains` → `get(k) != null`; `add` → `put(k, Boolean.TRUE)` |
|
||||
| `messageFormats` | `OgnlCache<MessageFormatKey, MessageFormat>` | `get` / `put` |
|
||||
| `classHierarchyCache` | `OgnlCache<TextCacheKey, String>` | `get` / `putIfAbsent` / `clear` / `size` |
|
||||
| `packageHierarchyCache` | `OgnlCache<TextCacheKey, String>` | `get` / `putIfAbsent` / `clear` / `size` |
|
||||
|
||||
`reloadBundles`, `clearBundle`, and `clearMissingBundlesCache` keep calling
|
||||
`clear()` / `remove()` on these caches exactly as before. The `NOT_FOUND` identity
|
||||
sentinel in the hierarchy caches is unchanged.
|
||||
|
||||
### Why eviction is correctness-safe
|
||||
|
||||
All five are pure caches: every entry is fully reconstructible on a miss (reload the
|
||||
bundle, re-record a miss, rebuild the `MessageFormat`, re-walk the class/package
|
||||
hierarchy). WTLFU eviction can therefore only cause an occasional recompute, never a
|
||||
wrong or stale localized result. This preserves the WW-5540 behaviour while bounding
|
||||
memory.
|
||||
|
||||
### Configuration
|
||||
|
||||
Two new constants, applied independently to each of the five caches:
|
||||
|
||||
| Constant | Values | Default |
|
||||
|---|---|---|
|
||||
| `struts.i18n.cacheType` | `basic` \| `lru` \| `wtlfu` | `wtlfu` |
|
||||
| `struts.i18n.cacheMaxSize` | integer | `10000` |
|
||||
|
||||
- Injected via `@Inject(..., required = false)` setters on
|
||||
`AbstractLocalizedTextProvider`, following the existing i18n setters
|
||||
(`setReloadBundles`, `setDevMode`, `setSearchDefaultBundlesFirst`).
|
||||
- New keys in `StrutsConstants`.
|
||||
- Documented in `default.properties` beside the OGNL/proxy cache settings.
|
||||
- A single `cacheMaxSize` governs all five caches for now. Per-cache tuning is
|
||||
deliberately out of scope — see Out of scope / follow-ups.
|
||||
|
||||
### Sizing note (accepted trade-off)
|
||||
|
||||
The WW-5540 hierarchy caches are keyed by `(classloader, className, key, locale)`.
|
||||
A very large application under normal single-locale traffic could have more than
|
||||
10,000 distinct `(class, key)` pairs, in which case a 10,000 bound causes eviction
|
||||
churn and partially erodes WW-5540's caching benefit (correctness unaffected, only
|
||||
recompute cost). WTLFU retains the hot entries, so typical applications are
|
||||
unaffected, and the bound is configurable. A single default of 10,000 (matching the
|
||||
OGNL/proxy caches) is accepted for this ticket; per-cache tuning is a follow-up.
|
||||
|
||||
## Part 2 — Consistent request-locale resolution
|
||||
|
||||
### Current state
|
||||
|
||||
Struts resolves a request locale in more than one place, inconsistently:
|
||||
|
||||
- `I18nInterceptor.getLocaleFromParam(...)` resolves the `request_locale`
|
||||
parameter / cookie / session value, then checks it against the available-locale
|
||||
set via `LocaleProvider.isValidLocale(...)` (→ `LocaleUtils.isAvailableLocale`),
|
||||
falling back to the default locale when it is not available.
|
||||
- `Dispatcher.getLocale(HttpServletRequest)` — used when `struts.locale` is unset,
|
||||
and as the fallback when a configured `struts.locale` is malformed — returns
|
||||
`request.getLocale()` directly, with no availability check.
|
||||
|
||||
### Approach
|
||||
|
||||
Add an opt-in flag and a single resolution helper in `Dispatcher`.
|
||||
|
||||
**New constant:**
|
||||
|
||||
| Constant | Values | Default |
|
||||
|---|---|---|
|
||||
| `struts.locale.validateRequestLocale` | boolean | `false` |
|
||||
|
||||
Default `false` preserves current behaviour byte-for-byte.
|
||||
|
||||
**Helper** `resolveRequestLocale(HttpServletRequest request)`, used at both
|
||||
`request.getLocale()` sites in `Dispatcher.getLocale` (the `struts.locale`-unset
|
||||
branch and the malformed-`struts.locale` fallback branch):
|
||||
|
||||
1. `locale = request.getLocale()`.
|
||||
2. If `!validateRequestLocale` → return `locale` (current behaviour).
|
||||
3. Else if `LocaleUtils.isAvailableLocale(locale)` → return `locale`.
|
||||
4. Else → fall back to the configured `struts.locale` if set and parseable,
|
||||
otherwise `Locale.getDefault()`; log at debug.
|
||||
|
||||
The existing `RuntimeException` handling around `request.getLocale()` (falling back
|
||||
to `Locale.getDefault()`) is retained.
|
||||
|
||||
**Note on the fallback at the current call sites.** Both call sites live inside
|
||||
branches that only execute when `struts.locale` is unset (branch 1) or set but
|
||||
malformed/unparseable (branch 2). In neither case is a configured `struts.locale`
|
||||
usable as a fallback, so in practice step 4 resolves to `Locale.getDefault()` today.
|
||||
The "configured `struts.locale` if set and parseable" clause is retained as the
|
||||
helper's general contract (matching the `I18nInterceptor` spirit and keeping the
|
||||
helper self-contained), but it is inert at the present call sites — the
|
||||
implementation must not add dead logic that assumes it fires.
|
||||
|
||||
**Reuse vs. duplication.** `Dispatcher` runs before an `ActionContext` /
|
||||
`LocaleProvider` is necessarily available, so the helper calls
|
||||
`LocaleUtils.isAvailableLocale(...)` directly — the same underlying check
|
||||
`I18nInterceptor` reaches through `LocaleProvider.isValidLocale`. The two paths share
|
||||
the same semantics without sharing a code path. Consolidating them into one shared
|
||||
locale-resolution component is deliberately out of scope.
|
||||
|
||||
**Injection:** `@Inject(value = "struts.locale.validateRequestLocale",
|
||||
required = false)` setter on `Dispatcher`, parsed to boolean, consistent with how
|
||||
`struts.locale` (`defaultLocale`) is already injected.
|
||||
|
||||
**Docs:** new constant in `default.properties`, commented, near `struts.locale`
|
||||
("restrict request-derived locales to the JVM's available-locale set").
|
||||
|
||||
## Testing
|
||||
|
||||
Core tests extend `XWorkTestCase` and are JUnit 3/4 — no JUnit-5 `@Test`
|
||||
annotations (they would silently not run). Test names stay neutral
|
||||
(bounds/eviction/resolution).
|
||||
|
||||
### Part 1
|
||||
|
||||
- **Bound invariant** (the regression test): with a small configured
|
||||
`cacheMaxSize` (e.g. 1000), drive `findText` with many distinct locales
|
||||
(e.g. 50,000); assert each of the five caches stays on the order of the bound,
|
||||
not the number of distinct locales.
|
||||
- **Correctness under eviction:** normal and repeated locales still return the
|
||||
correct localized text; a real bundle key still resolves under eviction
|
||||
pressure; the `NOT_FOUND` sentinel path still works.
|
||||
- **Configurability:** `cacheMaxSize` changes the ceiling; `cacheType` selects the
|
||||
implementation.
|
||||
- **Clear/remove:** `reloadBundles` empties the caches; `clearBundle` removes the
|
||||
targeted entry.
|
||||
|
||||
### Part 2 (extend the existing `Dispatcher` test)
|
||||
|
||||
- Flag **off** (default): any request locale passes through unchanged, including a
|
||||
valid-but-unavailable one (proves zero behaviour change).
|
||||
- Flag **on** + available locale → returned unchanged.
|
||||
- Flag **on** + valid-but-unavailable locale → falls back to configured
|
||||
`struts.locale` when set, otherwise `Locale.getDefault()`.
|
||||
|
||||
## Backward compatibility
|
||||
|
||||
- Part 1 changes internal cache implementations only; no public API or behavioural
|
||||
change for normal localized-text usage. Eviction is correctness-safe. Default
|
||||
`cacheMaxSize=10000` matches existing cache conventions. `protected` cache fields
|
||||
change type — acceptable for internal framework state; noted for subclasses.
|
||||
- Part 2 is fully opt-in; default `false` preserves current behaviour exactly.
|
||||
- The added `OgnlCache.remove` is additive.
|
||||
|
||||
## Out of scope / follow-ups (separate tickets)
|
||||
|
||||
- Rename `OgnlCache*` → `StrutsCache*` to remove the `Ognl` name leak now that the
|
||||
abstraction is used outside OGNL. (Separate ticket.)
|
||||
- Per-cache size constants (distinct bounds for the hierarchy caches vs.
|
||||
bundle/format/missing caches). (Separate ticket.)
|
||||
- Consolidating `Dispatcher` and `I18nInterceptor` locale resolution into one
|
||||
shared component. (Not planned.)
|
||||
Reference in New Issue
Block a user