FIXES JENKINS-18434 - Added validation error to Parameterized build c…

…hoice, removed hard error.
jenkinsci · Oct 7, 2013 · d244b6b · d244b6b
1 parent ce29cdb
commit d244b6b
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 9 deletions.
diff --git a/core/src/main/java/hudson/model/ChoiceParameterDefinition.java b/core/src/main/java/hudson/model/ChoiceParameterDefinition.java
@@ -15,25 +15,26 @@
  * @author huybrechts
  */
 public class ChoiceParameterDefinition extends SimpleParameterDefinition {
+    public static final String CHOICES_DELIMETER = "\\r?\\n";
+
     private final List<String> choices;
     private final String defaultValue;
 
+    public static boolean areValidChoices(String choices) {
+        String strippedChoices = choices.trim();
+        return !StringUtils.isEmpty(strippedChoices) && strippedChoices.split(CHOICES_DELIMETER).length > 0;
+    }
+
     @DataBoundConstructor
     public ChoiceParameterDefinition(String name, String choices, String description) {
         super(name, description);
-        this.choices = Arrays.asList(choices.split("\\r?\\n"));
-        if (choices.length()==0) {
-            throw new IllegalArgumentException("No choices found");
-        }
+        this.choices = Arrays.asList(choices.split(CHOICES_DELIMETER));
         defaultValue = null;
     }
 
     public ChoiceParameterDefinition(String name, String[] choices, String description) {
         super(name, description);
         this.choices = new ArrayList<String>(Arrays.asList(choices));
-        if (this.choices.isEmpty()) {
-            throw new IllegalArgumentException("No choices found");
-        }
         defaultValue = null;
     }
 
@@ -52,7 +53,7 @@ public ParameterDefinition copyWithDefaultValue(ParameterValue defaultValue) {
             return this;
         }
     }
-    
+
     @Exported
     public List<String> getChoices() {
         return choices;

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -30,6 +30,7 @@
 import com.google.inject.Injector;
 import hudson.ExtensionComponent;
 import hudson.ExtensionFinder;
+import hudson.model.ChoiceParameterDefinition;
 import hudson.model.LoadStatistics;
 import hudson.model.Messages;
 import hudson.model.Node;
@@ -3559,6 +3560,17 @@ public FormValidation doViewExistsCheck(@QueryParameter String value) {
             return FormValidation.error(Messages.Hudson_ViewAlreadyExists(view));
     }
 
+    /**
+     * Checks if parameterised build choices are valid.
+     */
+    public FormValidation doCheckChoices(@QueryParameter String value) {
+        if (ChoiceParameterDefinition.areValidChoices(value)) {
+            return FormValidation.ok();
+        } else {
+            return FormValidation.error(Messages.ChoiceParameterDefinition_MissingChoices());
+        }
+    }
+
     /**
      * Serves static resources placed along with Jelly view files.
      * <p>

diff --git a/core/src/main/resources/hudson/model/ChoiceParameterDefinition/config.jelly b/core/src/main/resources/hudson/model/ChoiceParameterDefinition/config.jelly
@@ -30,7 +30,7 @@ THE SOFTWARE.
 		<f:textbox name="parameter.name" value="${instance.name}" />
 	</f:entry>
 	<f:entry title="${%Choices}" help="/help/parameter/choice-choices.html">
-		<f:textarea name="parameter.choices" value="${instance.choicesText}" />
+		<f:textarea checkUrl="'${rootURL}/checkChoices?value='+encodeURIComponent(this.value)" name="parameter.choices" value="${instance.choicesText}" />
 	</f:entry>
     <f:entry title="${%Description}" help="/help/parameter/description.html">
         <f:textarea name="parameter.description" value="${instance.description}" />

diff --git a/core/src/main/resources/hudson/model/Messages.properties b/core/src/main/resources/hudson/model/Messages.properties
@@ -308,6 +308,7 @@ TextParameterDefinition.DisplayName=Text Parameter
 FileParameterDefinition.DisplayName=File Parameter
 BooleanParameterDefinition.DisplayName=Boolean Value
 ChoiceParameterDefinition.DisplayName=Choice
+ChoiceParameterDefinition.MissingChoices=Requires Choices.
 RunParameterDefinition.DisplayName=Run Parameter
 PasswordParameterDefinition.DisplayName=Password Parameter
 

diff --git a/core/src/test/java/hudson/model/ChoiceParameterDefinitionTest.java b/core/src/test/java/hudson/model/ChoiceParameterDefinitionTest.java
@@ -0,0 +1,17 @@
+package hudson.model;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class ChoiceParameterDefinitionTest {
+    @Test
+    public void shouldValidateChoices(){
+        assertFalse(ChoiceParameterDefinition.areValidChoices(""));
+        assertFalse(ChoiceParameterDefinition.areValidChoices("        "));
+        assertTrue(ChoiceParameterDefinition.areValidChoices("abc"));
+        assertTrue(ChoiceParameterDefinition.areValidChoices("abc\ndef"));
+        assertTrue(ChoiceParameterDefinition.areValidChoices("abc\r\ndef"));
+    }
+}
diff --git a/test/src/test/java/jenkins/model/JenkinsTest.java b/test/src/test/java/jenkins/model/JenkinsTest.java
@@ -46,9 +46,12 @@
 import org.jvnet.hudson.test.HudsonTestCase;
 import org.jvnet.hudson.test.TestExtension;
 import org.kohsuke.stapler.HttpResponse;
+
 import java.net.HttpURLConnection;
 import java.net.URL;
 
+import static org.junit.Assert.assertEquals;
+
 /**
  * @author kingfai
  *
@@ -307,6 +310,15 @@ public void testDoEval() throws Exception {
             assertEquals(HttpURLConnection.HTTP_FORBIDDEN, e.getStatusCode());
         }
     }
+
+    @Test
+    public void testCheckChoices() throws Exception {
+        Jenkins jenkins = Jenkins.getInstance();
+
+        assertEquals(FormValidation.Kind.OK, jenkins.doCheckChoices("abc\ndef").kind);
+        assertEquals(FormValidation.Kind.ERROR, jenkins.doCheckChoices("").kind);
+    }
+
     private String eval(WebClient wc) throws Exception {
         WebRequestSettings req = new WebRequestSettings(new URL(wc.getContextPath() + "eval"), HttpMethod.POST);
         req.setRequestBody("<j:jelly xmlns:j='jelly:core'>${1+2}</j:jelly>");