[FIXED JENKINS-38047] Allow multiple positional parameters.

Phew. That was kind of nutty.
jenkinsci · Sep 8, 2016 · 080ae00 · 080ae00
1 parent 3d25a37
commit 080ae00
Show file tree

Hide file tree

Showing 13 changed files with 229 additions and 71 deletions.
diff --git a/SYNTAX.md b/SYNTAX.md
@@ -208,9 +208,6 @@ postBuild {
 * No semicolons as statement separators. Each statement has to be on its own line
 * Block must only consists of method call statements, assignment statements, or property reference statement
     * A property reference statement is treated as no-arg method invocation. So for example, `input` is treated as `input()`
-* Method calls statements have no parenthesis
-    * However, method calls appearing inside an expression tree must have parenthesis. See the `secret()` function as an example
-* Method calls with parameters must always use named parameters, even if there's just one parameter.
 * Expression has to be one of the following:
     * Literals (except class literals)
     * Numbers: `1`, `3`

diff --git a/.../org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTPositionalArgumentList.groovy b/.../org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTPositionalArgumentList.groovy
@@ -0,0 +1,74 @@
+/*
+ * The MIT License
+ *
+ * Copyright (c) 2016, CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.pipeline.modeldefinition.ast
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+import groovy.transform.EqualsAndHashCode
+import groovy.transform.ToString
+import net.sf.json.JSONArray
+import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator
+
+/**
+ * Represents the positional parameters for a step in a list of {@link ModelASTValue}s.
+ *
+ * @author Kohsuke Kawaguchi
+ * @author Andrew Bayer
+ */
+@ToString(includeSuper = true, includeSuperProperties = true)
+@EqualsAndHashCode(callSuper = true)
+@SuppressFBWarnings(value="SE_NO_SERIALVERSIONID")
+public final class ModelASTPositionalArgumentList extends ModelASTArgumentList {
+    List<ModelASTValue> arguments = []
+
+    public ModelASTPositionalArgumentList(Object sourceLocation) {
+        super(sourceLocation)
+    }
+
+    @Override
+    public JSONArray toJSON() {
+        JSONArray a = new JSONArray()
+
+        arguments.each { v ->
+            a.add(v.toJSON())
+        }
+        return a
+    }
+
+    @Override
+    public void validate(ModelValidator validator) {
+        // Nothing to validate directly
+        arguments.each { v ->
+            v?.validate(validator)
+        }
+
+    }
+
+    @Override
+    public String toGroovy() {
+        return arguments.collect { v ->
+            v.toGroovy()
+        }.join(", ")
+    }
+}
diff --git a/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy b/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTValue.groovy
@@ -100,7 +100,11 @@ public abstract class ModelASTValue extends ModelASTElement {
 
             @Override
             public String toGroovy() {
-                return gstring
+                if (gstring.startsWith('${') && gstring.endsWith('}')) {
+                    return gstring.substring(2, gstring.length() - 1)
+                } else {
+                    return gstring
+                }
             }
         }
     }

diff --git a/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy b/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/JSONParser.groovy
@@ -36,6 +36,7 @@ import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTBranch
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTEnvironment
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTKey
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTNamedArgumentList
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPositionalArgumentList
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTSingleArgument
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStage
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStages
@@ -186,16 +187,32 @@ class JSONParser {
     public @CheckForNull ModelASTArgumentList parseArgumentList(Object o) {
         ModelASTArgumentList argList
         if (o instanceof JSONArray) {
-            argList = new ModelASTNamedArgumentList(o)
 
-            o.each { rawEntry ->
-                JSONObject entry = (JSONObject) rawEntry
-                // Passing the whole thing to parseKey to capture the JSONObject the "key" is in.
-                ModelASTKey key = parseKey(entry)
-
-                ModelASTValue value = parseValue(entry.getJSONObject("value"))
-
-                ((ModelASTNamedArgumentList)argList).arguments.put(key, value)
+            if (o.isEmpty()) {
+                argList = new ModelASTNamedArgumentList(o)
+            } else {
+                JSONObject firstElem = o.getJSONObject(0)
+                // If this is true, then we've got named parameters.
+                if (firstElem != null && firstElem.size() == 2 && firstElem.has("key") && firstElem.has("value")) {
+                    argList = new ModelASTNamedArgumentList(o)
+                    o.each { rawEntry ->
+                        JSONObject entry = (JSONObject) rawEntry
+                        // Passing the whole thing to parseKey to capture the JSONObject the "key" is in.
+                        ModelASTKey key = parseKey(entry)
+
+                        ModelASTValue value = parseValue(entry.getJSONObject("value"))
+
+                        ((ModelASTNamedArgumentList) argList).arguments.put(key, value)
+                    }
+                }
+                // Otherwise, we've got positional parameters.
+                else {
+                    argList = new ModelASTPositionalArgumentList(o)
+                    o.each { rawValue ->
+                        ModelASTValue value = parseValue((JSONObject) rawValue)
+                        ((ModelASTPositionalArgumentList)argList).arguments.add(value)
+                    }
+                }
             }
         } else if (o instanceof JSONObject) {
             argList = new ModelASTSingleArgument(o)

diff --git a/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy b/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy
@@ -38,6 +38,7 @@ import org.jenkinsci.plugins.pipeline.modeldefinition.ModelStepLoader
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTArgumentList
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTEnvironment
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTNamedArgumentList
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPositionalArgumentList
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTSingleArgument
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStage
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStages
@@ -431,9 +432,10 @@ class ModelParser {
                 return singleArg
             }
         default:
-            // positional args not allowed
-            ModelASTNamedArgumentList l = new ModelASTNamedArgumentList(args[1]);
-            errorCollector.error(l,"Expecting named arguments");
+            ModelASTPositionalArgumentList l = new ModelASTPositionalArgumentList(args[0]);
+            args.each { e ->
+                l.arguments.add(parseArgument(e))
+            }
             return l
         }
     }
@@ -614,6 +616,10 @@ class ModelParser {
         return null;
     }
 
+    protected String getSourceText(BinaryExpression e) {
+        return getSourceText(e.leftExpression) + e.operation.getText() + getSourceText(e.rightExpression)
+    }
+
     /**
      * Obtains the source text of the given {@link org.codehaus.groovy.ast.ASTNode}.
      */

diff --git a/...ain/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy b/...ain/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidator.groovy
@@ -39,6 +39,7 @@ import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTKey
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTNamedArgumentList
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTNotifications
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPipelineDef
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPositionalArgumentList
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTSingleArgument
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStage
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTStages
@@ -53,6 +54,7 @@ import org.jenkinsci.plugins.pipeline.modeldefinition.model.Agent
 import org.jenkinsci.plugins.pipeline.modeldefinition.model.Tools
 import org.jenkinsci.plugins.structs.SymbolLookup
 import org.jenkinsci.plugins.structs.describable.DescribableModel
+import org.jenkinsci.plugins.structs.describable.DescribableParameter
 import org.jenkinsci.plugins.workflow.steps.Step
 import org.jenkinsci.plugins.workflow.steps.StepDescriptor
 
@@ -227,6 +229,22 @@ class ModelValidator {
                                 valid = false
                             }
                         }
+                    } else if (step.args instanceof ModelASTPositionalArgumentList) {
+                        ModelASTPositionalArgumentList argList = (ModelASTPositionalArgumentList) step.args
+
+                        List<DescribableParameter> requiredParams = model.parameters.findAll { it.isRequired() }
+
+                        if (requiredParams.size() != argList.arguments.size()) {
+                            errorCollector.error(step, "Step '${step.name}' should have ${requiredParams.size()} arguments but has ${argList.arguments.size()} arguments instead.")
+                            valid = false
+                        } else {
+                            requiredParams.eachWithIndex { DescribableParameter entry, int i ->
+                                def argVal = argList.arguments.get(i)
+                                if (!validateParameterType(argVal, entry.erasedType)) {
+                                    valid = false
+                                }
+                            }
+                        }
                     } else {
                         assert step.args instanceof ModelASTSingleArgument;
                         ModelASTSingleArgument arg = (ModelASTSingleArgument) step.args;

diff --git a/src/main/resources/ast-schema.json b/src/main/resources/ast-schema.json
@@ -40,6 +40,13 @@
         "$ref": "#/definitions/argumentValue"
       }
     },
+    "positionalArgumentList": {
+      "description": "List of values used as positional parameters",
+      "type": "array",
+      "items": {
+        "$ref": "#/definitions/rawArgument"
+      }
+    },
     "singleArgument": {
       "description": "A single value used as a sole unnamed parameter",
       "$ref": "#/definitions/rawArgument"
@@ -52,6 +59,9 @@
         },
         {
           "$ref": "#/definitions/singleArgument"
+        },
+        {
+          "$ref": "#/definitions/positionalArgumentList"
         }
       ]
     },

diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
@@ -81,6 +81,7 @@ public abstract class AbstractModelDefTest {
     @Before
     public void setUp() throws Exception {
         ToolInstallations.configureMaven3();
+        initGlobalLibrary();
     }
 
     public static final List<String> SHOULD_PASS_CONFIGS = ImmutableList.of(
@@ -99,7 +100,8 @@ public void setUp() throws Exception {
             "simpleNotification",
             "simplePostBuild",
             "simpleTools",
-            "legacyMetaStepSyntax"
+            "legacyMetaStepSyntax",
+            "globalLibrarySuccess"
     );
 
     public static Iterable<Object[]> configsWithErrors() {

diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java
@@ -150,13 +150,13 @@ public void globalLibrarySuccess() throws Exception {
 
         j.assertLogContains("[nothing here]", b);
         j.assertLogContains("map call(1,2)", b);
-        /* TODO called the wrong way: closure1([a:1])
+        /* TODO called the wrong way: closure1([a:1]) */
         j.assertLogContains("closure1(1)", b);
-        */
+
         j.assertLogContains("running inside closure1", b);
-        /* TODO silently ignored
+        /* TODO silently ignored */
         j.assertLogContains("closure2(1, 2)", b);
         j.assertLogContains("running inside closure2", b);
-        */
+
     }
 }
diff --git a/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java b/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
@@ -232,19 +232,6 @@ public void globalLibraryObjectMethodCall() throws Exception {
     }
 
     @Test
-    public void globalLibraryUnnamedParameters() throws Exception {
-        // Test the case of calling a function with multiple unnamed parameters. This will fail.
-        prepRepoWithJenkinsfile("errors", "globalLibraryUnnamedParameters");
-
-        initGlobalLibrary();
-
-        WorkflowRun b = getAndStartBuild();
-
-        j.assertBuildStatus(Result.FAILURE, j.waitForCompletion(b));
-        j.assertLogContains("MultipleCompilationErrorsException: startup failed:", b);
-        j.assertLogContains("Expecting named arguments @ line", b);
-    }
-
     public void packageShouldNotSkipParsing() throws Exception {
         prepRepoWithJenkinsfile("errors", "packageShouldNotSkipParsing");
 

diff --git a/src/test/resources/errors/globalLibraryUnnamedParameters.groovy b/src/test/resources/errors/globalLibraryUnnamedParameters.groovy
diff --git a/src/test/resources/globalLibrarySuccess.groovy b/src/test/resources/globalLibrarySuccess.groovy
@@ -36,16 +36,16 @@ pipeline {
 
             // acmeVar.baz() will work since it doesn't rely on anything else - demonstrating that it will work as a step
             // argument, if not as a step itself.
-            echo '['+acmeVar.baz()+']'
+            echo('['+acmeVar.baz()+']')
 
             // TODO: Unnamed parameters won't work.
             //acmeFunc(1,2)
 
-            acmeFuncClosure1(a: 1) {
+            acmeFuncClosure1(1) {
                 echo 'running inside closure1'
             }
 
-            acmeFuncClosure2(a: 1, b: 2) {
+            acmeFuncClosure2(1, 2) {
                 echo 'running inside closure2'
             }