[JENKINS-26093] Better support for build step parameters

CHANGES.md

@@ -5,6 +5,7 @@ Only noting significant user-visible or major API changes, not internal code cle
 ## 1.3 (upcoming)
 
 ### User changes
+* JENKINS-26093: `build` can now accept `parameters` in a more uniform (and sandbox-friendly) syntax, and the _Snippet Generator_ proposes them based on the actual parameter definitions of the downstream job.
 * JENKINS-25890: deadlock during restart.
 * Fixed some file handle leaks caught by tests which may have affected Windows masters.
 * JENKINS-25779: snippet generator now omits default values of complex steps.

aggregator/src/test/java/org/jenkinsci/plugins/workflow/steps/build/BuildTriggerStepTest.java

@@ -160,12 +160,11 @@ public void cancelBuildQueue() throws Exception {
         Cause.UpstreamCause cause = ds1.getCause(Cause.UpstreamCause.class);
         assertNotNull(cause);
         assertEquals(us1, cause.getUpstreamRun());
-        // TODO JENKINS-26093 let us bind a simple Groovy map
-        us.setDefinition(new CpsFlowDefinition("build job: 'ds', parameters: [new hudson.model.StringParameterValue('branch', 'release')]"));
+        us.setDefinition(new CpsFlowDefinition("build job: 'ds', parameters: [[$class: 'StringParameterValue', name: 'branch', value: 'release']]", true));
         j.assertBuildStatusSuccess(us.scheduleBuild2(0));
-        // TODO IIRC there is an open PR regarding automatic filling in of default parameter values; should that be used, or is BuildTriggerStepExecution responsible, or ParameterizedJobMixIn.scheduleBuild2?
+        // TODO JENKINS-13768 proposes automatic filling in of default parameter values; should that be used, or is BuildTriggerStepExecution responsible, or ParameterizedJobMixIn.scheduleBuild2?
         j.assertLogContains("branch=release extra=", ds.getBuildByNumber(2));
-        us.setDefinition(new CpsFlowDefinition("build job: 'ds', parameters: [new hudson.model.StringParameterValue('branch', 'release'), new hudson.model.BooleanParameterValue('extra', true)]"));
+        us.setDefinition(new CpsFlowDefinition("build job: 'ds', parameters: [[$class: 'StringParameterValue', name: 'branch', value: 'release'], [$class: 'BooleanParameterValue', name: 'extra', value: true]]", true));
         j.assertBuildStatusSuccess(us.scheduleBuild2(0));
         j.assertLogContains("branch=release extra=true", ds.getBuildByNumber(3));
     }

cps/src/main/java/org/jenkinsci/plugins/workflow/cps/Snippetizer.java

@@ -26,6 +26,7 @@
 
 import hudson.Extension;
 import hudson.Functions;
+import hudson.model.Descriptor;
 import hudson.model.RootAction;
 import java.io.IOException;
 import java.util.Collection;
@@ -196,9 +197,10 @@ public HttpResponse doGenerateSnippet(StaplerRequest req, @QueryParameter String
             throw new IllegalStateException("Jenkins is not running");
         }
         Class<?> c = j.getPluginManager().uberClassLoader.loadClass(jsonO.getString("stapler-class"));
+        Descriptor<?> descriptor = j.getDescriptor(c.asSubclass(Step.class));
         Object o;
         try {
-            o = req.bindJSON(c, jsonO);
+            o = descriptor.newInstance(req, jsonO);
         } catch (RuntimeException x) { // e.g. IllegalArgumentException
             return HttpResponses.plainText(Functions.printThrowable(x));
         }

cps/src/test/java/org/jenkinsci/plugins/workflow/cps/SnippetizerTest.java

@@ -101,11 +101,7 @@ public class SnippetizerTest {
         BuildTriggerStep step = new BuildTriggerStep("downstream");
         assertRoundTrip(step, "build 'downstream'");
         step.setParameters(Arrays.asList(new StringParameterValue("branch", "default"), new BooleanParameterValue("correct", true)));
-        /* TODO figure out how to add support for ParameterValue without those having Descriptor’s yet
-                currently instantiate works but uninstantiate does not offer a FQN
-                (which does not matter in this case since BuildTriggerStep/config.jelly does not offer to bind parameters anyway)
-        assertRoundTrip(step, "build job: 'downstream', parameters: [[$class: 'hudson.model.StringParameterValue', name: 'branch', value: 'default'], [$class: 'hudson.model.BooleanParameterValue', name: 'correct', value: true]]");
-        */
+        assertRoundTrip(step, "build job: 'downstream', parameters: [[$class: 'StringParameterValue', name: 'branch', value: 'default'], [$class: 'BooleanParameterValue', name: 'correct', value: true]]");
     }
 
     @Issue("JENKINS-25779")

step-api/src/main/java/org/jenkinsci/plugins/workflow/structs/DescribableHelper.java

@@ -28,6 +28,8 @@
 import com.google.common.primitives.Primitives;
 import hudson.model.Describable;
 import hudson.model.Descriptor;
+import hudson.model.ParameterDefinition;
+import hudson.model.ParameterValue;
 import java.beans.Introspector;
 import java.lang.reflect.Array;
 import java.lang.reflect.Constructor;
@@ -390,6 +392,21 @@ static <T> Set<Class<? extends T>> findSubtypes(Class<T> supertype) {
                 clazzes.add(d.clazz.asSubclass(supertype));
             }
         }
+        if (supertype == ParameterValue.class) { // TODO JENKINS-26093 hack, pending core change
+            for (Class<? extends ParameterDefinition> d : findSubtypes(ParameterDefinition.class)) {
+                String name = d.getName();
+                if (name.endsWith("Definition")) {
+                    try {
+                        Class<?> c = d.getClassLoader().loadClass(name.replaceFirst("Definition$", "Value"));
+                        if (supertype.isAssignableFrom(c)) {
+                            clazzes.add(c.asSubclass(supertype));
+                        }
+                    } catch (ClassNotFoundException x) {
+                        // ignore
+                    }
+                }
+            }
+        }
         return clazzes;
     }
 

step-api/src/test/java/org/jenkinsci/plugins/workflow/structs/DescribableHelperTest.java

@@ -27,7 +27,9 @@
 import hudson.Extension;
 import hudson.Main;
 import hudson.model.AbstractDescribableImpl;
+import hudson.model.BooleanParameterValue;
 import hudson.model.Descriptor;
+import hudson.model.ParameterValue;
 import java.net.URL;
 import java.util.Arrays;
 import java.util.Collection;
@@ -430,6 +432,35 @@ public static final class DefaultStructArray {
         @Override public String toString() {return "DefaultStructArray" + Arrays.toString(bases) + ";stuff=" + stuff;}
     }
 
+    @Issue("JENKINS-26093")
+    @Test public void parameterValues() throws Exception {
+        assertTrue(DescribableHelper.findSubtypes(ParameterValue.class).contains(BooleanParameterValue.class)); //  do not want to enumerate ListSubversionTagsParameterValue etc.
+        // Omitting RunParameterValue since it is not friendly for a unit test.
+        // JobParameterDefinition is not registered as an extension, so not supporting FileParameterValue.
+        // FileParameterValue is unsupportable as an input to a WorkflowJob since it requires createBuildWrapper to work;
+        // as an argument to BuildTriggerStep it could perhaps work, but we would need to provide a custom FileItem implementation.
+        // PasswordParameterValue requires Secret.fromString and thus JenkinsRule.
+        // For others: https://github.com/search?type=Code&q=user%3Ajenkinsci+user%3Acloudbees+%22extends+ParameterDefinition%22
+        roundTrip(TakesParams.class, map("parameters", Arrays.asList(
+                map("$class", "BooleanParameterValue", "name", "flag", "value", true),
+                map("$class", "StringParameterValue", "name", "n", "value", "stuff"),
+                map("$class", "TextParameterValue", "name", "text", "value", "here\nthere"))),
+            "TakesParams;BooleanParameterValue:flag=true;StringParameterValue:n=stuff;TextParameterValue:text=here\nthere");
+    }
+    public static final class TakesParams {
+        public final List<ParameterValue> parameters;
+        @DataBoundConstructor public TakesParams(List<ParameterValue> parameters) {
+            this.parameters = parameters;
+        }
+        @Override public String toString() {
+            StringBuilder b = new StringBuilder("TakesParams");
+            for (ParameterValue v : parameters) {
+                b.append(';').append(v.getClass().getSimpleName()).append(':').append(v.getShortDescription());
+            }
+            return b.toString();
+        }
+    }
+
     private static Map<String,Object> map(Object... keysAndValues) {
         if (keysAndValues.length % 2 != 0) {
             throw new IllegalArgumentException();

support/src/main/java/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStep.java

@@ -3,15 +3,24 @@
 import hudson.Extension;
 import hudson.model.AutoCompletionCandidates;
 import hudson.model.ItemGroup;
+import hudson.model.Job;
+import hudson.model.ParameterDefinition;
 import hudson.model.ParameterValue;
+import hudson.model.ParametersDefinitionProperty;
+import java.util.ArrayList;
 import java.util.List;
+import jenkins.model.Jenkins;
 import jenkins.model.ParameterizedJobMixIn;
+import net.sf.json.JSONArray;
+import net.sf.json.JSONObject;
 import org.jenkinsci.plugins.workflow.steps.AbstractStepDescriptorImpl;
 import org.jenkinsci.plugins.workflow.steps.AbstractStepImpl;
+import org.jenkinsci.plugins.workflow.steps.Step;
 import org.kohsuke.stapler.AncestorInPath;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.DataBoundSetter;
 import org.kohsuke.stapler.QueryParameter;
+import org.kohsuke.stapler.StaplerRequest;
 
 /**
  * @author Vivek Pandey
@@ -54,6 +63,38 @@ public DescriptorImpl() {
             super(BuildTriggerStepExecution.class);
         }
 
+        @Override public Step newInstance(StaplerRequest req, JSONObject formData) throws FormException {
+            BuildTriggerStep step = (BuildTriggerStep) super.newInstance(req, formData);
+            // Cf. ParametersDefinitionProperty._doBuild:
+            JSONArray params = formData.optJSONArray("parameter");
+            if (params != null) {
+                Jenkins jenkins = Jenkins.getInstance();
+                Job<?,?> job = jenkins != null ? jenkins.getItemByFullName(step.getJob(), Job.class) : null;
+                if (job != null) {
+                    ParametersDefinitionProperty pdp = job.getProperty(ParametersDefinitionProperty.class);
+                    if (pdp != null) {
+                        List<ParameterValue> values = new ArrayList<ParameterValue>();
+                        for (Object o : params) {
+                            JSONObject jo = (JSONObject) o;
+                            String name = jo.getString("name");
+                            ParameterDefinition d = pdp.getParameterDefinition(name);
+                            if (d == null) {
+                                throw new IllegalArgumentException("No such parameter definition: " + name);
+                            }
+                            ParameterValue parameterValue = d.createValue(req, jo);
+                            if (parameterValue != null) {
+                                values.add(parameterValue);
+                            } else {
+                                throw new IllegalArgumentException("Cannot retrieve the parameter value: " + name);
+                            }
+                        }
+                        step.setParameters(values);
+                    }
+                }
+            }
+            return step;
+        }
+
         @Override
         public String getFunctionName() {
             return "build";

support/src/main/resources/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStep/DescriptorImpl/parameters.groovy

@@ -0,0 +1,27 @@
+package org.jenkinsci.plugins.workflow.support.steps.build.BuildTriggerStep;
+def st = namespace('jelly:stapler')
+def l = namespace('/lib/layout')
+l.ajax {
+    def jobName = request.getParameter('job')
+    if (jobName != null) {
+        def job = app.getItemByFullName(jobName, hudson.model.Job)
+        if (job != null) {
+            def pdp = job.getProperty(hudson.model.ParametersDefinitionProperty)
+            if (pdp != null) {
+                table(width: '100%', class: 'parameters') {
+                    for (parameterDefinition in pdp.parameterDefinitions) {
+                        tbody {
+                            st.include(it: parameterDefinition, page: parameterDefinition.descriptor.valuePage)
+                        }
+                    }
+                }
+            } else {
+                text("${job.fullDisplayName} is not parameterized")
+            }
+        } else {
+            text("no such job ${jobName}")
+        }
+    } else {
+        text('no job specified')
+    }
+}

support/src/main/resources/org/jenkinsci/plugins/workflow/support/steps/build/BuildTriggerStep/config.jelly

@@ -24,15 +24,30 @@ THE SOFTWARE.
 -->
 
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form">
+<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler">
+    <j:set var="jobFieldId" value="${h.generateId()}"/>
     <f:entry field="job" title="Project to Build">
-        <f:textbox/>
-    </f:entry>
-    <f:entry title="Parameters">
-        <!-- TODO JENKINS-26093 -->
-        <f:block><code>parameters</code> may be bound to a list of <code>ParameterValue</code>s.</f:block>
+        <f:textbox onblur="loadParams()" id="${jobFieldId}"/>
     </f:entry>
     <f:entry field="wait">
         <f:checkbox default="true" title="Wait for completion"/>
     </f:entry>
+    <f:entry title="Parameters">
+        <div id="params"/>
+    </f:entry>
+    <script>
+        function loadParams() {
+            var div = $$('params');
+            new Ajax.Request('${descriptor.descriptorUrl}/parameters?job=' + encodeURIComponent($$('${jobFieldId}').value), {
+                method : 'get',
+                onSuccess : function(x) {
+                    div.innerHTML = x.responseText;
+                    Behaviour.applySubtree(div);
+                },
+                onFailure : function(x) {
+                    div.innerHTML = "<b>ERROR</b>: Failed to load parameter definitions: " + x.statusText;
+                }
+            });
+        }
+    </script>
 </j:jelly>