catalog multi-item code review issues, incl version and better error msgs
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/fb23d41f Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/fb23d41f Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/fb23d41f Branch: refs/heads/master Commit: fb23d41fc4501d2750d0180b878f6aaf3e39e542 Parents: 6f75f40 Author: Alex Heneveld <[email protected]> Authored: Mon Apr 20 13:32:38 2015 +0100 Committer: Alex Heneveld <[email protected]> Committed: Mon Apr 20 15:02:35 2015 +0100 ---------------------------------------------------------------------- .../catalog/internal/BasicBrooklynCatalog.java | 60 +++++++++++++++----- .../brooklyn/catalog/internal/CatalogUtils.java | 17 +++++- .../internal/EntityManagementUtils.java | 13 ++++- .../policy/basic/AbstractEntityAdjunct.java | 4 ++ .../catalog/internal/CatalogVersioningTest.java | 23 ++++++++ docs/guide/ops/catalog/index.md | 12 ++-- .../catalog/CatalogYamlVersioningTest.java | 56 +++++++++++++++++- .../brooklyn/util/exceptions/Exceptions.java | 2 +- 8 files changed, 161 insertions(+), 26 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java index 680eaf1..cd1f8c2 100644 --- a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java @@ -584,7 +584,9 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { Map<Object,Object> catalogMetadata = MutableMap.builder().putAll(parentMetadata).putAll(itemMetadata).build(); - // libraries we treat specially, to append the list, with the child's list preferred in classloading order + // brooklyn.libraries we treat specially, to append the list, with the child's list preferred in classloading order + // `libraries` is supported in some places as a legacy syntax; it should always be `brooklyn.libraries` for new apps + // TODO in 0.8.0 require brooklyn.libraries, don't allow "libraries" on its own List<?> librariesNew = MutableList.copyOf(getFirstAs(itemMetadata, List.class, "brooklyn.libraries", "libraries").orNull()); Collection<CatalogBundle> libraryBundlesNew = CatalogItemDtoAbstract.parseLibraries(librariesNew); @@ -621,7 +623,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { PlanInterpreterGuessingType planInterpreter = new PlanInterpreterGuessingType(null, item, sourceYaml, itemType, loader, result).reconstruct(); if (!planInterpreter.isResolved()) { - throw new IllegalStateException("Could not resolve plan: "+sourceYaml); + throw Exceptions.create("Could not resolve item:\n"+sourceYaml, planInterpreter.getErrors()); } itemType = planInterpreter.getCatalogItemType(); Map<?, ?> itemAsMap = planInterpreter.getItem(); @@ -761,6 +763,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { DeploymentPlan plan; AbstractBrooklynObjectSpec<?,?> spec; boolean resolved = false; + List<Exception> errors = MutableList.of(); public PlanInterpreterGuessingType(@Nullable String id, Object item, String itemYaml, @Nullable CatalogItemType optionalCiType, BrooklynClassLoadingContext loader, List<CatalogItemDtoAbstract<?,?>> itemsDefinedSoFar) { @@ -781,12 +784,17 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } public PlanInterpreterGuessingType reconstruct() { - attemptType(null, CatalogItemType.ENTITY); - attemptType(null, CatalogItemType.TEMPLATE); - - attemptType("services", CatalogItemType.ENTITY); - attemptType(POLICIES_KEY, CatalogItemType.POLICY); - attemptType(LOCATIONS_KEY, CatalogItemType.LOCATION); + if (catalogItemType==CatalogItemType.TEMPLATE) { + // template *must* be explicitly defined, and if so, none of the other calls apply + attemptType(null, CatalogItemType.TEMPLATE); + + } else { + attemptType(null, CatalogItemType.ENTITY); + + attemptType("services", CatalogItemType.ENTITY); + attemptType(POLICIES_KEY, CatalogItemType.POLICY); + attemptType(LOCATIONS_KEY, CatalogItemType.LOCATION); + } if (!resolved && catalogItemType==CatalogItemType.TEMPLATE) { // anything goes, for an explicit template, because we can't easily recurse into the types @@ -799,6 +807,12 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { public boolean isResolved() { return resolved; } + /** Returns potentially useful errors encountered while guessing types. + * May only be available where the type is known. */ + public List<Exception> getErrors() { + return errors; + } + public CatalogItemType getCatalogItemType() { return catalogItemType; } @@ -821,14 +835,21 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } // first look in collected items, if a key is given String type = (String) item.get("type"); + String version = null; + if (CatalogUtils.looksLikeVersionedId(type)) { + version = CatalogUtils.getVersionFromVersionedId(type); + type = CatalogUtils.getIdFromVersionedId(type); + } if (type!=null && key!=null) { for (CatalogItemDtoAbstract<?,?> candidate: itemsDefinedSoFar) { if (type.equals(candidate.getSymbolicName()) || type.equals(candidate.getId())) { - // matched - exit - catalogItemType = candidateCiType; - planYaml = candidateYaml; - resolved = true; - return true; + if (version==null || version.equals(candidate.getVersion())) { + // matched - exit + catalogItemType = candidateCiType; + planYaml = candidateYaml; + resolved = true; + return true; + } } } } @@ -846,6 +867,19 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { return true; } catch (Exception e) { Exceptions.propagateIfFatal(e); + // record the error if we have reason to expect this guess to succeed + if (item.containsKey("services") && (candidateCiType==CatalogItemType.ENTITY || candidateCiType==CatalogItemType.TEMPLATE)) { + // explicit services supplied, so plan should have been parseable for an entity or a a service + errors.add(e); + } else if (catalogItemType!=null && key!=null) { + // explicit itemType supplied, so plan should be parseable in the cases where we're given a key + // (when we're not given a key, the previous block should apply) + errors.add(e); + } else { + // all other cases, the error is probably due to us not getting the type right, ignore it + if (log.isTraceEnabled()) + log.trace("Guessing type of plan, it looks like it isn't "+candidateCiType+"/"+key+": "+e); + } } // finally try parsing a cut-down plan, in case there is a nested reference to a newly defined catalog item http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/core/src/main/java/brooklyn/catalog/internal/CatalogUtils.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/catalog/internal/CatalogUtils.java b/core/src/main/java/brooklyn/catalog/internal/CatalogUtils.java index c72fc9b..7c9aa86 100644 --- a/core/src/main/java/brooklyn/catalog/internal/CatalogUtils.java +++ b/core/src/main/java/brooklyn/catalog/internal/CatalogUtils.java @@ -176,7 +176,22 @@ public class CatalogUtils { } public static boolean looksLikeVersionedId(String versionedId) { - return versionedId != null && versionedId.indexOf(VERSION_DELIMITER) != -1; + if (versionedId==null) return false; + int fi = versionedId.indexOf(VERSION_DELIMITER); + if (fi<0) return false; + int li = versionedId.lastIndexOf(VERSION_DELIMITER); + if (li!=fi) { + // if multiple colons, we say it isn't a versioned reference; the prefix in that case must understand any embedded versioning scheme + // this fixes the case of: http://localhost:8080 + return false; + } + String candidateVersion = versionedId.substring(li+1); + if (!candidateVersion.matches("[0-9]+(|(\\.|_).*)")) { + // version must start with a number, followed if by anything with full stop or underscore before any other characters + // e.g. foo:1 or foo:1.1 or foo:1_SNAPSHOT all supported, but not e.g. foo:bar (or chef:cookbook or docker:my/image) + return false; + } + return true; } public static String getIdFromVersionedId(String versionedId) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/core/src/main/java/brooklyn/management/internal/EntityManagementUtils.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/management/internal/EntityManagementUtils.java b/core/src/main/java/brooklyn/management/internal/EntityManagementUtils.java index 6734654..e6dc12a 100644 --- a/core/src/main/java/brooklyn/management/internal/EntityManagementUtils.java +++ b/core/src/main/java/brooklyn/management/internal/EntityManagementUtils.java @@ -112,8 +112,17 @@ public class EntityManagementUtils { Entities.startManagement((Application)app, mgmt); return (T) app; } else { - assembly = instantiator.instantiate(at, camp); - return (T) mgmt.getEntityManager().getEntity(assembly.getId()); + // currently, all brooklyn plans should produce the above; currently this will always throw Unsupported + try { + assembly = instantiator.instantiate(at, camp); + return (T) mgmt.getEntityManager().getEntity(assembly.getId()); + } catch (UnsupportedOperationException e) { + // map this (expected) error to a nicer message + throw new IllegalArgumentException("Unrecognized application blueprint format"); + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + throw new IllegalArgumentException("Invalid plan: "+at, e); + } } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/core/src/main/java/brooklyn/policy/basic/AbstractEntityAdjunct.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/policy/basic/AbstractEntityAdjunct.java b/core/src/main/java/brooklyn/policy/basic/AbstractEntityAdjunct.java index 1748de8..394ad03 100644 --- a/core/src/main/java/brooklyn/policy/basic/AbstractEntityAdjunct.java +++ b/core/src/main/java/brooklyn/policy/basic/AbstractEntityAdjunct.java @@ -76,6 +76,10 @@ public abstract class AbstractEntityAdjunct extends AbstractBrooklynObject imple private boolean _legacyNoConstructionInit; + /** + * @deprecated since 0.7.0; leftover properties are put into config, since when coming from yaml this is normal. + */ + @Deprecated protected Map<String,Object> leftoverProperties = Maps.newLinkedHashMap(); protected transient ExecutionContext execution; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/core/src/test/java/brooklyn/catalog/internal/CatalogVersioningTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/catalog/internal/CatalogVersioningTest.java b/core/src/test/java/brooklyn/catalog/internal/CatalogVersioningTest.java index 179ce8f..6c4dcc0 100644 --- a/core/src/test/java/brooklyn/catalog/internal/CatalogVersioningTest.java +++ b/core/src/test/java/brooklyn/catalog/internal/CatalogVersioningTest.java @@ -22,6 +22,7 @@ import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertTrue; +import org.testng.Assert; import org.testng.annotations.AfterMethod; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -52,6 +53,28 @@ public class CatalogVersioningTest { } @Test + public void testParsingVersion() { + assertVersionParsesAs("foo:1", "foo", "1"); + assertVersionParsesAs("foo", null, null); + assertVersionParsesAs("foo:1.1", "foo", "1.1"); + assertVersionParsesAs("foo:1_SNAPSHOT", "foo", "1_SNAPSHOT"); + assertVersionParsesAs("foo:10.9.8_SNAPSHOT", "foo", "10.9.8_SNAPSHOT"); + assertVersionParsesAs("foo:bar", null, null); + assertVersionParsesAs("chef:cookbook", null, null); + assertVersionParsesAs("http://foo:8080";, null, null); + } + + private static void assertVersionParsesAs(String versionedId, String id, String version) { + if (version==null) { + Assert.assertFalse(CatalogUtils.looksLikeVersionedId(versionedId)); + } else { + Assert.assertTrue(CatalogUtils.looksLikeVersionedId(versionedId)); + Assert.assertEquals(CatalogUtils.getIdFromVersionedId(versionedId), id); + Assert.assertEquals(CatalogUtils.getVersionFromVersionedId(versionedId), version); + } + } + + @Test public void testAddVersioned() { String symbolicName = "sampleId"; String version = "0.1.0"; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/docs/guide/ops/catalog/index.md ---------------------------------------------------------------------- diff --git a/docs/guide/ops/catalog/index.md b/docs/guide/ops/catalog/index.md index a2e4408..8477204 100644 --- a/docs/guide/ops/catalog/index.md +++ b/docs/guide/ops/catalog/index.md @@ -82,7 +82,7 @@ In addition to the above fields, exactly **one** of the following is also requir or a full application blueprint (in the usual YAML format) for a template; **or* - `items`: a list of catalog items, where each entry in the map follows the same schema as the `brooklyn.catalog` value, and the keys in these map override any metadata specified as - a sibling of this `items` key (or, in the case of `libraries` they add to the list); + a sibling of this `items` key (or, in the case of `brooklyn.libraries` they add to the list); if there are references between items, then order is important, `items` are processed in order, depth-first, and forward references are not supported. @@ -99,18 +99,18 @@ The following optional catalog metadata is supported: - `name`: a nicely formatted display name for the item, used when presenting it in a GUI - `description`: supplies an extended textual description for the item - `iconUrl`: points to an icon for the item, used when presenting it in a GUI. - The URL prefix `classpath` is supported but these URLs may *not* refer items in any OSGi bundle in the `libraries` section + The URL prefix `classpath` is supported but these URLs may *not* refer items in any OSGi bundle in the `brooklyn.libraries` section (to prevent requiring all OSGi bundles to be loaded at launch). Icons are instead typically installed either at the server from which the OSGi bundles or catalog items are supplied or in the `conf` folder of the Brooklyn distro. -- `libraries`: a list of pointers to OSGi bundles required for the catalog item,. +- `brooklyn.libraries`: a list of pointers to OSGi bundles required for the catalog item. This can be omitted if blueprints are pure YAML and everything required is included in the classpath and catalog. Where custom Java code or bundled resources is needed, however, OSGi JARs supply a convenient packaging format and a very powerful versioning format. Libraries should be supplied in the form - `libraries: [ "http://...";, "http://..."; ]`, + `brooklyn.libraries: [ "http://...";, "http://..."; ]`, or as - `libraries: [ { name: symbolic-name, version: 1.0, url: http://... }, ... ]` if `symbolic-name:1.0` + `brooklyn.libraries: [ { name: symbolic-name, version: 1.0, url: http://... }, ... ]` if `symbolic-name:1.0` might already be installed from a different URL and you want to skip the download. Note that these URLs should point at immutable OSGi bundles; if the contents at any of these URLs changes, the behaviour of the blueprint may change @@ -211,7 +211,7 @@ or POSTed to the applications endpoint to deploy an instance. POSTing to the applications endpoint, will ignored the `brooklyn.catalog` information; this means references to any `item` blocks in the `<catalog-metadata>` will not be resolved, -and any OSGi `libraries` defined there will not be loaded.) +and any OSGi `brooklyn.libraries` defined there will not be loaded.) http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlVersioningTest.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlVersioningTest.java b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlVersioningTest.java index 5b43c99..c1176ea 100644 --- a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlVersioningTest.java +++ b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlVersioningTest.java @@ -24,6 +24,7 @@ import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; import io.brooklyn.camp.brooklyn.AbstractYamlTest; +import org.testng.Assert; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; @@ -32,6 +33,9 @@ import brooklyn.catalog.CatalogItem; import brooklyn.catalog.CatalogPredicates; import brooklyn.catalog.internal.BasicBrooklynCatalog; import brooklyn.entity.Entity; +import brooklyn.entity.basic.BasicApplication; +import brooklyn.entity.basic.BasicEntity; +import brooklyn.entity.basic.ConfigKeys; import com.google.common.base.Predicates; import com.google.common.collect.Iterables; @@ -144,7 +148,7 @@ public class CatalogYamlVersioningTest extends AbstractYamlTest { String parentName = "parentId"; String v1 = "0.1.0"; String v2 = "0.2.0"; - String expectedType = "brooklyn.entity.basic.BasicApplication"; + String expectedType = BasicApplication.class.getName(); addCatalogEntity(symbolicName, v1, expectedType); addCatalogEntity(symbolicName, v2); @@ -163,7 +167,7 @@ public class CatalogYamlVersioningTest extends AbstractYamlTest { String parentName = "parentId"; String v1 = "0.1.0"; String v2 = "0.2.0"; - String expectedType = "brooklyn.entity.basic.BasicApplication"; + String expectedType = BasicApplication.class.getName(); addCatalogEntity(symbolicName, v1); addCatalogEntity(symbolicName, v2, expectedType); @@ -176,6 +180,52 @@ public class CatalogYamlVersioningTest extends AbstractYamlTest { assertEquals(app.getEntityType().getName(), expectedType); } + private void doTestVersionedReferenceJustAdded(boolean isVersionImplicitSyntax) throws Exception { + addCatalogItem( "brooklyn.catalog:", + " version: 0.9", + " items:", + " - id: referrent", + " item:", + " type: "+BasicEntity.class.getName(), + " - id: referrent", + " version: 1.1", + " item:", + " type: "+BasicEntity.class.getName(), + " brooklyn.config: { foo: bar }", + " - id: referrer", + " version: 1.0", + " item:", + (isVersionImplicitSyntax ? + " type: referrent:1.1" : + " type: referrent\n" + + " version: 1.1")); + + Iterable<CatalogItem<Object, Object>> items = catalog.getCatalogItems(CatalogPredicates.symbolicName(Predicates.equalTo("referrer"))); + Assert.assertEquals(Iterables.size(items), 1, "Wrong number of: "+items); + CatalogItem<Object, Object> item = Iterables.getOnlyElement(items); + Assert.assertEquals(item.getVersion(), "1.0"); + + Entity app = createAndStartApplication( + "services:", + (isVersionImplicitSyntax ? + "- type: referrer:1.0" : + "- type: referrer\n" + + " version: 1.0") ); + Entity child = Iterables.getOnlyElement(app.getChildren()); + Assert.assertTrue(child instanceof BasicEntity, "Wrong child: "+child); + Assert.assertEquals(child.getConfig(ConfigKeys.newStringConfigKey("foo")), "bar"); + } + + @Test + public void testVersionedReferenceJustAddedExplicitVersion() throws Exception { + doTestVersionedReferenceJustAdded(false); + } + + @Test + public void testVersionedReferenceJustAddedImplicitVersionSyntax() throws Exception { + doTestVersionedReferenceJustAdded(true); + } + private void assertSingleCatalogItem(String symbolicName, String version) { Iterable<CatalogItem<Object, Object>> items = catalog.getCatalogItems(CatalogPredicates.symbolicName(Predicates.equalTo(symbolicName))); CatalogItem<Object, Object> item = Iterables.getOnlyElement(items); @@ -184,7 +234,7 @@ public class CatalogYamlVersioningTest extends AbstractYamlTest { } private void addCatalogEntity(String symbolicName, String version) { - addCatalogEntity(symbolicName, version, "brooklyn.entity.basic.BasicEntity"); + addCatalogEntity(symbolicName, version, BasicEntity.class.getName()); } private void addCatalogEntity(String symbolicName, String version, String type) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fb23d41f/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java index 066ecea..4303e4e 100644 --- a/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java +++ b/utils/common/src/main/java/brooklyn/util/exceptions/Exceptions.java @@ -271,7 +271,7 @@ public class Exceptions { } if (exceptions.isEmpty()) { if (Strings.isBlank(prefix)) return new CompoundRuntimeException("(empty compound exception)", exceptions); - return new CompoundRuntimeException(prefix+": (empty compound exception)", exceptions); + return new CompoundRuntimeException(prefix, exceptions); } if (Strings.isBlank(prefix)) return new CompoundRuntimeException(exceptions.size()+" errors, including: " + Exceptions.collapseText(exceptions.iterator().next()), exceptions); return new CompoundRuntimeException(prefix+", "+exceptions.size()+" errors including: " + Exceptions.collapseText(exceptions.iterator().next()), exceptions);
