From a3228353689581b37ec328be599d9fb619fe8803 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Fri, 20 Jan 2017 15:22:03 -0800 Subject: [PATCH] Support for required property checking. All tests passing. --- .../ide/vscode/commons/yaml/ast/NodeUtil.java | 24 ++++++++++ .../SchemaBasedYamlASTReconciler.java | 44 +++++++++++++++---- .../yaml/schema/ASTDynamicSchemaContext.java | 14 +----- .../languageserver/testharness/Editor.java | 4 -- .../manifest/yaml/ManifestYamlEditorTest.java | 19 ++++---- 5 files changed, 73 insertions(+), 32 deletions(-) diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java index ef99875a6..954ec7d27 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/ast/NodeUtil.java @@ -11,12 +11,18 @@ package org.springframework.ide.vscode.commons.yaml.ast; +import java.util.Collections; +import java.util.Set; + import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; import org.yaml.snakeyaml.nodes.NodeId; +import org.yaml.snakeyaml.nodes.NodeTuple; import org.yaml.snakeyaml.nodes.ScalarNode; import org.yaml.snakeyaml.nodes.SequenceNode; +import com.google.common.collect.ImmutableSet; + /** * @author Kris De Volder */ @@ -74,4 +80,22 @@ public class NodeUtil { return null; } + /** + * Get the scalar values of all keys of the given {@link MappingNode} as Strings. + * Any non-scalar keys are silently ignored. + */ + public static Set getScalarKeys(MappingNode mapNode) { + if (mapNode!=null) { + ImmutableSet.Builder builder = ImmutableSet.builder(); + for (NodeTuple entry : mapNode.getValue()) { + String key = NodeUtil.asScalar(entry.getKeyNode()); + if (key!=null) { //key not a scalar? => something funky so skip it + builder.add(key); + } + } + return builder.build(); + } + return Collections.emptySet(); + } + } 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 974c5a6bc..17b5c497e 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 @@ -19,9 +19,12 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.springframework.ide.vscode.commons.languageserver.reconcile.IProblemCollector; import org.springframework.ide.vscode.commons.languageserver.util.DocumentRegion; +import org.springframework.ide.vscode.commons.util.CollectionUtil; import org.springframework.ide.vscode.commons.util.ExceptionUtil; import org.springframework.ide.vscode.commons.util.IntegerRange; import org.springframework.ide.vscode.commons.util.Log; @@ -61,14 +64,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { public void reconcile(YamlFileAST ast) { List nodes = ast.getNodes(); IntegerRange expectedDocs = schema.expectedNumberOfDocuments(); - if (expectedDocs.isInRange(nodes.size())) { - if (nodes!=null && !nodes.isEmpty()) { - for (int i = 0; i < nodes.size(); i++) { - Node node = nodes.get(i); - reconcile(ast.getDocument(), new YamlPath(YamlPathSegment.valueAt(i)), node, schema.getTopLevelType()); - } - } - } else { + if (!expectedDocs.isInRange(nodes.size())) { //wrong number of documents in the file. Figure out a good error message. if (nodes.isEmpty()) { problem(allOf(ast.getDocument()), "'"+schema.getName()+"' must have at least some Yaml content"); @@ -81,6 +77,12 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { problem(endOf(ast.getDocument()), "'"+schema.getName()+"' should have at least "+lowerBound+" Yaml Documents"); } } + if (nodes!=null && !nodes.isEmpty()) { + for (int i = 0; i < nodes.size(); i++) { + Node node = nodes.get(i); + reconcile(ast.getDocument(), new YamlPath(YamlPathSegment.valueAt(i)), node, schema.getTopLevelType()); + } + } } private DocumentRegion dashesAtStartOf(YamlFileAST ast, Node node) { @@ -117,6 +119,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { } } else if (typeUtil.isBean(type)) { Map beanProperties = typeUtil.getPropertiesMap(type, schemaContext); + checkRequiredProperties(map, type, beanProperties); for (NodeTuple entry : map.getValue()) { Node keyNode = entry.getKeyNode(); String key = NodeUtil.asScalar(keyNode); @@ -170,6 +173,31 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { } } + private void checkRequiredProperties(MappingNode map, YType type, Map beanProperties) { + 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. + if (allPropertiesKnown) { + Set missingProps = beanProperties.values().stream() + .filter(YTypedProperty::isRequired) + .map(YTypedProperty::getName) + .filter((required) -> !foundProps.contains(required)) + .collect(Collectors.toSet()); + if (!missingProps.isEmpty()) { + String message; + if (missingProps.size()==1) { + // slightly more specific message when only one missing property + String missing = missingProps.stream().findFirst().get(); + message = "Property '"+missing+"' is required for '"+type+"'"; + } else { + message = "Properties "+missingProps+" are required for '"+type+"'"; + } + problem(map, message); + } + } + } + + protected NodeId getNodeId(Node node) { NodeId id = node.getNodeId(); if (id==NodeId.mapping && isMoustacheVar(node)) { diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/ASTDynamicSchemaContext.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/ASTDynamicSchemaContext.java index f2508fa3e..1045820f3 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/ASTDynamicSchemaContext.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/schema/ASTDynamicSchemaContext.java @@ -50,24 +50,14 @@ public class ASTDynamicSchemaContext extends CachingSchemaContext { @Override protected Set computeDefinedProperties() { - if (mapNode!=null) { - ImmutableSet.Builder builder = ImmutableSet.builder(); - for (NodeTuple entry : mapNode.getValue()) { - String key = NodeUtil.asScalar(entry.getKeyNode()); - if (key!=null) { //key not a scalar? => something funky so skip it - builder.add(key); - } - } - return builder.build(); - } - return Collections.emptySet(); + return NodeUtil.getScalarKeys(mapNode); } @Override public IDocument getDocument() { return doc; } - + @Override public YamlPath getPath() { return path; diff --git a/vscode-extensions/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/Editor.java b/vscode-extensions/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/Editor.java index 368aad75f..b478fa532 100644 --- a/vscode-extensions/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/Editor.java +++ b/vscode-extensions/commons/language-server-test-harness/src/main/java/org/springframework/ide/vscode/languageserver/testharness/Editor.java @@ -24,8 +24,6 @@ import java.util.Iterator; import java.util.List; import java.util.Set; import java.util.stream.Collectors; -import java.util.stream.Stream; -import java.util.stream.Stream.Builder; import javax.swing.text.BadLocationException; @@ -39,8 +37,6 @@ import org.eclipse.lsp4j.Range; import org.eclipse.lsp4j.TextEdit; import org.junit.Assert; -import com.google.common.base.Strings; - import reactor.core.publisher.Flux; public class Editor { diff --git a/vscode-extensions/vscode-manifest-yaml/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java b/vscode-extensions/vscode-manifest-yaml/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java index feb04ab1b..b4e6f2f1e 100644 --- a/vscode-extensions/vscode-manifest-yaml/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java +++ b/vscode-extensions/vscode-manifest-yaml/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java @@ -10,7 +10,6 @@ *******************************************************************************/ package org.springframework.ide.vscode.manifest.yaml; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.when; @@ -132,6 +131,8 @@ public class ManifestYamlEditorTest { //Using a 'composite' element where a scalar type is expected editor = harness.newEditor( + "applications:\n" + + "- name: foo\n" + "memory:\n"+ "- bad sequence\n" + "buildpack:\n" + @@ -698,12 +699,16 @@ public class ManifestYamlEditorTest { "---\n" + "applications:\n"+ "- name: foo\n" + + " bad-one: xx\n" + "---\n" + "applications:\n"+ - "- name: foo\n" + "- name: foo\n" + + " bad-two: xx" ); editor.assertProblems( - "---|'Cloudfoundry Manifest' should not have more than 1 Yaml Document" + "bad-one|Unknown property", //should still reconcile the documents even thought there's too many of them! + "---|'Cloudfoundry Manifest' should not have more than 1 Yaml Document", + "bad-two|Unknown property" //should still reconcile the documents even thought there's too many of them! ); //also check the location of the marker since there are two occurrences of '---' in the editor text. Diagnostic problem = editor.assertProblem("---"); @@ -737,12 +742,10 @@ public class ManifestYamlEditorTest { //when the file is empty (there is no AST at all) editor = harness.newEditor( - "foo: v1\n" + "buildpack: some-buildpack" ); editor.assertProblems( - - "foo|Unkown property", - "foo: v1|'applications' is required" + "buildpack: some-buildpack|'applications' is required" ); } @@ -756,7 +759,7 @@ public class ManifestYamlEditorTest { ); editor.assertProblems( "memory: 1G|Property 'name' is required", - ":|should not be empty" + "|should not be empty" ); }