[
https://issues.apache.org/jira/browse/WW-3530?focusedWorklogId=1032156&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1032156
]
ASF GitHub Bot logged work on WW-3530:
--------------------------------------
Author: ASF GitHub Bot
Created on: 25/Jul/26 10:19
Start Date: 25/Jul/26 10:19
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1811:
URL: https://github.com/apache/struts/pull/1811#discussion_r3650019556
##########
core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java:
##########
@@ -48,20 +52,32 @@ protected String buildValidatorKey(Class clazz, String
context) {
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-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;
Review Comment:
The new per-call debug log in buildValidatorKey() is on a hot path and
includes `context`, which may be URL-derived for wildcard actions. This can
create noisy logs and potentially leak request-derived values when debug is
enabled. Consider removing the debug log and (if you still want this
visibility) logging at TRACE with a minimal message that omits `context`/the
full key.
##########
core/src/main/java/org/apache/struts2/validator/AnnotationActionValidatorManager.java:
##########
@@ -48,20 +52,32 @@ protected String buildValidatorKey(Class clazz, String
context) {
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 '{}'
Review Comment:
Grammar in the WW-4536 comment is off: “allows defines actions” should be
“allows defining actions”.
Issue Time Tracking
-------------------
Worklog Id: (was: 1032156)
Time Spent: 0.5h (was: 20m)
> Invalid cache key in AnnotationActionValidationManager.buildValidatorKey
> when using visitor field validators
> -------------------------------------------------------------------------------------------------------------
>
> Key: WW-3530
> URL: https://issues.apache.org/jira/browse/WW-3530
> Project: Struts 2
> Issue Type: Bug
> Components: XML Validators
> Affects Versions: 2.2.1
> Reporter: Johnny Yu
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Error scenerio:
> MyAction-validation.xml:
> <validators>
> <field name="myField"> <!-- MyClass -->
> <field-validator type="visitor">
> <param name="context">basic</param>
> <param name="appendPrefix">true</param>
> <message/>
> </field-validator>
> </field>
> <field name="myField">
> <field-validator type="visitor">
> <param name="context">additional</param>
> <param name="appendPrefix">true</param>
> <message/>
> </field-validator>
> </field>
> </validators>
> In this case, validators in MyClass-basic-validation.xml will be executed two
> times, but validators in MyClass-additional-validation.xml will just ignored.
> The problem is caused by
> AnnotationActionValidationManager.buildValidatorKey(Class) which returns the
> same cache key for the validator cache.
> The current cache key is created by:
> StringBuilder sb = new StringBuilder(clazz.getName());
> sb.append("/");
> sb.append(proxy.getConfig().getName());
> sb.append("|");
> sb.append(proxy.getMethod());
> The context is not a part of the cache key. Therefore, the two visitor
> validator will just get the same cache key.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
