[FIXED JENKINS-40418] Fix validation for a number of extended types

Turns out we do need validate methods on child classes - the parent validate method will end up calling validator.validateElement(ParentClass) which is...not what we want. So, fixing that in a bunch of places and adding tests. Also, added an expectError convenience method.
jenkinsci · Dec 13, 2016 · 6bfbffd · 6bfbffd
1 parent 6c8318a
commit 6bfbffd
Show file tree

Hide file tree

Showing 12 changed files with 250 additions and 55 deletions.
diff --git a/.../main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTBuildParameter.java b/.../main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTBuildParameter.java
@@ -3,6 +3,7 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import groovy.transform.EqualsAndHashCode;
 import groovy.transform.ToString;
+import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
 
 /**
  * A single parameter definition, eventually corresponding to a {@code ParameterDefinition}
@@ -14,6 +15,12 @@ public ModelASTBuildParameter(Object sourceLocation) {
         super(sourceLocation);
     }
 
+    @Override
+    public void validate(final ModelValidator validator) {
+        validator.validateElement(this);
+        super.validate(validator);
+    }
+
     @Override
     public String toString() {
         return "ModelASTBuildParameter{" + super.toString() + "}";

diff --git a/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTJobProperty.java b/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTJobProperty.java
@@ -1,5 +1,7 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
+import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
+
 /**
  * A single job property, corresponding eventually to {@code JobProperty}
  *
@@ -10,6 +12,12 @@ public ModelASTJobProperty(Object sourceLocation) {
         super(sourceLocation);
     }
 
+    @Override
+    public void validate(final ModelValidator validator) {
+        validator.validateElement(this);
+        super.validate(validator);
+    }
+
     @Override
     public String toString() {
         return "ModelASTJobProperty{"+super.toString()+"}";

diff --git a/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTScriptBlock.java b/...src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTScriptBlock.java
@@ -1,5 +1,7 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
+import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
+
 /**
  * Represents the special step for {@code ScriptStep}, which are executed without validation against the declarative subset.
  *
@@ -9,4 +11,9 @@ public class ModelASTScriptBlock extends AbstractModelASTCodeBlock {
     public ModelASTScriptBlock(Object sourceLocation) {
         super(sourceLocation, "script");
     }
+
+    @Override
+    public void validate(final ModelValidator validator) {
+        // no-op - we don't do validation of script blocks
+    }
 }
diff --git a/...api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTTrigger.java b/...api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTTrigger.java
@@ -1,5 +1,7 @@
 package org.jenkinsci.plugins.pipeline.modeldefinition.ast;
 
+import org.jenkinsci.plugins.pipeline.modeldefinition.validator.ModelValidator;
+
 /**
  * A single trigger, corresponding eventually to a {@code Trigger}
  *
@@ -10,6 +12,12 @@ public ModelASTTrigger(Object sourceLocation) {
         super(sourceLocation);
     }
 
+    @Override
+    public void validate(final ModelValidator validator) {
+        validator.validateElement(this);
+        super.validate(validator);
+    }
+
     @Override
     public String toString() {
         return "ModelASTTrigger{" +

diff --git a/...api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWrapper.java b/...api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTWrapper.java
@@ -39,9 +39,7 @@ public ModelASTWrapper(Object sourceLocation) {
     @Override
     public void validate(final ModelValidator validator) {
         validator.validateElement(this);
-        for (ModelASTMethodArg arg : getArgs()) {
-            arg.validate(validator);
-        }
+        super.validate(validator);
     }
 
     @Override

diff --git a/...est/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorTest.java b/...est/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorTest.java
@@ -4,8 +4,12 @@
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTBranch;
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTBuildCondition;
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTBuildConditionsContainer;
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTMethodArg;
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTMethodCall;
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPostBuild;
 import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPostStage;
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTTrigger;
+import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTValue;
 import org.junit.Test;
 import org.mockito.InOrder;
 import org.mockito.Mockito;
@@ -44,4 +48,18 @@ public void ensure_postStage_calls_own_validator_and_super() {
         inOrder.verify(validator, Mockito.times(1)).validateElement(branch);
     }
 
+    @Test
+    public void triggerValidateCallsRightMethod() throws Exception {
+        ModelValidator validator = Mockito.mock(ModelValidator.class);
+
+        ModelASTTrigger trigger = new ModelASTTrigger(null);
+        ModelASTMethodArg arg = ModelASTValue.fromConstant("bar", null);
+        trigger.setArgs(Collections.singletonList(arg));
+        trigger.setName("foo");
+        trigger.validate(validator);
+        InOrder inOrder = Mockito.inOrder(validator);
+        inOrder.verify(validator, Mockito.times(1)).validateElement(trigger);
+        inOrder.verify(validator, Mockito.times(1)).validateElement((ModelASTMethodCall)trigger);
+    }
+
 }
diff --git a/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy b/...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
@@ -342,7 +342,6 @@ class ModelValidatorImpl implements ModelValidator {
 
     public boolean validateElement(@Nonnull ModelASTTrigger trig) {
         boolean valid = true
-
         if (trig.name == null) {
             // This means that we failed at compilation time so can move on.
         }

diff --git a/...on/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java b/...on/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
@@ -404,6 +404,10 @@ protected ExpectationsBuilder expect(String resourceParent, String resource) {
         return new ExpectationsBuilder(resourceParent, resource);
     }
 
+    protected ExpectationsBuilder expectError(String resource) {
+        return expect(Result.FAILURE, "errors", resource);
+    }
+
     protected ExpectationsBuilder expect(Result result, String resourceParent, String resource) {
         return new ExpectationsBuilder(result, resourceParent, resource);
     }