[
https://issues.apache.org/jira/browse/WW-5819?focusedWorklogId=1050274&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1050274
]
ASF GitHub Bot logged work on WW-5819:
--------------------------------------
Author: ASF GitHub Bot
Created on: 26/Oct/26 13:28
Start Date: 26/Oct/26 13:28
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #2062:
URL: https://github.com/apache/struts/pull/2062#discussion_r4377727285
##########
core/src/test/java/org/apache/struts2/test/LogCapture.java:
##########
@@ -36,13 +38,15 @@ public class LogCapture implements AutoCloseable {
private final Logger logger;
private final AbstractAppender appender;
private final List<String> messages = new CopyOnWriteArrayList<>();
+ private final List<Level> levels = new CopyOnWriteArrayList<>();
public LogCapture(Class<?> source) {
logger = (Logger) LogManager.getLogger(source);
appender = new AbstractAppender("LogCapture-" + System.nanoTime(),
null, null, false, Property.EMPTY_ARRAY) {
@Override
public void append(LogEvent event) {
String message = event.getMessage().getFormattedMessage();
+ levels.add(event.getLevel());
messages.add(event.getThrown() == null ? message : message + "
" + event.getThrown());
Review Comment:
Changed to one list that stores each message with its level (517bdace2). The
`IndexOutOfBoundsException` can't actually happen: the level is added before
the message, so a reader never sees more messages than levels. But one list is
simpler.
##########
core/src/test/java/org/apache/struts2/test/LogCapture.java:
##########
@@ -54,6 +58,13 @@ public List<String> messages() {
return messages;
}
+ public List<String> messages(Level level) {
+ return IntStream.range(0, messages.size())
Review Comment:
Same fix as the thread above: one list of entries in 517bdace2.
##########
core/src/test/java/org/apache/struts2/test/LogCapture.java:
##########
@@ -54,6 +58,13 @@ public List<String> messages() {
return messages;
}
+ public List<String> messages(Level level) {
+ return IntStream.range(0, messages.size())
+ .filter(i -> levels.get(i) == level)
Review Comment:
`Level.equals` in log4j-api is `obj instanceof Level && obj == this`, so
`==` behaves the same as `equals`.
##########
core/src/main/java/org/apache/struts2/factory/DefaultValidatorFactory.java:
##########
@@ -43,10 +55,37 @@ public void setReflectionProvider(ReflectionProvider
reflectionProvider) {
this.reflectionProvider = reflectionProvider;
}
+ @Inject(value = StrutsConstants.STRUTS_DEVMODE, required = false)
+ public void setDevMode(String devMode) {
+ this.devMode = BooleanUtils.toBoolean(devMode);
+ }
+
public Validator buildValidator(String className, Map<String, Object>
params, Map<String, Object> extraContext) throws Exception {
Validator validator = (Validator) objectFactory.buildBean(className,
extraContext);
- reflectionProvider.setProperties(params, validator, extraContext);
+ if (params != null) {
+ params.forEach((name, value) -> setParam(className, validator,
name, value, extraContext));
+ }
return validator;
}
+
+ private void setParam(String className, Validator validator, String name,
Object value, Map<String, Object> extraContext) {
+ try {
+ reflectionProvider.setProperty(name, value, validator,
extraContext, true);
+ } catch (ReflectionException e) {
+ if (hasWritableProperty(validator, name)) {
+ LOG.log(devMode ? Level.ERROR : Level.WARN, "Validator [{}]
ignores param [{}]: value [{}] cannot be converted ({})",
+ className, name, value,
ExceptionUtils.getRootCauseMessage(e));
Review Comment:
Keeping the configured `className` on purpose. It's the name the developer
wrote in their config. If the object factory returns a CGLIB proxy, the runtime
class name is mangled and harder to match back to the config.
##########
core/src/test/java/org/apache/struts2/test/LogCapture.java:
##########
@@ -54,6 +58,13 @@ public List<String> messages() {
return messages;
}
+ public List<String> messages(Level level) {
+ return IntStream.range(0, messages.size())
+ .filter(i -> levels.get(i) == level)
+ .mapToObj(messages::get)
+ .toList();
Review Comment:
The build targets Java 17 (`maven.compiler.release`), and `core/src/main`
already uses `Stream#toList()`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1050274)
Time Spent: 40m (was: 0.5h)
> Log a validator param that fails type conversion instead of ignoring it
> silently
> --------------------------------------------------------------------------------
>
> Key: WW-5819
> URL: https://issues.apache.org/jira/browse/WW-5819
> Project: Struts 2
> Issue Type: Improvement
> Components: XML Validators
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> h2. Problem
> {{DefaultValidatorFactory}} builds each validator through
> {{ObjectFactory.buildValidator}}, which sets the configured params with type
> conversion errors swallowed. A param that cannot be converted is left unset,
> and nothing is logged. For a range bound, that turns the check off without
> any trace:
> {code:xml}<field-validator type="localdate">
> <param name="min">01/02/2026</param> <!-- not an ISO date: min stays
> null -->
> </field-validator>
> {code}
> h2. Proposal
> Keep the current behaviour (the validator is still built and the param is
> still left unset), but log the swallowed conversion error. The message names
> the validator type, the param, the value and the cause:
> * WARN by default
> * ERROR in devMode
> h2. Scope
> A logging-only change with no behaviour change, so it is safe for 7.5.0.
> Failing the configuration instead is WW-5818 (8.0.0).
> h2. Tests
> * an unconvertible param on an XML-configured validator produces the log
> entry at the expected level, and the validator is still built
> * a valid param produces no entry
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
