Fix catalog items recursive check Identical sibling items shouldn't trigger the recursive check abort.
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/73d27909 Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/73d27909 Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/73d27909 Branch: refs/heads/master Commit: 73d279097d7a15eb7902c587ee2d5b4400c207b1 Parents: 073c203 Author: Svetoslav Neykov <[email protected]> Authored: Wed Oct 14 15:35:40 2015 +0300 Committer: Svetoslav Neykov <[email protected]> Committed: Wed Oct 14 17:11:07 2015 +0300 ---------------------------------------------------------------------- .../core/mgmt/EntityManagementUtils.java | 4 ++-- .../BrooklynAssemblyTemplateInstantiator.java | 4 ++-- .../BrooklynComponentTemplateResolver.java | 25 ++++++++++++++------ .../brooklyn/spi/creation/CampCatalogUtils.java | 11 +++++++-- .../service/CatalogServiceSpecResolver.java | 16 +++++++++---- .../brooklyn/catalog/CatalogYamlEntityTest.java | 21 ++++++++++++++++ 6 files changed, 64 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/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 cda3f5b..0dca83c 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 @@ -44,7 +44,6 @@ import org.apache.brooklyn.core.plan.PlanToSpecTransformer; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; -import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.task.TaskBuilder; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.text.Strings; @@ -57,6 +56,7 @@ import com.google.common.base.Function; import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; @@ -99,7 +99,7 @@ public class EntityManagementUtils { } public static <T,SpecT extends AbstractBrooklynObjectSpec<? extends T, SpecT>> SpecT createCatalogSpec(ManagementContext mgmt, CatalogItem<T, SpecT> item) { - return createCatalogSpec(mgmt, item, MutableSet.<String>of()); + return createCatalogSpec(mgmt, item, ImmutableSet.<String>of()); } public static <T,SpecT extends AbstractBrooklynObjectSpec<? extends T, SpecT>> SpecT createCatalogSpec(ManagementContext mgmt, final CatalogItem<T, SpecT> item, final Set<String> encounteredTypes) { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java index 19aa100..f295e90 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java @@ -39,11 +39,11 @@ import org.apache.brooklyn.core.mgmt.HasBrooklynManagementContext; import org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContext; import org.apache.brooklyn.core.mgmt.classloading.JavaBrooklynClassLoadingContext; import org.apache.brooklyn.entity.stock.BasicApplicationImpl; -import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.flags.TypeCoercions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Sets; @@ -90,7 +90,7 @@ public class BrooklynAssemblyTemplateInstantiator implements AssemblyTemplateSpe BrooklynComponentTemplateResolver resolver = BrooklynComponentTemplateResolver.Factory.newInstance( loader, buildWrapperAppTemplate(template)); - EntitySpec<? extends Application> app = resolver.resolveSpec(MutableSet.<String>of()); + EntitySpec<? extends Application> app = resolver.resolveSpec(ImmutableSet.<String>of()); app.configure(EntityManagementUtils.WRAPPER_APP_MARKER, Boolean.TRUE); // first build the children into an empty shell app http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/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 05252f5..b57d4df 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 @@ -29,6 +29,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; +import org.apache.brooklyn.api.catalog.CatalogItem; import org.apache.brooklyn.api.entity.Entity; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.location.Location; @@ -64,6 +65,7 @@ import org.slf4j.LoggerFactory; import com.google.common.base.Function; import com.google.common.base.Splitter; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; @@ -150,13 +152,22 @@ public class BrooklynComponentTemplateResolver { EntitySpec<?> spec = serviceSpecResolver.resolve(type, loader, encounteredCatalogTypes); if (spec == null) { - String proto = Urls.getProtocol(type); - if (proto != null) { - log.debug("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."); + // Try to provide some troubleshooting details + String msgDetails = ""; + CatalogItem<?, ?> item = CatalogUtils.getCatalogItemOptionalVersion(mgmt, Strings.removeFromStart(type, "catalog:")); + if (item != null && encounteredCatalogTypes.contains(item.getSymbolicName())) { + msgDetails = "Cycle between catalog items detected, starting from " + type + + ". Other catalog items being resolved up the stack are " + encounteredCatalogTypes + + ". Tried loading it as a Java class instead but failed."; + } else { + String proto = Urls.getProtocol(type); + 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."; + } } - throw new IllegalStateException("Unable to create spec for type " + type + ". No resolver knew how to handle it."); + throw new IllegalStateException("Unable to create spec for type " + type + ". No resolver knew how to handle it. " + msgDetails); } populateSpec(spec, encounteredCatalogTypes); @@ -352,7 +363,7 @@ public class BrooklynComponentTemplateResolver { @SuppressWarnings("unchecked") Map<String, Object> resolvedConfig = (Map<String, Object>)transformSpecialFlags(specConfig.getSpecConfiguration()); specConfig.setSpecConfiguration(resolvedConfig); - return Factory.newInstance(getLoader(), specConfig.getSpecConfiguration()).resolveSpec(MutableSet.<String>of()); + return Factory.newInstance(getLoader(), specConfig.getSpecConfiguration()).resolveSpec(ImmutableSet.<String>of()); } if (flag instanceof ManagementContextInjectable) { log.debug("Injecting Brooklyn management context info object: {}", flag); http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampCatalogUtils.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampCatalogUtils.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampCatalogUtils.java index 4865241..29791dd 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampCatalogUtils.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampCatalogUtils.java @@ -31,10 +31,11 @@ import org.apache.brooklyn.core.mgmt.classloading.BrooklynClassLoadingContext; import org.apache.brooklyn.util.text.Strings; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableSet; public class CampCatalogUtils { - public static AbstractBrooklynObjectSpec<?, ?> createSpec(ManagementContext mgmt, CatalogItem<?, ?> item, Set<String> encounteredTypes) { + public static AbstractBrooklynObjectSpec<?, ?> createSpec(ManagementContext mgmt, CatalogItem<?, ?> item, Set<String> parentEncounteredTypes) { // preferred way is to parse the yaml, to resolve references late; // the parsing on load is to populate some fields, but it is optional. // TODO messy for location and policy that we need brooklyn.{locations,policies} root of the yaml, but it works; @@ -43,9 +44,15 @@ public class CampCatalogUtils { BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(mgmt, item); Preconditions.checkNotNull(item.getCatalogItemType(), "catalog item type for "+item.getPlanYaml()); + Set<String> encounteredTypes; // symbolicName could be null if coming from the catalog parser where it tries to load before knowing the id if (item.getSymbolicName() != null) { - encounteredTypes.add(item.getSymbolicName()); + encounteredTypes = ImmutableSet.<String>builder() + .addAll(parentEncounteredTypes) + .add(item.getSymbolicName()) + .build(); + } else { + encounteredTypes = parentEncounteredTypes; } AbstractBrooklynObjectSpec<?, ?> spec; http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/CatalogServiceSpecResolver.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/CatalogServiceSpecResolver.java b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/CatalogServiceSpecResolver.java index 19a76b3..81207ee 100644 --- a/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/CatalogServiceSpecResolver.java +++ b/usage/camp/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/service/CatalogServiceSpecResolver.java @@ -32,6 +32,8 @@ import org.apache.brooklyn.core.mgmt.persist.DeserializingClassRenamesProvider; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import com.google.common.collect.ImmutableSet; + public class CatalogServiceSpecResolver extends AbstractServiceSpecResolver { private static final Logger log = LoggerFactory.getLogger(CatalogServiceSpecResolver.class); @@ -61,7 +63,7 @@ public class CatalogServiceSpecResolver extends AbstractServiceSpecResolver { } @Override - public EntitySpec<?> resolve(String type, BrooklynClassLoadingContext loader, Set<String> encounteredTypes) { + public EntitySpec<?> resolve(String type, BrooklynClassLoadingContext loader, Set<String> parentEncounteredTypes) { String localType = getLocalType(type); CatalogItem<Entity, EntitySpec<?>> item = getCatalogItem(mgmt, localType); if (item != null) { @@ -70,9 +72,15 @@ public class CatalogServiceSpecResolver extends AbstractServiceSpecResolver { //Take the symbolicName part of the catalog item only for recursion detection to prevent //cross referencing of different versions. Not interested in non-catalog item types. //Prevent catalog items self-referencing even if explicitly different version. - boolean firstOccurrence = encounteredTypes.add(item.getSymbolicName()); - boolean nonRecursiveCall = firstOccurrence; + 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" }) @@ -84,7 +92,7 @@ public class CatalogServiceSpecResolver extends AbstractServiceSpecResolver { return null; } } else { - return hardcodedResolver.resolve(type, loader, encounteredTypes); + return hardcodedResolver.resolve(type, loader, parentEncounteredTypes); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/73d27909/usage/camp/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/usage/camp/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java index 53cd146..9699b8e 100644 --- a/usage/camp/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java +++ b/usage/camp/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java @@ -670,6 +670,27 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { } @Test + public void testRecursiveCheckForDepenentsOnly() throws Exception { + String symbolicName = "my.catalog.app.id.basic"; + addCatalogItems( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION, + "", + "services:", + "- type: org.apache.brooklyn.entity.stock.BasicEntity"); + + createAndStartApplication( + "services:", + "- type: " + ver(symbolicName), + " brooklyn.children:", + " - type: " + ver(symbolicName), + "- type: " + ver(symbolicName), + " brooklyn.children:", + " - type: " + ver(symbolicName)); + } + + @Test public void testOsgiNotLeakingToParent() { addCatalogOSGiEntity(SIMPLE_ENTITY_TYPE); try {
