diff --git a/core/src/main/java/hudson/cli/BuildCommand.java b/core/src/main/java/hudson/cli/BuildCommand.java index dc2b636badee8a0801641d3f4890f8e8050c0aaa..06093e110d987ba558d2cdc3cef4336ff979f03a 100644 --- a/core/src/main/java/hudson/cli/BuildCommand.java +++ b/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 values = new ArrayList(); for (Entry 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); diff --git a/core/src/main/java/hudson/model/ParameterDefinition.java b/core/src/main/java/hudson/model/ParameterDefinition.java index 32c89fb00a6a40d58849c9d7f1a9473675c9cd3c..48df40b0e6fb788ab7d79105ed908f694188530e 100644 --- a/core/src/main/java/hudson/model/ParameterDefinition.java +++ b/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; diff --git a/core/src/main/java/hudson/model/ParametersAction.java b/core/src/main/java/hudson/model/ParametersAction.java index 3839d9541f25422874c02f57f5765218a2e8963f..85ed085c80859cc4c24a2b74e8ec944e2771e28d 100644 --- a/core/src/main/java/hudson/model/ParametersAction.java +++ b/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, Queue public void createBuildWrappers(AbstractBuild build, Collection 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, Queue public VariableResolver 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(resolvers); } - + public Iterator iterator() { return parameters.iterator(); } @@ -120,14 +127,17 @@ public class ParametersAction implements Action, Iterable, 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, 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 overrides) { if(overrides == null) { return new ParametersAction(parameters); @@ -175,10 +187,12 @@ public class ParametersAction implements Action, Iterable, Queue Set 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, 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()); diff --git a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java index 84efcc1ae4385464da4f0d12c2496b6a570ad0cc..13a6156b5c88a26d7ea23dd2f9a064f4fe0dd92d 100644 --- a/core/src/main/java/hudson/model/ParametersDefinitionProperty.java +++ b/core/src/main/java/hudson/model/ParametersDefinitionProperty.java @@ -147,7 +147,11 @@ public class ParametersDefinitionProperty extends JobProperty> 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( diff --git a/core/src/test/java/hudson/model/ParametersActionTest.java b/core/src/test/java/hudson/model/ParametersActionTest.java index ee565fb50d61fa2c4aa50ce872ce1bacbe4aa5b8..69c76ebc73806a097497bd3c311dd5c74ddcadd8 100644 --- a/core/src/test/java/hudson/model/ParametersActionTest.java +++ b/core/src/test/java/hudson/model/ParametersActionTest.java @@ -1,11 +1,21 @@ 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 wrappers = new LinkedList(); + 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()); + } } diff --git a/test/src/test/groovy/hudson/cli/BuildCommandTest.groovy b/test/src/test/groovy/hudson/cli/BuildCommandTest.groovy index 78978981b8beb9712b6e9c00e19eb8f1be4d02f3..42894585a9a023419f7574dea0168d8215e44d2c 100644 --- a/test/src/test/groovy/hudson/cli/BuildCommandTest.groovy +++ b/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"; + } + } + } }