Skip to content
Permalink
Browse files
[FIXED JENKINS-38047] Allow multiple positional parameters.
Phew. That was kind of nutty.
  • Loading branch information
abayer committed Sep 8, 2016
1 parent 3d25a37 commit 080ae00ba0c5c0b301a98019b1c69bb604a2c83a
@@ -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`
@@ -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(", ")
}
}
@@ -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
}
}
}
}
@@ -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)
@@ -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}.
*/
@@ -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;
@@ -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"
}
]
},
@@ -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() {
@@ -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);
*/

}
}
@@ -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");

This file was deleted.

@@ -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'
}

0 comments on commit 080ae00

Please sign in to comment.