From 55960a8c1e7f4caa16bbd84f92de204baa9244f0 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Thu, 29 Jun 2017 17:08:59 -0700 Subject: [PATCH 1/2] Fix regression causing bogus warnings when there are no cf targets --- .../yaml/ManifestYamlCFBuildpacksProvider.java | 14 ++++++-------- .../yaml/ManifestYamlCFDomainsProvider.java | 12 ++++++------ .../yaml/ManifestYamlCFServicesProvider.java | 13 +++++-------- .../yaml/ManifestYamlLanguageServer.java | 1 - .../yaml/ManifestYamlStacksProvider.java | 12 +++++------- .../manifest/yaml/ManifestYamlEditorTest.java | 17 +++++++++++++++++ 6 files changed, 39 insertions(+), 30 deletions(-) diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFBuildpacksProvider.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFBuildpacksProvider.java index 449198e1b..1200e52ce 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFBuildpacksProvider.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFBuildpacksProvider.java @@ -30,9 +30,12 @@ public class ManifestYamlCFBuildpacksProvider extends AbstractCFHintsProvider { @Override public Collection getHints(List targets) throws Exception { - + if (targets==null || targets.isEmpty()) { + //no targets... means we don't know anything. Indicate this by returning null... + // this "don't know" value will suppress bogus warnings in the reconciler. + return null; + } List hints = new ArrayList<>(); - for (CFTarget cfTarget : targets) { List buildpacks = cfTarget.getBuildpacks(); @@ -51,12 +54,7 @@ public class ManifestYamlCFBuildpacksProvider extends AbstractCFHintsProvider { } } } - // Contract for the reconciler: return null if values cannot be - // resolved. Otherwise - // return non-empty list of buildpacks. For CF targets, a non-empty list - // of buildpacks is - // typically expected. - return !hints.isEmpty() ? hints : null; + return hints; } protected String getBuildpackLabel(CFTarget target, CFBuildpack buildpack) { diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFDomainsProvider.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFDomainsProvider.java index e588e81f3..d3496e32a 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFDomainsProvider.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFDomainsProvider.java @@ -29,9 +29,12 @@ public class ManifestYamlCFDomainsProvider extends AbstractCFHintsProvider { @Override public Collection getHints(List targets) throws Exception { - + if (targets==null || targets.isEmpty()) { + //no targets... means we don't know anything. Indicate this by returning null... + // this "don't know" value will suppress bogus warnings in the reconciler. + return null; + } List hints = new ArrayList<>(); - for (CFTarget cfTarget : targets) { List domains = cfTarget.getDomains(); @@ -48,10 +51,7 @@ public class ManifestYamlCFDomainsProvider extends AbstractCFHintsProvider { } } } - // Contract for the reconciler: return null if values cannot be - // resolved. Otherwise - // return non-empty list - return !hints.isEmpty() ? hints : null; + return hints; } protected String getLabel(CFTarget target, CFDomain domain) { diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFServicesProvider.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFServicesProvider.java index c2f773738..e2d329039 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFServicesProvider.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlCFServicesProvider.java @@ -30,14 +30,12 @@ public class ManifestYamlCFServicesProvider extends AbstractCFHintsProvider { @Override public Collection getHints(List targets) throws Exception { - - // NOTE: empty list of services is a VALID result. A CF target may have - // no service instances - // created, so if empty list is returned from the client, then RETURN empty list. don't - // return null - // for empty services cases + if (targets==null || targets.isEmpty()) { + //no targets... means we don't know anything. Indicate this by returning null... + // this "don't know" value will suppress bogus warnings in the reconciler. + return null; + } List hints = new ArrayList<>(); - for (CFTarget cfTarget : targets) { List services = cfTarget.getServices(); Renderable targetLabel = Renderables.text(cfTarget.getLabel()); @@ -54,7 +52,6 @@ public class ManifestYamlCFServicesProvider extends AbstractCFHintsProvider { } } } - return hints; } diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlLanguageServer.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlLanguageServer.java index 943fc7cd5..f6ff34ad0 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlLanguageServer.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlLanguageServer.java @@ -33,7 +33,6 @@ import org.springframework.ide.vscode.commons.languageserver.reconcile.IReconcil import org.springframework.ide.vscode.commons.languageserver.util.SimpleLanguageServer; import org.springframework.ide.vscode.commons.languageserver.util.SimpleTextDocumentService; import org.springframework.ide.vscode.commons.languageserver.util.SimpleWorkspaceService; -import org.springframework.ide.vscode.commons.languageserver.util.TextDocumentContentChange; import org.springframework.ide.vscode.commons.util.text.LanguageId; import org.springframework.ide.vscode.commons.util.text.TextDocument; import org.springframework.ide.vscode.commons.yaml.ast.YamlASTProvider; diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlStacksProvider.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlStacksProvider.java index 542069a71..3a2179850 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlStacksProvider.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlStacksProvider.java @@ -35,13 +35,12 @@ public class ManifestYamlStacksProvider extends AbstractCFHintsProvider { @Override protected Collection getHints(List targets) throws Exception { - // NOTE: empty list of services is a VALID result. A CF target may have - // no service instances - // created, so if empty list is returned from the client, then RETURN empty list. don't - // return null - // for empty services cases + if (targets==null || targets.isEmpty()) { + //no targets... means we don't know anything. Indicate this by returning null... + // this "don't know" value will suppress bogus warnings in the reconciler. + return null; + } List hints = new ArrayList<>(); - for (CFTarget cfTarget : targets) { List stacks = cfTarget.getStacks(); Renderable targetLabel = Renderables.text(cfTarget.getLabel()); @@ -57,7 +56,6 @@ public class ManifestYamlStacksProvider extends AbstractCFHintsProvider { } } } - return hints; } diff --git a/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java b/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java index 91512fa59..b2896892f 100644 --- a/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java +++ b/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java @@ -24,6 +24,7 @@ import org.eclipse.lsp4j.CompletionItem; import org.eclipse.lsp4j.Diagnostic; import org.eclipse.lsp4j.DiagnosticSeverity; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.mockito.Mockito; import org.springframework.ide.vscode.commons.cloudfoundry.client.CFBuildpack; @@ -997,6 +998,22 @@ public class ManifestYamlEditorTest { ); } + @Test public void noReconcileErrorsWhenNoTargets() throws Exception { + cloudfoundry.reset(); + when(cloudfoundry.defaultParamsProvider.getParams()).thenReturn(ImmutableList.of()); + Editor editor = harness.newEditor( + "applications:\n" + + "- name: foo\n" + + " buildpack: bad-buildpack\n" + + " stack: blah\n" + + " domain: something-domain.com\n" + + " services:\n" + + " - bad-service\n" + + " bogus: bad" //a token error to make sure reconciler is actually running! + ); + editor.assertProblems("bogus|Unknown property"); + } + @Test public void noReconcileErrorsWhenCFFactoryThrows() throws Exception { reset(cloudfoundry.factory); From 8d570e748ea7baeaa9e85a8724ad93be04733eb6 Mon Sep 17 00:00:00 2001 From: Kris De Volder Date: Fri, 30 Jun 2017 10:15:44 -0700 Subject: [PATCH 2/2] Fix handling of 'unknown domains' case in routes validation --- .../manifest/yaml/ManifestYmlSchema.java | 2 +- .../manifest/yaml/RouteValueParser.java | 26 +++++++++++-------- .../manifest/yaml/ManifestYamlEditorTest.java | 22 +++++++++++++++- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYmlSchema.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYmlSchema.java index a662de7fc..2dca078bd 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYmlSchema.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/ManifestYmlSchema.java @@ -42,7 +42,7 @@ import com.google.common.collect.ImmutableSet; /** * @author Kris De Volder */ -public class ManifestYmlSchema implements YamlSchema { +public final class ManifestYmlSchema implements YamlSchema { private static final String HEALTH_CHECK_HTTP_ENDPOINT_PROP = "health-check-http-endpoint"; private static final String HEALTH_CHECK_TYPE_PROP = "health-check-type"; diff --git a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/RouteValueParser.java b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/RouteValueParser.java index 440957a57..0f037aa26 100644 --- a/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/RouteValueParser.java +++ b/headless-services/manifest-yaml-language-server/src/main/java/org/springframework/ide/vscode/manifest/yaml/RouteValueParser.java @@ -13,28 +13,32 @@ import org.springframework.ide.vscode.commons.util.RegexpParser; import org.springframework.ide.vscode.commons.util.ValueParseException; public class RouteValueParser extends RegexpParser { - + private static final String ROUTE_REGEX = "^([\\da-z\\.-]+)(:\\d{1,5})?((\\/[\\dA-Za-z\\.-]+)*\\/?)?$"; private static final String ROUTE_TYPE_NAME = "Route"; private static final String ROUTE_DESCRIPTION = "HTTP or TCP application root route"; private static final int MAX_PORT_NUMBER = 65535; - + private Callable> domains; public RouteValueParser(Callable> domains) { super(ROUTE_REGEX, ROUTE_TYPE_NAME, ROUTE_DESCRIPTION); this.domains = domains; } - + private Matcher staticValidation(String str) throws Exception { return (Matcher) super.parse(str); } - + private Object dynamicValidation(String str, Matcher matcher) throws Exception { + if (domains==null) { + // If domains is unknown we can't do the dynamic checks, so bail out. + return str; + } try { Collection cloudDomains = Collections.emptyList(); try { - cloudDomains = domains == null ? Collections.emptyList() : domains.call(); + cloudDomains = domains.call(); } catch (ValueParseException e) { /* * If domains hint provider throws exception it is @@ -43,9 +47,9 @@ public class RouteValueParser extends RegexpParser { */ return matcher; } - // Ensure cloud domains is empty list instead of null if (cloudDomains == null) { - cloudDomains = Collections.emptyList(); + // If domains is unknown we can't do the dynamic checks, so bail out. + return str; } CFRoute route = CFRoute.builder().from(str, cloudDomains).build(); if (route.getDomain() == null || route.getDomain().isEmpty()) { @@ -65,13 +69,13 @@ public class RouteValueParser extends RegexpParser { String hostDomain = matcher.group(1); throw new ReconcileException("Unknown 'Domain'. Valid domains are: "+cloudDomains, ManifestYamlSchemaProblemsTypes.UNKNOWN_DOMAIN_PROBLEM, hostDomain.lastIndexOf(route.getDomain()), hostDomain.length()); } - return route; + return str; } catch (ConnectionException | NoTargetsException e) { // No connection to CF? Abort dynamic validation - return matcher; + return str; } } - + @Override public Object parse(String str) throws Exception { Matcher matcher = staticValidation(str); @@ -80,5 +84,5 @@ public class RouteValueParser extends RegexpParser { } return null; } - + } diff --git a/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java b/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java index b2896892f..4ec4ddbf6 100644 --- a/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java +++ b/headless-services/manifest-yaml-language-server/src/test/java/org/springframework/ide/vscode/manifest/yaml/ManifestYamlEditorTest.java @@ -999,9 +999,11 @@ public class ManifestYamlEditorTest { } @Test public void noReconcileErrorsWhenNoTargets() throws Exception { + Editor editor; cloudfoundry.reset(); when(cloudfoundry.defaultParamsProvider.getParams()).thenReturn(ImmutableList.of()); - Editor editor = harness.newEditor( + + editor = harness.newEditor( "applications:\n" + "- name: foo\n" + " buildpack: bad-buildpack\n" + @@ -1012,6 +1014,15 @@ public class ManifestYamlEditorTest { " bogus: bad" //a token error to make sure reconciler is actually running! ); editor.assertProblems("bogus|Unknown property"); + + editor = harness.newEditor( + "applications:\n" + + "- name: foo-foo\n" + + " buildpack: java_buildpack\n" + + " routes:\n" + + " - route: foo.blah/fooo\n" + ); + editor.assertProblems(/*NONE*/); } @Test @@ -1027,6 +1038,15 @@ public class ManifestYamlEditorTest { " bogus: bad" //a token error to make sure reconciler is actually running! ); editor.assertProblems("bogus|Unknown property"); + + editor = harness.newEditor( + "applications:\n" + + "- name: foo-foo\n" + + " buildpack: java_buildpack\n" + + " routes:\n" + + " - route: foo.blah/fooo\n" + ); + editor.assertProblems(/*NONE*/); } @Test