Address code review comments (PR 955)
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/5fe288f2 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/5fe288f2 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/5fe288f2 Branch: refs/heads/master Commit: 5fe288f27ad9902f52ad6601c09f7361f822d433 Parents: d79fd90 Author: Svetoslav Neykov <[email protected]> Authored: Thu Oct 15 12:13:30 2015 +0300 Committer: Svetoslav Neykov <[email protected]> Committed: Thu Oct 15 12:13:30 2015 +0300 ---------------------------------------------------------------------- .../brooklyn/core/mgmt/EntityManagementUtils.java | 3 +++ .../brooklyn/core/plan/PlanToSpecTransformer.java | 7 ++++++- .../core/resolve/CatalogServiceSpecResolver.java | 9 +-------- .../brooklyn/core/resolve/ServiceSpecResolver.java | 12 ++++++++++++ .../spi/creation/BrooklynComponentTemplateResolver.java | 6 +++--- .../brooklyn/spi/creation/CampToSpecTransformer.java | 7 ++++++- .../spi/creation/service/UrlServiceSpecResolver.java | 2 +- 7 files changed, 32 insertions(+), 14 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java index 0dca83c..5536a1e 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/EntityManagementUtils.java @@ -103,6 +103,9 @@ public class EntityManagementUtils { } public static <T,SpecT extends AbstractBrooklynObjectSpec<? extends T, SpecT>> SpecT createCatalogSpec(ManagementContext mgmt, final CatalogItem<T, SpecT> item, final Set<String> encounteredTypes) { + if (encounteredTypes.contains(item.getSymbolicName())) { + throw new IllegalStateException("Already encountered types " + encounteredTypes + " must not contain catalog item being resolver " + item.getSymbolicName()); + } return PlanToSpecFactory.attemptWithLoaders(mgmt, new Function<PlanToSpecTransformer, SpecT>() { @Override public SpecT apply(PlanToSpecTransformer input) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/core/src/main/java/org/apache/brooklyn/core/plan/PlanToSpecTransformer.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/plan/PlanToSpecTransformer.java b/core/src/main/java/org/apache/brooklyn/core/plan/PlanToSpecTransformer.java index b9ca8ca..24753aa 100644 --- a/core/src/main/java/org/apache/brooklyn/core/plan/PlanToSpecTransformer.java +++ b/core/src/main/java/org/apache/brooklyn/core/plan/PlanToSpecTransformer.java @@ -56,7 +56,12 @@ public interface PlanToSpecTransformer extends ManagementContextInjectable { * the catalog item might be known by type, or its source plan fragment text might be inspected and transformed. * implementations will typically look at the {@link CatalogItem#getCatalogItemType()} first. * <p> - * should throw {@link PlanNotRecognizedException} if this transformer does not know what to do with the plan. */ + * should throw {@link PlanNotRecognizedException} if this transformer does not know what to do with the plan. + * + * @param item - The catalog item to convert to a spec. The item might not be fully populated (i.e. missing {@code symbolicName} if called + * from the catalog parser). + * @param encounteredTypes - The {@code symbolicName}s of catalog items being resolved up the stack, but not including {@code item}. + */ <T,SpecT extends AbstractBrooklynObjectSpec<? extends T, SpecT>> SpecT createCatalogSpec(CatalogItem<T, SpecT> item, Set<String> encounteredTypes) throws PlanNotRecognizedException; } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/core/src/main/java/org/apache/brooklyn/core/resolve/CatalogServiceSpecResolver.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/CatalogServiceSpecResolver.java b/core/src/main/java/org/apache/brooklyn/core/resolve/CatalogServiceSpecResolver.java index f05355a..ded4ed3 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/CatalogServiceSpecResolver.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/CatalogServiceSpecResolver.java @@ -73,19 +73,12 @@ public class CatalogServiceSpecResolver extends AbstractServiceSpecResolver { //Prevent catalog items self-referencing even if explicitly different version. boolean nonRecursiveCall = !parentEncounteredTypes.contains(item.getSymbolicName()); if (nonRecursiveCall) { - // Make a copy of the encountered types, so that we add the item being resolved for - // dependency items only. Siblings must not see we are resolving this item. - Set<String> encounteredTypes = ImmutableSet.<String>builder() - .addAll(parentEncounteredTypes) - .add(item.getSymbolicName()) - .build(); - // CatalogItem generics are just getting in the way, better get rid of them, we // are casting anyway. @SuppressWarnings({ "rawtypes" }) CatalogItem rawItem = item; @SuppressWarnings({ "rawtypes", "unchecked" }) - AbstractBrooklynObjectSpec rawSpec = EntityManagementUtils.createCatalogSpec(mgmt, rawItem, encounteredTypes); + AbstractBrooklynObjectSpec rawSpec = EntityManagementUtils.createCatalogSpec(mgmt, rawItem, parentEncounteredTypes); return (EntitySpec<?>) rawSpec; } else { return null; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/core/src/main/java/org/apache/brooklyn/core/resolve/ServiceSpecResolver.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/ServiceSpecResolver.java b/core/src/main/java/org/apache/brooklyn/core/resolve/ServiceSpecResolver.java index 5e30cfa..64a6c66 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/ServiceSpecResolver.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/ServiceSpecResolver.java @@ -50,6 +50,18 @@ public interface ServiceSpecResolver extends ManagementContextInjectable { /** * Create a spec for the service type + * + * @param type - the string representation which should be converted to an EntitySpec + * @param loader - use it to load any Java classes + * @param encounteredTypes - an immutable set of the items which are currently being resolved up the stack, + * used to prevent cycles. Implementations should not try to resolve the type if the symbolicName is + * already contained in here. When resolving a type add it to a copy of the list before + * passing the new instance down the stack. See {@link CatalogServiceSpecResolver} for example usage. + * + * @return The {@link EntitySpec} corresponding to the passed {@code type} argument, possibly pre-configured + * based on the information contained in {@code type}. Return {@code null} value to indicate that + * the implementation doesn't know how to convert {@code type} to an {@link EntitySpec}. Throw an + * exception if {@code type} looks like a supported value, but can't be loaded. */ @Nullable EntitySpec<?> resolve(String type, BrooklynClassLoadingContext loader, Set<String> encounteredTypes); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java index e6a866e..cfdfab2 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynComponentTemplateResolver.java @@ -163,7 +163,7 @@ public class BrooklynComponentTemplateResolver { } else if (proto != null) { msgDetails = "The reference " + type + " looks like a URL (running the CAMP Brooklyn assembly-template instantiator) but the protocol " + proto + " isn't white listed (" + BrooklynCampConstants.YAML_URL_PROTOCOL_WHITELIST + "). " + - "Not a catalog item or java type as well."; + "It's also neither a catalog item nor a java type."; } else { msgDetails = "No resolver knew how to handle it. Using resolvers: " + serviceSpecResolver; } @@ -226,10 +226,10 @@ public class BrooklynComponentTemplateResolver { if (childLocations != null) spec.locations(childLocations); - decoreateSpec(spec); + decorateSpec(spec); } - private <T extends Entity> void decoreateSpec(EntitySpec<T> spec) { + private <T extends Entity> void decorateSpec(EntitySpec<T> spec) { new BrooklynEntityDecorationResolver.PolicySpecResolver(yamlLoader).decorate(spec, attrs); new BrooklynEntityDecorationResolver.EnricherSpecResolver(yamlLoader).decorate(spec, attrs); new BrooklynEntityDecorationResolver.InitializerResolver(yamlLoader).decorate(spec, attrs); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampToSpecTransformer.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampToSpecTransformer.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampToSpecTransformer.java index 9104f8f..60f00c6 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampToSpecTransformer.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampToSpecTransformer.java @@ -93,7 +93,12 @@ public class CampToSpecTransformer implements PlanToSpecTransformer { @Override public <T, SpecT extends AbstractBrooklynObjectSpec<? extends T, SpecT>> SpecT createCatalogSpec(CatalogItem<T, SpecT> item, Set<String> encounteredTypes) { // Ignore old-style java type catalog items - if (item.getPlanYaml() == null) return null; + if (item.getPlanYaml() == null) { + throw new PlanNotRecognizedException("Old style catalog item " + item + " not supported."); + } + if (encounteredTypes.contains(item.getSymbolicName())) { + throw new IllegalStateException("Already encountered types " + encounteredTypes + " must not contain catalog item being resolver " + item.getSymbolicName()); + } // Not really clear what should happen to the top-level attributes, ignored until a good use case appears. return (SpecT) CampCatalogUtils.createSpec(mgmt, (CatalogItem)item, encounteredTypes); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/5fe288f2/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/UrlServiceSpecResolver.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/UrlServiceSpecResolver.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/UrlServiceSpecResolver.java index b55c064..bde501b 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/UrlServiceSpecResolver.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/UrlServiceSpecResolver.java @@ -54,7 +54,7 @@ public class UrlServiceSpecResolver implements ServiceSpecResolver { try { yaml = ResourceUtils.create(this).getResourceAsString(type); } catch (Exception e) { - log.warn("AssemblyTemplate type " + type + " which looks like a URL can't be fetched.", e); + log.warn("AssemblyTemplate type " + type + " looks like a URL that can't be fetched.", e); return null; } // Referenced specs are expected to be CAMP format as well.
