[FIXED JENKINS-42498] Stop evaluating when conditions at runtime

Pull the when conditions in by translating them from the AST model, which does require some evaluation of strings here and there, but avoids having any Closures in the Describables.
jenkinsci · Mar 10, 2017 · 5cd7237 · 5cd7237
1 parent 0a0b0d7
commit 5cd7237
Show file tree

Hide file tree

Showing 19 changed files with 238 additions and 32 deletions.
diff --git a/...in/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/AbstractModelASTCodeBlock.java b/...in/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/AbstractModelASTCodeBlock.java
@@ -50,7 +50,7 @@ public String toGroovy() {
         return result.toString();
     }
 
-    protected String codeBlockAsString() {
+    public String codeBlockAsString() {
         if (getArgs() == null) {
             return null;
         } else if (isLiteralSingleArg()) {

diff --git a/...rc/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTArgumentList.java b/...rc/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTArgumentList.java
@@ -1,5 +1,10 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
+import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
+
+import java.util.List;
+import java.util.Map;
+
 /**
  * Either single value, or named args
  *
@@ -32,6 +37,8 @@ public boolean equals(Object o) {
 
     }
 
+    public abstract Map<String,?> argListToMap();
+
     @Override
     public int hashCode() {
         return super.hashCode();

diff --git a/...in/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTNamedArgumentList.java b/...in/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTNamedArgumentList.java
@@ -103,6 +103,17 @@ public void setArguments(Map<ModelASTKey, ModelASTValue> arguments) {
         this.arguments = arguments;
     }
 
+    @Override
+    public Map<String,?> argListToMap() {
+        Map<String,Object> m = new LinkedHashMap<>();
+
+        for (Map.Entry<ModelASTKey,ModelASTValue> entry : arguments.entrySet()) {
+            m.put(entry.getKey().getKey(), entry.getValue().getValue());
+        }
+
+        return m;
+    }
+
     @Override
     public String toString() {
         return "ModelASTNamedArgumentList{" +

diff --git a/...va/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTPositionalArgumentList.java b/...va/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTPositionalArgumentList.java
@@ -1,9 +1,13 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+
 import net.sf.json.JSONArray;
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
+import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
 
 /**
  * Represents the positional parameters for a step in a list of {@link ModelASTValue}s.
@@ -66,6 +70,16 @@ public void setArguments(List<ModelASTValue> arguments) {
         this.arguments = arguments;
     }
 
+    @Override
+    public Map<String,?> argListToMap() {
+        List<Object> argList = new ArrayList<>();
+        for (ModelASTValue v : arguments) {
+            argList.add(v.getValue());
+        }
+
+        return Collections.singletonMap(UninstantiatedDescribable.ANONYMOUS_KEY, argList);
+    }
+
     @Override
     public String toString() {
         return "ModelASTPositionalArgumentList{" +

diff --git a/.../main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTSingleArgument.java b/.../main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTSingleArgument.java
@@ -1,6 +1,10 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
+import org.jenkinsci.plugins.structs.describable.UninstantiatedDescribable;
+
+import java.util.Collections;
+import java.util.Map;
 
 /**
  * Represents a single unnamed argument.
@@ -49,6 +53,12 @@ public void setValue(ModelASTValue value) {
         this.value = value;
     }
 
+    @Override
+    public Map<String,?> argListToMap() {
+        return Collections.singletonMap(UninstantiatedDescribable.ANONYMOUS_KEY,
+                getValue().getValue());
+    }
+
     @Override
     public String toString() {
         return "ModelASTSingleArgument{" +

diff --git a/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWhenContent.java b/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWhenContent.java
@@ -29,4 +29,5 @@
  * @author Andrew Bayer
  */
 public interface ModelASTWhenContent extends ModelASTMarkerInterface {
+    String getName();
 }
diff --git a/...el-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy b/...el-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy
@@ -37,9 +37,20 @@ import org.jenkinsci.plugins.pipeline.StageStatus
 import org.jenkinsci.plugins.pipeline.StageTagsMetadata
 import org.jenkinsci.plugins.pipeline.SyntheticStage
 import org.jenkinsci.plugins.pipeline.modeldefinition.actions.ExecutionModelAction
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTArgumentList
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTNamedArgumentList
 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
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTWhen
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTWhenCondition
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTWhenContent
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTWhenExpression
 import org.jenkinsci.plugins.pipeline.modeldefinition.model.MethodsToList
+import org.jenkinsci.plugins.pipeline.modeldefinition.model.Root
+import org.jenkinsci.plugins.pipeline.modeldefinition.model.Stage
 import org.jenkinsci.plugins.pipeline.modeldefinition.model.StageConditionals
 import org.jenkinsci.plugins.pipeline.modeldefinition.model.StepsBlock
 import org.jenkinsci.plugins.pipeline.modeldefinition.parser.Converter
@@ -200,19 +211,100 @@ public class Utils {
         }
     }
 
-    static void attachDeclarativeActions(CpsScript script) {
+    static Root attachDeclarativeActions(@Nonnull Root root, CpsScript script) {
         WorkflowRun r = script.$build()
         ModelASTPipelineDef model = Converter.parseFromWorkflowRun(r)
 
-        ModelASTStages stages = model.stages
+        if (model != null) {
+            ModelASTStages stages = model.stages
 
-        stages.removeSourceLocation()
-        if (r.getAction(SyntheticStageGraphListener.GraphListenerAction.class) == null) {
-            r.addAction(new SyntheticStageGraphListener.GraphListenerAction())
+            stages.removeSourceLocation()
+            if (r.getAction(SyntheticStageGraphListener.GraphListenerAction.class) == null) {
+                r.addAction(new SyntheticStageGraphListener.GraphListenerAction())
+            }
+            if (r.getAction(ExecutionModelAction.class) == null) {
+                r.addAction(new ExecutionModelAction(stages))
+            }
+
+            if (root != null) {
+                root = populateWhen(root, model)
+            }
+        }
+
+        return root
+    }
+
+    /**
+     * Attaches the {@link StageConditionals} to the appropriate {@link Stage}s, pulling from the AST model.
+     *
+     * @param root
+     * @param model
+     * @return an updated {@link Root}
+     */
+    static Root populateWhen(@Nonnull Root root, @Nonnull ModelASTPipelineDef model) {
+        List<Stage> stagesWithWhen = []
+
+        root.stages.stages.each { s ->
+            ModelASTStage astStage = model.stages.stages.find { it.name == s.name }
+            List<DeclarativeStageConditional> conditions = []
+            if (astStage.when != null) {
+                astStage.when?.conditions?.each { c ->
+                    conditions.add(stageConditionalFromAST(c))
+                }
+                s.when(new StageConditionals(conditions))
+            }
+            stagesWithWhen.add(s)
+        }
+
+        root.stages.stages = stagesWithWhen
+
+        return root
+    }
+
+    /**
+     * Translates the {@link ModelASTWhenContent} into a {@link DeclarativeStageConditional}.
+     *
+     * @param w
+     * @return A populated {@link DeclarativeStageConditional}
+     */
+    private static DeclarativeStageConditional stageConditionalFromAST(ModelASTWhenContent w) {
+        DeclarativeStageConditional c = null
+        DeclarativeStageConditionalDescriptor desc = DeclarativeStageConditionalDescriptor.byName(w.name)
+
+        if (w instanceof ModelASTWhenCondition) {
+            if (desc.allowedNestedCount == 0) {
+                Object[] arg = new Object[1]
+                arg[0] = w.args?.argListToMap()
+                c = (DeclarativeStageConditional)getDescribable(w.name, desc.clazz, arg).instantiate()
+            } else if (desc.allowedNestedCount == 1) {
+                DeclarativeStageConditional single = stageConditionalFromAST(w.children.first())
+                c = (DeclarativeStageConditional)getDescribable(w.name, desc.clazz, single).instantiate()
+            } else {
+                List<DeclarativeStageConditional> nested = w.children.collect { stageConditionalFromAST(it) }
+                c = (DeclarativeStageConditional)getDescribable(w.name, desc.clazz, nested).instantiate()
+            }
+        } else if (w instanceof ModelASTWhenExpression) {
+            ModelASTWhenExpression expr = (ModelASTWhenExpression)w
+
+            c = (DeclarativeStageConditional)getDescribable(w.name, desc.clazz, expr.codeBlockAsString()).instantiate()
         }
-        if (r.getAction(ExecutionModelAction.class) == null) {
-            r.addAction(new ExecutionModelAction(stages))
+
+        return c
+    }
+
+    /**
+     * Takes a string and makes sure it starts/ends with double quotes so that it can be evaluated correctly.
+     *
+     * @param s The original string
+     * @return Either the original string, if it already starts/ends with double quotes, or the original string
+     * prepended/appended with double quotes.
+     */
+    public static String prepareForEvalToString(String s) {
+        String toEval = s
+        if (!toEval.startsWith('"') || !toEval.endsWith('"')) {
+            toEval = '"' + toEval + '"'
         }
+        return toEval
     }
 
     static Predicate<FlowNode> endNodeForStage(final StepStartNode startNode) {

diff --git a/...main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/StageConditionals.groovy b/...main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/StageConditionals.groovy
@@ -68,9 +68,7 @@ class StageConditionals implements MethodsToList<DeclarativeStageConditional<? e
 
     public List<DeclarativeStageConditional> conditions = []
 
-    public StageConditionals(List<UninstantiatedDescribable> input) {
-        input.each { i ->
-            conditions.add((DeclarativeStageConditional<? extends DeclarativeStageConditional>) i.instantiate())
-        }
+    public StageConditionals(List<DeclarativeStageConditional<? extends DeclarativeStageConditional>> inList) {
+        conditions.addAll(inList)
     }
 }
diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
@@ -218,6 +218,12 @@ class ModelValidatorImpl implements ModelValidator {
                 } else if (condition.children.size() != desc.getAllowedChildrenCount()) {
                     errorCollector.error(condition, Messages.ModelValidatorImpl_NestedWhenWrongChildrenCount(condition.name, desc.getAllowedChildrenCount()))
                     valid = false
+                } else {
+                    condition.children.each { c ->
+                        if (!c.validate(this)) {
+                            valid = false
+                        }
+                    }
                 }
             } else {
                 if (!condition.children.isEmpty()) {

diff --git a/...main/java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditional.java b/...main/java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditional.java
@@ -52,16 +52,20 @@ public BranchConditional(String compare) {
         this.compare = compare;
     }
 
-    public boolean branchMatches(String actualBranch) {
-        if (isEmpty(actualBranch) && isEmpty(this.compare)) {
+    public String getCompare() {
+        return compare;
+    }
+
+    public boolean branchMatches(String toCompare, String actualBranch) {
+        if (isEmpty(actualBranch) && isEmpty(toCompare)) {
             return true;
-        } else if (isEmpty(actualBranch) || isEmpty(this.compare)) {
+        } else if (isEmpty(actualBranch) || isEmpty(toCompare)) {
             return false;
         }
         // Replace the Git directory separator character (always '/')
         // with the platform specific directory separator before
         // invoking Ant's platform specific path matching.
-        String safeCompare = compare.replace('/', File.separatorChar);
+        String safeCompare = toCompare.replace('/', File.separatorChar);
         String safeName = actualBranch.replace('/', File.separatorChar);
         return SelectorUtils.matchPath(safeCompare, safeName, false);
     }

diff --git a/...java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/EnvironmentConditional.java b/...java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/EnvironmentConditional.java
@@ -73,15 +73,15 @@ public void setIgnoreCase(boolean ignoreCase) {
         this.ignoreCase = ignoreCase;
     }
 
-    public boolean environmentMatches(String var) {
-        if (isEmpty(var) && isEmpty(value)) {
+    public boolean environmentMatches(String v, String var) {
+        if (isEmpty(var) && isEmpty(v)) {
             return true;
         } else if (isEmpty(var)) {
             return false;
         } else if (ignoreCase) {
-            return var.equalsIgnoreCase(value);
+            return var.equalsIgnoreCase(v);
         } else {
-            return var.equals(value);
+            return var.equals(v);
         }
     }
 

diff --git a/.../java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/ExpressionConditional.java b/.../java/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/ExpressionConditional.java
@@ -36,14 +36,14 @@
  * As populated by {@link jenkins.branch.BranchNameContributor}
  */
 public class ExpressionConditional extends DeclarativeStageConditional<ExpressionConditional> {
-    private final StepsBlock block;
+    private final String block;
 
     @DataBoundConstructor
-    public ExpressionConditional(StepsBlock block) {
+    public ExpressionConditional(String block) {
         this.block = block;
     }
 
-    public StepsBlock getBlock() {
+    public String getBlock() {
         return block;
     }
 

diff --git a/...in/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ClosureModelTranslator.groovy b/...in/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ClosureModelTranslator.groovy
@@ -118,7 +118,7 @@ public class ClosureModelTranslator implements MethodMissingWrapper, Serializabl
             def actualType = Utils.actualFieldType(actualClass, methodName)
 
             // We care about the field name actually being a thing.
-            if (actualFieldName != null) {
+            if (actualFieldName != null && actualType != StageConditionals.class) {
                 // Due to Stage taking an argument, not just a closure, we need to handle it differently.
                 if (Utils.assignableFromWrapper(Stage.class, actualType)) {
                     Object[] origArgs = args

diff --git a/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy b/...src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy
@@ -61,7 +61,7 @@ public class ModelInterpreter implements Serializable {
 
         if (root != null) {
             // Attach the stages model to the run for introspection etc.
-            Utils.attachDeclarativeActions(script)
+            root = Utils.attachDeclarativeActions(root, script)
             boolean postBuildRun = false
 
             try {

diff --git a/...s/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditionalScript.groovy b/...s/org/jenkinsci/plugins/pipeline/modeldefinition/when/impl/BranchConditionalScript.groovy
@@ -25,6 +25,7 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.when.impl
 
+import org.jenkinsci.plugins.pipeline.modeldefinition.Utils
 import org.jenkinsci.plugins.pipeline.modeldefinition.when.DeclarativeStageConditionalScript
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
@@ -36,6 +37,7 @@ class BranchConditionalScript extends DeclarativeStageConditionalScript<BranchCo
 
     @Override
     public boolean evaluate() {
-        return describable.branchMatches(script.getProperty("env").getProperty("BRANCH_NAME"))
+        return describable.branchMatches((String)script.evaluate(Utils.prepareForEvalToString(describable.compare)),
+            (String)script.getProperty("env").getProperty("BRANCH_NAME"))
     }
 }
diff --git a/.../jenkinsci/plugins/pipeline/modeldefinition/when/impl/EnvironmentConditionalScript.groovy b/.../jenkinsci/plugins/pipeline/modeldefinition/when/impl/EnvironmentConditionalScript.groovy
@@ -25,6 +25,7 @@
 
 package org.jenkinsci.plugins.pipeline.modeldefinition.when.impl
 
+import org.jenkinsci.plugins.pipeline.modeldefinition.Utils
 import org.jenkinsci.plugins.pipeline.modeldefinition.when.DeclarativeStageConditionalScript
 import org.jenkinsci.plugins.workflow.cps.CpsScript
 
@@ -36,6 +37,8 @@ class EnvironmentConditionalScript extends DeclarativeStageConditionalScript<Env
 
     @Override
     public boolean evaluate() {
-        return describable.environmentMatches(script.getProperty("env").getProperty(describable.getName()))
+        String n = (String)script.evaluate(Utils.prepareForEvalToString(describable.getName()))
+        String v = (String)script.evaluate(Utils.prepareForEvalToString(describable.getValue()))
+        return describable.environmentMatches(v, (String)script.getProperty("env").getProperty(n))
     }
 }
diff --git a/...g/jenkinsci/plugins/pipeline/modeldefinition/when/impl/ExpressionConditionalScript.groovy b/...g/jenkinsci/plugins/pipeline/modeldefinition/when/impl/ExpressionConditionalScript.groovy
@@ -37,12 +37,10 @@ class ExpressionConditionalScript extends DeclarativeStageConditionalScript<Expr
     @Override
     public boolean evaluate() {
         Object retVal = null
-        Closure c = describable.block?.closure
+        String block = describable.block
 
-        if (c != null) {
-            c.delegate = script
-            c.resolveStrategy = Closure.DELEGATE_FIRST
-            retVal = c.call()
+        if (block != null) {
+            retVal = script.evaluate(block)
         }
 
         return retVal ? true : false