From 6db1cbc03f753d1e364dff6a0708fb64f1b2bbaf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 10:00:09 +0200 Subject: [PATCH 01/19] WW-5540 docs: add caching design spec for AbstractLocalizedTextProvider Design for caching the class/package hierarchy traversal result in findText, keyed on (classloader, class name, textKey, locale). Caches the raw resolved pattern (or a NOT_FOUND marker) only; translation and formatting stay per-call. Wires invalidation into the existing reloadBundles/clearBundle/clearMissingBundlesCache sites. Co-Authored-By: Claude Opus 4.8 --- ...40-localized-text-provider-caching-design.md | Bin 0 -> 11923 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 docs/superpowers/specs/2026-07-23-WW-5540-localized-text-provider-caching-design.md 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 0000000000000000000000000000000000000000..47c8928ccc8c21c375c49ce9835cc4083b9d7fc0 GIT binary patch literal 11923 zcmc&)+j1L6lAY)NiW+Ml04&j%@nvR~G{;MmrY%dN6trYdgu|h3q6Rn=uYub&t5I7{F8Q@elu&);2|*S=iDb1(m^nvE~} zAEQYKlU!rX$gNUe#^rpqb+9uv9cQk{oyVWs0Agg5$${Hs4R*<0v8iDQyaE5nk||7v zb?dDw{k#zyh3@q#t=!tzt0Hmu3rp22?^Z=!`>J-ExL!dvJ_{CQy!N+6d41^8+Fe1Q zO1E`yp1L|-x-7o(8MaOAF%F(4F(tqTq7rPJ!w+Haq$%mKF`oPve69|I;PtBXUVPa@ z_^kACvUUD0g&2?^dffVl^zG=X$&<`iFXBxkURwI9$ZoJ*bcw&3a_+yBHzIenbzUZ+MY)daO4p3cy!y~qf}UW%eD7<-6Ar0c+Sj>XLL)bJPPZsZ zx7OF(#H61D!38446$`3;-D-qpnM{hNo)_!($NRhi5p?ahICGH)dqe&Dy`D0DAO z27C$Km)`kmdY3t@iRcG5n3bS>$ads`1Jj@sX3L!jWW!r_$zz=~3Ul(x}1gC^SISc#cX?av+Ob6^Ok>0f!Ymmn`>(}n(*B8%c(ZBr?hT;22jttM3(o8NQl1A7`md^dHGoU^#@~Va$5j44^ z5=V190c60!j>-}9L6MB`CZFnCnlU{^mj$n=WS`8QU%ml!rE_shDwuX;as?ojI4@wR z$?atqVQ-))j#PgWS1V!;z#i(7TpYMvG@1cScHtm&-vRC>&+_6ncUihlYtjA$HiJ`} zWZ%B{2A)4hdh+>vwT|J%aRO24I1~;pU8w$=+?ghzV zROl2&y-8zgNeD_@R{l+$W+9SVUS)CZzvn^V!quq5)?p&0e)f?VXNotQEX5{b`aSpI zCzL+gP17I_)MP~G&dy3;a~`=h{{wZ>x#rB|2tvN~P#(oWc%%dfgEx`zkmu`^0B|Bo zg|Cn;L=JxNea8nPhxKbOxer+p(Iya|1DCj22{l85n!!v9ZX{?JUA%n#_Wb3Wi!;ay zukhuVCKX>zLKyY6=@HrwD10Bu5cD<%%0t4=7V1K0zDk$58=Y)G6o}DZN)qkBeQGc$ zyNnt2Puw3vcL_0Pwt=gle|R5RDf|lB#QEIHXS6ze_E)kvjvd!HGf4uBDH>=G?(aZr zp_#Y}qNQ(1R^*Zb(V6C1ij-LwNxX%dMp5*0d=m$)0AO{58&!hk?~y%(mS45mTPULM z_*`$P0f{?sFRy+EY5nmh;df*A3yklat=7Wnu^aX7>VALhaxlKJyvet}aKGyIL%I5; z`Xvsj9h3Y>7TzHyZoP2FAT{89JI_ZZVV8}LAkJ*o5iWHh24@@M=gNL$3qurDP5%JIh!Ys=Uh@_=Y6T{!#R9h>!W zKEY$Uf?BLDx8^={3j0|X3UN{uenjE*{r8A)Z)JHF$5drU6){N>kS;;)+J z=%WF_b3gv_{wGpu-AMlX`;QFk`y<r*TK zRjKkfZTTj@L%Hw`(3$>w;}W0$bp!*n`1RX2F}l6C&7+%iyD%B0bE9e&I9;AGLCqyU z`rX^N-A`}d{_Z0L20%oaH44m%3_JIkg22>$&zxtK1yF;@z!F>w??`KWRIN~bfY5h~ zBS+d=ZWOx&)Gh^25&j1=Mu@dGOM`~2H}W;T;P@DP(Vg^P!_urZQM7f*eW(2b#4g+YP3ubK8uaas!rb;z)ZmC~KZ?a~#v2AIdH$Pyd_`%ESg zdI0Jq&!r@#C$OtwmXOtnaf4^%6uMBfgL@Ge04WgGjc{hL@@N*Vu-++nP1F)0;0i8M2YZ94*W0eH0TIwmgH@^f^`u1Dkw$$soIAgE|E$r$irbRgUY#=xdAG%Ih# z2x9h?S8u!s--R!bPP#El_l}8(ZpR1aZ#{Q_wmRw~evn{QQA&3N(F7M6K{Rd;!5W;zW)wT9ZJCdgBcQTYR}{I}0B>$k=)p8#Vysc_uY3o!6^h6=1Rvlvgp1t2 zyJ%owKn(FRVk&BC-`bsc-1QT7Td_Fs3RNQQHuR-(liomiA5TnvmwV*|-? zfAu!iNXu!BHj;H$$*oh`$*bH=o)G)@#s`7sY}-zwAGGxF`^2iy`u4?aH&0ex_afFSU1ko|aV&$aDxXx(yp^&^g}h zY8CnEeGmYSBshA1p7k-&H$27irzfZ1e03Jk#gAcbDe6xd3_D>BXDw?yCd`|x2XF=f z_V#20@TGnXl{y?8TS)B{yLhera5$0Z$~7Kl&;hXq$f{+lgEL@DFX>m#-id$8_!^5^ z2kV6LQkYgJs8>Mn(aUXw#=j5Md#(I`nE#>Pme~Hyg?jL?tufVh7s6X-f?c%GnJhlrCW0)|zaYJV zXD9+mz@mFRNgKe?z2yu`0uX(6me^rLuN%lo*pU`dAPsJ=t4&NvT?3bt|J%Cq_ujcV zjWgjbtK8EXYzGt;3W9lz=4l@>f@F+Bx4>LWJ8Tjp2xt@)HW|}KZH)qxWuSbcWzrl> zn57DOB~rPfK*o8cchb(rRFEQ1Vhkn_#jiMq1tZK4bFkiJbVM`$)<~3chntHyQ|UVSI=LboPC83s%Zvk zFJ6M>vRRNo+vrv6z?95}Low@|R^I2Kdzy1qypc58Hy`cl`*;Bz`3N&BCPboRMUgn1 zkYM0r@)e*v`_*|Epwj@OiJH}B_q0?x>&a_QW<&$e;aqvkLRmJ%0gNf5OHXMFjL$HL zfg>$|!YY~hnHu^CV9SFh7{_6HY$H0%qp&WL9UQ>LbI#U?*6IwOH~cC@thfF=fe?-oGVFj zkA#yr9)b4~wyB4*`^T7(f{#lZXEJ$(+<_Yp=rrSgOq-xuDI?R`xpciltuaPYW9&)B zstV3;s%({ws(FXtqk%I>KJ8=)nzOz@s~nvd^(mDot34If6%zp!px)u6Zcz!I>#SVT zo~_m;T784#So)n-_iWPSa07c6C21vnD1FD#H!-l2GUk+DwJjfLkAvRVC_?Zx5*Plc z)F5y|6eXpfRporfiz73RUuL{Pq?d_i3gsW;$Gdul+WMiwf{TXy7-{Bqm7>>;4}g(K zMSuGNLwx>z8Ut4aLvfkuO^(YbpUggm<+qtXWCdE)f4jQ__Q15#aB52v=Yx zQ84?4ZS)-IGD@P@rA1@DcJIQWR(IpgNf45xd1JEN2+> z>Lr8wS2r)6vBSY~k*go+BHC8(3J6r$=;Wmfj8PSV6SPb@c{_CR0zo4mDT)v^Q9&rE z&^fycMyQH+$OosC*GPb{mAL**mpmRq4cp8z8T&p%Jv~sS>i`KV>On)WxCeqEbQnw^ z2>`D9S2V)l?Wl<#01FeP|3)t;AUBo;FjBdbLI=o&fM>Q1gU>!>W$42U*r2T|(`&nD z;iYIwpk}?~fx}szkj^v<@J%|u#^Xi?Pu6wL5jIe@-L3jlpQbfGOFvVRviXiau1=Y@ z+oUZXn1BMceqvcP4E`+Z?cf@7jEOG=*;EjxLsx`n3ePj+v+vXJ+KWJ%=EObb90q?; z^~=4t0k4)Nq@`-xq1oIOUYsoPdYjbO@J)#l`Y57@4ys#S$*6s5MF72Az>~ThAm@K@ z|4YBbYP`f?yK$AirT;5P2BzAOwsU*flc>Snq}|BmGbJ3N(kGB2b8Q_q1Gf~M-M<(T z1g{W$32tRknzqH0$jlz)BE%Z-K$5u0q`RyF+3y*3Krh37c+ek9JxuS>frlGAG9SU6 zsxRC5394@PC8DXIFK+?T`*S0S&Mq0bS%8i~IZg8g#z5fa3C$#Cfx^2BHTPaR5rbAR z71%N;5D$t(BiFZrwO<$I7W2Usra97NzX4JerIi(UAF%nsZ6MGVd?JISB}};~Do!2I z{-h4&s*t&j5HxNoj2A)+(zBxLmYw510)d&0u5_N(+an+=8cBkW@b^7St2^Rzx%K1& zGFsMk0>PLjSy>CeC5}wzjoiJmz*o08M~WeBZ4jtJyrM4g@`BRiiTBYvl$v(opX^lb zPEnB$cjvBru-8z=6N@`x4P9gCGQO3PvHkVchwW^xMmG}lc_xFg!2nR5yQ9?XW@dtMu literal 0 HcmV?d00001 From d6b80362dc6f63d48109cce2abbf25085f744e1a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 10:21:21 +0200 Subject: [PATCH 02/19] WW-5540 docs: add implementation plan and refine spec Add the 3-task TDD implementation plan and record the formatWithNullDetection fall-through decision in the spec. Co-Authored-By: Claude Opus 4.8 --- ...WW-5540-localized-text-provider-caching.md | 818 ++++++++++++++++++ ...-localized-text-provider-caching-design.md | Bin 11923 -> 13454 bytes 2 files changed, 818 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-23-WW-5540-localized-text-provider-caching.md 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..c139bd4d63 --- /dev/null +++ b/docs/superpowers/plans/2026-07-23-WW-5540-localized-text-provider-caching.md @@ -0,0 +1,818 @@ +# 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`. + +- [ ] **Step 1: Add `getRawMessage` and `formatMessage`, and re-express `getMessage` via them** + +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: + +```java +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`.) + +- [ ] **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; +} +``` + +Leave the existing `findMessage` and the existing `getMessage` callers untouched otherwise. + +- [ ] **Step 3: 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 4: Run the full regression suite for this class** + +Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest` +Expected: PASS — all existing tests green (proves the `getMessage`/`formatMessage` refactor is behavior-preserving). + +- [ ] **Step 5: 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, and +re-express getMessage in terms of them. Pure refactor, no behavior +change; groundwork for the hierarchy-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(); +} +``` + +- [ ] **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()); +} +``` + +- [ ] **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` +Expected: PASS (7 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 index 47c8928ccc8c21c375c49ce9835cc4083b9d7fc0..a39032683a53e85f1e064021458789deb39fe972 100644 GIT binary patch delta 1521 zcmb7EO^)0|6jqc4QUO9?6Db7c(OEFl^mM{96JCAQ z|BeL(PD5~!3OBTELHmXsY3eveT{BfabUH;!o>Ifq+QdP(6jQLSreY;ct74fXLuVt1-Ld3K4{bW#(hqF(5SM$C6qekxDvuO7Z$71s;)HZ}71#5hhGI^pa;uGK>9=~k)r`bRc4V#_(m5PGQh z(I&fN`i>meI{!2pe2uQ&WFC85%>fmn+OhEGq${8l1oIFjolK2Qn`7Coufe-qQPMF< z5NiXqylXGXR?`|EMwlfrDmg+Iu%(=LXM%HcZ8Vzz?9LY9F`axU-GqZLi03dI$|Rq3v`NXvq#2KAw|u5EsGY5 z-I4P5#qKp={eSo@aNrk%g`4twVN?mBffqg{O30jdVqgyCl-V3GnYirE&}PK>;{eo) zrIxH^Pv(ymmkOW+h;Mx@>gP;pW4MRAub!#Dzxd=8b+M)=hx>Q7_eAp(dHm)HiP=wQ f0;J?Iy}tVC&Zjp%rs44CS8qS2{_5G?r(gaHF@YGm delta 68 zcmeCnoE*C$lxy<`{uBIcMX4pFMR}9=%j<3CQQ&4o;!n<2>_OtPaB=DDPc~qZ+?=7L GCIkS4`xmYN From 96e08e91916b7053403fdc22c6dc82833fa7772f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 12:54:29 +0200 Subject: [PATCH 03/19] WW-5540 docs: deprecate+delegate findMessage/getMessage in plan Resolve pre-flight duplication/dead-code finding: old traversal helpers delegate to the raw twins and are marked @Deprecated instead of being duplicated. Add a direct characterization test for the findMessage delegator. Co-Authored-By: Claude Opus 4.8 --- ...WW-5540-localized-text-provider-caching.md | 73 +++++++++++++++---- 1 file changed, 60 insertions(+), 13 deletions(-) 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 index c139bd4d63..f6fbc0bcd3 100644 --- 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 @@ -36,8 +36,11 @@ Extract the "find the raw pattern" and "render a pattern" steps from `getMessage - `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** +- [ ] **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`): @@ -75,9 +78,16 @@ protected String formatMessage(String rawPattern, Locale locale, ValueStack valu } ``` -Then replace the body of the existing `getMessage` (currently at `AbstractLocalizedTextProvider.java:513-532`) with: +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) { @@ -96,7 +106,7 @@ protected String getMessage(String bundleName, Locale locale, String key, ValueS } ``` -(This keeps `getMessage`'s order — `findResourceBundle` → `reloadBundles` → `getString` — identical; only the trailing translate/format is now delegated to `formatMessage`.) +(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`** @@ -161,27 +171,51 @@ private String findMessageRaw(Class clazz, String key, String indexedKey, Loc } ``` -Leave the existing `findMessage` and the existing `getMessage` callers untouched otherwise. +- [ ] **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`): -- [ ] **Step 3: Compile** +```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"}. + */ +@Deprecated +protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, + ValueStack valueStack) { + String rawPattern = findMessageRaw(clazz, key, indexedKey, locale, checked); + return rawPattern != null ? formatMessage(rawPattern, locale, valueStack, args) : null; +} +``` + +- [ ] **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 4: Run the full regression suite for this class** +- [ ] **Step 5: Run the full regression suite for this class** Run: `mvn test -DskipAssembly -pl core -Dtest=StrutsLocalizedTextProviderTest` -Expected: PASS — all existing tests green (proves the `getMessage`/`formatMessage` refactor is behavior-preserving). +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 5: Commit** +- [ ] **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, and -re-express getMessage in terms of them. Pure refactor, no behavior -change; groundwork for the hierarchy-traversal caches. +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 " ``` @@ -272,6 +306,10 @@ In `StrutsLocalizedTextProviderTest.java`, inside the nested `TestStrutsLocalize 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** @@ -368,6 +406,15 @@ public void testClearBundleAndClearMissingCacheEmptyClassHierarchyCache() { 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** @@ -563,8 +610,8 @@ Leave the package loop (lines 111-134), child-property block, and default-messag 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` -Expected: PASS (7 tests green). +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** From abd8a55e8824432804b188bf76958c332bff6d12 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:07:59 +0200 Subject: [PATCH 04/19] 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. The deprecated findMessage triggers the bundle reload on entry, preserving the reload side effect the old getMessage-per-probe walk provided. Groundwork for the traversal caches. Co-Authored-By: Claude Opus 4.8 --- .../text/AbstractLocalizedTextProvider.java | 98 ++++++++++++++----- 1 file changed, 71 insertions(+), 27 deletions(-) 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..222d234a6f 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -507,9 +507,45 @@ 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. */ + @Deprecated protected String getMessage(String bundleName, Locale locale, String key, ValueStack valueStack, Object[] args) { ResourceBundle bundle = findResourceBundle(bundleName, locale); if (bundle == null) { @@ -519,12 +555,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 +564,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,15 +576,12 @@ 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 = getRawMessage(clazz.getName(), locale, key); if (msg != null) { return msg; } - if (indexedKey != null) { - msg = getMessage(clazz.getName(), locale, indexedKey, valueStack, args); - + msg = getRawMessage(clazz.getName(), locale, indexedKey); if (msg != null) { return msg; } @@ -562,17 +589,13 @@ protected String findMessage(Class clazz, String key, String indexedKey, Loca // look in properties of implemented interfaces Class[] interfaces = clazz.getInterfaces(); - for (Class anInterface : interfaces) { - msg = getMessage(anInterface.getName(), locale, key, valueStack, args); - + msg = getRawMessage(anInterface.getName(), locale, key); if (msg != null) { return msg; } - if (indexedKey != null) { - msg = getMessage(anInterface.getName(), locale, indexedKey, valueStack, args); - + msg = getRawMessage(anInterface.getName(), locale, indexedKey); if (msg != null) { return msg; } @@ -582,23 +605,44 @@ protected String findMessage(Class clazz, String key, String indexedKey, Loca // traverse up hierarchy if (clazz.isInterface()) { interfaces = clazz.getInterfaces(); - for (Class anInterface : interfaces) { - msg = findMessage(anInterface, key, indexedKey, locale, args, checked, valueStack); - + msg = findMessageRaw(anInterface, key, indexedKey, locale, checked); if (msg != null) { return msg; } } } else { if (!clazz.equals(Object.class) && !clazz.isPrimitive()) { - return findMessage(clazz.getSuperclass(), key, indexedKey, locale, args, checked, valueStack); + return findMessageRaw(clazz.getSuperclass(), key, indexedKey, locale, checked); } } return null; } + /** + * 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 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; + } + protected String extractIndexedName(String textKey) { String indexedTextName = null; // calculate indexedTextName (collection[*]) if applicable From 77bdf23b4875e446f91016024e25ce21f137db8f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:07:59 +0200 Subject: [PATCH 05/19] WW-5540 docs: refine Task 1 plan (deprecate/delegate + reload-on-entry) Co-Authored-By: Claude Opus 4.8 --- .../2026-07-23-WW-5540-localized-text-provider-caching.md | 6 ++++++ 1 file changed, 6 insertions(+) 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 index f6fbc0bcd3..8d524519ed 100644 --- 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 @@ -187,15 +187,21 @@ Replace the entire body of the existing `findMessage` (currently at `AbstractLoc * 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` From 775bf8b2174b037074dc75018534b09d3314d59a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:12:23 +0200 Subject: [PATCH 06/19] 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 --- .../text/AbstractLocalizedTextProvider.java | 71 ++++++++++++ .../text/StrutsLocalizedTextProvider.java | 27 +++-- .../org/apache/struts2/text/CacheFixture.java | 37 ++++++ .../text/StrutsLocalizedTextProviderTest.java | 107 ++++++++++++++++++ .../struts2/text/CacheFixture.properties | 4 + 5 files changed, 238 insertions(+), 8 deletions(-) create mode 100644 core/src/test/java/org/apache/struts2/text/CacheFixture.java create mode 100644 core/src/test/resources/org/apache/struts2/text/CacheFixture.properties 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 222d234a6f..dc647d6d23 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,7 @@ 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"; + 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 +67,7 @@ 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<>(); @Override public void addDefaultResourceBundle(String bundleName) { @@ -90,6 +92,15 @@ protected ClassLoader getCurrentThreadContextClassLoader() { return Thread.currentThread().getContextClassLoader(); } + private int currentLoaderHashCode() { + return getCurrentThreadContextClassLoader().hashCode(); + } + + /** Test-support accessor: current number of cached class-hierarchy resolutions. */ + protected int classHierarchyCacheSize() { + return classHierarchyCache.size(); + } + @Inject(value = StrutsConstants.STRUTS_CUSTOM_I18N_RESOURCES, required = false) public void setCustomI18NResources(String bundles) { if (bundles == null || bundles.isEmpty()) { @@ -187,6 +198,7 @@ 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(); LOG.debug("Clearing resource bundle [{}], locale [{}], result: [{}].", bundleName, locale, removedBundle != null); } @@ -204,6 +216,7 @@ protected void clearBundle(final String bundleName, Locale locale) { */ protected void clearMissingBundlesCache() { missingBundles.clear(); + classHierarchyCache.clear(); LOG.debug("Cleared the missing bundles cache."); } @@ -222,6 +235,7 @@ protected void reloadBundles(Map context) { } if (!reloaded) { bundlesMap.clear(); + classHierarchyCache.clear(); clearResourceBundleClassloaderCaches(); // now, for the true and utter hack, if we're running in tomcat, clear @@ -620,6 +634,29 @@ private String findMessageRaw(Class clazz, String key, String indexedKey, Loc return null; } + /** + * 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; + } + /** * Traverse up class hierarchy looking for message. Looks at class, then implemented interface, * before going up hierarchy. @@ -702,6 +739,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..dcee2e5796 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,11 @@ 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. + reloadBundles(valueStack != null ? valueStack.getContext() : null); + 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 +86,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, indexedTextName, locale); + String msg = null; + if (!isNotFound(classHierarchyRaw)) { + msg = formatMessage(classHierarchyRaw, locale, valueStack, args); + if (msg != null) { + return msg; + } } if (ModelDriven.class.isAssignableFrom(startClazz)) { @@ -99,9 +107,12 @@ 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, indexedTextName, locale); + if (!isNotFound(modelRaw)) { + msg = formatMessage(modelRaw, 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..b29426e5e8 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,105 @@ 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); + } + + 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)); + } + @Override protected void setUp() throws Exception { super.setUp(); @@ -616,5 +715,13 @@ public boolean getBundlesReloadedIndicatorValue() { final Object reloadedObject = ActionContext.getContext().get(RELOADED); return reloadedObject instanceof Boolean && (Boolean) reloadedObject; } + + 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); + } } } 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..71e6ae1823 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties @@ -0,0 +1,4 @@ +cache.static=Static cached value +cache.withparam=Value with param {0} +cache.withognl=Hello ${name} +cache.nullformat={0} From 028488429596f0b2294c91314625f7bc84a53805 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:18:38 +0200 Subject: [PATCH 07/19] WW-5540 docs: draft follow-up ticket for null-control-flow cleanup Capture the deferred result-wrapper refactor (raised during WW-5540) as a ready-to-file Jira draft; keep WW-5540 focused on caching. Co-Authored-By: Claude Opus 4.8 --- .../localized-text-provider-result-wrapper.md | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 docs/superpowers/followups/localized-text-provider-result-wrapper.md diff --git a/docs/superpowers/followups/localized-text-provider-result-wrapper.md b/docs/superpowers/followups/localized-text-provider-result-wrapper.md new file mode 100644 index 0000000000..f5c7557901 --- /dev/null +++ b/docs/superpowers/followups/localized-text-provider-result-wrapper.md @@ -0,0 +1,51 @@ +# Follow-up ticket draft — replace null-overloaded control flow in LocalizedTextProvider with an explicit result type + +> Ready to file at https://issues.apache.org/jira/projects/WW. Not yet filed — do **not** reference a +> `WW-XXXX` id in source until this exists (per the no-placeholder-TODO convention). + +## Type / Component +Improvement (code quality / maintainability) — Core. + +## Summary +Replace the `null`-overloaded control flow in `AbstractLocalizedTextProvider` / +`StrutsLocalizedTextProvider` message resolution with an explicit result type, so that +"not found" and "found but the value degraded to the literal `null`" are represented +distinctly instead of both collapsing to a Java `null` that callers must branch on. + +## Background +Message lookup uses `null` as an overloaded signal: + +- `formatWithNullDetection` returns `null` when a formatted message equals the literal string + `"null"` (e.g. a `{0}` pattern rendered with a `null` argument). +- Callers (`findText`'s tiers, the deprecated `findMessage`) treat that `null` identically to a + genuine "key not found" and fall through to the next source: `if (msg != null) return msg;`. + +Both cases legitimately mean "keep searching", so the overloading is not a bug — but it is a +readability/robustness wart. The codebase already has a wrapper for the analogous default-message +path: `GetDefaultMessageReturnArg { String message; boolean foundInBundle; }`. Extending a similar +explicit result to the hierarchy path would make the intent self-documenting and consistent. + +Context: this was surfaced while implementing WW-5540 (hierarchy-traversal caching). WW-5540 +deliberately kept the existing `null` convention and only added a cache-boundary marker +(`NOT_FOUND` sentinel + `isNotFound(...)`) plus a fall-through mitigation, to stay behavior-preserving +and focused. This ticket is the orthogonal control-flow cleanup that WW-5540 deferred. + +## Proposed change +Introduce an internal result type (e.g. `sealed`/enum-tagged: `Found(String value)` vs +`ContinueSearch`) used by the raw-resolution + formatting path, unwrapped to `String`/`null` at the +public `findText` boundary (the public method signatures return `String` and must not change). +Consider whether `formatWithNullDetection`, `getMessage`, `findDefaultText`, and `getDefaultMessage` +should adopt the same type for consistency, or whether the change should be scoped to the hierarchy +path only. + +## Scope / risk notes +- Behavior must remain identical (this is a refactor, not a behavior change). +- Touches long-standing framework internals many call sites branch on, plus the `@Deprecated` + `getMessage`/`findMessage` retained for descendant classes — review the ripple carefully. +- Purely a readability/maintainability gain; the branch itself does not disappear, it becomes + explicit (`result.isFound()` instead of `msg != null`). + +## Acceptance +- No public API signature changes. +- Existing `StrutsLocalizedTextProviderTest` (and related i18n tests) stay green with no behavior change. +- `null` is no longer used to mean "found but continue searching" anywhere in the resolution path. From 04cce1c9a68809fcd5b2e6c31215041af41d8afe Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:25:45 +0200 Subject: [PATCH 08/19] 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 --- .../text/AbstractLocalizedTextProvider.java | 55 +++++++++++++++++++ .../text/StrutsLocalizedTextProvider.java | 28 ++-------- .../text/StrutsLocalizedTextProviderTest.java | 30 ++++++++++ 3 files changed, 91 insertions(+), 22 deletions(-) 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 dc647d6d23..efe5687043 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -68,6 +68,7 @@ abstract class AbstractLocalizedTextProvider implements LocalizedTextProvider { 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) { @@ -101,6 +102,11 @@ 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()) { @@ -199,6 +205,7 @@ 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); } @@ -217,6 +224,7 @@ protected void clearBundle(final String bundleName, Locale locale) { protected void clearMissingBundlesCache() { missingBundles.clear(); classHierarchyCache.clear(); + packageHierarchyCache.clear(); LOG.debug("Cleared the missing bundles cache."); } @@ -236,6 +244,7 @@ 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 @@ -657,6 +666,52 @@ 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 = 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; + } + /** * Traverse up class hierarchy looking for message. Looks at class, then implemented interface, * before going up hierarchy. 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 dcee2e5796..9c1d36ec56 100644 --- a/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java @@ -119,28 +119,12 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin } } - // 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, indexedTextName, 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/StrutsLocalizedTextProviderTest.java b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java index b29426e5e8..ad35de8e4e 100644 --- a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java @@ -637,6 +637,32 @@ public void testClearBundleAndClearMissingCacheEmptyClassHierarchyCache() { 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 testDeprecatedFindMessageStillDelegates() { // findMessage leaves findText's hot path in this task; this locks the deprecated delegator. TestStrutsLocalizedTextProvider provider = new TestStrutsLocalizedTextProvider(); @@ -720,6 +746,10 @@ public int classHierarchyCacheSize() { return super.classHierarchyCacheSize(); } + public int packageHierarchyCacheSize() { + return super.packageHierarchyCacheSize(); + } + public String callFindMessage(Class clazz, String key, Locale locale, ValueStack valueStack) { return super.findMessage(clazz, key, null, locale, null, null, valueStack); } From b1e0071804d13bbfcfd4f5ec50f432fae2b4f769 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:35:15 +0200 Subject: [PATCH 09/19] WW-5540 test(core): tighten localized-text cache tests Assert single cache entry in the per-call-format tests (proves the raw pattern is cached, not the formatted result), and mirror the package-cache clearBundle/clearMissingBundlesCache invalidation test. Co-Authored-By: Claude Opus 4.8 --- .../text/StrutsLocalizedTextProviderTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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 ad35de8e4e..e4dc0c8319 100644 --- a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java @@ -582,6 +582,7 @@ public void testFormattingIsPerCallNotCached() { 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() { @@ -597,6 +598,7 @@ public void testOgnlTranslationIsPerCall() { assertEquals("Hello World", world); assertEquals("Hello Mars", mars); + assertEquals("Raw pattern should be cached once across value stacks ?", 1, provider.classHierarchyCacheSize()); } public void testNullFormattingFallsThroughToDefault() { @@ -663,6 +665,21 @@ public void testReloadClearsPackageHierarchyCache() { 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(); From 1c983be9044d091725123a63936612a8b582e54c Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:35:32 +0200 Subject: [PATCH 10/19] WW-5540 docs: note devMode null-valueStack eager-reload edge Co-Authored-By: Claude Opus 4.8 --- ...-localized-text-provider-caching-design.md | Bin 13454 -> 13776 bytes 1 file changed, 0 insertions(+), 0 deletions(-) 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 index a39032683a53e85f1e064021458789deb39fe972..5a00c88bc42bdefe39bdede131f1c52c288c2aab 100644 GIT binary patch delta 336 zcmXw#F-`+95Jh_m&M-|RkP_tpl+;u~q>a~}#G^GHYdqeCDx83dgCKDLjzG@9NtjKR z&c*-#Jr From a55a7a946e307b67fd4ffacc13b824c2a081eac9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:48:26 +0200 Subject: [PATCH 11/19] WW-5540 docs: link follow-up doc to filed ticket WW-5655 Co-Authored-By: Claude Opus 4.8 --- .../followups/localized-text-provider-result-wrapper.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/followups/localized-text-provider-result-wrapper.md b/docs/superpowers/followups/localized-text-provider-result-wrapper.md index f5c7557901..3d937ec3b9 100644 --- a/docs/superpowers/followups/localized-text-provider-result-wrapper.md +++ b/docs/superpowers/followups/localized-text-provider-result-wrapper.md @@ -1,7 +1,6 @@ -# Follow-up ticket draft — replace null-overloaded control flow in LocalizedTextProvider with an explicit result type +# WW-5655 — replace null-overloaded control flow in LocalizedTextProvider with an explicit result type -> Ready to file at https://issues.apache.org/jira/projects/WW. Not yet filed — do **not** reference a -> `WW-XXXX` id in source until this exists (per the no-placeholder-TODO convention). +> Filed as [WW-5655](https://issues.apache.org/jira/browse/WW-5655). ## Type / Component Improvement (code quality / maintainability) — Core. From a4e673950e90692aaf6db4378e20dde9b9b9350a Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:52:10 +0200 Subject: [PATCH 12/19] WW-5540 chore(core): add ASF license header to CacheFixture.properties Co-Authored-By: Claude Opus 4.8 --- .../struts2/text/CacheFixture.properties | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties index 71e6ae1823..5593df2f66 100644 --- a/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties +++ b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties @@ -1,3 +1,21 @@ +# +# 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} From a4e9993dd510a51c40e356f3a107403592ea0407 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:56:19 +0200 Subject: [PATCH 13/19] WW-5540 chore(core): add since/forRemoval to @Deprecated annotations Co-Authored-By: Claude Opus 4.8 --- .../apache/struts2/text/AbstractLocalizedTextProvider.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 efe5687043..0b6a8d525e 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -568,7 +568,7 @@ protected String formatMessage(String rawPattern, Locale locale, ValueStack valu * ({@link #formatMessage(String, Locale, ValueStack, Object[])} over a raw lookup). Retained for * backward compatibility with descendant classes. */ - @Deprecated + @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) { @@ -727,7 +727,7 @@ protected String resolvePackageHierarchyRaw(Class startClazz, String textKey, * lazily per bundle probe, preserving the reload side effect that the previous getMessage-per-probe * walk provided. */ - @Deprecated + @Deprecated(since = "7.3.0", forRemoval = true) protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, ValueStack valueStack) { reloadBundles(valueStack != null ? valueStack.getContext() : null); From 1bad305aca50b0b4bb592dc9a5c3e1f83e8e736d Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 13:56:19 +0200 Subject: [PATCH 14/19] WW-5540 docs: drop follow-up draft superseded by WW-5655 The ticket is filed; the draft's content now lives in WW-5655 itself. Co-Authored-By: Claude Opus 4.8 --- .../localized-text-provider-result-wrapper.md | 50 ------------------- 1 file changed, 50 deletions(-) delete mode 100644 docs/superpowers/followups/localized-text-provider-result-wrapper.md diff --git a/docs/superpowers/followups/localized-text-provider-result-wrapper.md b/docs/superpowers/followups/localized-text-provider-result-wrapper.md deleted file mode 100644 index 3d937ec3b9..0000000000 --- a/docs/superpowers/followups/localized-text-provider-result-wrapper.md +++ /dev/null @@ -1,50 +0,0 @@ -# WW-5655 — replace null-overloaded control flow in LocalizedTextProvider with an explicit result type - -> Filed as [WW-5655](https://issues.apache.org/jira/browse/WW-5655). - -## Type / Component -Improvement (code quality / maintainability) — Core. - -## Summary -Replace the `null`-overloaded control flow in `AbstractLocalizedTextProvider` / -`StrutsLocalizedTextProvider` message resolution with an explicit result type, so that -"not found" and "found but the value degraded to the literal `null`" are represented -distinctly instead of both collapsing to a Java `null` that callers must branch on. - -## Background -Message lookup uses `null` as an overloaded signal: - -- `formatWithNullDetection` returns `null` when a formatted message equals the literal string - `"null"` (e.g. a `{0}` pattern rendered with a `null` argument). -- Callers (`findText`'s tiers, the deprecated `findMessage`) treat that `null` identically to a - genuine "key not found" and fall through to the next source: `if (msg != null) return msg;`. - -Both cases legitimately mean "keep searching", so the overloading is not a bug — but it is a -readability/robustness wart. The codebase already has a wrapper for the analogous default-message -path: `GetDefaultMessageReturnArg { String message; boolean foundInBundle; }`. Extending a similar -explicit result to the hierarchy path would make the intent self-documenting and consistent. - -Context: this was surfaced while implementing WW-5540 (hierarchy-traversal caching). WW-5540 -deliberately kept the existing `null` convention and only added a cache-boundary marker -(`NOT_FOUND` sentinel + `isNotFound(...)`) plus a fall-through mitigation, to stay behavior-preserving -and focused. This ticket is the orthogonal control-flow cleanup that WW-5540 deferred. - -## Proposed change -Introduce an internal result type (e.g. `sealed`/enum-tagged: `Found(String value)` vs -`ContinueSearch`) used by the raw-resolution + formatting path, unwrapped to `String`/`null` at the -public `findText` boundary (the public method signatures return `String` and must not change). -Consider whether `formatWithNullDetection`, `getMessage`, `findDefaultText`, and `getDefaultMessage` -should adopt the same type for consistency, or whether the change should be scoped to the hierarchy -path only. - -## Scope / risk notes -- Behavior must remain identical (this is a refactor, not a behavior change). -- Touches long-standing framework internals many call sites branch on, plus the `@Deprecated` - `getMessage`/`findMessage` retained for descendant classes — review the ripple carefully. -- Purely a readability/maintainability gain; the branch itself does not disappear, it becomes - explicit (`result.isFound()` instead of `msg != null`). - -## Acceptance -- No public API signature changes. -- Existing `StrutsLocalizedTextProviderTest` (and related i18n tests) stay green with no behavior change. -- `null` is no longer used to mean "found but continue searching" anywhere in the resolution path. From e4166f87692f918363bcf8c82d5fee800caf8212 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 14:06:38 +0200 Subject: [PATCH 15/19] WW-5540 fix(core): address fresh-eyes review findings - Document that the deprecated getMessage/findMessage are no longer invoked by findText, and name formatMessage as the override point - Fall back to the ActionContext-based reloadBundles() when findText is called without a value stack, so the RELOADED flag is tracked and the caches can warm on that path in reload/devMode - Narrow resolveClassHierarchyRaw/resolvePackageHierarchyRaw to package-private (the cache key omits indexedKey, which is safe only when derived from textKey as the internal call sites do) - Suppress java:S2129 on the NOT_FOUND identity sentinel Co-Authored-By: Claude Opus 4.8 --- .../text/AbstractLocalizedTextProvider.java | 21 ++++++++++++++----- .../text/StrutsLocalizedTextProvider.java | 10 +++++++-- 2 files changed, 24 insertions(+), 7 deletions(-) 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 0b6a8d525e..c5a98e1ed5 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,7 @@ 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<>(); @@ -566,7 +567,10 @@ protected String formatMessage(String rawPattern, Locale locale, ValueStack valu * @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. + * 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) { @@ -649,7 +653,7 @@ private String findMessageRaw(Class clazz, String key, String indexedKey, Loc * 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) { + 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) { @@ -700,7 +704,7 @@ private String findPackageMessageRaw(Class startClazz, String textKey, String * {@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) { + 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) { @@ -719,7 +723,10 @@ protected String resolvePackageHierarchyRaw(Class startClazz, String textKey, * @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 + * 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"}. @@ -730,7 +737,11 @@ protected String resolvePackageHierarchyRaw(Class startClazz, String textKey, @Deprecated(since = "7.3.0", forRemoval = true) protected String findMessage(Class clazz, String key, String indexedKey, Locale locale, Object[] args, Set checked, ValueStack valueStack) { - reloadBundles(valueStack != null ? valueStack.getContext() : null); + 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; } 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 9c1d36ec56..83410f12a0 100644 --- a/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java @@ -67,8 +67,14 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin } // 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); + // 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); From e19fb489e566bf8f03760b0135fa37202b7e25d4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 14:06:38 +0200 Subject: [PATCH 16/19] WW-5540 docs: strip stray NUL bytes from design spec Two literal NUL bytes in the sentinel example made git/GitHub treat the whole markdown file as binary and unreviewable in the PR UI; align the example with the shipped sentinel name. Co-Authored-By: Claude Opus 4.8 --- ...-localized-text-provider-caching-design.md | Bin 13776 -> 13786 bytes 1 file changed, 0 insertions(+), 0 deletions(-) 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 index 5a00c88bc42bdefe39bdede131f1c52c288c2aab..9b01de70a07bbb6e90e21da6e1ff306a9086e6d4 100644 GIT binary patch delta 38 tcmcbReJgu|te9kcd~irmXh?8;h-*Yhyq|wayqkZhpG$oFW*xC5@&Fhq4Rrtj delta 28 jcmcbWeIa{;tQbE-e7v83NW7bWsGm!GJi}%)u_f{Vmg@<9 From c3dac52eacc89d319502f42f85021445b8ba08d9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 14:15:33 +0200 Subject: [PATCH 17/19] WW-5540 test(core): cover ModelDriven tier, per-locale keys, indexed keys Close the review-noted coverage gaps: the ModelDriven tier resolves via the shared class-hierarchy cache (action miss + model hit), each locale gets its own cache entry backed by a new _de fixture bundle, and indexed keys (name[N] -> name[*]) resolve and cache per full textKey. Co-Authored-By: Claude Opus 4.8 --- .../text/StrutsLocalizedTextProviderTest.java | 59 +++++++++++++++++++ .../struts2/text/CacheFixture.properties | 1 + .../struts2/text/CacheFixture_de.properties | 19 ++++++ 3 files changed, 79 insertions(+) create mode 100644 core/src/test/resources/org/apache/struts2/text/CacheFixture_de.properties 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 e4dc0c8319..19a2dcc497 100644 --- a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java @@ -689,6 +689,65 @@ public void testDeprecatedFindMessageStillDelegates() { 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(); diff --git a/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties index 5593df2f66..7d19337b3a 100644 --- a/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties +++ b/core/src/test/resources/org/apache/struts2/text/CacheFixture.properties @@ -20,3 +20,4 @@ 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 From b04e0f4166f4c67fed8731568158174a09e59429 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 14:19:55 +0200 Subject: [PATCH 18/19] WW-5540 fix(core): address Copilot review comments - Partition the caches by System.identityHashCode of the context classloader so a custom ClassLoader overriding hashCode() cannot collide or collapse the per-loader partitions - Derive the indexed key inside the resolvers (miss-only) instead of accepting it as a parameter, so the cache key trivially covers every input that influences the resolution result Co-Authored-By: Claude Opus 4.8 --- .../text/AbstractLocalizedTextProvider.java | 16 +++++++++++----- .../text/StrutsLocalizedTextProvider.java | 6 +++--- 2 files changed, 14 insertions(+), 8 deletions(-) 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 c5a98e1ed5..c553e94fa3 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -95,7 +95,9 @@ protected ClassLoader getCurrentThreadContextClassLoader() { } private int currentLoaderHashCode() { - return getCurrentThreadContextClassLoader().hashCode(); + // 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. */ @@ -653,13 +655,15 @@ private String findMessageRaw(Class clazz, String key, String indexedKey, Loc * 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, String indexedKey, Locale locale) { + 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; } - String raw = findMessageRaw(clazz, textKey, indexedKey, locale, null); + // 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; @@ -704,13 +708,15 @@ private String findPackageMessageRaw(Class startClazz, String textKey, String * {@link #NOT_FOUND} when absent. Same keying and get + putIfAbsent discipline as * {@link #resolveClassHierarchyRaw}. */ - String resolvePackageHierarchyRaw(Class startClazz, String textKey, String indexedTextName, Locale locale) { + 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; } - String raw = findPackageMessageRaw(startClazz, textKey, indexedTextName, locale); + // 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; 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 83410f12a0..71340ffe82 100644 --- a/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java @@ -93,7 +93,7 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin } // search up class hierarchy (cached raw resolution; format per call) - String classHierarchyRaw = resolveClassHierarchyRaw(startClazz, textKey, indexedTextName, locale); + String classHierarchyRaw = resolveClassHierarchyRaw(startClazz, textKey, locale); String msg = null; if (!isNotFound(classHierarchyRaw)) { msg = formatMessage(classHierarchyRaw, locale, valueStack, args); @@ -113,7 +113,7 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin if (action instanceof ModelDriven) { Object model = ((ModelDriven) action).getModel(); if (model != null) { - String modelRaw = resolveClassHierarchyRaw(model.getClass(), textKey, indexedTextName, locale); + String modelRaw = resolveClassHierarchyRaw(model.getClass(), textKey, locale); if (!isNotFound(modelRaw)) { msg = formatMessage(modelRaw, locale, valueStack, args); if (msg != null) { @@ -126,7 +126,7 @@ public String findText(Class startClazz, String textKey, Locale locale, Strin } // search the package hierarchy (cached raw resolution; format per call) - String packageRaw = resolvePackageHierarchyRaw(startClazz, textKey, indexedTextName, locale); + String packageRaw = resolvePackageHierarchyRaw(startClazz, textKey, locale); if (!isNotFound(packageRaw)) { msg = formatMessage(packageRaw, locale, valueStack, args); if (msg != null) { From feb2f38731de6efaa642bff578fdaa4ba4542963 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Thu, 23 Jul 2026 14:30:11 +0200 Subject: [PATCH 19/19] WW-5540 fix(core): resolve SonarCloud quality-gate findings - Suppress S4973 on isNotFound: the identity comparison against the non-interned NOT_FOUND sentinel is the design, not a bug - Reduce findMessageRaw cognitive complexity (S3776) by extracting getRawMessageWithAlternate, reused by the package walk - Add missing @Override annotations and suppress the deliberate deprecated-delegator call in the test helper (S1161, S5738) Co-Authored-By: Claude Opus 4.8 --- .../text/AbstractLocalizedTextProvider.java | 49 ++++++++----------- .../text/StrutsLocalizedTextProviderTest.java | 3 ++ 2 files changed, 23 insertions(+), 29 deletions(-) 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 c553e94fa3..d0bc09d85e 100644 --- a/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java +++ b/core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java @@ -605,50 +605,46 @@ private String findMessageRaw(Class clazz, String key, String indexedKey, Loc } // look in properties of this class - String msg = getRawMessage(clazz.getName(), locale, key); + String msg = getRawMessageWithAlternate(clazz.getName(), locale, key, indexedKey); 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); + for (Class anInterface : clazz.getInterfaces()) { + msg = getRawMessageWithAlternate(anInterface.getName(), locale, key, indexedKey); 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) { + 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); - } + } else if (!clazz.equals(Object.class) && !clazz.isPrimitive()) { + return findMessageRaw(clazz.getSuperclass(), key, indexedKey, locale, checked); } return null; } + /** + * 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 @@ -670,6 +666,7 @@ String resolveClassHierarchyRaw(Class clazz, String textKey, Locale locale) { } /** @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; } @@ -688,16 +685,10 @@ private String findPackageMessageRaw(Class startClazz, String textKey, String while (basePackageName.lastIndexOf('.') != -1) { basePackageName = basePackageName.substring(0, basePackageName.lastIndexOf('.')); String packageName = basePackageName + ".package"; - String msg = getRawMessage(packageName, locale, textKey); + String msg = getRawMessageWithAlternate(packageName, locale, textKey, indexedTextName); if (msg != null) { return msg; } - if (indexedTextName != null) { - msg = getRawMessage(packageName, locale, indexedTextName); - if (msg != null) { - return msg; - } - } } } return null; 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 19a2dcc497..d7157c5520 100644 --- a/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java +++ b/core/src/test/java/org/apache/struts2/text/StrutsLocalizedTextProviderTest.java @@ -818,14 +818,17 @@ public boolean getBundlesReloadedIndicatorValue() { 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); }