Merge pull request #1229 from synopsys-arc-oss/CLI_null_ParameteValue

[READY][FIXED JENKINS-15094] - Handle null parameter values to avoid massive executor deaths

core/src/main/java/hudson/cli/BuildCommand.java

@@ -51,6 +51,7 @@ import java.util.ArrayList;
 import java.util.Map.Entry;
 import java.io.FileNotFoundException;
 import java.io.PrintStream;
+import javax.annotation.Nonnull;
 
 import jenkins.model.Jenkins;
 
@@ -101,15 +102,21 @@ public class BuildCommand extends CLICommand {
             if (pdp==null)
                 throw new AbortException(job.getFullDisplayName()+" is not parameterized but the -p option was specified");
 
+            //TODO: switch to type annotations after the migration to Java 1.8
             List<ParameterValue> values = new ArrayList<ParameterValue>();
 
             for (Entry<String, String> e : parameters.entrySet()) {
                 String name = e.getKey();
                 ParameterDefinition pd = pdp.getParameterDefinition(name);
-                if (pd==null)
+                if (pd==null) {
                     throw new AbortException(String.format("\'%s\' is not a valid parameter. Did you mean %s?",
                             name, EditDistance.findNearest(name, pdp.getParameterDefinitionNames())));
-                values.add(pd.createValue(this, Util.fixNull(e.getValue())));
+                }
+                ParameterValue val = pd.createValue(this, Util.fixNull(e.getValue()));
+                if (val == null) {
+                    throw new AbortException(String.format("Cannot resolve the value for the parameter \'%s\'.",name));
+                }
+                values.add(val);
             }
 
             // handle missing parameters by adding as default values ISSUE JENKINS-7162
@@ -118,7 +125,11 @@ public class BuildCommand extends CLICommand {
                     continue;
 
                 // not passed in use default
-                values.add(pd.getDefaultParameterValue());
+                ParameterValue defaultValue = pd.getDefaultParameterValue();
+                if (defaultValue == null) {
+                    throw new AbortException(String.format("No default value for the parameter \'%s\'.",pd.getName()));
+                }
+                values.add(defaultValue);
             }
 
             a = new ParametersAction(values);

core/src/main/java/hudson/model/ParameterDefinition.java

@@ -33,6 +33,7 @@ import hudson.util.DescriptorList;
 import java.io.Serializable;
 import java.io.IOException;
 import java.util.logging.Logger;
+import javax.annotation.CheckForNull;
 
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
@@ -165,6 +166,7 @@ public abstract class ParameterDefinition implements
      * This method is invoked when the user fills in the parameter values in the HTML form
      * and submits it to the server.
      */
+    @CheckForNull
     public abstract ParameterValue createValue(StaplerRequest req, JSONObject jo);
     
     /**
@@ -183,6 +185,7 @@ public abstract class ParameterDefinition implements
      * @throws IllegalStateException
      *      If the parameter is deemed required but was missing in the submission.
      */
+    @CheckForNull
     public abstract ParameterValue createValue(StaplerRequest req);
 
 
@@ -200,6 +203,7 @@ public abstract class ParameterDefinition implements
      *      the command exits with an error code.
      * @since 1.334
      */
+    @CheckForNull
     public ParameterValue createValue(CLICommand command, String value) throws IOException, InterruptedException {
         throw new AbortException("CLI parameter submission is not supported for the "+getClass()+" type. Please file a bug report for this");
     }
@@ -210,6 +214,7 @@ public abstract class ParameterDefinition implements
      * @return default parameter value or null if no defaults are available
      * @since 1.253
      */
+    @CheckForNull
     @Exported
     public ParameterValue getDefaultParameterValue() {
         return null;

core/src/main/java/hudson/model/ParametersAction.java

@@ -45,6 +45,8 @@ import java.util.Set;
 
 import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Sets.newHashSet;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
  * Records the parameter values used for a build.
@@ -74,14 +76,17 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
 
     public void createBuildWrappers(AbstractBuild<?,?> build, Collection<? super BuildWrapper> result) {
         for (ParameterValue p : parameters) {
+            if (p == null) continue;
             BuildWrapper w = p.createBuildWrapper(build);
             if(w!=null) result.add(w);
         }
     }
 
     public void buildEnvVars(AbstractBuild<?,?> build, EnvVars env) {
-        for (ParameterValue p : parameters)
-            p.buildEnvironment(build, env);
+        for (ParameterValue p : parameters) {
+            if (p == null) continue;
+            p.buildEnvironment(build, env); 
+        }
     }
 
     // TODO do we need an EnvironmentContributingAction variant that takes Run so this can implement it?
@@ -102,14 +107,16 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     public VariableResolver<String> createVariableResolver(AbstractBuild<?,?> build) {
         VariableResolver[] resolvers = new VariableResolver[parameters.size()+1];
         int i=0;
-        for (ParameterValue p : parameters)
+        for (ParameterValue p : parameters) {
+            if (p == null) continue;
             resolvers[i++] = p.createVariableResolver(build);
-
+        }
+            
         resolvers[i] = build.getBuildVariableResolver();
 
         return new VariableResolver.Union<String>(resolvers);
     }
-
+    
     public Iterator<ParameterValue> iterator() {
         return parameters.iterator();
     }
@@ -120,14 +127,17 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     }
 
     public ParameterValue getParameter(String name) {
-        for (ParameterValue p : parameters)
+        for (ParameterValue p : parameters) {
+            if (p == null) continue;
             if (p.getName().equals(name))
                 return p;
+        }
         return null;
     }
 
     public Label getAssignedLabel(SubTask task) {
         for (ParameterValue p : parameters) {
+            if (p == null) continue;
             Label l = p.getAssignedLabel(task);
             if (l!=null)    return l;
         }
@@ -166,7 +176,9 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
     /**
      * Creates a new {@link ParametersAction} that contains all the parameters in this action
      * with the overrides / new values given as parameters.
+     * @return New {@link ParametersAction}. The result may contain null {@link ParameterValue}s
      */
+    @Nonnull
     public ParametersAction createUpdated(Collection<? extends ParameterValue> overrides) {
         if(overrides == null) {
             return new ParametersAction(parameters);
@@ -175,10 +187,12 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
         Set<String> names = newHashSet();
 
         for(ParameterValue v : overrides) {
+            if (v == null) continue;
             names.add(v.getName());
         }
 
         for (ParameterValue v : parameters) {
+            if (v == null) continue;
             if (!names.contains(v.getName())) {
                 combinedParameters.add(v);
             }
@@ -187,8 +201,14 @@ public class ParametersAction implements Action, Iterable<ParameterValue>, Queue
         return new ParametersAction(combinedParameters);
     }
 
-    public ParametersAction merge(ParametersAction overrides) {
-        if(overrides == null) {
+    /*
+     * Creates a new {@link ParametersAction} that contains all the parameters in this action
+     * with the overrides / new values given as another {@link ParametersAction}.
+     * @return New {@link ParametersAction}. The result may contain null {@link ParameterValue}s
+     */
+    @Nonnull
+    public ParametersAction merge(@CheckForNull ParametersAction overrides) {
+        if (overrides == null) {
             return new ParametersAction(parameters);
         }
         return createUpdated(overrides.getParameters());

core/src/main/java/hudson/model/ParametersDefinitionProperty.java

@@ -147,7 +147,11 @@ public class ParametersDefinitionProperty extends JobProperty<Job<?, ?>>
             if(d==null)
                 throw new IllegalArgumentException("No such parameter definition: " + name);
             ParameterValue parameterValue = d.createValue(req, jo);
-            values.add(parameterValue);
+            if (parameterValue != null) {
+                values.add(parameterValue);
+            } else {
+                throw new IllegalArgumentException("Cannot retrieve the parameter value: " + name);
+            }
         }
 
     	WaitingItem item = Jenkins.getInstance().getQueue().schedule(

core/src/test/java/hudson/model/ParametersActionTest.java

 package hudson.model;
 
+import hudson.EnvVars;
+import hudson.model.queue.SubTask;
+import hudson.tasks.BuildWrapper;
+import java.util.LinkedList;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotSame;
+import org.junit.runner.RunWith;
+import org.jvnet.hudson.test.Bug;
+import static org.powermock.api.mockito.PowerMockito.mock;
+import org.powermock.modules.junit4.PowerMockRunner;
 
+@RunWith(PowerMockRunner.class)
 public class ParametersActionTest {
 
     private ParametersAction baseParamsAB;
@@ -76,4 +86,35 @@ public class ParametersActionTest {
 
         assertNotSame(baseParamsAB, params);
     }
+    
+    @Test
+    @Bug(15094)   
+    public void checkNullParamaterValues() {
+        SubTask subtask = mock(SubTask.class);
+        Build build = mock(Build.class);
+                   
+        // Prepare parameters Action
+        StringParameterValue A = new StringParameterValue("A", "foo");
+        StringParameterValue B = new StringParameterValue("B", "bar");
+        ParametersAction parametersAction = new ParametersAction(A, null, B);
+        ParametersAction parametersAction2 = new ParametersAction(A,null);
+        
+        // Non existent parameter
+        assertNull(parametersAction.getParameter("C"));   
+        assertNull(parametersAction.getAssignedLabel(subtask));
+        
+        // Interaction with build
+        EnvVars vars = new EnvVars();
+        parametersAction.buildEnvVars(build, vars);
+        assertEquals(2, vars.size());   
+        parametersAction.createVariableResolver(build);
+        
+        LinkedList<BuildWrapper> wrappers = new LinkedList<BuildWrapper>();
+        parametersAction.createBuildWrappers(build, wrappers);
+        assertEquals(0, wrappers.size());
+        
+        // Merges and overrides
+        assertEquals(3, parametersAction.createUpdated(parametersAction2.getParameters()).getParameters().size());
+        assertEquals(3, parametersAction.merge(parametersAction2).getParameters().size());        
+    }
 }

test/src/test/groovy/hudson/cli/BuildCommandTest.groovy

@@ -25,25 +25,37 @@ package hudson.cli
 
 import org.apache.commons.io.output.TeeOutputStream
 import static org.junit.Assert.*
+import hudson.Extension
 import org.junit.Rule
 import org.junit.Test
+import org.jvnet.hudson.test.Bug
+import org.jvnet.hudson.test.CaptureEnvironmentBuilder
 import org.jvnet.hudson.test.JenkinsRule
 import org.jvnet.hudson.test.RandomlyFails
 import org.jvnet.hudson.test.TestBuilder
 import org.jvnet.hudson.test.TestExtension
+import org.kohsuke.stapler.StaplerRequest
 
 import hudson.Launcher
 import hudson.model.AbstractBuild
 import hudson.model.Action
 import hudson.model.BuildListener
+import hudson.model.Executor
 import hudson.model.FreeStyleProject;
+import hudson.model.ParameterDefinition.ParameterDescriptor
+import hudson.model.ParameterValue
 import hudson.model.ParametersAction
 import hudson.model.ParametersDefinitionProperty
 import hudson.model.Queue.QueueDecisionHandler
 import hudson.model.Queue.Task;
+import hudson.model.SimpleParameterDefinition
 import hudson.model.StringParameterDefinition
+import hudson.model.StringParameterValue
+import hudson.model.labels.LabelAtom
 import hudson.tasks.Shell
 import hudson.util.OneShotEvent
+import java.util.concurrent.Executor
+import net.sf.json.JSONObject
 
 /**
  * {@link BuildCommand} test.
@@ -212,4 +224,77 @@ public class BuildCommandTest {
         assertEquals("Command is expected to succeed", 0, result.returnCode());
         assertEquals("a=b", project.getBuildByNumber(1).getBuildVariables().get("expr"));
     }
+    
+    @Bug(15094)
+    @Test public void executorsAliveOnParameterWithNullDefaultValue() throws Exception {    
+        def slave = j.createSlave();
+        FreeStyleProject project = j.createFreeStyleProject("foo");
+        project.setAssignedNode(slave);
+        
+        // Create test parameter with Null default value 
+        def nullDefaultDefinition = new NullDefaultValueParameterDefinition();
+        ParametersDefinitionProperty pdp = new ParametersDefinitionProperty(
+                new StringParameterDefinition("string", "defaultValue", "description"),
+                nullDefaultDefinition);
+        project.addProperty(pdp);
+        CaptureEnvironmentBuilder builder = new CaptureEnvironmentBuilder();
+        project.getBuildersList().add(builder);
+     
+        // Warmup
+        j.buildAndAssertSuccess(project);
+        
+        for (def exec : slave.toComputer().getExecutors()) {
+            assertTrue("Executor has died before the test start: "+exec, exec.isActive());
+        }
+        
+        // Create CLI & run command
+        def invoker = new CLICommandInvoker(j, new BuildCommand());
+        def result = invoker
+                .authorizedTo(jenkins.model.Jenkins.ADMINISTER)
+                .invokeWithArgs("foo","-p","string=value");
+        assertEquals("Command is expected to fail with -1 code. \nSTDOUT="+result.stdout()
+            +"\nSTDERR: "+result.stderr(), -1, result.returnCode());
+        assertTrue("Unexpected error message", 
+            result.stderr().startsWith("No default value for the parameter \'FOO\'."));        
+        
+        // Give the job 5 seconds to be submitted
+        def q = j.jenkins.getQueue().getItem(project);
+        Thread.sleep(5000);
+        
+        // Check executors health after a timeout
+        for (def exec : slave.toComputer().getExecutors()) {
+            assertTrue("Executor is dead: "+exec, exec.isActive());
+        }
+    }
+    
+    public static final class NullDefaultValueParameterDefinition extends SimpleParameterDefinition {
+        
+        /*package*/ NullDefaultValueParameterDefinition() {
+            super("FOO", "Always null default value");
+        }
+        
+        @Override
+        public ParameterValue createValue(String value) {
+            return new StringParameterValue("FOO", "BAR");
+        }
+        
+        @Override
+        public ParameterValue createValue(StaplerRequest req, JSONObject jo) {
+            return createValue("BAR");
+        }
+        
+        @Override
+        public ParameterValue getDefaultParameterValue() {
+            return null; // Equals to super.getDefaultParameterValue();
+        }
+        
+        @Extension
+        public static class DescriptorImpl extends ParameterDescriptor {
+
+            @Override
+            public String getDisplayName() {
+                return "Parameter with the default NULL value"; 
+            }   
+        }
+    }
 }