From 2790976983cce6cea8e63650c2ebc0c9afb1dd43 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Fri, 31 Mar 2017 17:42:55 -0700 Subject: [PATCH] Fix PT-142788419 more accurate checking of constraints The use of 'image' and 'image_resource' in an embedded task config is only required if it is not already defined in the task itself. --- .../vscode/commons/util/text/IDocument.java | 1 + .../SchemaBasedYamlASTReconciler.java | 30 +++---- .../yaml/schema/SchemaContextAware.java | 9 ++- .../commons/yaml/schema/YTypeFactory.java | 19 +++-- .../vscode/commons/yaml/schema/YTypeUtil.java | 3 +- .../yaml/schema/constraints/Constraint.java | 40 +++++++++ .../yaml/schema/constraints/Constraints.java | 81 +++++++++++++++++++ .../vscode/concourse/PipelineYmlSchema.java | 19 ++++- .../vscode/concourse/ConcourseEditorTest.java | 74 +++++++++++++++++ 9 files changed, 245 insertions(+), 31 deletions(-) create mode 100644 vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraint.java create mode 100644 vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraints.java diff --git a/vscode-extensions/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java b/vscode-extensions/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java index 31e472cb6..ed2385e53 100644 --- a/vscode-extensions/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java +++ b/vscode-extensions/commons/commons-util/src/main/java/org/springframework/ide/vscode/commons/util/text/IDocument.java @@ -28,5 +28,6 @@ public interface IDocument { int getLineOffset(int line) throws BadLocationException; void replace(int start, int len, String text) throws BadLocationException; String textBetween(int start, int end) throws BadLocationException; + String getLanguageId(); } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/SchemaBasedYamlASTReconciler.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/SchemaBasedYamlASTReconciler.java index e6b77d777..6aed7ba25 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/SchemaBasedYamlASTReconciler.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/SchemaBasedYamlASTReconciler.java @@ -27,7 +27,6 @@ import java.util.stream.Collectors; import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector; import org.springframework.ide.vscode.commons.languageserver.reconcile.ProblemType; import org.springframework.ide.vscode.commons.languageserver.reconcile.ProblemTypeProvider; -import org.springframework.ide.vscode.commons.languageserver.reconcile.ReconcileException; import org.springframework.ide.vscode.commons.languageserver.util.DocumentRegion; import org.springframework.ide.vscode.commons.util.ExceptionUtil; import org.springframework.ide.vscode.commons.util.IntegerRange; @@ -42,10 +41,12 @@ import org.springframework.ide.vscode.commons.yaml.path.YamlPath; import org.springframework.ide.vscode.commons.yaml.path.YamlPathSegment; import org.springframework.ide.vscode.commons.yaml.schema.ASTDynamicSchemaContext; import org.springframework.ide.vscode.commons.yaml.schema.DynamicSchemaContext; +import org.springframework.ide.vscode.commons.yaml.schema.SchemaContextAware; import org.springframework.ide.vscode.commons.yaml.schema.YType; import org.springframework.ide.vscode.commons.yaml.schema.YTypeUtil; import org.springframework.ide.vscode.commons.yaml.schema.YTypedProperty; import org.springframework.ide.vscode.commons.yaml.schema.YamlSchema; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraint; import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; import org.yaml.snakeyaml.nodes.NodeId; @@ -53,8 +54,6 @@ import org.yaml.snakeyaml.nodes.NodeTuple; import org.yaml.snakeyaml.nodes.ScalarNode; import org.yaml.snakeyaml.nodes.SequenceNode; -import com.google.gson.internal.Streams; - public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { private final IProblemCollector problems; @@ -136,7 +135,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { } } else if (typeUtil.isBean(type)) { Map beanProperties = typeUtil.getPropertiesMap(type); - checkRequiredProperties(map, type, beanProperties); + checkRequiredProperties(map, type, beanProperties, schemaContext); for (NodeTuple entry : map.getValue()) { Node keyNode = entry.getKeyNode(); String key = NodeUtil.asScalar(keyNode); @@ -215,7 +214,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { return e instanceof ProblemTypeProvider ? ((ProblemTypeProvider) e).getProblemType() : YamlSchemaProblems.SCHEMA_PROBLEM; } - private void checkRequiredProperties(MappingNode map, YType type, Map beanProperties) { + private void checkRequiredProperties(MappingNode map, YType type, Map beanProperties, DynamicSchemaContext dc) { Set foundProps = NodeUtil.getScalarKeys(map); boolean allPropertiesKnown = beanProperties.keySet().containsAll(foundProps); //Don't check for missing properties if some properties look like they might be spelled incorrectly. @@ -238,22 +237,11 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { problem(map, message, YamlSchemaProblems.MISSING_PROPERTY); } - //Check for missing/extra 'one-of' constrained properties - for (String[] _requiredProps : typeUtil.getOneOfConstraints(type)) { - List requiredProps = Arrays.asList(_requiredProps); - long foundPropsCount = requiredProps.stream() - .filter(foundProps::contains) - .count(); - if (foundPropsCount==0) { - problem(map, "One of "+requiredProps+" is required for '"+type+"'", YamlSchemaProblems.MISSING_PROPERTY); - } else if (foundPropsCount>1) { - //Mark each of the found keys as a violation: - for (NodeTuple entry : map.getValue()) { - String key = NodeUtil.asScalar(entry.getKeyNode()); - if (key!=null && requiredProps.contains(key)) { - problem(entry.getKeyNode(), "Only one of "+requiredProps+" should be defined for '"+type+"'", YamlSchemaProblems.EXTRA_PROPERTY); - } - } + //Check for other constraints attached to the type + for (SchemaContextAware _constraint : typeUtil.getConstraints(type)) { + Constraint constraint = _constraint.withContext(dc); + if (constraint!=null) { + constraint.verify(map, type, foundProps, problems); } } } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/SchemaContextAware.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/SchemaContextAware.java index 2a611f1b6..aa989c42e 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/SchemaContextAware.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/SchemaContextAware.java @@ -14,9 +14,16 @@ package org.springframework.ide.vscode.commons.yaml.schema; * Interface that can be implemented by something producing another * component (of some type `T`) where the returned component needs to * configured with a {@link DynamicSchemaContext}. - * + * * @author Kris De Volder */ public interface SchemaContextAware { T withContext(DynamicSchemaContext dc); + + /** + * Convert a plain value into a {@link SchemaContextAware} that ignores the context and simply returns the value. + */ + public static SchemaContextAware just(T it) { + return (dc) -> it; + } } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeFactory.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeFactory.java index cf1dcb512..1c2c4f386 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeFactory.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeFactory.java @@ -30,6 +30,8 @@ import org.springframework.ide.vscode.commons.util.Renderable; import org.springframework.ide.vscode.commons.util.Renderables; import org.springframework.ide.vscode.commons.util.ValueParser; import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.AbstractType; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraint; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraints; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList.Builder; @@ -143,8 +145,8 @@ public class YTypeFactory { } @Override - public List getOneOfConstraints(YType type) { - return ((AbstractType)type).getOneOfConstraints(); + public List> getConstraints(YType type) { + return ((AbstractType)type).getConstraints(); } }; @@ -161,7 +163,7 @@ public class YTypeFactory { private Map cachedPropertyMap; private SchemaContextAware>> hintProvider; - private List oneOfConstraints = new ArrayList<>(1); + private List> constraints = new ArrayList<>(2); public boolean isSequenceable() { return false; @@ -224,8 +226,8 @@ public class YTypeFactory { return ImmutableList.of(); } - public List getOneOfConstraints() { - return ImmutableList.copyOf(oneOfConstraints); + public List> getConstraints() { + return ImmutableList.copyOf(constraints); } public List getProperties() { @@ -299,9 +301,12 @@ public class YTypeFactory { return parser == null ? null : parser.withContext(dc); } + public void require(SchemaContextAware dynamicConstraint) { + this.constraints.add(dynamicConstraint); + } + public void requireOneOf(String... properties) { - Assert.isLegal(properties.length>1); - this.oneOfConstraints.add(properties); + this.constraints.add(SchemaContextAware.just(Constraints.requireOneOf(properties))); } public String[] getPropertyNames() { diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeUtil.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeUtil.java index 783819d31..56ec6d0e4 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeUtil.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/YTypeUtil.java @@ -14,6 +14,7 @@ import java.util.List; import java.util.Map; import org.springframework.ide.vscode.commons.util.ValueParser; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraint; /** * An implementation of YTypeUtil provides implementations of various @@ -45,5 +46,5 @@ public interface YTypeUtil { * should be returned. */ YType inferMoreSpecificType(YType type, DynamicSchemaContext dc); - List getOneOfConstraints(YType type); + List> getConstraints(YType type); } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraint.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraint.java new file mode 100644 index 000000000..19d74964b --- /dev/null +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraint.java @@ -0,0 +1,40 @@ +/******************************************************************************* + * Copyright (c) 2017 Pivotal, Inc. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Pivotal, Inc. - initial API and implementation + *******************************************************************************/ +package org.springframework.ide.vscode.commons.yaml.schema.constraints; + +import java.util.Set; + +import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector; +import org.springframework.ide.vscode.commons.yaml.schema.YType; +import org.yaml.snakeyaml.nodes.MappingNode; + +/** + * An implementations of this interface represents some 'programatic' constraint attached to a schema type. + * Essentially, it encapsulates a verification procedure to be called upon to validate something when + * visiting a node in the YamlAST that has been deterimed to be of that type. + */ +@FunctionalInterface +public interface Constraint { + + /** + * Implemetors gain access to various bits of context information passed as parameters and + * are supposed to use this information in whatever way they like to check if the + * constraint is satisfied. When the constrain is not satisfied they should report any + * violations by adding problems to the provide {@link IProblemCollector}. + * + * @param map The node being validated + * @param type The inferred type of the node. + * @param foundProps The properties this node defines. + * @param problems Problem collector where to which the constraint should add the validation problems it finds. + */ + void verify(MappingNode map, YType type, Set foundProps, IProblemCollector problems); + +} diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraints.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraints.java new file mode 100644 index 000000000..b8a227e12 --- /dev/null +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/constraints/Constraints.java @@ -0,0 +1,81 @@ +/******************************************************************************* + * Copyright (c) 2017 Pivotal, Inc. + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * which accompanies this distribution, and is available at + * http://www.eclipse.org/legal/epl-v10.html + * + * Contributors: + * Pivotal, Inc. - initial API and implementation + *******************************************************************************/ +package org.springframework.ide.vscode.commons.yaml.schema.constraints; + +import static org.springframework.ide.vscode.commons.yaml.reconcile.YamlSchemaProblems.EXTRA_PROPERTY; +import static org.springframework.ide.vscode.commons.yaml.reconcile.YamlSchemaProblems.MISSING_PROPERTY; +import static org.springframework.ide.vscode.commons.yaml.reconcile.YamlSchemaProblems.problem; + +import java.util.Arrays; +import java.util.List; +import java.util.Set; + +import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector; +import org.springframework.ide.vscode.commons.util.Assert; +import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil; +import org.springframework.ide.vscode.commons.yaml.schema.YType; +import org.yaml.snakeyaml.nodes.MappingNode; +import org.yaml.snakeyaml.nodes.NodeTuple; + +/** + * Various static methods for constructing/composing {@link Constraint}s. + * + * @author Kris De Volder + */ +public class Constraints { + + public static Constraint requireOneOf(String... properties) { + return new RequireOneOf(properties); + } + + public static Constraint requireAtMostOneOf(String... properties) { + return new RequireOneOf(properties).allowFewer(true); + } + + static private class RequireOneOf implements Constraint { + + private final String[] _requiredProps; + private boolean allowFewer = false; + + public RequireOneOf(String[] properties) { + Assert.isLegal(properties.length>1); + this._requiredProps = properties; + } + + public Constraint allowFewer(boolean b) { + this.allowFewer = b; + return this; + } + + @Override + public void verify(MappingNode map, YType type, Set foundProps, IProblemCollector problems) { + List requiredProps = Arrays.asList(_requiredProps); + long foundPropsCount = requiredProps.stream() + .filter(foundProps::contains) + .count(); + if (foundPropsCount==0) { + if (!allowFewer) { + problems.accept(problem(MISSING_PROPERTY, + "One of "+requiredProps+" is required for '"+type+"'", map)); + } + } else if (foundPropsCount>1) { + //Mark each of the found keys as a violation: + for (NodeTuple entry : map.getValue()) { + String key = NodeUtil.asScalar(entry.getKeyNode()); + if (key!=null && requiredProps.contains(key)) { + problems.accept(problem(EXTRA_PROPERTY, + "Only one of "+requiredProps+" should be defined for '"+type+"'", entry.getKeyNode())); + } + } + } + } + } +} \ No newline at end of file diff --git a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java index 2b2af9f7f..22ba0d9f8 100644 --- a/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java +++ b/vscode-extensions/vscode-concourse/src/main/java/org/springframework/ide/vscode/concourse/PipelineYmlSchema.java @@ -13,6 +13,7 @@ package org.springframework.ide.vscode.concourse; import java.util.Set; import java.util.stream.Collectors; +import org.springframework.ide.vscode.commons.languageserver.LanguageIds; import org.springframework.ide.vscode.commons.util.MimeTypes; import org.springframework.ide.vscode.commons.util.Renderable; import org.springframework.ide.vscode.commons.util.Renderables; @@ -22,6 +23,7 @@ import org.springframework.ide.vscode.commons.yaml.ast.YamlFileAST; import org.springframework.ide.vscode.commons.yaml.path.YamlPath; import org.springframework.ide.vscode.commons.yaml.path.YamlPathSegment; import org.springframework.ide.vscode.commons.yaml.schema.DynamicSchemaContext; +import org.springframework.ide.vscode.commons.yaml.schema.SchemaContextAware; 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; @@ -29,6 +31,8 @@ import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YAtomicTy import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YBeanType; import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YBeanUnionType; import org.springframework.ide.vscode.commons.yaml.schema.YTypeFactory.YTypedPropertyImpl; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraint; +import org.springframework.ide.vscode.commons.yaml.schema.constraints.Constraints; import org.springframework.ide.vscode.commons.yaml.schema.YTypeUtil; import org.springframework.ide.vscode.commons.yaml.schema.YTypedProperty; import org.springframework.ide.vscode.commons.yaml.schema.YValueHint; @@ -211,7 +215,20 @@ public class PipelineYmlSchema implements YamlSchema { addProp(task, "outputs", f.yseq(t_output)); addProp(task, "run", t_command).isRequired(true); addProp(task, "params", t_string_params); - task.requireOneOf("image_resource", "image"); + task.require((dc) -> { + String languageId = dc.getDocument().getLanguageId(); + if (languageId==LanguageIds.CONCOURSE_PIPELINE) { + Node parentImageDef = getParentPropertyNode("image", models, dc); + if (parentImageDef==null) { + return Constraints.requireOneOf("image_resource", "image"); + } else { + // TODO: something like this: return Constraints.deprecated("image_resource", "image"); + return null; + } + } else { + return Constraints.requireAtMostOneOf("image_resource", "image"); + } + }); AbstractType t_put_get_name = f.contextAware("Name", (dc) -> { if (getParentPropertyNode("resource", models, dc)!=null) { diff --git a/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java b/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java index 8fda0ebfc..aa0229f6c 100644 --- a/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java +++ b/vscode-extensions/vscode-concourse/src/test/java/org/springframework/ide/vscode/concourse/ConcourseEditorTest.java @@ -2491,6 +2491,80 @@ public class ConcourseEditorTest { editor.assertCompletions(/*NONE*/); } + @Test public void resourceInEmbeddedTaskConfigNotRequiredIfSpecifiedInTask() throws Exception { + Editor editor; + + editor = harness.newEditor( + "resources:\n" + + "- name: docker-image\n" + + " type: docker-image\n" + + " source:\n" + + " username: {{docker_hub_username}}\n" + + " password: {{docker_hub_password}}\n" + + " repository: kdvolder/sts3-build-env\n" + + "jobs:\n" + + "- name: build-commons-update-site\n" + + " plan:\n" + + " - task: hello-world\n" + + " image: docker-image\n" + //Given here! So not required in config! + " config:\n" + + " inputs:\n" + + " - name: commons-git\n" + + " platform: linux\n" + + " run:\n" + + " path: which\n" + + " args:\n" + + " - mvn" + ); + editor.assertProblems(/*none*/); + + editor = harness.newEditor( + "resources:\n" + + "- name: docker-image\n" + + " type: docker-image\n" + + " source:\n" + + " username: {{docker_hub_username}}\n" + + " password: {{docker_hub_password}}\n" + + " repository: kdvolder/sts3-build-env\n" + + "jobs:\n" + + "- name: build-commons-update-site\n" + + " plan:\n" + + " - task: hello-world\n" + + " config:\n" + + " inputs:\n" + + " - name: commons-git\n" + + " platform: linux\n" + + " run:\n" + + " path: which\n" + + " args:\n" + + " - mvn" + ); + + Diagnostic problem = editor.assertProblem( + "inputs:\n" + + " - name: commons-git\n" + + " platform: linux\n" + + " run:\n" + + " path: which\n" + + " args:\n" + + " - mvn" + ); + assertContains("One of [image_resource, image] is required", problem.getMessage()); + } + + @Test public void resourceInTaskConfigFileNotRequired() throws Exception { + Editor editor = harness.newEditor(LanguageIds.CONCOURSE_TASK, + "inputs:\n" + + "- name: commons-git\n" + + "platform: linux\n" + + "run:\n" + + " path: which\n" + + " args:\n" + + " - mvn" + ); + editor.assertProblems(/*NONE*/); + } + @Ignore @Test public void relaxedIndentContextMoreSpaces() throws Exception { Editor editor;