Skip to content

WW-3530 Fix visitor-validator cache-key collision under wildcard actions#1811

Open
lukaszlenart wants to merge 8 commits into
mainfrom
WW-3530-visitor-validator-cache-key
Open

WW-3530 Fix visitor-validator cache-key collision under wildcard actions#1811
lukaszlenart wants to merge 8 commits into
mainfrom
WW-3530-visitor-validator-cache-key

Conversation

@lukaszlenart

Copy link
Copy Markdown
Member

Fixes WW-3530

Problem

Two visitor field validators declared on the same field with different context
params (e.g. basic and additional) collide in the validator cache under
wildcard / named-pattern actions: both produce the same cache key, so the
first context's validators are returned for both and the second context's
validators are silently dropped.

Originally reported in 2010 (v2.2.1) when the cache key omitted context
entirely. WW-2996 / WW-3753 / WW-4536 later reworked the key so context is
included for normal actions — fixing the report for non-wildcard actions. The
defect still reproduces for wildcard actions, where the key intentionally
substitutes the action config name for context.

Root cause

AnnotationActionValidatorManager.buildValidatorKey substitutes the action's
config name + method for context whenever the current action is a
wildcard/named-pattern action. That substitution is designed for the action's
own class (where context is the volatile URL-derived action name that WW-2996
must not key on). But it was applied too broadly — it also fired when validating
a visited object under a visitor validator, whose context is a stable,
explicit param, and wrongly discarded it.

Fix

Narrow the substitution to the case it was designed for: apply the config-name
key only when the class being validated is the action's own class
(clazz.equals(invocation.getAction().getClass())). For any other class (a
visited object), key on context — matching DefaultActionValidatorManager.

The request-validation hot path (ValidationInterceptor
getValidators(action.getClass(), …)) always validates the live action
instance's own class, so WW-2996's single-cache-entry-per-wildcard-action
behavior is fully preserved. The cache-key change only affects cache reuse; it
never changes which validators load (buildValidatorConfigs receives context
directly).

Tests

  • New unit tests in AnnotationActionValidatorManagerTest:
    • visited class under a wildcard action → distinct keys per context (the
      failing-before / passing-after case)
    • action's own class under a wildcard action → key ignores context (WW-2996
      behavior pinned)
  • Regression: full AnnotationActionValidatorManagerTest (14/14) and
    VisitorFieldValidatorTest / VisitorFieldValidatorModelTest /
    DefaultActionValidatorManagerTest (23/23) pass.
  • Draft note: the full core module suite has not yet been run in this
    branch — to be completed before marking ready for review.

Known limitations (follow-up ticket to be filed)

Both need an explicit visitor signal threaded through the manager API to fix
cleanly, and are memory-only (correct validators still load):

  • Default-context visitor under a wildcard action: a visitor validator with
    no explicit context defaults to the resolved action name, which is volatile
    under a wildcard action — so the visited-class key moves from stable
    (configName|method) to volatile, causing bounded WW-2996-style cache growth.
    Disappears when the visitor declares an explicit context.
  • <s:form> client-side JS-validation render path: resolves the action
    class by name rather than from the live action instance, so for a
    cross-action or proxied form the action's own top-level lookup falls to the
    context branch under a wildcard action (extra cache entries, correctness
    unaffected).

Design spec and implementation plan are included under docs/superpowers/.

🤖 Generated with Claude Code

lukaszlenart and others added 7 commits July 25, 2026 09:05
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… fix

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g under wildcard actions

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…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>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sitor 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>
… 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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a long-standing validator cache-key collision affecting visitor field validators under wildcard/named-pattern actions by narrowing when wildcard config-name substitution is applied. This ensures distinct visitor context values don’t collapse into the same cached validator entry for visited model classes, while preserving the WW-2996 wildcard-action caching behavior for the action’s own class.

Changes:

  • Adjust AnnotationActionValidatorManager.buildValidatorKey() to only substitute configName|method for wildcard actions when validating the action’s own class.
  • Add unit tests that pin both the WW-3530 visited-class behavior and the WW-2996 action-class behavior under wildcard actions.
  • Add design/spec and implementation plan documentation under docs/superpowers/.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
docs/superpowers/specs/2026-07-25-WW-3530-visitor-validator-cache-key-design.md Design/spec documenting root cause, fix approach, and known limitations.
docs/superpowers/plans/2026-07-25-WW-3530-visitor-validator-cache-key.md Step-by-step implementation plan and test guidance for WW-3530.
core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java Narrow wildcard cache-key substitution to the action’s own class; adds debug logging (flagged in review).
core/src/test/java/org/apache/struts2/validator/AnnotationActionValidatorManagerTest.java Adds targeted unit tests for visited-class vs action-class key behavior under wildcard actions.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

… 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>
@lukaszlenart
lukaszlenart marked this pull request as ready for review July 25, 2026 10:30
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants