Skip to content

Commit

Permalink
[FIXED JENKINS-47781] Narrow model lookups when appropriate
Browse files Browse the repository at this point in the history 
The original problem here is that if you just look up describables by
the "upstream" symbol, you'll get one from Copy Artifact before you
get `ReverseBuildTrigger`. Which...ok. Except that our syntax checking
treats kicks in for the one from Copy Artifact! So let's narrow our
model lookups when appropriate.

I don't *love* this solution - it could be smarter and have a better
understanding of context. But this works for now, so I'll go with it.
  • Loading branch information
@abayer
abayer committed Nov 2, 2017
1 parent 31c504a commit 7332653
Show file tree
Hide file tree
Showing 7 changed files with 154 additions and 25 deletions.
79 changes: 57 additions & 22 deletions ...i/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/DescriptorLookupCache.java
View file
Expand Up @@ -37,6 +37,7 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.CheckForNull;
import java.util.LinkedHashMap;
import java.util.Map;

Expand Down Expand Up @@ -80,14 +81,24 @@ public synchronized DescribableModel<? extends Step> modelForStep(String n) {
}

public synchronized DescribableModel<? extends Describable> modelForDescribable(String n) {
if (!describableModelMap.containsKey(n)) {
final Descriptor<? extends Describable> function = lookupFunction(n);
Class<? extends Describable> c = (function == null ? null : function.clazz);
describableModelMap.put(n, c != null ? new DescribableModel<>(c) : null);
}
return modelForDescribable(n, null);
}

public synchronized DescribableModel<? extends Describable> modelForDescribable(String n, @CheckForNull Class<? extends Describable> describable) {
if (describable == null) {
if (!describableModelMap.containsKey(n)) {
final Descriptor<? extends Describable> function = lookupFunction(n);
Class<? extends Describable> c = (function == null ? null : function.clazz);
describableModelMap.put(n, c != null ? new DescribableModel<>(c) : null);
}

return describableModelMap.get(n);
return describableModelMap.get(n);
} else {
// TODO: Cache models for parent describables too
final Descriptor<? extends Describable> function = lookupFunction(n, describable);
Class<? extends Describable> c = (function == null ? null : function.clazz);
return c != null ? new DescribableModel<>(c) : null;
}
}

public synchronized StepDescriptor lookupStepDescriptor(String n) {
Expand All @@ -100,50 +111,74 @@ public synchronized StepDescriptor lookupStepDescriptor(String n) {
}

public synchronized Descriptor<? extends Describable> lookupFunction(String n) {
if (!describableMap.containsKey(n)) {
try {
Descriptor<? extends Describable> d = SymbolLookup.get().findDescriptor(Describable.class, n);
describableMap.put(n, d);
} catch (NullPointerException e) {
describableMap.put(n, null);
}
return lookupFunction(n, null);
}

}
public synchronized Descriptor<? extends Describable> lookupFunction(String n, @CheckForNull Class<? extends Describable> describable) {
if (describable == null) {
if (!describableMap.containsKey(n)) {
try {
Descriptor<? extends Describable> d = SymbolLookup.get().findDescriptor(Describable.class, n);
describableMap.put(n, d);
} catch (NullPointerException e) {
describableMap.put(n, null);
}

}

return describableMap.get(n);
return describableMap.get(n);
} else {
// TODO: Switch to caching when we're looking up specific describables
return SymbolLookup.get().findDescriptor(describable, n);
}
}

public synchronized Descriptor<? extends Describable> lookupStepFirstThenFunction(String name) {
return lookupStepDescriptor(name) != null ? lookupStepDescriptor(name) : lookupFunction(name);
return lookupStepFirstThenFunction(name, null);
}

public synchronized Descriptor<? extends Describable> lookupFunctionFirstThenStep(String name) {
return lookupFunction(name) != null ? lookupFunction(name) : lookupStepDescriptor(name);
return lookupFunctionFirstThenStep(name, null);
}

public synchronized DescribableModel<? extends Describable> modelForStepFirstThenFunction(String name) {
return modelForStepFirstThenFunction(name, null);
}

public synchronized DescribableModel<? extends Describable> modelForFunctionFirstThenStep(String name) {
return modelForFunctionFirstThenStep(name, null);
}

public synchronized Descriptor<? extends Describable> lookupStepFirstThenFunction(String name, Class<? extends Describable> describable) {
return lookupStepDescriptor(name) != null ? lookupStepDescriptor(name) : lookupFunction(name, describable);
}

public synchronized Descriptor<? extends Describable> lookupFunctionFirstThenStep(String name, Class<? extends Describable> describable) {
return lookupFunction(name, describable) != null ? lookupFunction(name, describable) : lookupStepDescriptor(name);
}

public synchronized DescribableModel<? extends Describable> modelForStepFirstThenFunction(String name, Class<? extends Describable> describable) {
Descriptor<? extends Describable> desc = lookupStepDescriptor(name);
DescribableModel<? extends Describable> model = null;

if (desc != null) {
model = modelForStep(name);
} else {
desc = lookupFunction(name);
desc = lookupFunction(name, describable);
if (desc != null) {
model = modelForDescribable(name);
model = modelForDescribable(name, describable);
}
}

return model;
}

public synchronized DescribableModel<? extends Describable> modelForFunctionFirstThenStep(String name) {
Descriptor<? extends Describable> desc = lookupFunction(name);
public synchronized DescribableModel<? extends Describable> modelForFunctionFirstThenStep(String name, Class<? extends Describable> describable) {
Descriptor<? extends Describable> desc = lookupFunction(name, describable);
DescribableModel<? extends Describable> model = null;

if (desc != null) {
model = modelForDescribable(name);
model = modelForDescribable(name, describable);
} else {
desc = lookupStepDescriptor(name);
if (desc != null) {
Expand Down
5 changes: 5 additions & 0 deletions pipeline-model-definition/pom.xml
View file
Expand Up @@ -219,6 +219,11 @@
<artifactId>ant</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>copyartifact</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
Expand Down
19 changes: 19 additions & 0 deletions ...el-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy
View file
Expand Up @@ -48,8 +48,13 @@ import org.jenkinsci.plugins.pipeline.StageTagsMetadata
import org.jenkinsci.plugins.pipeline.SyntheticStage
import org.jenkinsci.plugins.pipeline.modeldefinition.actions.DeclarativeJobPropertyTrackerAction
import org.jenkinsci.plugins.pipeline.modeldefinition.actions.ExecutionModelAction
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTBuildParameter
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTMethodCall
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTOption
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTTrigger
import org.jenkinsci.plugins.pipeline.modeldefinition.model.Environment
import org.jenkinsci.plugins.pipeline.modeldefinition.model.StepsBlock
import org.jenkinsci.plugins.pipeline.modeldefinition.options.DeclarativeOption
import org.jenkinsci.plugins.pipeline.modeldefinition.steps.CredentialWrapper
import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.Whitelisted
import org.jenkinsci.plugins.structs.SymbolLookup
Expand All @@ -74,6 +79,7 @@ import org.jenkinsci.plugins.workflow.job.WorkflowJob
import org.jenkinsci.plugins.workflow.job.WorkflowRun
import org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty
import org.jenkinsci.plugins.workflow.steps.FlowInterruptedException
import org.jenkinsci.plugins.workflow.steps.Step
import org.jenkinsci.plugins.workflow.steps.StepDescriptor
import org.jenkinsci.plugins.workflow.support.steps.StageStep

Expand Down Expand Up @@ -712,4 +718,17 @@ public class Utils {

return result.toString().trim();
}

@Nonnull
static List<Class<? extends Describable>> parentsForMethodCall(@Nonnull ModelASTMethodCall meth) {
if (meth instanceof ModelASTTrigger) {
return [Trigger.class]
} else if (meth instanceof ModelASTBuildParameter) {
return [ParameterDefinition.class]
} else if (meth instanceof ModelASTOption) {
return [JobProperty.class, DeclarativeOption.class, Step.class]
} else {
return []
}
}
}
23 changes: 20 additions & 3 deletions ...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
View file
Expand Up @@ -50,6 +50,7 @@ import org.jenkinsci.plugins.structs.describable.DescribableModel
import org.jenkinsci.plugins.structs.describable.DescribableParameter
import org.jenkinsci.plugins.workflow.flow.FlowExecution

import javax.annotation.CheckForNull
import javax.annotation.Nonnull

/**
Expand Down Expand Up @@ -386,10 +387,26 @@ class ModelValidatorImpl implements ModelValidator {
boolean valid = true

if (Jenkins.getInstance() != null) {
Descriptor desc = lookup.lookupFunctionFirstThenStep(meth.name)
DescribableModel<? extends Describable> model
if (desc != null) {
model = lookup.modelForFunctionFirstThenStep(meth.name)

List<Class<? extends Describable>> parentDescribables = Utils.parentsForMethodCall(meth)

if (!parentDescribables.isEmpty()) {
model = parentDescribables.collect { p ->
Descriptor fromParent = lookup.lookupFunctionFirstThenStep(meth.name, p)
if (fromParent != null) {
def m = lookup.modelForFunctionFirstThenStep(meth.name, p)
return m
} else {
return null
}
}.find { it != null }
} else {
Descriptor desc = lookup.lookupFunctionFirstThenStep(meth.name)

if (desc != null) {
model = lookup.modelForFunctionFirstThenStep(meth.name)
}
}

if (model != null) {
Expand Down
8 changes: 8 additions & 0 deletions ...efinition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
View file
Expand Up @@ -770,4 +770,12 @@ public void whenContainingNonCondition() throws Exception {
.logContains(Messages.ModelParser_ExpectedWhen())
.go();
}

@Issue("JENKINS-47781")
@Test
public void specificDescribableMatch() throws Exception {
expectError("specificDescribableMatch")
.logContains(Messages.ModelValidatorImpl_InvalidStepParameter("upstreamWhat", "upstreamProjects"))
.go();
}
}
40 changes: 40 additions & 0 deletions pipeline-model-definition/src/test/resources/errors/specificDescribableMatch.groovy
View file
@@ -0,0 +1,40 @@
/*
* The MIT License
*
* Copyright (c) 2017, 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
triggers {
upstream(upstreamWhat:"monkey")
}
stages {
stage("foo") {
steps {
echo "hello"
}
}
}
}



5 changes: 5 additions & 0 deletions pom.xml
View file
Expand Up @@ -253,6 +253,11 @@
<artifactId>ant</artifactId>
<version>1.5</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>copyartifact</artifactId>
<version>1.39</version>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down

0 comments on commit 7332653

Please sign in to comment.