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”.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
