Skip to content
Permalink
Browse files

[FIXED JENKINS-47781] Narrow model lookups when appropriate

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 committed Nov 2, 2017
1 parent 31c504a commit 73326536fc49dfcbb199f6926fd75f96e098b853
@@ -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;

@@ -80,14 +81,24 @@ public synchronized void invalidateAll() {
}

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) {
@@ -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) {
@@ -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>
@@ -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
@@ -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

@@ -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 []
}
}
}
@@ -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

/**
@@ -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) {
@@ -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();
}
}
@@ -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"
}
}
}
}



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

0 comments on commit 7332653

Please sign in to comment.
You can’t perform that action at this time.