[
https://issues.apache.org/jira/browse/WW-5819?focusedWorklogId=1050241&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1050241
]
ASF GitHub Bot logged work on WW-5819:
--------------------------------------
Author: ASF GitHub Bot
Created on: 26/Oct/26 12:14
Start Date: 26/Oct/26 12:14
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2062:
URL: https://github.com/apache/struts/pull/2062#discussion_r4377067163
##########
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:
`org.apache.logging.log4j.Level` is not an enum, so comparing with `==`
relies on reference identity and can silently fail. Use `.equals(...)` (or
compare `intLevel()` values) to ensure level filtering works reliably.
##########
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:
Using `Stream#toList()` requires JDK 16+. If this project/module still
targets JDK 8/11 for tests, this will not compile. Using
`collect(Collectors.toList())` avoids coupling the test utility to newer JDKs.
##########
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:
`messages` and `levels` are maintained as two separate
`CopyOnWriteArrayList`s, but updates to them aren’t atomic as a pair. If
`messages(Level)` is called while another thread is appending,
`IntStream.range(0, messages.size())` can observe a larger `messages.size()`
than `levels.size()` and throw `IndexOutOfBoundsException`. Consider storing a
single combined record per event (e.g., a small DTO with `Level` + message) or
iterating up to `min(messages.size(), levels.size())`.
##########
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:
The log reports `className` passed into `buildValidator`, which may not
match the runtime validator class (e.g., if `ObjectFactory` returns a
subclass/proxy). Logging `validator.getClass().getName()` would make the
message more accurate and easier to correlate with the actual instance.
##########
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:
`messages` and `levels` are maintained as two separate
`CopyOnWriteArrayList`s, but updates to them aren’t atomic as a pair. If
`messages(Level)` is called while another thread is appending,
`IntStream.range(0, messages.size())` can observe a larger `messages.size()`
than `levels.size()` and throw `IndexOutOfBoundsException`. Consider storing a
single combined record per event (e.g., a small DTO with `Level` + message) or
iterating up to `min(messages.size(), levels.size())`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1050241)
Time Spent: 20m (was: 10m)
> 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: 20m
> 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)
