WW-2934 Skip field validators when a field has a conversion error (opt-in)

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
      */

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()) {

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

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;
+    }
+}

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;
+    }
+}

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<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));
+    }
+
 }

core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipAction-validation.xml

@@ -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>

core/src/test/resources/org/apache/struts2/validator/ConversionErrorSkipBean-validation.xml

@@ -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>