Concourse Reconciler: check for unused resource

This commit is contained in:
Kris De Volder
2017-05-03 17:19:31 -07:00
parent d7eda61c99
commit 642acea0d9
7 changed files with 202 additions and 34 deletions

View File

@@ -64,6 +64,12 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler {
private final YTypeUtil typeUtil;
private final ITypeCollector typeCollector;
private final YamlQuickfixes quickfixes;
private List<Runnable> delayedConstraints = new ArrayList<>();
// keeps track of dynamic constraints discovered during reconciler walk
// the constraints are validated at the end of the walk rather than during the walk.
// This facilitates constraints that depend on, for example, the contents of the ast type cache being
// populated prior to checking.
public SchemaBasedYamlASTReconciler(IProblemCollector problems, YamlSchema schema, ITypeCollector typeCollector, YamlQuickfixes quickfixes) {
this.problems = problems;
@@ -76,6 +82,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler {
@Override
public void reconcile(YamlFileAST ast) {
if (typeCollector!=null) typeCollector.beginCollecting(ast);
delayedConstraints.clear();
try {
List<Node> nodes = ast.getNodes();
IntegerRange expectedDocs = schema.expectedNumberOfDocuments();
@@ -102,6 +109,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler {
if (typeCollector!=null) {
typeCollector.endCollecting(ast);
}
verifyDelayedConstraints();
}
}
@@ -276,11 +284,19 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler {
//Check for other constraints attached to the type
for (Constraint constraint : typeUtil.getConstraints(type)) {
if (constraint!=null) {
constraint.verify(dc, parent, node, type, problems);
delayedConstraints.add(() -> {
constraint.verify(dc, parent, node, type, problems);
});
}
}
}
private void verifyDelayedConstraints() {
for (Runnable runnable : delayedConstraints) {
runnable.run();
}
delayedConstraints.clear();
}
protected NodeId getNodeId(Node node) {
NodeId id = node.getNodeId();

View File

@@ -10,19 +10,22 @@
*******************************************************************************/
package org.springframework.ide.vscode.concourse;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import org.springframework.ide.vscode.commons.util.Assert;
import org.springframework.ide.vscode.commons.yaml.ast.YamlFileAST;
import org.springframework.ide.vscode.commons.yaml.reconcile.ITypeCollector;
import org.springframework.ide.vscode.commons.yaml.schema.YType;
import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YAtomicType;
import org.yaml.snakeyaml.nodes.Node;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.Multimap;
/**
* An implementation of {@link ITypeCollector} which keeps track of the
@@ -32,6 +35,42 @@ import com.google.common.collect.ImmutableMap;
*/
public class ASTTypeCache implements ITypeCollector {
public interface NodeTypes {
Collection<Node> getNodes(YType type);
Map<Node, YType> getTypes();
}
/**
* Wraps around a {@link ImmutableMap}<Node, Type> and lazy builds the inverse
* map as needed.
*/
private static class NodeTypesImpl implements NodeTypes {
private ImmutableMap<Node, YType> node2type;
private Multimap<YType, Node> type2node = null; //lazy initialized when used.
public NodeTypesImpl(ImmutableMap<Node, YType> node2type) {
this.node2type = node2type;
}
@Override
public synchronized Collection<Node> getNodes(YType type) {
if (type2node==null) {
ImmutableMultimap.Builder<YType, Node> builder = ImmutableMultimap.builder();
for (Entry<Node, YType> e : node2type.entrySet()) {
builder.put(e.getValue(), e.getKey());
}
type2node = builder.build();
}
return type2node.get(type);
}
@Override
public Map<Node, YType> getTypes() {
return node2type;
}
}
/**
* Set upon commencing a reconciler session.
*/
@@ -43,7 +82,7 @@ public class ASTTypeCache implements ITypeCollector {
private ImmutableMap.Builder<Node, YType> currentTypes = null;
private final Set<YType> interestingTypes = new HashSet<>();
private final Map<String, ImmutableMap<Node, YType>> typeIndex = new HashMap<>();
private final Map<String, NodeTypes> typeIndex = new HashMap<>();
@Override
public void beginCollecting(YamlFileAST ast) {
@@ -56,7 +95,7 @@ public class ASTTypeCache implements ITypeCollector {
public synchronized void endCollecting(YamlFileAST ast) {
Assert.isLegal(currentAst==ast);
String uri = ast.getDocument().getUri();
typeIndex.put(uri, currentTypes.build());
typeIndex.put(uri, new NodeTypesImpl(currentTypes.build()));
this.currentAst = null;
this.currentTypes = null;
}
@@ -69,9 +108,9 @@ public class ASTTypeCache implements ITypeCollector {
}
public synchronized YType getType(YamlFileAST ast, Node node) {
ImmutableMap<Node, YType> types = typeIndex.get(ast.getDocument().getUri());
NodeTypes types = typeIndex.get(ast.getDocument().getUri());
if (types!=null) {
return types.get(node);
return types.getTypes().get(node);
}
return null;
}
@@ -84,7 +123,7 @@ public class ASTTypeCache implements ITypeCollector {
this.interestingTypes.add(type);
}
public synchronized ImmutableMap<Node, YType> getNodes(String uri) {
public synchronized NodeTypes getNodeTypes(String uri) {
return typeIndex.get(uri);
}

View File

@@ -61,7 +61,7 @@ public class ConcourseDocumentSymbolHandler implements DocumentSymbolHandler {
public List<? extends SymbolInformation> handle(DocumentSymbolParams params) {
Builder<SymbolInformation> builder = ImmutableList.builder();
TextDocument doc = documents.getDocument(params.getTextDocument().getUri());
for (Entry<Node, YType> entry : astTypeCache.getNodes(params.getTextDocument().getUri()).entrySet()) {
for (Entry<Node, YType> entry : astTypeCache.getNodeTypes(params.getTextDocument().getUri()).getTypes().entrySet()) {
if (definitionTypes.contains(entry.getValue())) {
try {
builder.add(createSymbol(doc, entry.getKey(), entry.getValue()));

View File

@@ -18,6 +18,7 @@ import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
@@ -27,6 +28,7 @@ import org.springframework.ide.vscode.commons.languageserver.util.SimpleLanguage
import org.springframework.ide.vscode.commons.languageserver.util.SnippetBuilder;
import org.springframework.ide.vscode.commons.util.Assert;
import org.springframework.ide.vscode.commons.util.Log;
import org.springframework.ide.vscode.commons.util.StringUtil;
import org.springframework.ide.vscode.commons.util.text.IDocument;
import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil;
import org.springframework.ide.vscode.commons.yaml.ast.YamlASTProvider;
@@ -44,9 +46,11 @@ import org.springframework.ide.vscode.commons.yaml.schema.YType;
import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory;
import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.AbstractType;
import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YBeanUnionType;
import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraint;
import org.springframework.ide.vscode.commons.yaml.schema.YTypedProperty;
import org.springframework.ide.vscode.commons.yaml.schema.YValueHint;
import org.springframework.ide.vscode.commons.yaml.util.Streams;
import org.springframework.ide.vscode.concourse.ASTTypeCache.NodeTypes;
import org.springframework.ide.vscode.concourse.util.CollectorUtil;
import org.springframework.ide.vscode.concourse.util.StaleFallbackCache;
import org.yaml.snakeyaml.Yaml;
@@ -66,6 +70,33 @@ import com.google.common.collect.Multiset;
*/
public class ConcourseModel {
/**
* Verification of a 'isUsed' contraint. Basically this consults the ast-type cache, (which should be
* fully populated at the end reconciling) to see if the nodes of any nodes of a given type (representing
* a 'use' of something, contain the value of the current node (which is supposed to be a definition of
* that same type of something).
*/
public Constraint isUsed(YType refType, String entityTypeName) {
getAstTypeCache().addInterestingType(refType); //ensure the type is tracked in the type-cache
return new Constraint() {
@Override
public void verify(DynamicSchemaContext dc, Node parent, Node node, YType type, IProblemCollector problems) {
String defName = NodeUtil.asScalar(node);
if (StringUtil.hasText(defName)) { //Avoid silly 'not used' errors for empty names (will have an other error already).
NodeTypes nodeTypes = getAstTypeCache().getNodeTypes(dc.getDocument().getUri());
if (nodeTypes!=null) {
Optional<Node> reference = nodeTypes.getNodes(refType).stream()
.filter(refNode -> defName.equals(NodeUtil.asScalar(refNode)))
.findAny();
if (!reference.isPresent()) {
problems.accept(YamlSchemaProblems.schemaProblem("Unused '"+entityTypeName+"'", node));
}
}
}
}
};
}
/**
* Verification of contraint: a job used in the 'passed' attribute of a step
* must interact with the resource in question.

View File

@@ -74,7 +74,7 @@ public class ConcourseValueParsers {
//okay
return input;
}
throw new IllegalArgumentException("Duplicate "+typeName+" '"+input+"'");
throw new ValueParseException("Duplicate "+typeName+" '"+input+"'");
};
};
};

View File

@@ -13,7 +13,6 @@ package org.springframework.ide.vscode.concourse;
import java.time.ZoneId;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector;
@@ -23,7 +22,6 @@ import org.springframework.ide.vscode.commons.util.Renderables;
import org.springframework.ide.vscode.commons.util.ValueParseException;
import org.springframework.ide.vscode.commons.util.ValueParser;
import org.springframework.ide.vscode.commons.util.ValueParsers;
import org.springframework.ide.vscode.commons.util.text.IDocument;
import org.springframework.ide.vscode.commons.util.text.LanguageId;
import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil;
import org.springframework.ide.vscode.commons.yaml.ast.YamlFileAST;
@@ -101,7 +99,7 @@ public class PipelineYmlSchema implements YamlSchema {
public final YType t_strictly_pos_integer = f.yatomic("Strictly Positive Integer")
.parseWith(ValueParsers.integerAtLeast(1));
public final YAtomicType t_resource_name;
public final AbstractType t_resource_name;
public final AbstractType t_job_name;
public final YAtomicType t_resource_type_name;
public final YType t_mime_type = f.yatomic("MimeType")
@@ -191,8 +189,9 @@ public class PipelineYmlSchema implements YamlSchema {
}
).require(models::passedJobHasInteractionWithResource);
YAtomicType resourceNameDef = f.yatomic("Resource Name");
resourceNameDef.parseWith(ConcourseValueParsers.resourceNameDef(models));
YAtomicType t_resource_name_def = f.yatomic("Resource Name");
t_resource_name_def.parseWith(ConcourseValueParsers.resourceNameDef(models));
t_resource_name_def.require(models.isUsed(t_resource_name, "Resource"));
YAtomicType jobNameDef = f.yatomic("Job Name");
jobNameDef.parseWith(ConcourseValueParsers.jobNameDef(models));
YAtomicType resourceTypeNameDef = f.yatomic("ResourceType Name");
@@ -203,7 +202,7 @@ public class PipelineYmlSchema implements YamlSchema {
);
AbstractType t_resource = f.ybean("Resource");
addProp(t_resource, "name", resourceNameDef).isRequired(true);
addProp(t_resource, "name", t_resource_name_def).isRequired(true);
addProp(t_resource, "type", t_resource_type_name).isRequired(true);
addProp(t_resource, "source", resourceSource);
addProp(t_resource, "check_every", t_duration);
@@ -372,7 +371,7 @@ public class PipelineYmlSchema implements YamlSchema {
definitionTypes = ImmutableList.of(
jobNameDef,
resourceTypeNameDef,
resourceNameDef
t_resource_name_def
);
initializeDefaultResourceTypes();

View File

@@ -54,7 +54,10 @@ public class ConcourseEditorTest {
" username: someone\n" +
"# Confuse"
);
Diagnostic problem = editor.assertProblems("-|'type' is required").get(0);
Diagnostic problem = editor.assertProblems(
"-|'type' is required",
"foo|Unused"
).get(0);
CodeAction quickfix = editor.assertCodeAction(problem);
assertEquals("Add property 'type'", quickfix.getLabel());
quickfix.perform();
@@ -77,7 +80,9 @@ public class ConcourseEditorTest {
" source:\n" +
" username: someone\n"
);
Diagnostic problem = editor.assertProblems("source|[branch, pool, uri] are required").get(0);
Diagnostic problem = editor.assertProblems(
"foo|Unused",
"source|[branch, pool, uri] are required").get(1);
CodeAction quickfix = editor.assertCodeAction(problem);
assertEquals("Add properties: [branch, pool, uri]", quickfix.getLabel());
quickfix.perform();
@@ -600,7 +605,10 @@ public class ConcourseEditorTest {
);
editor.assertProblems(
"sts4|Duplicate resource name",
"sts4|Duplicate resource name"
"sts4|Unused",
"utils|Unused",
"sts4|Duplicate resource name",
"sts4|Unused"
);
}
@@ -649,9 +657,9 @@ public class ConcourseEditorTest {
);
{
List<Diagnostic> problems = editor.assertProblems(
"config|[platform, run] are required",
"config|Only one of [config, file]",
"config|One of [image_resource, image]",
"config|[platform, run] are required",
"file|Only one of [config, file]"
);
//All of the problems in this example are property contraint violations! So all should be warnings.
@@ -753,7 +761,9 @@ public class ConcourseEditorTest {
editor.assertProblems(
"resources|Duplicate key",
"my-repo|Unused 'Resource'",
"resources|Duplicate key",
"your-repo|Unused 'Resource'",
"type|Duplicate key",
"type|Duplicate key"
);
@@ -888,7 +898,9 @@ public class ConcourseEditorTest {
" days:\n" +
" - Thursday\n"
);
editor.assertProblems(/*NONE*/);
editor.assertProblems(
"every5minutes|Unused"
);
editor = harness.newEditor(
"resources:\n" +
@@ -904,6 +916,7 @@ public class ConcourseEditorTest {
" - Someday\n"
);
editor.assertProblems(
"every5minutes|Unused",
"some-location|Unknown 'Location'",
"the-start-time|not a valid 'Time'",
"the-stop-time|not a valid 'Time'",
@@ -956,6 +969,7 @@ public class ConcourseEditorTest {
" gpg_keyserver: hkp://somekeyserver.net"
);
editor.assertProblems(
"sts4-out|Unused",
"bogus|Unknown property",
"not-a-list|Expecting a 'Sequence'",
"also-not-a-list|Expecting a 'Sequence'",
@@ -1353,7 +1367,10 @@ public class ConcourseEditorTest {
"resources:\n" +
"- name: foo"
);
editor.assertProblems("-^ name: foo|'type' is required");
editor.assertProblems(
"-^ name: foo|'type' is required",
"foo|Unused"
);
//Both name and type missing:
editor = harness.newEditor(
@@ -1398,7 +1415,10 @@ public class ConcourseEditorTest {
" source:\n" +
" branch: master"
);
editor.assertProblems("source|'uri' is required");
editor.assertProblems(
"foo|Unused",
"source|'uri' is required"
);
//addProp(group, "name", t_ne_string).isRequired(true);
editor = harness.newEditor(
@@ -1455,7 +1475,10 @@ public class ConcourseEditorTest {
" source:\n" +
" tag: latest\n"
);
editor.assertProblems("source|'repository' is required");
editor.assertProblems(
"my-docker-image|Unused 'Resource'",
"source|'repository' is required"
);
editor = harness.newEditor(
"resources:\n" +
@@ -1490,6 +1513,7 @@ public class ConcourseEditorTest {
" bogus_client_cert_prop: bad\n"
);
editor.assertProblems(
"my-docker-image|Unused 'Resource'",
"no-list|Expecting a 'Sequence'",
"bogus_ca_certs_prop|Unknown property", //ca_certs
"bogus_client_cert_prop|Unknown property" //client_certs
@@ -1618,8 +1642,9 @@ public class ConcourseEditorTest {
" access_key_id: the-key"
);
editor.assertProblems(
"source|One of [regexp, versioned_file] is required",
"source|'bucket' is required"
"s3-snapshots|Unused 'Resource'",
"source|'bucket' is required",
"source|One of [regexp, versioned_file] is required"
);
editor = harness.newEditor(
@@ -1642,6 +1667,7 @@ public class ConcourseEditorTest {
" versioned_file: path/to/file.tar.gz\n"
);
editor.assertProblems(
"s3-snapshots|Unused 'Resource'",
"bogus-region|unknown 'S3Region'",
"is-private|'boolean'",
"no_ssl_checking|'boolean'",
@@ -1809,6 +1835,7 @@ public class ConcourseEditorTest {
" private_key: stuff"
);
editor.assertProblems(
"swimming-pool|Unused",
"source|[branch, pool, uri] are required"
);
@@ -1831,6 +1858,7 @@ public class ConcourseEditorTest {
" retry_delay: retry-after\n"
);
editor.assertProblems(
"the--locks|Unused",
"retry-after|'Duration'"
);
@@ -1908,7 +1936,10 @@ public class ConcourseEditorTest {
" type: semver\n" +
" source: an-atom"
);
editor.assertProblems("an-atom|Expecting a 'Map'");
editor.assertProblems(
"version|Unused 'Resource'",
"an-atom|Expecting a 'Map'"
);
}
@Test public void semverResourceSourceReconcileRequiredProps() throws Exception {
@@ -1923,6 +1954,7 @@ public class ConcourseEditorTest {
" driver: s3"
);
editor.assertProblems(
"version|Unused",
"source|[access_key_id, bucket, key, secret_access_key] are required"
);
@@ -1933,6 +1965,7 @@ public class ConcourseEditorTest {
" source: {}"
);
editor.assertProblems(
"version|Unused",
"source|[access_key_id, bucket, key, secret_access_key] are required"
);
@@ -1945,6 +1978,7 @@ public class ConcourseEditorTest {
" driver: git"
);
editor.assertProblems(
"version|Unused",
"source|[branch, file, uri] are required"
);
@@ -1957,6 +1991,7 @@ public class ConcourseEditorTest {
" driver: swift"
);
editor.assertProblems(
"version|Unused",
"source|'openstack' is required"
);
}
@@ -1969,7 +2004,10 @@ public class ConcourseEditorTest {
" source:\n" +
" driver: bad-driver"
);
editor.assertProblems("bad-driver|'SemverDriver'");
editor.assertProblems(
"version|Unused",
"bad-driver|'SemverDriver'"
);
}
@Test public void semverGitResourceSourceContentAssist() throws Exception {
@@ -2031,6 +2069,7 @@ public class ConcourseEditorTest {
" bogus: bad"
);
editor.assertProblems(
"version|Unused",
"bogus|Unknown property"
);
@@ -2065,10 +2104,12 @@ public class ConcourseEditorTest {
" bogus-prop: bad"
);
editor.assertProblems(
"version|Unused 'Resource'",
"bogus-region|'S3Region'",
"no-use-ssl|'boolean'",
"bogus-prop|Unknown property"
);
//with explicit 'driver: s3'
editor = harness.newEditor(
@@ -2088,6 +2129,7 @@ public class ConcourseEditorTest {
" bogus-prop: bad"
);
editor.assertProblems(
"version|Unused 'Resource'",
"bogus-region|'S3Region'",
"no-use-ssl|'boolean'",
"bogus-prop|Unknown property"
@@ -2117,7 +2159,7 @@ public class ConcourseEditorTest {
" item_name: flubber-blub\n" +
" region_name: us-west-1\n"
);
editor.assertProblems(/*NONE*/);
editor.assertProblems("version|Unused 'Resource'");
editor.assertHoverContains("openstack", "All openstack configuration");
}
@@ -2435,7 +2477,10 @@ public class ConcourseEditorTest {
"- name: the-resource\n" +
" type: "+badName
);
editor.assertProblems(badName+"|Resource Type does not exist");
editor.assertProblems(
"the-resource|Unused 'Resource'",
badName+"|Resource Type does not exist"
);
}
//All the good names are accepted:
@@ -2446,7 +2491,9 @@ public class ConcourseEditorTest {
"- name: the-resource\n" +
" type: "+goodName
);
editor.assertProblems(/*None*/);
editor.assertProblems(/*None*/
"the-resource|Unused 'Resource'"
);
}
}
@@ -2811,6 +2858,7 @@ public class ConcourseEditorTest {
);
editor.assertProblems(
"docker-image|Unused",
"config|One of [image_resource, image] is required"
);
}
@@ -3438,9 +3486,8 @@ public class ConcourseEditorTest {
" - put: <*>"
);
//Should be de-indentation relaxation. These should not be
// allowed if they cause the context node to be split. So in this example
// de-indented completions shouldn't be suggested.
// De-indentation relaxation should not be allowed if they cause the context node to be split.
// So in this example de-indented completions shouldn't be suggested.
editor = harness.newEditor(
"jobs:\n" +
"- name: build-docker-image\n" +
@@ -3453,6 +3500,42 @@ public class ConcourseEditorTest {
editor.assertNoCompletionsWithLabel(label -> label.startsWith(Unicodes.LEFT_ARROW+" "));;
}
@Test public void reconcileUnusedResources() throws Exception {
Editor editor;
editor = harness.newEditor(
"resources:\n" +
"- name: version\n" +
" type: semver\n" +
"- name: source-repo\n" +
" type: git\n" +
"jobs:\n" +
"- name: build-it\n" +
" plan:\n" +
" - get: version\n"
);
editor.assertProblems("source-repo|Unused 'Resource'");
editor = harness.newEditor(
"resources:\n" +
"- name: version\n" +
" type: semver\n" +
"- name: source-repo\n" +
" type: git\n" +
" source:\n" +
" branch: master\n" +
" uri: git@github.com/blah\n" +
"jobs:\n" +
"- name: build-it\n" +
" plan:\n" +
" - aggregate:\n" +
" - get: the-version\n" +
" resource: version\n" +
" - put: source-repo\n"
);
editor.assertProblems(/*NONE*/);
}
//////////////////////////////////////////////////////////////////////////////
private void assertContextualCompletions(String conText, String textBefore, String... textAfter) throws Exception {