diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java index e76fac4e5d..7530bd307f 100644 --- a/core/src/main/java/org/apache/struts2/StrutsConstants.java +++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java @@ -419,6 +419,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 */ diff --git a/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java b/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java index 2f2b758b79..9e1c5a7a6f 100644 --- a/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java +++ b/core/src/main/java/org/apache/struts2/validator/DefaultActionValidatorManager.java @@ -25,6 +25,7 @@ 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; @@ -75,6 +76,7 @@ public class DefaultActionValidatorManager implements ActionValidatorManager { protected ValidatorFileParser validatorFileParser; protected FileManager fileManager; protected boolean reloadingConfigs; + protected boolean skipValidatorsOnConversionError; protected TextProviderFactory textProviderFactory; @Inject @@ -97,6 +99,18 @@ public void setReloadingConfigs(String reloadingConfigs) { 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 void validate(Object object, String context, ValidatorContext validatorCo 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()) { diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties index 1c501cb330..76a1ff9a24 100644 --- a/core/src/main/resources/org/apache/struts2/default.properties +++ b/core/src/main/resources/org/apache/struts2/default.properties @@ -146,6 +146,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 diff --git a/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java b/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java new file mode 100644 index 0000000000..e9b22fc3e4 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipAction.java @@ -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; + } +} diff --git a/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipBean.java b/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipBean.java new file mode 100644 index 0000000000..57a2e5b2bc --- /dev/null +++ b/core/src/test/java/org/apache/struts2/validator/ConversionErrorSkipBean.java @@ -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; + } +} diff --git a/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java index 06914d31f5..ee004ea1de 100644 --- a/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java +++ b/core/src/test/java/org/apache/struts2/validator/DefaultActionValidatorManagerTest.java @@ -18,11 +18,17 @@ */ package org.apache.struts2.validator; +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionInvocation; +import org.apache.struts2.ActionProxy; 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; @@ -35,8 +41,10 @@ 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.util.ArrayList; @@ -375,4 +383,116 @@ public void testFieldErrorsOrder() throws Exception { 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 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 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 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 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 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)); + } + } diff --git a/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml b/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml new file mode 100644 index 0000000000..e8a6af67ea --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml @@ -0,0 +1,43 @@ + + + + + + + Age must be a valid number + + + Age is required + + + + + + Name is required + + + + + false + Action level always fails + + diff --git a/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipBean-validation.xml b/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipBean-validation.xml new file mode 100644 index 0000000000..354bfeb828 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipBean-validation.xml @@ -0,0 +1,32 @@ + + + + + + + Zip must be a valid number + + + Zip is required + + + diff --git a/docs/superpowers/plans/2026-07-23-WW-2934-skip-validators-on-conversion-error.md b/docs/superpowers/plans/2026-07-23-WW-2934-skip-validators-on-conversion-error.md new file mode 100644 index 0000000000..911c1bc300 --- /dev/null +++ b/docs/superpowers/plans/2026-07-23-WW-2934-skip-validators-on-conversion-error.md @@ -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` 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 + + + + + Age must be a valid number + + + Age is required + + + + + + Name is required + + + + + false + Action level always fails + + +``` + +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 `` (not ``) 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 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 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 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 " +``` + +--- + +## 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. diff --git a/docs/superpowers/specs/2026-07-23-WW-2934-skip-validators-on-conversion-error-design.md b/docs/superpowers/specs/2026-07-23-WW-2934-skip-validators-on-conversion-error-design.md new file mode 100644 index 0000000000..53e895fccc --- /dev/null +++ b/docs/superpowers/specs/2026-07-23-WW-2934-skip-validators-on-conversion-error-design.md @@ -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` 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.