From 05bd1dfcd634fad57e022b3af341fd53a65957fe Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Fri, 16 Dec 2016 17:56:22 -0800 Subject: [PATCH] NPE caused by ambiguous keys fixed. --- .../completion/AbstractYamlAssistContext.java | 3 +- .../yaml/completion/YTypeAssistContext.java | 2 +- .../commons/yaml/path/YamlNavigable.java | 31 ++++- .../vscode/commons/yaml/path/YamlPath.java | 20 ++-- .../yaml/structure/YamlStructureParser.java | 110 ++++++++++++------ .../ApplicationYamlAssistContext.java | 26 +---- .../boot/test/ApplicationYamlEditorTest.java | 7 +- 7 files changed, 129 insertions(+), 70 deletions(-) diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/AbstractYamlAssistContext.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/AbstractYamlAssistContext.java index b1ab4c360..1a410d74c 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/AbstractYamlAssistContext.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/AbstractYamlAssistContext.java @@ -91,7 +91,8 @@ public abstract class AbstractYamlAssistContext implements YamlAssistContext { } protected SNode getContextNode() throws Exception { - return contextPath.traverse((SNode)getContextRoot(getDocument())); + SNode root = (SNode)getContextRoot(getDocument()); + return contextPath.traverse(root); } protected SDocNode getContextRoot(YamlDocument file) throws Exception { diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/YTypeAssistContext.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/YTypeAssistContext.java index e634c2dbe..17f1981be 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/YTypeAssistContext.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/completion/YTypeAssistContext.java @@ -167,7 +167,7 @@ public class YTypeAssistContext extends AbstractYamlAssistContext { return contextWith(s, typeUtil.getDomainType(type)); } String key = s.toPropString(); - SNode contextNode = getContextNode(); + SNode contextNode = getContextNode(); DynamicSchemaContext dynamicCtxt = new SNodeDynamicSchemaContext(contextNode); Map subproperties = typeUtil.getPropertiesMap(type, dynamicCtxt); if (subproperties!=null) { diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlNavigable.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlNavigable.java index f22d67328..32d631786 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlNavigable.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlNavigable.java @@ -10,13 +10,42 @@ *******************************************************************************/ package org.springframework.ide.vscode.commons.yaml.path; +import java.util.stream.Stream; + +import org.springframework.ide.vscode.commons.util.Log; import org.springframework.ide.vscode.commons.yaml.structure.YamlStructureParser.SNode; /** * Different types of things (e.g. {@link ApplicationYamlAssistContext}, {@link SNode} ...) can * be traversed interpeting {@link YamlPath} as 'navigation operations'. To facilitate * 'reusable' traversal code, they can implement this interface. + *

+ * WARNING: both methods in this interface have default implementation. However at least one + * of them must be implemented explicitly otherwise they will call eachother in an infinite recursion! */ public interface YamlNavigable { - T traverse(YamlPathSegment s) throws Exception; + + /** + * Traversal which silently ignores ambiguity by picking the first valid target + * returned by traverseAmbiguously. + */ + default T traverse(YamlPathSegment s) throws Exception { + return traverseAmbiguously(s).findFirst().orElse(null); + } + + /** + * To support traversal in the face of ambiguous steps (i.e. when a step may lead to multiple valid targets), + * implement this method. For convenience a default implementation is provided which calls `traverse`. + * Obviously, this implementation doesn't truly support ambiguity but it is sufficient for YamlNavigables + * where there is no ambiguity, or if you don't care about it. + */ + default Stream traverseAmbiguously(YamlPathSegment s) { + try { + T it = traverse(s); + return it == null ? Stream.empty() : Stream.of(it); + } catch (Exception e) { + Log.log(e); + return Stream.empty(); + } + } } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlPath.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlPath.java index 98618518a..7567fb342 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlPath.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/path/YamlPath.java @@ -13,6 +13,7 @@ package org.springframework.ide.vscode.commons.yaml.path; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.stream.Stream; import org.springframework.ide.vscode.commons.yaml.ast.NodeRef; import org.springframework.ide.vscode.commons.yaml.ast.NodeUtil; @@ -121,18 +122,18 @@ public class YamlPath { } public > T traverse(T startNode) { - try { - T node = startNode; + return traverseAmbiguously(startNode).findFirst().orElse(null); + } + + public > Stream traverseAmbiguously(T startNode) { + if (startNode!=null) { + Stream result = Stream.of(startNode); for (YamlPathSegment s : segments) { - if (node==null) { - return null; - } - node = node.traverse(s); + result = result.flatMap((node) -> node.traverseAmbiguously(s)); } - return node; - } catch (Exception e) { - return null; + return result; } + return Stream.empty(); } public YamlPath dropFirst(int dropCount) { @@ -265,4 +266,5 @@ public class YamlPath { return new YamlPath(common); } + } diff --git a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/structure/YamlStructureParser.java b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/structure/YamlStructureParser.java index cc488933f..f050ba8a6 100644 --- a/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/structure/YamlStructureParser.java +++ b/vscode-extensions/commons/commons-yaml/src/main/java/org/springframework/ide/vscode/commons/yaml/structure/YamlStructureParser.java @@ -3,22 +3,35 @@ package org.springframework.ide.vscode.commons.yaml.structure; import java.io.StringWriter; import java.io.Writer; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.regex.Pattern; +import java.util.stream.Stream; import org.springframework.ide.vscode.commons.util.Assert; import org.springframework.ide.vscode.commons.util.CollectionUtil; +import org.springframework.ide.vscode.commons.util.Log; import org.springframework.ide.vscode.commons.util.StringUtil; import org.springframework.ide.vscode.commons.util.text.IRegion; import org.springframework.ide.vscode.commons.yaml.path.KeyAliases; import org.springframework.ide.vscode.commons.yaml.path.YamlNavigable; 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.structure.YamlStructureParser.SKeyNode; import org.springframework.ide.vscode.commons.yaml.util.YamlIndentUtil; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableList.Builder; +import com.google.common.collect.ListMultimap; +import com.google.common.collect.Multimap; +import com.google.common.collect.MultimapBuilder; + +import reactor.util.function.Tuple2; +import reactor.util.function.Tuples; + /** * A robust, coarse-grained parser that guesses the structure of a * yml document based on indentation levels. @@ -251,34 +264,56 @@ public class YamlStructureParser { * Default implementation, doesn't support any type of traversal operation. * Subclasses must override and implement where appropriate. */ - @Override public SNode traverse(YamlPathSegment s) throws Exception { return null; } protected abstract void dump(Writer out, int indent) throws Exception; - public YamlPath getPath() throws Exception { - ArrayList segments = new ArrayList(); - buildPath(this, segments); - return new YamlPath(segments); + public ImmutableList getPathNodes() throws Exception { + ImmutableList.Builder nodes = ImmutableList.builder(); + buildPath(this, nodes); + return nodes.build(); } - private static void buildPath(SNode node, ArrayList segments) throws Exception { + private static void buildPath(SNode node, Builder nodes) { + if (node!=null) { + buildPath(node.getParent(), nodes); + nodes.add(node); + } + } + + public YamlPath getPath() throws Exception { + List path = new ArrayList<>(); + for (SNode node : getPathNodes()) { + YamlPathSegment segment = getSegment(node); + if (segment!=null) { + path.add(segment); + } + } + return new YamlPath(path); + } + + /** + * Determine a YamlPathSegment that corresponds to given node. This may be + * null because not all SNodes can be interpreted as 'step' in the yml + * structure (e.g. raw nodes will return null, as will the 'root' node). + */ + private YamlPathSegment getSegment(SNode node) throws Exception { if (node!=null) { - buildPath(node.getParent(), segments); SNodeType nodeType = node.getNodeType(); if (nodeType==SNodeType.KEY) { String key = ((SKeyNode)node).getKey(); - segments.add(YamlPathSegment.valueAt(key)); + return YamlPathSegment.valueAt(key); } else if (nodeType==SNodeType.SEQ) { int index = ((SSeqNode)node).getIndex(); - segments.add(YamlPathSegment.valueAt(index)); + return YamlPathSegment.valueAt(index); } else if (nodeType==SNodeType.DOC) { int index = ((SDocNode)node).getIndex(); - segments.add(YamlPathSegment.valueAt(index)); + return YamlPathSegment.valueAt(index); } } + return null; } public SRootNode getRoot() { @@ -360,7 +395,7 @@ public class YamlStructureParser { public abstract class SChildBearingNode extends SNode { private List children = null; - private Map keyMap = null; //lazily constructed index of children children. + private Multimap keyMap = null; //lazily constructed index of children. public SChildBearingNode(SChildBearingNode parent, YamlDocument doc, int indent, int start, int end) { super(parent, doc, indent, start, end); @@ -422,14 +457,14 @@ public class YamlStructureParser { } @Override - public SNode traverse(YamlPathSegment s) throws Exception { + public Stream traverseAmbiguously(YamlPathSegment s) { switch (s.getType()) { case VAL_AT_KEY: - return this.getChildWithKey(s.toPropString()); + return this.getChildrenWithKey(s.toPropString()); case VAL_AT_INDEX: - return this.getSeqChildWithIndex(s.toIndex()); + return Stream.of(this.getSeqChildWithIndex(s.toIndex())); default: - return null; + return Stream.empty(); } } @@ -446,36 +481,43 @@ public class YamlStructureParser { return null; } - public SKeyNode getChildWithKey(String key) throws Exception { + public Stream getChildrenWithKey(String key) { if (CollectionUtil.hasElements(children)) { - SKeyNode child = keyMap().get(key); - if (child==null) { - Iterable keyAliases = getKeyAliases(key); - if (keyAliases!=null) { - for (String keyAlias : keyAliases) { - child = keyMap().get(keyAlias); - if (child!=null) { - return child; - } + Stream allChildren = Stream.empty(); + Collection plainChildren = keyMap().get(key); + if (CollectionUtil.hasElements(plainChildren)) { + allChildren = plainChildren.stream(); + } + Iterable keyAliases = getKeyAliases(key); + if (keyAliases!=null) { + for (String keyAlias : keyAliases) { + Collection aliasChildren = keyMap().get(keyAlias); + if (CollectionUtil.hasElements(aliasChildren)) { + allChildren = Stream.concat(allChildren, aliasChildren.stream()); } } } - return child; + return allChildren; } - return null; + return Stream.empty(); + } + + public SKeyNode getChildWithKey(String key) { + return (SKeyNode)getChildrenWithKey(key).findFirst().orElse(null); } - private Map keyMap() throws Exception { + private Multimap keyMap() { if (keyMap==null) { - HashMap index = new HashMap(); + ListMultimap index = MultimapBuilder.hashKeys().arrayListValues().build(); for (SNode node: getChildren()) { - if (node.getNodeType()==SNodeType.KEY) { - SKeyNode keyNode = (SKeyNode)node; - String key = ((SKeyNode)node).getKey(); - SKeyNode existing = index.get(key); - if (existing==null) { + try { + if (node.getNodeType()==SNodeType.KEY) { + SKeyNode keyNode = (SKeyNode)node; + String key = ((SKeyNode)node).getKey(); index.put(key, keyNode); } + } catch (Exception e) { + Log.log(e); } } keyMap = index; diff --git a/vscode-extensions/vscode-boot-properties/src/main/java/org/springframework/ide/vscode/boot/yaml/completions/ApplicationYamlAssistContext.java b/vscode-extensions/vscode-boot-properties/src/main/java/org/springframework/ide/vscode/boot/yaml/completions/ApplicationYamlAssistContext.java index ef6019d0c..4e75f3eca 100644 --- a/vscode-extensions/vscode-boot-properties/src/main/java/org/springframework/ide/vscode/boot/yaml/completions/ApplicationYamlAssistContext.java +++ b/vscode-extensions/vscode-boot-properties/src/main/java/org/springframework/ide/vscode/boot/yaml/completions/ApplicationYamlAssistContext.java @@ -17,6 +17,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.stream.Stream; import org.springframework.boot.configurationmetadata.Deprecation; import org.springframework.ide.vscode.boot.common.InformationTemplates; @@ -125,27 +126,6 @@ public abstract class ApplicationYamlAssistContext extends AbstractYamlAssistCon return new IndexContext(doc, documentSelector, YamlPath.EMPTY, IndexNavigator.with(index), completionFactory, typeUtil, conf); } - public static YamlAssistContext forPath(YamlDocument doc, YamlPath contextPath, FuzzyMap index, PropertyCompletionFactory completionFactory, TypeUtil typeUtil, RelaxedNameConfig conf) { - try { - YamlPathSegment documentSelector = contextPath.getSegment(0); - if (documentSelector!=null) { - contextPath = contextPath.dropFirst(1); - YamlAssistContext context = ApplicationYamlAssistContext.subdocument(doc, documentSelector.toIndex(), index, completionFactory, typeUtil, conf); - for (YamlPathSegment s : contextPath.getSegments()) { - if (context==null) return null; - context = context.traverse(s); - } - return context; - } - } catch (Exception e) { - Log.log(e); - } - return null; - } - - @Override - abstract public YamlAssistContext traverse(YamlPathSegment s) throws Exception; - private static class TypeContext extends ApplicationYamlAssistContext { private PropertyCompletionFactory completionFactory; @@ -415,7 +395,7 @@ public abstract class ApplicationYamlAssistContext extends AbstractYamlAssistCon if (!matchingProps.isEmpty()) { ArrayList completions = new ArrayList(); for (Match match : matchingProps) { - DocumentEdits edits = createEdits(doc, offset, query, match); + DocumentEdits edits = createEdits(doc, node, offset, query, match); ScoreableProposal completion = completionFactory.property( doc.getDocument(), edits, match, typeUtil ); @@ -430,7 +410,7 @@ public abstract class ApplicationYamlAssistContext extends AbstractYamlAssistCon } protected DocumentEdits createEdits(final YamlDocument file, - final int offset, final String query, final Match match) + SNode node, final int offset, final String query, final Match match) throws Exception { //Edits created lazyly as they are somwehat expensive to compute and mostly // we need only the edits for the one proposal that user picks. diff --git a/vscode-extensions/vscode-boot-properties/src/test/java/org/springframework/ide/vscode/boot/test/ApplicationYamlEditorTest.java b/vscode-extensions/vscode-boot-properties/src/test/java/org/springframework/ide/vscode/boot/test/ApplicationYamlEditorTest.java index cc83ea306..b818b3f6c 100644 --- a/vscode-extensions/vscode-boot-properties/src/test/java/org/springframework/ide/vscode/boot/test/ApplicationYamlEditorTest.java +++ b/vscode-extensions/vscode-boot-properties/src/test/java/org/springframework/ide/vscode/boot/test/ApplicationYamlEditorTest.java @@ -3311,7 +3311,12 @@ public class ApplicationYamlEditorTest extends AbstractPropsEditorTest { " activemq:\n" + " broker-u<*>" , // ==> - "fill it in later" + "spring:\n" + + " application:\n" + + " name: my-app\n" + + "spring:\n" + + " activemq:\n" + + " broker-url: <*>" ); }