(struts) 03/10: WW-5540 docs: deprecate+delegate findMessage/getMessage in plan

[lu...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

lukaszlenart pushed a commit to branch WW-5540-localized-text-provider-caching
in repository https://gitbox.apache.org/repos/asf/struts.git

commit 96e08e91916b7053403fdc22c6dc82833fa7772f
Author: Lukasz Lenart <lu...@apache.org>
AuthorDate: Thu Jul 23 12:54:29 2026 +0200

    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 <no...@anthropic.com>
---
 ...7-23-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 c139bd4d6..f6fbc0bcd 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<String> 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<String> 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 <no...@anthropic.com>"
 ```
@@ -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**