From 88013ef8039c451eb2cd44529e4473fd4e3327ca Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Mon, 3 Apr 2017 12:11:34 -0700 Subject: [PATCH] Fix PT-140709005 better underlining for missing property warnings Try to underline corresponding parent property key instead of the entire node that is missing the property. --- .../SchemaBasedYamlASTReconciler.java | 20 ++++----- .../yaml/reconcile/YamlSchemaProblems.java | 14 +++++- .../yaml/schema/constraints/Constraint.java | 3 +- .../yaml/schema/constraints/Constraints.java | 6 +-- .../vscode/concourse/ConcourseEditorTest.java | 43 ++++++++----------- 5 files changed, 46 insertions(+), 40 deletions(-) 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 3c9b22a24..e0c372256 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 @@ -90,7 +90,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { 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()); + reconcile(ast.getDocument(), new YamlPath(YamlPathSegment.valueAt(i)), /*parent*/null, node, schema.getTopLevelType()); } } } finally { @@ -116,7 +116,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { return allOf(ast, node); } - private void reconcile(IDocument doc, YamlPath path, Node node, YType type) { + private void reconcile(IDocument doc, YamlPath path, Node parent, Node node, YType type) { if (type!=null) { DynamicSchemaContext schemaContext = new ASTDynamicSchemaContext(doc, path, node); type = typeUtil.inferMoreSpecificType(type, schemaContext); @@ -130,12 +130,12 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { if (typeUtil.isMap(type)) { for (NodeTuple entry : map.getValue()) { String key = NodeUtil.asScalar(entry.getKeyNode()); - reconcile(doc, keyAt(path, key), entry.getKeyNode(), typeUtil.getKeyType(type)); - reconcile(doc, valueAt(path, key), entry.getValueNode(), typeUtil.getDomainType(type)); + reconcile(doc, keyAt(path, key), map, entry.getKeyNode(), typeUtil.getKeyType(type)); + reconcile(doc, valueAt(path, key), map, entry.getValueNode(), typeUtil.getDomainType(type)); } } else if (typeUtil.isBean(type)) { Map beanProperties = typeUtil.getPropertiesMap(type); - checkRequiredProperties(map, type, beanProperties, schemaContext); + checkRequiredProperties(parent, map, type, beanProperties, schemaContext); for (NodeTuple entry : map.getValue()) { Node keyNode = entry.getKeyNode(); String key = NodeUtil.asScalar(keyNode); @@ -149,7 +149,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { if (prop.isDeprecated()) { problems.accept(YamlSchemaProblems.deprecatedProperty(keyNode, type, prop)); } - reconcile(doc, valueAt(path, key), entry.getValueNode(), prop.getType()); + reconcile(doc, valueAt(path, key), map, entry.getValueNode(), prop.getType()); } } } @@ -162,7 +162,7 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { if (typeUtil.isSequencable(type)) { for (int i = 0; i < seq.getValue().size(); i++) { Node el = seq.getValue().get(i); - reconcile(doc, valueAt(path, i), el, typeUtil.getDomainType(type)); + reconcile(doc, valueAt(path, i), seq, el, typeUtil.getDomainType(type)); } } else { expectTypeButFoundSequence(type, node); @@ -214,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, DynamicSchemaContext dc) { + private void checkRequiredProperties(Node parent, 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. @@ -234,14 +234,14 @@ public class SchemaBasedYamlASTReconciler implements YamlASTReconciler { } else { message = "Properties "+missingProps+" are required for '"+type+"'"; } - problems.accept(YamlSchemaProblems.missingProperty(message, map)); + problems.accept(YamlSchemaProblems.missingProperty(message, parent, map)); } //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); + constraint.verify(parent, map, type, foundProps, problems); } } } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/YamlSchemaProblems.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/YamlSchemaProblems.java index 209c944dd..afeed88fc 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/YamlSchemaProblems.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/reconcile/YamlSchemaProblems.java @@ -17,10 +17,15 @@ import org.springframework.ide.vscode.commons.languageserver.reconcile.ProblemTy import org.springframework.ide.vscode.commons.languageserver.reconcile.ReconcileProblem; import org.springframework.ide.vscode.commons.languageserver.reconcile.ReconcileProblemImpl; import org.springframework.ide.vscode.commons.languageserver.util.DocumentRegion; +import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil; +import org.springframework.ide.vscode.commons.yaml.path.NodeCursor; +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.YType; import org.springframework.ide.vscode.commons.yaml.schema.YTypedProperty; import org.yaml.snakeyaml.nodes.MappingNode; import org.yaml.snakeyaml.nodes.Node; +import org.yaml.snakeyaml.nodes.NodeTuple; import com.google.common.collect.ImmutableSet; @@ -91,7 +96,14 @@ public class YamlSchemaProblems { return new ReconcileProblemImpl(problemType, msg, start, end-start); } - public static ReconcileProblem missingProperty(String msg, MappingNode map) { + public static ReconcileProblem missingProperty(String msg, Node parent, MappingNode map) { + if (parent instanceof MappingNode) { + for (NodeTuple prop : ((MappingNode) parent).getValue()) { + if (prop.getValueNode()==map) { + return problem(MISSING_PROPERTY, msg, prop.getKeyNode()); + } + } + } return problem(MISSING_PROPERTY, msg, map); } 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 index 19d74964b..f04b6369c 100644 --- 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 @@ -15,6 +15,7 @@ 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; +import org.yaml.snakeyaml.nodes.Node; /** * An implementations of this interface represents some 'programatic' constraint attached to a schema type. @@ -35,6 +36,6 @@ public interface Constraint { * @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); + void verify(Node parent, 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 index bee7825a2..8d978b897 100644 --- 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 @@ -61,7 +61,7 @@ public class Constraints { } @Override - public void verify(MappingNode map, YType type, Set foundProps, IProblemCollector problems) { + public void verify(Node parent, MappingNode map, YType type, Set foundProps, IProblemCollector problems) { List requiredProps = Arrays.asList(_requiredProps); long foundPropsCount = requiredProps.stream() .filter(foundProps::contains) @@ -69,7 +69,7 @@ public class Constraints { if (foundPropsCount==0) { if (!allowFewer) { problems.accept(missingProperty( - "One of "+requiredProps+" is required for '"+type+"'", map)); + "One of "+requiredProps+" is required for '"+type+"'", parent, map)); } } else if (foundPropsCount>1) { //Mark each of the found keys as a violation: @@ -86,7 +86,7 @@ public class Constraints { public static Constraint deprecated(Function messageFormatter, String... _deprecatedNames) { Set deprecatedNames = ImmutableSet.copyOf(_deprecatedNames); - return (MappingNode map, YType type, Set foundProps, IProblemCollector problems) -> { + return (Node parent, MappingNode map, YType type, Set foundProps, IProblemCollector problems) -> { for (NodeTuple prop : map.getValue()) { Node keyNode = prop.getKeyNode(); String name = NodeUtil.asScalar(keyNode); 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 6d9b3c085..4fc7d4c71 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 @@ -572,8 +572,8 @@ public class ConcourseEditorTest { { List problems = editor.assertProblems( "config|Only one of [config, file]", - "{}|[platform, run] are required", - "{}|One of [image_resource, image]", + "config|[platform, run] are required", + "config|One of [image_resource, image]", "file|Only one of [config, file]" ); //All of the problems in this example are property contraint violations! So all should be warnings. @@ -601,7 +601,7 @@ public class ConcourseEditorTest { " args: [\"Hello, world!\"]" ); editor.assertProblems( - "config|something" + "config|'platform' is required" ); } @@ -1007,7 +1007,7 @@ public class ConcourseEditorTest { " - put: my-git\n" + " params: {}\n" ); - editor.assertProblems("{}|'repository' is required"); + editor.assertProblems("params|'repository' is required"); editor = harness.newEditor( "resources:\n" + @@ -1195,7 +1195,7 @@ public class ConcourseEditorTest { " source:\n" + " branch: master" ); - editor.assertProblems("branch: master|'uri' is required"); + editor.assertProblems("source|'uri' is required"); //addProp(gitSource, "branch", t_string).isRequired(true); editor = harness.newEditor( @@ -1205,7 +1205,7 @@ public class ConcourseEditorTest { " source:\n" + " uri: https://yada" ); - editor.assertProblems("uri: https://yada|'branch' is required"); + editor.assertProblems("source|'branch' is required"); //addProp(group, "name", t_ne_string).isRequired(true); editor = harness.newEditor( @@ -1226,7 +1226,7 @@ public class ConcourseEditorTest { " source:\n" + " tag: latest\n" ); - editor.assertProblems("tag: latest|'repository' is required"); + editor.assertProblems("source|'repository' is required"); editor = harness.newEditor( "resources:\n" + @@ -1389,8 +1389,8 @@ public class ConcourseEditorTest { " access_key_id: the-key" ); editor.assertProblems( - "access_key_id: the-key|'bucket' is required", - "access_key_id: the-key|One of [regexp, versioned_file] is required" + "source|'bucket' is required", + "source|One of [regexp, versioned_file] is required" ); editor = harness.newEditor( @@ -1502,7 +1502,7 @@ public class ConcourseEditorTest { " no-params-expected: bad" ); editor.assertProblems( - "acl: public-read|'file' is required", + "params|'file' is required", "no-params-expected|Unknown property" ); @@ -1580,7 +1580,7 @@ public class ConcourseEditorTest { " private_key: stuff" ); editor.assertProblems( - "private_key: stuff|[branch, pool, uri] are required" + "source|[branch, pool, uri] are required" ); editor = harness.newEditor( @@ -1695,7 +1695,7 @@ public class ConcourseEditorTest { " driver: s3" ); editor.assertProblems( - "driver: s3|[access_key_id, bucket, key, secret_access_key] are required" + "source|[access_key_id, bucket, key, secret_access_key] are required" ); editor = harness.newEditor( @@ -1705,7 +1705,7 @@ public class ConcourseEditorTest { " source: {}" ); editor.assertProblems( - "{}|[access_key_id, bucket, key, secret_access_key] are required" + "source|[access_key_id, bucket, key, secret_access_key] are required" ); // required props for git driver @@ -1717,7 +1717,7 @@ public class ConcourseEditorTest { " driver: git" ); editor.assertProblems( - "driver: git|[branch, file, uri] are required" + "source|[branch, file, uri] are required" ); //required props for swift driver @@ -1729,7 +1729,7 @@ public class ConcourseEditorTest { " driver: swift" ); editor.assertProblems( - "driver: swift|'openstack' is required" + "source|'openstack' is required" ); } @@ -2437,7 +2437,7 @@ public class ConcourseEditorTest { "run:\n" + " user: admin\n" ); - editor.assertProblems("user: admin|'path' is required"); + editor.assertProblems("run|'path' is required"); } @Test public void nameAndPathHoversInTaskInputsAndOutputs() throws Exception { @@ -2565,16 +2565,9 @@ public class ConcourseEditorTest { " - mvn" ); - Diagnostic problem = editor.assertProblem( - "inputs:\n" + - " - name: commons-git\n" + - " platform: linux\n" + - " run:\n" + - " path: which\n" + - " args:\n" + - " - mvn" + editor.assertProblems( + "config|One of [image_resource, image] is required" ); - assertContains("One of [image_resource, image] is required", problem.getMessage()); } @Test public void resourceInTaskConfigFileNotRequired() throws Exception {