[FIXED JENKINS-39799] Properly handle bad content in post blocks.

We actually would have errored out properly-ish if it wasn't for the fact that we ended up with a ModelASTBuildCondition with null condition and null branch, leading to the NPE during compilation. I decided to improve the error messaging and make sure we don't attempt to validate further when condition or branch are null.
jenkinsci · Nov 17, 2016 · bfc38f9 · bfc38f9
1 parent 5b2c5b3
commit bfc38f9
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 4 deletions.
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
@@ -36,6 +36,7 @@ import org.codehaus.groovy.control.SourceUnit
 import org.codehaus.groovy.syntax.Types
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.*
 import org.jenkinsci.plugins.pipeline.modeldefinition.ModelStepLoader
+import org.jenkinsci.plugins.pipeline.modeldefinition.model.BuildCondition
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ErrorCollector
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator
 import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidatorImpl
@@ -675,7 +676,10 @@ class ModelParser {
             errorCollector.error(responder,"Expected a block");
         } else {
             eachStatement(m.body.code) {
-                responder.conditions.add(parseBuildCondition(it));
+                ModelASTBuildCondition bc = parseBuildCondition(it)
+                if (bc.condition != null && bc.branch != null) {
+                    responder.conditions.add(bc);
+                }
             }
         }
         return responder;
@@ -685,7 +689,8 @@ class ModelParser {
         ModelASTBuildCondition b = new ModelASTBuildCondition(st)
         def m = matchBlockStatement(st);
         if (m == null) {
-            errorCollector.error(b,"Expected a build condition")
+            errorCollector.error(b,"The 'post' section can only contain build condition names with code blocks. "
+                + "Valid condition names are " + BuildCondition.getOrderedConditionNames())
         } else {
             b.branch = parseBranch("default", asBlock(m.body.code))
 

diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
@@ -108,8 +108,9 @@ class ModelValidatorImpl implements ModelValidator {
     public boolean validateElement(@Nonnull ModelASTBuildCondition buildCondition) {
         boolean valid = true
 
-        // Only do the symbol lookup if we have a Jenkins instance
-        if (Jenkins.getInstance() != null) {
+        // Only do the symbol lookup if we have a Jenkins instance and condition/branch aren't null. That only happens
+        // when there's a failure at parse-time.
+        if (Jenkins.getInstance() != null && buildCondition.condition != null && buildCondition.branch != null) {
             if (SymbolLookup.get().find(BuildCondition.class, buildCondition.condition) == null) {
                 errorCollector.error(buildCondition, "Invalid condition '${buildCondition.condition}' - valid conditions are ${BuildCondition.getOrderedConditionNames()}")
                 valid = false

diff --git a/...efinition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java b/...efinition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
@@ -391,4 +391,16 @@ public void notificationsSectionRemoved() throws Exception {
                 .logContains("The 'notifications' section has been removed as of version 0.6. Use 'post' for all post-build actions.")
                 .go();
     }
+
+    @Issue("JENKINS-39799")
+    @Test
+    public void badPostContent() throws Exception {
+        expect(Result.FAILURE, "errors", "badPostContent")
+                .logContains("MultipleCompilationErrorsException: startup failed:",
+                        "The 'post' section can only contain build condition names with code blocks. "
+                                + "Valid condition names are " + BuildCondition.getOrderedConditionNames(),
+                        "post can not be empty")
+                .logNotContains("Caused by: java.lang.NullPointerException")
+                .go();
+    }
 }
diff --git a/pipeline-model-definition/src/test/resources/errors/badPostContent.groovy b/pipeline-model-definition/src/test/resources/errors/badPostContent.groovy
@@ -0,0 +1,40 @@
+/*
+ * 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.
+ */
+
+pipeline {
+    agent none
+    stages {
+        stage("foo") {
+            steps {
+                echo "hello"
+            }
+        }
+    }
+    post {
+        echo "I HAVE FINISHED"
+    }
+}
+
+
+