mirror of
https://github.com/apache/struts.git
synced 2026-08-05 14:47:09 +00:00
WW-2934 Skip field validators when a field has a conversion error (opt-in) (#1810)
* WW-2934 docs: add design spec for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 docs: add implementation plan for skipping validators on conversion error Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 feat(core): skip field validators on conversion error behind opt-in flag Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): cover annotation manager + document conversion-error skip exemption Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * WW-2934 test(core): assert custom conversion message survives and cover nested field skip Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -424,6 +424,9 @@ public final class StrutsConstants {
|
||||
*/
|
||||
public static final String STRUTS_ACTIONVALIDATORMANAGER = "struts.actionValidatorManager";
|
||||
|
||||
/** @see org.apache.struts2.validator.DefaultActionValidatorManager */
|
||||
public static final String STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR = "struts.validators.skipValidatorsOnConversionError";
|
||||
|
||||
/**
|
||||
* The {@link org.apache.struts2.util.ValueStackFactory} implementation class
|
||||
*/
|
||||
|
||||
@@ -25,6 +25,7 @@ import org.apache.struts2.text.TextProviderFactory;
|
||||
import org.apache.struts2.inject.Inject;
|
||||
import org.apache.struts2.util.ClassLoaderUtil;
|
||||
import org.apache.struts2.util.ValueStack;
|
||||
import org.apache.struts2.validator.validators.ConversionErrorFieldValidator;
|
||||
import org.apache.logging.log4j.LogManager;
|
||||
import org.apache.logging.log4j.Logger;
|
||||
import org.apache.struts2.StrutsConstants;
|
||||
@@ -73,6 +74,7 @@ public class DefaultActionValidatorManager implements ActionValidatorManager {
|
||||
protected ValidatorFileParser validatorFileParser;
|
||||
protected FileManager fileManager;
|
||||
protected boolean reloadingConfigs;
|
||||
protected boolean skipValidatorsOnConversionError;
|
||||
protected TextProviderFactory textProviderFactory;
|
||||
|
||||
@Inject
|
||||
@@ -95,6 +97,18 @@ public class DefaultActionValidatorManager implements ActionValidatorManager {
|
||||
this.reloadingConfigs = Boolean.parseBoolean(reloadingConfigs);
|
||||
}
|
||||
|
||||
/**
|
||||
* When set to {@code true}, a field's remaining validators are skipped once that field has a
|
||||
* type-conversion error. The field's own {@link ConversionErrorFieldValidator} still runs (so its
|
||||
* message is still shown), and action-level validators are unaffected. Defaults to {@code false}.
|
||||
*
|
||||
* @param skipValidatorsOnConversionError whether to skip a field's remaining validators when it has a conversion error.
|
||||
*/
|
||||
@Inject(value = StrutsConstants.STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR, required = false)
|
||||
public void setSkipValidatorsOnConversionError(String skipValidatorsOnConversionError) {
|
||||
this.skipValidatorsOnConversionError = Boolean.parseBoolean(skipValidatorsOnConversionError);
|
||||
}
|
||||
|
||||
@Inject
|
||||
public void setTextProviderFactory(TextProviderFactory textProviderFactory) {
|
||||
this.textProviderFactory = textProviderFactory;
|
||||
@@ -183,6 +197,13 @@ public class DefaultActionValidatorManager implements ActionValidatorManager {
|
||||
LOG.debug("Short-circuited, skipping");
|
||||
continue;
|
||||
}
|
||||
|
||||
if (skipValidatorsOnConversionError
|
||||
&& !(validator instanceof ConversionErrorFieldValidator)
|
||||
&& ActionContext.getContext().getConversionErrors().containsKey(fullFieldName)) {
|
||||
LOG.debug("Skipping validator {} for field {} due to a conversion error", validator, fullFieldName);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
if (validator instanceof ShortCircuitableValidator && ((ShortCircuitableValidator) validator).isShortCircuit()) {
|
||||
|
||||
@@ -149,6 +149,12 @@ struts.mapper.action.prefix.crossNamespaces = false
|
||||
### them right away.
|
||||
struts.devMode = false
|
||||
|
||||
### When set to true, a field's remaining validators are skipped once that field
|
||||
### has a type conversion error, avoiding a duplicate error (WW-2934).
|
||||
### The field's own conversion validator still runs, so its message is still shown.
|
||||
### valid values are: true, false (false is the default)
|
||||
struts.validators.skipValidatorsOnConversionError = false
|
||||
|
||||
### when set to true, resource bundles will be reloaded on _every_ request.
|
||||
### this is good during development, but should never be used in production
|
||||
# struts.i18n.reload=false
|
||||
|
||||
@@ -0,0 +1,48 @@
|
||||
/*
|
||||
* Licensed to the Apache Software Foundation (ASF) under one
|
||||
* or more contributor license agreements. See the NOTICE file
|
||||
* distributed with this work for additional information
|
||||
* regarding copyright ownership. The ASF licenses this file
|
||||
* to you under the Apache License, Version 2.0 (the
|
||||
* "License"); you may not use this file except in compliance
|
||||
* with the License. You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing,
|
||||
* software distributed under the License is distributed on an
|
||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
* KIND, either express or implied. See the License for the
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
package org.apache.struts2.validator;
|
||||
|
||||
import org.apache.struts2.ActionSupport;
|
||||
|
||||
/**
|
||||
* Fixture for WW-2934: an Integer field ("age") that carries both a conversion
|
||||
* validator and a required validator, plus an unrelated required String field
|
||||
* ("name") and an action-level validator (see the matching -validation.xml).
|
||||
*/
|
||||
public class ConversionErrorSkipAction extends ActionSupport {
|
||||
|
||||
private Integer age;
|
||||
private String name;
|
||||
|
||||
public Integer getAge() {
|
||||
return age;
|
||||
}
|
||||
|
||||
public void setAge(Integer age) {
|
||||
this.age = age;
|
||||
}
|
||||
|
||||
public String getName() {
|
||||
return name;
|
||||
}
|
||||
|
||||
public void setName(String name) {
|
||||
this.name = name;
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,38 @@
|
||||
/*
|
||||
* Licensed to the Apache Software Foundation (ASF) under one
|
||||
* or more contributor license agreements. See the NOTICE file
|
||||
* distributed with this work for additional information
|
||||
* regarding copyright ownership. The ASF licenses this file
|
||||
* to you under the Apache License, Version 2.0 (the
|
||||
* "License"); you may not use this file except in compliance
|
||||
* with the License. You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing,
|
||||
* software distributed under the License is distributed on an
|
||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
* KIND, either express or implied. See the License for the
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
package org.apache.struts2.validator;
|
||||
|
||||
/**
|
||||
* Nested-bean fixture for WW-2934: an Integer field ("zip") carrying both a
|
||||
* conversion validator and a required validator (see the matching
|
||||
* -validation.xml). Used to exercise the conversion-error skip against a
|
||||
* prefixed full field name (e.g. "bean.zip").
|
||||
*/
|
||||
public class ConversionErrorSkipBean {
|
||||
|
||||
private Integer zip;
|
||||
|
||||
public Integer getZip() {
|
||||
return zip;
|
||||
}
|
||||
|
||||
public void setZip(Integer zip) {
|
||||
this.zip = zip;
|
||||
}
|
||||
}
|
||||
+120
@@ -18,12 +18,18 @@
|
||||
*/
|
||||
package org.apache.struts2.validator;
|
||||
|
||||
import org.apache.struts2.ActionContext;
|
||||
import org.apache.struts2.ActionInvocation;
|
||||
import org.apache.struts2.ActionProxy;
|
||||
import org.apache.struts2.FileManager;
|
||||
import org.apache.struts2.FileManagerFactory;
|
||||
import org.apache.struts2.SimpleAction;
|
||||
import org.apache.struts2.TestBean;
|
||||
import org.apache.struts2.ValidationOrderAction;
|
||||
import org.apache.struts2.ValidationAwareSupport;
|
||||
import org.apache.struts2.XWorkTestCase;
|
||||
import org.apache.struts2.config.entities.ActionConfig;
|
||||
import org.apache.struts2.conversion.impl.ConversionData;
|
||||
import org.apache.struts2.interceptor.ValidationAware;
|
||||
import org.apache.struts2.test.DataAware2;
|
||||
import org.apache.struts2.test.SimpleAction3;
|
||||
@@ -38,8 +44,10 @@ import org.apache.struts2.validator.validators.LongRangeFieldValidator;
|
||||
import org.apache.struts2.validator.validators.RequiredFieldValidator;
|
||||
import org.apache.struts2.validator.validators.RequiredStringValidator;
|
||||
import org.apache.struts2.validator.validators.ShortRangeFieldValidator;
|
||||
import org.apache.struts2.validator.validators.VisitorFieldValidator;
|
||||
import org.apache.struts2.StrutsException;
|
||||
import org.assertj.core.api.Assertions;
|
||||
import org.easymock.EasyMock;
|
||||
import org.xml.sax.SAXParseException;
|
||||
|
||||
import java.io.IOException;
|
||||
@@ -389,6 +397,118 @@ public class DefaultActionValidatorManagerTest extends XWorkTestCase {
|
||||
assertEquals((e.getValue()).get(0), "password hint is required");
|
||||
}
|
||||
|
||||
public void testConversionError_bothErrorsWhenFlagDisabledByDefault() throws Exception {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> ageErrors = action.getFieldErrors().get("age");
|
||||
assertNotNull(ageErrors);
|
||||
assertEquals(2, ageErrors.size()); // conversion + required, current behavior
|
||||
assertTrue(ageErrors.contains("Age must be a valid number"));
|
||||
assertTrue(ageErrors.contains("Age is required"));
|
||||
}
|
||||
|
||||
public void testConversionError_fieldValidatorsSkippedWhenEnabled() throws Exception {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> ageErrors = action.getFieldErrors().get("age");
|
||||
assertNotNull(ageErrors);
|
||||
// the conversion validator itself still runs and its custom message survives...
|
||||
assertEquals(1, ageErrors.size());
|
||||
assertEquals("Age must be a valid number", ageErrors.get(0));
|
||||
// ...while the required validator on the same field is skipped, not merely absent
|
||||
assertFalse(ageErrors.contains("Age is required"));
|
||||
}
|
||||
|
||||
public void testConversionError_nestedFieldValidatorsSkippedWhenEnabled() throws Exception {
|
||||
// A nested/visitor-validated field is keyed by its full (prefixed) name, e.g. "bean.zip".
|
||||
// The skip guard must match on that full field name, exempting only the conversion validator.
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
// The bean supplies the field values; a separate ValidationAware collects the errors,
|
||||
// exactly as the visitor validator wires things up when it recurses into a nested bean.
|
||||
ConversionErrorSkipBean bean = new ConversionErrorSkipBean();
|
||||
ValidationAware sink = new ValidationAwareSupport();
|
||||
ValidatorContext parent = new DelegatingValidatorContext(sink, actionValidatorManager.textProviderFactory);
|
||||
VisitorFieldValidator.AppendingValidatorContext nested =
|
||||
new VisitorFieldValidator.AppendingValidatorContext(parent, parent, "bean", "");
|
||||
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("bean.zip", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
|
||||
actionValidatorManager.validate(bean, null, nested);
|
||||
|
||||
List<String> zipErrors = sink.getFieldErrors().get("bean.zip");
|
||||
assertNotNull(zipErrors);
|
||||
// required is skipped on the prefixed field; the conversion validator still runs
|
||||
assertEquals(1, zipErrors.size());
|
||||
assertEquals("Zip must be a valid number", zipErrors.get(0));
|
||||
assertFalse(zipErrors.contains("Zip is required"));
|
||||
}
|
||||
|
||||
public void testConversionError_unrelatedFieldStillValidatedWhenEnabled() throws Exception {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> nameErrors = action.getFieldErrors().get("name");
|
||||
assertNotNull(nameErrors); // "name" has no conversion error, still validated
|
||||
assertEquals(1, nameErrors.size());
|
||||
assertEquals("Name is required", nameErrors.get(0));
|
||||
}
|
||||
|
||||
public void testConversionError_actionLevelValidatorUnaffectedWhenEnabled() throws Exception {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
assertTrue(action.hasActionErrors());
|
||||
assertTrue(action.getActionErrors().contains("Action level always fails"));
|
||||
}
|
||||
|
||||
public void testConversionError_skipFiresForAnnotationManager() throws Exception {
|
||||
// AnnotationActionValidatorManager.buildValidatorKey() needs an ActionInvocation/ActionProxy
|
||||
// on the ActionContext to resolve the package name/config for the validator cache key.
|
||||
ActionConfig config = new ActionConfig.Builder("packageName", "name", "").build();
|
||||
ActionInvocation invocation = EasyMock.createNiceMock(ActionInvocation.class);
|
||||
ActionProxy proxy = EasyMock.createNiceMock(ActionProxy.class);
|
||||
EasyMock.expect(invocation.getProxy()).andReturn(proxy).anyTimes();
|
||||
EasyMock.expect(proxy.getMethod()).andReturn("execute").anyTimes();
|
||||
EasyMock.expect(proxy.getConfig()).andReturn(config).anyTimes();
|
||||
EasyMock.replay(invocation);
|
||||
EasyMock.replay(proxy);
|
||||
ActionContext.getContext().withActionInvocation(invocation);
|
||||
|
||||
AnnotationActionValidatorManager annMgr = container.inject(AnnotationActionValidatorManager.class);
|
||||
annMgr.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
|
||||
annMgr.validate(action, null);
|
||||
|
||||
List<String> ageErrors = action.getFieldErrors().get("age");
|
||||
assertNotNull(ageErrors);
|
||||
// required is skipped; the conversion validator itself still runs
|
||||
assertEquals(1, ageErrors.size());
|
||||
assertEquals("Age must be a valid number", ageErrors.get(0));
|
||||
}
|
||||
|
||||
public void testConcurrentGetValidatorsReturnsConsistentResults() throws Exception {
|
||||
final int threads = 16;
|
||||
ExecutorService pool = Executors.newFixedThreadPool(threads);
|
||||
|
||||
+43
@@ -0,0 +1,43 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<!--
|
||||
/*
|
||||
* Licensed to the Apache Software Foundation (ASF) under one
|
||||
* or more contributor license agreements. See the NOTICE file
|
||||
* distributed with this work for additional information
|
||||
* regarding copyright ownership. The ASF licenses this file
|
||||
* to you under the Apache License, Version 2.0 (the
|
||||
* "License"); you may not use this file except in compliance
|
||||
* with the License. You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing,
|
||||
* software distributed under the License is distributed on an
|
||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
* KIND, either express or implied. See the License for the
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
-->
|
||||
<!DOCTYPE validators PUBLIC "-//Apache Struts//XWork Validator 1.0.2//EN" "https://struts.apache.org/dtds/xwork-validator-1.0.2.dtd">
|
||||
<validators>
|
||||
<field name="age">
|
||||
<field-validator type="conversion">
|
||||
<message>Age must be a valid number</message>
|
||||
</field-validator>
|
||||
<field-validator type="required">
|
||||
<message>Age is required</message>
|
||||
</field-validator>
|
||||
</field>
|
||||
|
||||
<field name="name">
|
||||
<field-validator type="required">
|
||||
<message>Name is required</message>
|
||||
</field-validator>
|
||||
</field>
|
||||
|
||||
<validator type="expression">
|
||||
<param name="expression">false</param>
|
||||
<message>Action level always fails</message>
|
||||
</validator>
|
||||
</validators>
|
||||
+32
@@ -0,0 +1,32 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<!--
|
||||
/*
|
||||
* Licensed to the Apache Software Foundation (ASF) under one
|
||||
* or more contributor license agreements. See the NOTICE file
|
||||
* distributed with this work for additional information
|
||||
* regarding copyright ownership. The ASF licenses this file
|
||||
* to you under the Apache License, Version 2.0 (the
|
||||
* "License"); you may not use this file except in compliance
|
||||
* with the License. You may obtain a copy of the License at
|
||||
*
|
||||
* http://www.apache.org/licenses/LICENSE-2.0
|
||||
*
|
||||
* Unless required by applicable law or agreed to in writing,
|
||||
* software distributed under the License is distributed on an
|
||||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
|
||||
* KIND, either express or implied. See the License for the
|
||||
* specific language governing permissions and limitations
|
||||
* under the License.
|
||||
*/
|
||||
-->
|
||||
<!DOCTYPE validators PUBLIC "-//Apache Struts//XWork Validator 1.0.2//EN" "https://struts.apache.org/dtds/xwork-validator-1.0.2.dtd">
|
||||
<validators>
|
||||
<field name="zip">
|
||||
<field-validator type="conversion">
|
||||
<message>Zip must be a valid number</message>
|
||||
</field-validator>
|
||||
<field-validator type="required">
|
||||
<message>Zip is required</message>
|
||||
</field-validator>
|
||||
</field>
|
||||
</validators>
|
||||
@@ -0,0 +1,293 @@
|
||||
# WW-2934 Skip Validators on Conversion Error — Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** When opted in, skip a field's remaining validators once that field has a type-conversion error, so the user sees only the conversion message instead of a redundant duplicate (e.g. conversion + `required`).
|
||||
|
||||
**Architecture:** Add a global constant `struts.validators.skipValidatorsOnConversionError` (default `false`). Inject it into `DefaultActionValidatorManager`. Inside its validator loop, when the flag is on and the current validator is a `FieldValidator` (but not the `conversion` validator itself) whose full field name is present in `ActionContext.getConversionErrors()`, skip it. Action-level validators are untouched.
|
||||
|
||||
**Tech Stack:** Java, Struts 2 core, JUnit 3-style `XWorkTestCase` tests, AssertJ available but the surrounding test class uses `junit.framework.TestCase` assertions.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- **Commit prefix:** every commit message starts with `WW-2934` (e.g. `WW-2934 feat(core): ...`).
|
||||
- **Constant name (verbatim):** `struts.validators.skipValidatorsOnConversionError`, default value `false`.
|
||||
- **Java field naming:** `Struts*` prefix convention is for default implementation *classes*, not relevant here — reuse the existing `DefaultActionValidatorManager`.
|
||||
- **Test style:** core tests are JUnit 3/4 — test methods are `public void testXxx()` on a class extending `XWorkTestCase`. A `@Test` annotation here silently never runs. Do NOT use `@Test`.
|
||||
- **License header:** every new `.java` and `.xml` file must begin with the Apache license header (copy from an existing sibling file in the same directory).
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Skip field validators on conversion error (opt-in)
|
||||
|
||||
**Files:**
|
||||
- Create: `core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java` (test fixture action)
|
||||
- Create: `core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml` (test fixture validators)
|
||||
- Modify: `core/src/main/java/org/apache/struts2/StrutsConstants.java` (add constant, near the other validator constants around line 420)
|
||||
- Modify: `core/src/main/resources/org/apache/struts2/default.properties` (document the constant)
|
||||
- Modify: `core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java` (inject flag + add skip check in `validate(...)`)
|
||||
- Test: `core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java` (add four test methods)
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `ActionContext.getContext().getConversionErrors()` → `Map<String, ConversionData>` keyed by full field name; `ConversionData(Object value, Class toClass)`; `DefaultActionValidatorManager.validate(Object object, String context)`.
|
||||
- Produces: `DefaultActionValidatorManager.setSkipValidatorsOnConversionError(String)` (public, `@Inject(required=false)`); `StrutsConstants.STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR` = `"struts.validators.skipValidatorsOnConversionError"`.
|
||||
|
||||
---
|
||||
|
||||
- [ ] **Step 1: Create the test fixture action**
|
||||
|
||||
Create `core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java` (prepend the Apache license header copied from a sibling file such as `DefaultActionValidatorManagerTest.java`):
|
||||
|
||||
```java
|
||||
package org.apache.struts2.validator;
|
||||
|
||||
import org.apache.struts2.ActionSupport;
|
||||
|
||||
/**
|
||||
* Fixture for WW-2934: an Integer field ("age") that carries both a conversion
|
||||
* validator and a required validator, plus an unrelated required String field
|
||||
* ("name") and an action-level validator (see the matching -validation.xml).
|
||||
*/
|
||||
public class ConversionErrorSkipAction extends ActionSupport {
|
||||
|
||||
private Integer age;
|
||||
private String name;
|
||||
|
||||
public Integer getAge() {
|
||||
return age;
|
||||
}
|
||||
|
||||
public void setAge(Integer age) {
|
||||
this.age = age;
|
||||
}
|
||||
|
||||
public String getName() {
|
||||
return name;
|
||||
}
|
||||
|
||||
public void setName(String name) {
|
||||
this.name = name;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Create the test fixture validation XML**
|
||||
|
||||
Create `core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml` (prepend the Apache license header as an XML comment, copied from a sibling such as `core/src/test/resources/org/apache/struts2/test/User-validation.xml`):
|
||||
|
||||
```xml
|
||||
<!DOCTYPE validators PUBLIC "-//Apache Struts//XWork Validator 1.0.2//EN" "https://struts.apache.org/dtds/xwork-validator-1.0.2.dtd">
|
||||
<validators>
|
||||
<field name="age">
|
||||
<field-validator type="conversion">
|
||||
<message>Age must be a valid number</message>
|
||||
</field-validator>
|
||||
<field-validator type="required">
|
||||
<message>Age is required</message>
|
||||
</field-validator>
|
||||
</field>
|
||||
|
||||
<field name="name">
|
||||
<field-validator type="required">
|
||||
<message>Name is required</message>
|
||||
</field-validator>
|
||||
</field>
|
||||
|
||||
<validator type="expression">
|
||||
<param name="expression">false</param>
|
||||
<message>Action level always fails</message>
|
||||
</validator>
|
||||
</validators>
|
||||
```
|
||||
|
||||
Note: `age` is an `Integer` (defaults to `null`) so the `required` validator actually fires. The `conversion` validator only adds its message when a conversion error exists for the field. The bare `<validator>` (not `<field-validator>`) is action-level.
|
||||
|
||||
- [ ] **Step 3: Write the failing tests**
|
||||
|
||||
Add these four methods to `DefaultActionValidatorManagerTest` (before the closing brace of the class). Add imports at the top: `import org.apache.struts2.ActionContext;`, `import org.apache.struts2.conversion.impl.ConversionData;` (`java.util.List` and `java.util.Map` are already imported).
|
||||
|
||||
```java
|
||||
public void testConversionError_bothErrorsWhenFlagDisabledByDefault() {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> ageErrors = action.getFieldErrors().get("age");
|
||||
assertNotNull(ageErrors);
|
||||
assertEquals(2, ageErrors.size()); // conversion + required, current behavior
|
||||
assertTrue(ageErrors.contains("Age must be a valid number"));
|
||||
assertTrue(ageErrors.contains("Age is required"));
|
||||
}
|
||||
|
||||
public void testConversionError_fieldValidatorsSkippedWhenEnabled() {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> ageErrors = action.getFieldErrors().get("age");
|
||||
assertNotNull(ageErrors);
|
||||
// required is skipped; the conversion validator itself still runs
|
||||
assertEquals(1, ageErrors.size());
|
||||
assertEquals("Age must be a valid number", ageErrors.get(0));
|
||||
}
|
||||
|
||||
public void testConversionError_unrelatedFieldStillValidatedWhenEnabled() {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
List<String> nameErrors = action.getFieldErrors().get("name");
|
||||
assertNotNull(nameErrors); // "name" has no conversion error, still validated
|
||||
assertEquals(1, nameErrors.size());
|
||||
assertEquals("Name is required", nameErrors.get(0));
|
||||
}
|
||||
|
||||
public void testConversionError_actionLevelValidatorUnaffectedWhenEnabled() {
|
||||
ConversionErrorSkipAction action = new ConversionErrorSkipAction();
|
||||
ActionContext.getContext().getConversionErrors()
|
||||
.put("age", new ConversionData(new String[]{"one"}, Integer.class));
|
||||
actionValidatorManager.setSkipValidatorsOnConversionError("true");
|
||||
|
||||
actionValidatorManager.validate(action, null);
|
||||
|
||||
assertTrue(action.hasActionErrors());
|
||||
assertTrue(action.getActionErrors().contains("Action level always fails"));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Run the tests to verify they fail**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=DefaultActionValidatorManagerTest`
|
||||
Expected: FAIL — compilation error, `cannot find symbol: method setSkipValidatorsOnConversionError(String)` (the production setter does not exist yet).
|
||||
|
||||
- [ ] **Step 5: Add the constant**
|
||||
|
||||
In `core/src/main/java/org/apache/struts2/StrutsConstants.java`, add near the other validator constants (e.g. just after `STRUTS_ACTIONVALIDATORMANAGER` around line 420):
|
||||
|
||||
```java
|
||||
/** @see org.apache.struts2.validator.DefaultActionValidatorManager */
|
||||
public static final String STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR = "struts.validators.skipValidatorsOnConversionError";
|
||||
```
|
||||
|
||||
- [ ] **Step 6: Document the constant in default.properties**
|
||||
|
||||
In `core/src/main/resources/org/apache/struts2/default.properties`, add (grouping it with other behavior toggles; place it after an existing active property such as `struts.devMode = false`):
|
||||
|
||||
```properties
|
||||
### When set to true, a field's remaining validators are skipped once that field
|
||||
### has a type conversion error, avoiding a duplicate error (WW-2934).
|
||||
### valid values are: true, false (false is the default)
|
||||
struts.validators.skipValidatorsOnConversionError = false
|
||||
```
|
||||
|
||||
- [ ] **Step 7: Inject the flag into the manager**
|
||||
|
||||
In `core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java`:
|
||||
|
||||
Add the import (with the other `org.apache.struts2.validator.validators` usages are not yet imported here — add both needed imports):
|
||||
|
||||
```java
|
||||
import org.apache.struts2.validator.validators.ConversionErrorFieldValidator;
|
||||
```
|
||||
|
||||
Add a field next to `reloadingConfigs` (after line 77, `protected boolean reloadingConfigs;`):
|
||||
|
||||
```java
|
||||
protected boolean skipValidatorsOnConversionError;
|
||||
```
|
||||
|
||||
Add the injected setter next to `setReloadingConfigs` (after its closing brace, around line 98):
|
||||
|
||||
```java
|
||||
@Inject(value = StrutsConstants.STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR, required = false)
|
||||
public void setSkipValidatorsOnConversionError(String skipValidatorsOnConversionError) {
|
||||
this.skipValidatorsOnConversionError = Boolean.parseBoolean(skipValidatorsOnConversionError);
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 8: Add the skip check in `validate(...)`**
|
||||
|
||||
In the same file, inside `validate(Object, String, ValidatorContext, String)`, extend the existing `if (validator instanceof FieldValidator)` block. It currently reads:
|
||||
|
||||
```java
|
||||
if (validator instanceof FieldValidator) {
|
||||
fValidator = (FieldValidator) validator;
|
||||
fullFieldName = validatorContext.getFullFieldName(fValidator.getFieldName());
|
||||
|
||||
if ((shortcircuitedFields != null) && shortcircuitedFields.contains(fullFieldName)) {
|
||||
LOG.debug("Short-circuited, skipping");
|
||||
continue;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Add the conversion-error skip as a second guard, immediately before that block's closing brace:
|
||||
|
||||
```java
|
||||
if (validator instanceof FieldValidator) {
|
||||
fValidator = (FieldValidator) validator;
|
||||
fullFieldName = validatorContext.getFullFieldName(fValidator.getFieldName());
|
||||
|
||||
if ((shortcircuitedFields != null) && shortcircuitedFields.contains(fullFieldName)) {
|
||||
LOG.debug("Short-circuited, skipping");
|
||||
continue;
|
||||
}
|
||||
|
||||
if (skipValidatorsOnConversionError
|
||||
&& !(validator instanceof ConversionErrorFieldValidator)
|
||||
&& ActionContext.getContext().getConversionErrors().containsKey(fullFieldName)) {
|
||||
LOG.debug("Skipping validator {} for field {} due to a conversion error", validator, fullFieldName);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
The `!(validator instanceof ConversionErrorFieldValidator)` clause keeps the `conversion` validator itself running so its (possibly custom) message is still reported.
|
||||
|
||||
- [ ] **Step 9: Run the tests to verify they pass**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest=DefaultActionValidatorManagerTest`
|
||||
Expected: PASS — all four new methods plus the pre-existing methods in the class are green.
|
||||
|
||||
- [ ] **Step 10: Run the broader validator suite for regressions**
|
||||
|
||||
Run: `mvn test -DskipAssembly -pl core -Dtest='org.apache.struts2.validator.*'`
|
||||
Expected: PASS — no regressions in the validator package (default behavior unchanged because the flag defaults to `false`).
|
||||
|
||||
- [ ] **Step 11: Commit**
|
||||
|
||||
```bash
|
||||
git add core/src/main/java/org/apache/struts2/StrutsConstants.java \
|
||||
core/src/main/resources/org/apache/struts2/default.properties \
|
||||
core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java \
|
||||
core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java \
|
||||
core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java \
|
||||
core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml
|
||||
git commit -m "WW-2934 feat(core): skip field validators on conversion error behind opt-in flag
|
||||
|
||||
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage:**
|
||||
- Rollout — global constant default OFF → Steps 5, 6 (constant + `default.properties` = false).
|
||||
- Where — `DefaultActionValidatorManager.validate()` → Steps 7, 8.
|
||||
- Behavior — skip FieldValidator on conversion error, exempt `conversion` validator, action-level untouched → Step 8 + tests in Step 3 (`fieldValidatorsSkippedWhenEnabled`, `actionLevelValidatorUnaffectedWhenEnabled`).
|
||||
- Use `getConversionErrors()` keyed by full field name → Step 8 (`getFullFieldName` + `getConversionErrors().containsKey`).
|
||||
- Testing — all five spec test cases: flag off both errors, flag on skip, custom conversion validator still runs (asserted as the sole remaining `age` error), action-level unaffected, field without conversion error still validated → Step 3 four methods (the "custom conversion validator still runs" case is covered by asserting the remaining `age` error equals the conversion message).
|
||||
- Backward compatibility — default `false`, verified by Step 10 regression run and the disabled-by-default test.
|
||||
|
||||
**Placeholder scan:** none — every code and command step is concrete.
|
||||
|
||||
**Type consistency:** `setSkipValidatorsOnConversionError(String)` and `STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR` are named identically in the Interfaces block, Steps 5/7, and the tests. `ConversionData(Object, Class)` matches the confirmed constructor.
|
||||
+133
@@ -0,0 +1,133 @@
|
||||
# WW-2934 — Skip field validators when a field has a conversion error
|
||||
|
||||
- **Jira:** [WW-2934](https://issues.apache.org/jira/browse/WW-2934)
|
||||
- **Type:** Improvement
|
||||
- **Target version:** 7.3.0
|
||||
- **Date:** 2026-07-23
|
||||
|
||||
## Problem
|
||||
|
||||
When a field fails type conversion (for example, a user types `one` into an
|
||||
`Integer age` field), the user receives two errors for the same field:
|
||||
|
||||
1. A conversion error, added by `ConversionErrorInterceptor` (which runs before
|
||||
validation in the default stack).
|
||||
2. A redundant field-validator error, e.g. from a `requiredstring` or `int`
|
||||
range validator, because binding failed and the field holds its default value.
|
||||
|
||||
The two messages are confusing and, per the reporter, a long-standing pain point
|
||||
for real-world projects. Once a field's value could not be converted, its
|
||||
remaining field validators are operating on a value the user never actually
|
||||
entered, so they should be skipped.
|
||||
|
||||
## Current mechanics (confirmed)
|
||||
|
||||
- Default interceptor stack order is `conversionError` → `validation` →
|
||||
`workflow`. Conversion errors are therefore already recorded **before**
|
||||
validators run — both as field errors and in
|
||||
`ActionContext.getConversionErrors()`, a `Map<String, ConversionData>` keyed by
|
||||
**full field name**.
|
||||
- `DefaultActionValidatorManager.validate(...)` runs each validator in order and
|
||||
already has a per-field short-circuit mechanism (`shortcircuitedFields`) that
|
||||
skips later validators for a field once an earlier short-circuit validator
|
||||
fails.
|
||||
- The `conversion` field validator (`ConversionErrorFieldValidator`, extending
|
||||
`RepopulateConversionErrorFieldValidatorSupport`) exists specifically to
|
||||
*report* the conversion error (and optionally repopulate the field). It must
|
||||
never be skipped.
|
||||
- `DefaultActionValidatorManager` already injects a Struts constant
|
||||
(`struts.configuration.xml.reload`) via `@Inject(..., required = false)`, so a
|
||||
new constant follows an established pattern.
|
||||
|
||||
## Goals
|
||||
|
||||
- When enabled, skip a field's validators once that field has a conversion error.
|
||||
- Preserve the `conversion` validator so custom conversion messages and
|
||||
`repopulateField` still work.
|
||||
- Leave action-level (non-field) validators untouched.
|
||||
- Change nothing for existing applications unless they opt in.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- No change to interceptor stack ordering.
|
||||
- No per-field or per-validator configuration syntax.
|
||||
- No change to how conversion errors themselves are produced or reported.
|
||||
|
||||
## Design
|
||||
|
||||
### Rollout: global constant, default OFF
|
||||
|
||||
Add a new global constant:
|
||||
|
||||
```
|
||||
struts.validators.skipValidatorsOnConversionError = false (default)
|
||||
```
|
||||
|
||||
Default `false` preserves today's behavior (both errors shown). Applications
|
||||
opt in by setting the constant to `true`. This is the safest rollout for a
|
||||
15+-year-old default that some applications may rely on.
|
||||
|
||||
### Where: `DefaultActionValidatorManager.validate(...)`
|
||||
|
||||
This method is the natural home — it owns the validator loop and the existing
|
||||
`shortcircuitedFields` skip logic, and it is the single path for both XML and
|
||||
annotation-driven validation. Alternatives were rejected:
|
||||
|
||||
- `ValidationInterceptor` does not run individual validators, so it could only
|
||||
strip errors after the fact — fragile, and unable to distinguish a
|
||||
conversion-driven error from a legitimately-added one.
|
||||
- `ConversionErrorInterceptor` knows the conversion errors but has no handle on
|
||||
the validator list, so it would still need to hand state to the manager.
|
||||
|
||||
### Behavior
|
||||
|
||||
Inside the validator loop, when the flag is enabled and the current validator is
|
||||
a `FieldValidator` that is **not** a `ConversionErrorFieldValidator`, check
|
||||
whether the field's full name is present in
|
||||
`ActionContext.getContext().getConversionErrors()`. If so, skip that validator
|
||||
(`continue`).
|
||||
|
||||
`fullFieldName` is already computed in the loop as
|
||||
`validatorContext.getFullFieldName(fValidator.getFieldName())` — the same key the
|
||||
conversion errors map and the repopulation logic use — so nested and indexed
|
||||
fields compare correctly.
|
||||
|
||||
**Why the conversion errors map, not "any pre-existing field error":** the map
|
||||
is the precise, authoritative source, populated during parameter binding before
|
||||
validation runs. Checking generic field errors would risk skipping a validator
|
||||
because of an error added by an earlier validator in the same pass.
|
||||
|
||||
### Implementation sketch
|
||||
|
||||
1. `StrutsConstants`: add
|
||||
`STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR =
|
||||
"struts.validators.skipValidatorsOnConversionError"`.
|
||||
2. `default.properties`: document the constant (commented, default `false`).
|
||||
3. `DefaultActionValidatorManager`: add a `boolean
|
||||
skipValidatorsOnConversionError` field (default `false`) with
|
||||
`@Inject(value = STRUTS_VALIDATORS_SKIP_VALIDATORS_ON_CONVERSION_ERROR,
|
||||
required = false)` setter parsing the boolean.
|
||||
4. In `validate(...)`, after resolving `fValidator`/`fullFieldName` and before
|
||||
running the validator, add the skip check described above.
|
||||
|
||||
## Testing
|
||||
|
||||
Unit tests (in `DefaultActionValidatorManagerTest` or a focused test class),
|
||||
each asserting resulting field errors:
|
||||
|
||||
- **Flag off (default):** field with a conversion error still gets both the
|
||||
conversion error and its other field-validator errors (current behavior
|
||||
unchanged).
|
||||
- **Flag on:** field with a conversion error gets only the conversion error;
|
||||
`required`/range validators for that field are skipped.
|
||||
- **Flag on + custom `conversion` validator:** the `conversion` validator still
|
||||
runs (custom message present).
|
||||
- **Flag on, action-level validator:** action-level (non-field) validators are
|
||||
unaffected.
|
||||
- **Flag on, field without a conversion error:** its validators run normally.
|
||||
|
||||
## Backward compatibility
|
||||
|
||||
Default `false` means zero behavior change for existing applications. Opting in
|
||||
is a single constant. No configuration files, action code, or validator
|
||||
definitions need to change.
|
||||
Reference in New Issue
Block a user