mirror of
https://github.com/apache/struts.git
synced 2026-08-05 14:47:09 +00:00
WW-3530 Fix visitor-validator cache-key collision under wildcard actions (#1811)
* WW-3530 docs: add design spec for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: add implementation plan for visitor-validator cache-key fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): cover visitor-validator cache-key context handling under wildcard actions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 fix(core): keep visitor-validator context in cache key under wildcard actions Apply the wildcard config-name substitution only when validating the action's own class. Visited objects carry a stable, explicit visitor context that must remain part of the cache key, otherwise two visitor validators on one field with different contexts collide and the second is silently dropped. Fixes https://issues.apache.org/jira/browse/WW-3530 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: document <s:form> render-path caching limitation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 docs: correct WW-2996 scope claim and note default-context visitor limitation Final-review finding: default-context visitor validators under wildcard actions key on the volatile resolved action name for the visited class, reintroducing bounded WW-2996-style cache growth (memory only; correct validators still load). Correct the 'WW-2996 untouched' wording to 'untouched for the action's own class', document the subpath as an accepted limitation folded into the follow-up ticket, and clarify that end-to-end visitor execution is covered by existing visitor suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 chore(core): add DEBUG logging for validator cache-key branch decision Log the built key together with clazz, context, validatingActionClass, wildcard, and the action config name, so the wildcard-vs-visited-object branch taken in buildValidatorKey can be diagnosed at runtime. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-3530 test(core): use assertNotEquals for cache-key inequality; fix comment grammar Address SonarCloud S5785 (assertFalse+equals -> assertNotEquals) and a Copilot grammar nit in the WW-4536 comment. DEBUG logging kept as-is per author decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
+20
-4
@@ -23,6 +23,8 @@ import org.apache.struts2.ActionInvocation;
|
||||
import org.apache.struts2.ActionProxy;
|
||||
import org.apache.struts2.config.entities.ActionConfig;
|
||||
import org.apache.commons.lang3.StringUtils;
|
||||
import org.apache.logging.log4j.LogManager;
|
||||
import org.apache.logging.log4j.Logger;
|
||||
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
@@ -36,6 +38,8 @@ import java.util.List;
|
||||
*/
|
||||
public class AnnotationActionValidatorManager extends DefaultActionValidatorManager {
|
||||
|
||||
private static final Logger LOG = LogManager.getLogger(AnnotationActionValidatorManager.class);
|
||||
|
||||
@Override
|
||||
protected String buildValidatorKey(Class clazz, String context) {
|
||||
ActionInvocation invocation = ActionContext.getContext().getActionInvocation();
|
||||
@@ -48,20 +52,32 @@ public class AnnotationActionValidatorManager extends DefaultActionValidatorMana
|
||||
sb.append(config.getPackageName());
|
||||
sb.append("/");
|
||||
}
|
||||
|
||||
Object action = invocation.getAction();
|
||||
boolean validatingActionClass = action != null && clazz.equals(action.getClass());
|
||||
String configName = config.getName();
|
||||
boolean wildcard = configName.contains(ActionConfig.WILDCARD)
|
||||
|| (configName.contains("{") && configName.contains("}"));
|
||||
|
||||
// WW-2996: key needs to use the name of the action from the config file, instead of the url,
|
||||
// so wildcard actions will have the same validator
|
||||
// WW-3753: Using the config name instead of the context only for wildcard actions to keep the flexibility
|
||||
// provided by the original design (such as mapping different contexts to the same action and method if desired)
|
||||
// WW-4536: Using NamedVariablePatternMatcher allows defines actions with patterns enclosed with '{}'
|
||||
String configName = config.getName();
|
||||
if (configName.contains(ActionConfig.WILDCARD) || (configName.contains("{") && configName.contains("}"))) {
|
||||
// WW-4536: Using NamedVariablePatternMatcher allows defining actions with patterns enclosed with '{}'
|
||||
// WW-3530: the config-name substitution only makes sense for the action's own class; a visited object
|
||||
// (visitor validator) carries a stable, explicit context that must remain part of the key
|
||||
if (validatingActionClass && wildcard) {
|
||||
sb.append(configName);
|
||||
sb.append("|");
|
||||
sb.append(proxy.getMethod());
|
||||
} else {
|
||||
sb.append(context);
|
||||
}
|
||||
return sb.toString();
|
||||
|
||||
String validatorKey = sb.toString();
|
||||
LOG.debug("Built validator key [{}] for class [{}] and context [{}]: validatingActionClass={}, wildcard={}, action config [{}]",
|
||||
validatorKey, clazz.getName(), context, validatingActionClass, wildcard, configName);
|
||||
return validatorKey;
|
||||
}
|
||||
|
||||
@Override
|
||||
|
||||
+41
@@ -47,6 +47,8 @@ import org.easymock.EasyMock;
|
||||
|
||||
import java.util.List;
|
||||
|
||||
import static org.junit.Assert.assertNotEquals;
|
||||
|
||||
|
||||
|
||||
/**
|
||||
@@ -92,11 +94,50 @@ public class AnnotationActionValidatorManagerTest extends XWorkTestCase {
|
||||
super.tearDown();
|
||||
}
|
||||
|
||||
private void installInvocation(String configName, Object action) {
|
||||
ActionConfig config = new ActionConfig.Builder("packageName", configName, "").build();
|
||||
ActionInvocation invocation = EasyMock.createNiceMock(ActionInvocation.class);
|
||||
ActionProxy proxy = EasyMock.createNiceMock(ActionProxy.class);
|
||||
|
||||
EasyMock.expect(invocation.getProxy()).andReturn(proxy).anyTimes();
|
||||
EasyMock.expect(invocation.getAction()).andReturn(action).anyTimes();
|
||||
EasyMock.expect(proxy.getMethod()).andReturn("execute").anyTimes();
|
||||
EasyMock.expect(proxy.getConfig()).andReturn(config).anyTimes();
|
||||
|
||||
EasyMock.replay(invocation, proxy);
|
||||
ActionContext.getContext().withActionInvocation(invocation);
|
||||
}
|
||||
|
||||
public void testBuildValidatorKey() {
|
||||
String validatorKey = annotationActionValidatorManager.buildValidatorKey(SimpleAnnotationAction.class, "name");
|
||||
assertEquals(SimpleAnnotationAction.class.getName() + "/packageName/name", validatorKey);
|
||||
}
|
||||
|
||||
public void testBuildValidatorKeyKeepsContextForVisitedClassUnderWildcardAction() {
|
||||
// Action is wildcard-mapped; the validated class is a visited model object
|
||||
// (not the action class), so its explicit, stable context must be preserved.
|
||||
installInvocation("*", new SimpleAnnotationAction());
|
||||
|
||||
String basicKey = annotationActionValidatorManager.buildValidatorKey(AnnotatedTestBean.class, "basic");
|
||||
String additionalKey = annotationActionValidatorManager.buildValidatorKey(AnnotatedTestBean.class, "additional");
|
||||
|
||||
assertEquals(AnnotatedTestBean.class.getName() + "/packageName/basic", basicKey);
|
||||
assertEquals(AnnotatedTestBean.class.getName() + "/packageName/additional", additionalKey);
|
||||
assertNotEquals("visited-object keys must differ per context", additionalKey, basicKey);
|
||||
}
|
||||
|
||||
public void testBuildValidatorKeyIgnoresContextForActionClassUnderWildcardAction() {
|
||||
installInvocation("*", new SimpleAnnotationAction());
|
||||
|
||||
String key1 = annotationActionValidatorManager.buildValidatorKey(SimpleAnnotationAction.class, "foo");
|
||||
String key2 = annotationActionValidatorManager.buildValidatorKey(SimpleAnnotationAction.class, "bar");
|
||||
|
||||
// WW-2996: a wildcard action's own validators share one cache entry across
|
||||
// all resolved names, so the volatile context must NOT be part of the key.
|
||||
assertEquals(key1, key2);
|
||||
assertEquals(SimpleAnnotationAction.class.getName() + "/packageName/*|execute", key1);
|
||||
}
|
||||
|
||||
public void testBuildsValidatorsForAlias() {
|
||||
List validatorList = annotationActionValidatorManager.getValidators(SimpleAnnotationAction.class, alias);
|
||||
|
||||
|
||||
@@ -0,0 +1,224 @@
|
||||
# WW-3530 Visitor-Validator Cache-Key Fix 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:** Stop `AnnotationActionValidatorManager` from collapsing distinct visitor-validator `context` values into one cache key under wildcard actions, so multiple visitor validators on the same field run again.
|
||||
|
||||
**Architecture:** Narrow the wildcard config-name substitution in `buildValidatorKey` so it fires only when the class being validated is the action's own class. Visited objects (whose `context` is a stable, explicit visitor param) always key on `context`, exactly like `DefaultActionValidatorManager`.
|
||||
|
||||
**Tech Stack:** Java, Maven, JUnit 3-style `XWorkTestCase` (extends `junit.framework.TestCase`), EasyMock.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Target version: **7.3.0** (SNAPSHOT is a placeholder; real version chosen at release).
|
||||
- Commit messages MUST be prefixed with the Jira ticket: `WW-3530 <type>: ...`.
|
||||
- Only `core` module is touched. Build/test with `mvn test -DskipAssembly -pl core`.
|
||||
- `core` tests are JUnit 3/4 — they extend `XWorkTestCase` and use `testXxx` method names with `junit.framework` assertions. An `@Test` annotation would silently never run. Do NOT convert to JUnit 5.
|
||||
- `ActionConfig.WILDCARD` == `"*"`. Named-pattern detection also matches names containing both `{` and `}`.
|
||||
- Change is behavior-preserving for: non-wildcard actions, and wildcard actions validating their own class (WW-2996 caching must stay intact).
|
||||
|
||||
---
|
||||
|
||||
## Task 1: Add failing tests for the cache-key logic
|
||||
|
||||
**Files:**
|
||||
- Test: `core/src/test/java/org/apache/struts2/validator/AnnotationActionValidatorManagerTest.java`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `AnnotationActionValidatorManager.buildValidatorKey(Class clazz, String context)` (protected, same package as the test) — returns the cache key `String`.
|
||||
- Consumes: existing test classes already imported in the file — `SimpleAnnotationAction` (used as the action class) and `AnnotatedTestBean` (used as a visited/foreign class).
|
||||
- Produces: a private helper `installInvocation(String configName, Object action)` used by the new tests.
|
||||
|
||||
- [ ] **Step 1: Add the mock-installer helper**
|
||||
|
||||
Add this private method to `AnnotationActionValidatorManagerTest` (place it just after `tearDown`, before `testBuildValidatorKey`). It installs an `ActionInvocation` whose proxy/config drive `buildValidatorKey`, overriding the one from `setUp`:
|
||||
|
||||
```java
|
||||
private void installInvocation(String configName, Object action) {
|
||||
ActionConfig config = new ActionConfig.Builder("packageName", configName, "").build();
|
||||
ActionInvocation invocation = EasyMock.createNiceMock(ActionInvocation.class);
|
||||
ActionProxy proxy = EasyMock.createNiceMock(ActionProxy.class);
|
||||
|
||||
EasyMock.expect(invocation.getProxy()).andReturn(proxy).anyTimes();
|
||||
EasyMock.expect(invocation.getAction()).andReturn(action).anyTimes();
|
||||
EasyMock.expect(proxy.getMethod()).andReturn("execute").anyTimes();
|
||||
EasyMock.expect(proxy.getConfig()).andReturn(config).anyTimes();
|
||||
|
||||
EasyMock.replay(invocation, proxy);
|
||||
ActionContext.getContext().withActionInvocation(invocation);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add the WW-3530 reproduction test (visited class under a wildcard action)**
|
||||
|
||||
Add this test method to the class. Under a wildcard action, a *visited* object's key must include its stable context so `basic` and `additional` don't collide:
|
||||
|
||||
```java
|
||||
public void testBuildValidatorKeyKeepsContextForVisitedClassUnderWildcardAction() {
|
||||
// Action is wildcard-mapped; the validated class is a visited model object
|
||||
// (not the action class), so its explicit, stable context must be preserved.
|
||||
installInvocation("*", new SimpleAnnotationAction());
|
||||
|
||||
String basicKey = annotationActionValidatorManager.buildValidatorKey(AnnotatedTestBean.class, "basic");
|
||||
String additionalKey = annotationActionValidatorManager.buildValidatorKey(AnnotatedTestBean.class, "additional");
|
||||
|
||||
assertEquals(AnnotatedTestBean.class.getName() + "/packageName/basic", basicKey);
|
||||
assertEquals(AnnotatedTestBean.class.getName() + "/packageName/additional", additionalKey);
|
||||
assertFalse("visited-object keys must differ per context", basicKey.equals(additionalKey));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Add the WW-2996 regression test (action's own class under a wildcard action)**
|
||||
|
||||
Add this test method. When the validated class IS the action class under a wildcard, the key must ignore context and use the stable config name — preserving WW-2996 behavior:
|
||||
|
||||
```java
|
||||
public void testBuildValidatorKeyIgnoresContextForActionClassUnderWildcardAction() {
|
||||
installInvocation("*", new SimpleAnnotationAction());
|
||||
|
||||
String key1 = annotationActionValidatorManager.buildValidatorKey(SimpleAnnotationAction.class, "foo");
|
||||
String key2 = annotationActionValidatorManager.buildValidatorKey(SimpleAnnotationAction.class, "bar");
|
||||
|
||||
// WW-2996: a wildcard action's own validators share one cache entry across
|
||||
// all resolved names, so the volatile context must NOT be part of the key.
|
||||
assertEquals(key1, key2);
|
||||
assertEquals(SimpleAnnotationAction.class.getName() + "/packageName/*|execute", key1);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the new tests to verify they FAIL**
|
||||
|
||||
Run:
|
||||
```bash
|
||||
mvn test -DskipAssembly -pl core -Dtest='AnnotationActionValidatorManagerTest#testBuildValidatorKeyKeepsContextForVisitedClassUnderWildcardAction+testBuildValidatorKeyIgnoresContextForActionClassUnderWildcardAction'
|
||||
```
|
||||
Expected: `testBuildValidatorKeyKeepsContextForVisitedClassUnderWildcardAction` FAILS — before the fix the visited-class key is `.../packageName/*|execute` for both contexts, so `basicKey` equals `additionalKey` and the `assertEquals(".../basic", ...)` assertion fails. `testBuildValidatorKeyIgnoresContextForActionClassUnderWildcardAction` PASSES already (documents the behavior to preserve).
|
||||
|
||||
- [ ] **Step 5: Commit the tests**
|
||||
|
||||
```bash
|
||||
git add core/src/test/java/org/apache/struts2/validator/AnnotationActionValidatorManagerTest.java
|
||||
git commit -m "WW-3530 test(core): cover visitor-validator cache-key context handling under wildcard actions
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: Narrow the wildcard substitution to the action class
|
||||
|
||||
**Files:**
|
||||
- Modify: `core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java:40-65`
|
||||
- Test: `core/src/test/java/org/apache/struts2/validator/AnnotationActionValidatorManagerTest.java` (from Task 1)
|
||||
|
||||
**Interfaces:**
|
||||
- Produces: unchanged signature `protected String buildValidatorKey(Class clazz, String context)`. New behavior: config-name substitution applies only when `clazz.equals(invocation.getAction().getClass())`.
|
||||
|
||||
- [ ] **Step 1: Replace the `buildValidatorKey` method body**
|
||||
|
||||
Replace the whole method (currently lines 40-65) with:
|
||||
|
||||
```java
|
||||
@Override
|
||||
protected String buildValidatorKey(Class clazz, String context) {
|
||||
ActionInvocation invocation = ActionContext.getContext().getActionInvocation();
|
||||
ActionProxy proxy = invocation.getProxy();
|
||||
ActionConfig config = proxy.getConfig();
|
||||
|
||||
StringBuilder sb = new StringBuilder(clazz.getName());
|
||||
sb.append("/");
|
||||
if (StringUtils.isNotBlank(config.getPackageName())) {
|
||||
sb.append(config.getPackageName());
|
||||
sb.append("/");
|
||||
}
|
||||
|
||||
Object action = invocation.getAction();
|
||||
boolean validatingActionClass = action != null && clazz.equals(action.getClass());
|
||||
String configName = config.getName();
|
||||
boolean wildcard = configName.contains(ActionConfig.WILDCARD)
|
||||
|| (configName.contains("{") && configName.contains("}"));
|
||||
|
||||
// WW-2996: key needs to use the name of the action from the config file, instead of the url,
|
||||
// so wildcard actions will have the same validator
|
||||
// WW-3753: Using the config name instead of the context only for wildcard actions to keep the flexibility
|
||||
// provided by the original design (such as mapping different contexts to the same action and method if desired)
|
||||
// WW-4536: Using NamedVariablePatternMatcher allows defines actions with patterns enclosed with '{}'
|
||||
// WW-3530: the config-name substitution only makes sense for the action's own class; a visited object
|
||||
// (visitor validator) carries a stable, explicit context that must remain part of the key
|
||||
if (validatingActionClass && wildcard) {
|
||||
sb.append(configName);
|
||||
sb.append("|");
|
||||
sb.append(proxy.getMethod());
|
||||
} else {
|
||||
sb.append(context);
|
||||
}
|
||||
return sb.toString();
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the new tests to verify they PASS**
|
||||
|
||||
Run:
|
||||
```bash
|
||||
mvn test -DskipAssembly -pl core -Dtest='AnnotationActionValidatorManagerTest#testBuildValidatorKeyKeepsContextForVisitedClassUnderWildcardAction+testBuildValidatorKeyIgnoresContextForActionClassUnderWildcardAction'
|
||||
```
|
||||
Expected: BUILD SUCCESS, both tests PASS.
|
||||
|
||||
- [ ] **Step 3: Run the full validator test class for regressions**
|
||||
|
||||
Run:
|
||||
```bash
|
||||
mvn test -DskipAssembly -pl core -Dtest='AnnotationActionValidatorManagerTest'
|
||||
```
|
||||
Expected: BUILD SUCCESS. In particular `testBuildValidatorKey` still passes (non-wildcard config `name`, null action → context branch → `<class>/packageName/name`).
|
||||
|
||||
- [ ] **Step 4: Run the visitor-validator regression suites**
|
||||
|
||||
Run:
|
||||
```bash
|
||||
mvn test -DskipAssembly -pl core -Dtest='VisitorFieldValidatorTest+VisitorFieldValidatorModelTest+DefaultActionValidatorManagerTest'
|
||||
```
|
||||
Expected: BUILD SUCCESS. Confirms non-wildcard visitor behavior and the `no-annotations` manager are unaffected.
|
||||
|
||||
- [ ] **Step 5: Commit the fix**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java
|
||||
git commit -m "WW-3530 fix(core): keep visitor-validator context in cache key under wildcard actions
|
||||
|
||||
Apply the wildcard config-name substitution only when validating the action's
|
||||
own class. Visited objects carry a stable, explicit visitor context that must
|
||||
remain part of the cache key, otherwise two visitor validators on one field with
|
||||
different contexts collide and the second is silently dropped.
|
||||
|
||||
Fixes https://issues.apache.org/jira/browse/WW-3530
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: Full core test run
|
||||
|
||||
**Files:** none (verification only).
|
||||
|
||||
- [ ] **Step 1: Run the full core module test suite**
|
||||
|
||||
Run:
|
||||
```bash
|
||||
mvn test -DskipAssembly -pl core
|
||||
```
|
||||
Expected: BUILD SUCCESS with no new failures. This guards against any other caller of `buildValidatorKey` / `getValidators` relying on the old wildcard behavior for foreign classes (e.g. the `Form` component's client-side validation path).
|
||||
|
||||
- [ ] **Step 2: If green, no commit needed**
|
||||
|
||||
Verification-only task; nothing to commit if the suite passes.
|
||||
|
||||
---
|
||||
|
||||
## Self-Review Notes
|
||||
|
||||
- **Test scope note:** Tasks 1-2 verify the fix at the `buildValidatorKey` unit level (distinct cache keys per context), a deterministic proxy for spec test #1. End-to-end "both visitor validators actually execute" is covered by the existing `VisitorFieldValidatorTest` / `VisitorFieldValidatorModelTest` suites (run as regression in Task 2 Step 4), not by a new integration test here.
|
||||
- **Spec coverage:** Root-cause narrowing → Task 2. Reproduction test (distinct cache entries per context for visited objects, spec test #1) → Task 1 Step 2. WW-2996 regression guard (spec test #2) → Task 1 Step 3. Non-wildcard visitor + `DefaultActionValidatorManager` unchanged (spec test #3) → Task 2 Step 4. Null-action guard (spec edge case) → `action != null` in Task 2 Step 1. Self-visiting wildcard limitation is intentionally not exercised (accepted known limitation per spec).
|
||||
- **Placeholder scan:** none.
|
||||
- **Type consistency:** `installInvocation(String, Object)`, `buildValidatorKey(Class, String)`, and `configName`/`wildcard`/`validatingActionClass` locals are consistent across tasks.
|
||||
@@ -0,0 +1,164 @@
|
||||
# WW-3530: Fix visitor-validator cache-key collision under wildcard actions
|
||||
|
||||
- **Jira:** [WW-3530](https://issues.apache.org/jira/browse/WW-3530)
|
||||
- **Component:** XML Validators
|
||||
- **Target version:** 7.3.0
|
||||
- **Date:** 2026-07-25
|
||||
|
||||
## Problem
|
||||
|
||||
When two visitor field validators are declared on the same field with different
|
||||
`context` params (e.g. `basic` and `additional`) and they visit an object of the
|
||||
same class, the second validator is silently ignored: both produce the same
|
||||
validator-cache key, so the first context's validators are returned for both and
|
||||
run twice.
|
||||
|
||||
Originally reported in 2010 (v2.2.1) when the cache key omitted `context`
|
||||
entirely. Intervening changes (WW-2996, WW-3753, WW-4536) reworked the key so
|
||||
`context` is now included for normal actions — which fixed the report for
|
||||
non-wildcard actions. The defect still reproduces for **wildcard / named-pattern
|
||||
actions**, where the key intentionally drops `context`.
|
||||
|
||||
### Root cause
|
||||
|
||||
`AnnotationActionValidatorManager.buildValidatorKey(Class clazz, String context)`
|
||||
(`core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java`)
|
||||
substitutes the action's config name + method for `context` whenever the current
|
||||
action is a wildcard/named-pattern action:
|
||||
|
||||
```java
|
||||
String configName = config.getName();
|
||||
if (configName.contains(ActionConfig.WILDCARD)
|
||||
|| (configName.contains("{") && configName.contains("}"))) {
|
||||
sb.append(configName).append("|").append(proxy.getMethod());
|
||||
} else {
|
||||
sb.append(context);
|
||||
}
|
||||
```
|
||||
|
||||
The word "context" has two meanings:
|
||||
|
||||
- **Action-level validation** (`ValidationInterceptor`): `clazz` is the action
|
||||
class and `context` is the action name — for a wildcard action this is derived
|
||||
from the URL and varies per request. Keying on it caused the WW-2996 memory
|
||||
leak (one cache entry per resolved URL). WW-3753 correctly substitutes the
|
||||
stable config name here.
|
||||
- **Visitor validation** (`VisitorFieldValidator`): `clazz` is the *visited
|
||||
object's* class (not the action class) and `context` is the visitor's explicit,
|
||||
hand-written `context` param — stable, and the only thing distinguishing
|
||||
`basic` from `additional`.
|
||||
|
||||
The bug is that the config-name substitution — designed for the action's own
|
||||
class under a wildcard — is applied **too broadly**, firing for visited objects
|
||||
too and wrongly discarding their stable `context`.
|
||||
|
||||
## Fix
|
||||
|
||||
Narrow the substitution to the case it was designed for: only swap in the config
|
||||
name when `clazz` is the action's own class. For any other class (a visited
|
||||
object), always key on `context`.
|
||||
|
||||
```java
|
||||
protected String buildValidatorKey(Class clazz, String context) {
|
||||
ActionInvocation invocation = ActionContext.getContext().getActionInvocation();
|
||||
ActionProxy proxy = invocation.getProxy();
|
||||
ActionConfig config = proxy.getConfig();
|
||||
|
||||
StringBuilder sb = new StringBuilder(clazz.getName());
|
||||
sb.append("/");
|
||||
if (StringUtils.isNotBlank(config.getPackageName())) {
|
||||
sb.append(config.getPackageName()).append("/");
|
||||
}
|
||||
|
||||
Object action = invocation.getAction();
|
||||
boolean validatingActionClass = action != null && clazz.equals(action.getClass());
|
||||
String configName = config.getName();
|
||||
boolean wildcard = configName.contains(ActionConfig.WILDCARD)
|
||||
|| (configName.contains("{") && configName.contains("}"));
|
||||
|
||||
if (validatingActionClass && wildcard) {
|
||||
// WW-2996/WW-3753/WW-4536: wildcard actions share validators across
|
||||
// resolved names; key on the stable config name, not the volatile context.
|
||||
sb.append(configName).append("|").append(proxy.getMethod());
|
||||
} else {
|
||||
// Normal actions AND all visited objects (WW-3530): context is stable.
|
||||
sb.append(context);
|
||||
}
|
||||
return sb.toString();
|
||||
}
|
||||
```
|
||||
|
||||
### Behavioral change
|
||||
|
||||
The only difference: when `clazz` is **not** the action class (a visited object
|
||||
under a visitor validator), the key now includes `context` even if the current
|
||||
action is a wildcard. `basic` and `additional` therefore get distinct cache
|
||||
entries and both run. The wildcard-action caching path (WW-2996) is untouched
|
||||
**for the action's own class**, because the `configName|method` branch now fires
|
||||
only when `clazz == action.getClass()` — the request-validation hot path
|
||||
(`ValidationInterceptor` → `getValidators(action.getClass(), …)`) is fully
|
||||
preserved. See the default-context visitor note below for the one subpath where
|
||||
keying moves from stable to volatile.
|
||||
|
||||
## Scope, edge cases, non-goals
|
||||
|
||||
- **`DefaultActionValidatorManager`** (the `no-annotations` bean) already keys on
|
||||
`clazz + "/" + context` unconditionally — no bug, no change.
|
||||
- **Null action guard:** if `invocation.getAction()` is null, fall through to the
|
||||
`context` branch (safe default).
|
||||
- **Self-visiting wildcard action** (an action that visits an object of its *own*
|
||||
class under a wildcard mapping, with two different contexts): still collides,
|
||||
because `clazz == action.getClass()` takes the wildcard branch. Accepted known
|
||||
limitation — extremely contrived, and resolving it would require reintroducing
|
||||
the volatile-vs-stable-context ambiguity this fix avoids. Documented, not fixed.
|
||||
- **Default-context visitor under a wildcard action** (visitor validator with no
|
||||
explicit `context` param): `VisitorFieldValidator` (`VisitorFieldValidator.java:141`)
|
||||
defaults `visitorContext` to the *resolved* action name when `context == null`.
|
||||
Under a wildcard action that name is volatile (per URL). Before this change the
|
||||
visited object keyed on the stable `configName|method`; after it, keying on
|
||||
`context` (= the volatile action name) creates one cache entry per resolved name
|
||||
for that visited class — WW-2996-style cache growth. This is a **caching-efficiency**
|
||||
regression only (correct validators still load; the growth is bounded by the
|
||||
number of resolved wildcard names, narrower than the original WW-2996 leak), and
|
||||
it disappears the moment the visitor validator declares an explicit `context`.
|
||||
Accepted for this change; folded into the same follow-up ticket as the render-path
|
||||
item (a clean fix needs the rejected "Approach C" — an explicit visitor signal
|
||||
through the manager API).
|
||||
- **`<s:form>` client-side JS-validation render path** (`Form.getValidators`,
|
||||
`Form.java:295`): this path resolves `actionClass` by *name* (via
|
||||
`ServletUrlRenderer`), not from the live action instance. When the form targets
|
||||
a different action than the one executing, or the live action is a Spring/CGLIB
|
||||
proxy, `clazz.equals(action.getClass())` is false for the action's own top-level
|
||||
lookup, so the key falls to `context` (= action name) instead of
|
||||
`configName|method`. Effect is **caching efficiency only** — extra cache entries
|
||||
per resolved action name on this render path; the correct validators still load,
|
||||
because `buildValidatorConfigs` receives `context` directly, independent of the
|
||||
key. The request-validation hot path (`ValidationInterceptor`) is unaffected:
|
||||
there `clazz` is the live action instance's own class, so WW-2996 caching is
|
||||
fully preserved. Accepted for this change; a follow-up ticket should refine the
|
||||
discriminator for the render path (a robust fix needs an explicit visitor signal
|
||||
threaded through the manager API — the rejected "Approach C").
|
||||
- **Non-goal:** no change to the `ActionValidatorManager` interface, to
|
||||
`VisitorFieldValidator`, or to `DefaultActionValidatorManager`.
|
||||
|
||||
## Testing
|
||||
|
||||
Tests live in
|
||||
`core/src/test/java/org/apache/struts2/validator/AnnotationActionValidatorManagerTest.java`
|
||||
(plus existing `VisitorFieldValidatorTest` / `VisitorFieldValidatorModelTest`
|
||||
for regression).
|
||||
|
||||
1. **Bug reproduction (fails before, passes after):** a wildcard action with two
|
||||
visitor validators on the same field, visiting the same class with different
|
||||
contexts → assert **both** contexts' validators execute.
|
||||
2. **WW-2996 regression guard:** a wildcard action's own validators still resolve
|
||||
to a single cache entry across two different resolved action names.
|
||||
3. **Existing coverage stays green:** non-wildcard visitor case and current
|
||||
wildcard action-level validation behavior unchanged.
|
||||
|
||||
## References
|
||||
|
||||
- WW-3530 — this issue
|
||||
- WW-2996 — memory leak from keying on volatile wildcard action names
|
||||
- WW-3753 — introduced config-name substitution for wildcard actions
|
||||
- WW-4536 — extended it to `NamedVariablePatternMatcher` (`{...}`) actions
|
||||
Reference in New Issue
Block a user