Don't recurse into same symbolicName catalog items when resolving CAMP dependencies.
Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/fecec16d Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/fecec16d Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/fecec16d Branch: refs/heads/master Commit: fecec16df9dafad6479ee0c47f41779477ae0afc Parents: beeb4d8 Author: Svetoslav Neykov <[email protected]> Authored: Wed Dec 10 15:50:39 2014 +0200 Committer: Svetoslav Neykov <[email protected]> Committed: Thu Jan 29 16:41:25 2015 +0200 ---------------------------------------------------------------------- .../api/AssemblyTemplateSpecInstantiator.java | 5 ++ .../catalog/internal/BasicBrooklynCatalog.java | 13 ++--- .../camp/lite/TestAppAssemblyInstantiator.java | 6 +++ .../BrooklynAssemblyTemplateInstantiator.java | 23 ++++++--- .../brooklyn/catalog/CatalogYamlEntityTest.java | 52 ++++++++++++++++++++ 5 files changed, 87 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fecec16d/core/src/main/java/brooklyn/camp/brooklyn/api/AssemblyTemplateSpecInstantiator.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/brooklyn/camp/brooklyn/api/AssemblyTemplateSpecInstantiator.java b/core/src/main/java/brooklyn/camp/brooklyn/api/AssemblyTemplateSpecInstantiator.java index 25f3cbd..60b7921 100644 --- a/core/src/main/java/brooklyn/camp/brooklyn/api/AssemblyTemplateSpecInstantiator.java +++ b/core/src/main/java/brooklyn/camp/brooklyn/api/AssemblyTemplateSpecInstantiator.java @@ -21,11 +21,16 @@ package brooklyn.camp.brooklyn.api; import io.brooklyn.camp.CampPlatform; import io.brooklyn.camp.spi.AssemblyTemplate; import io.brooklyn.camp.spi.instantiate.AssemblyTemplateInstantiator; + +import java.util.Set; + import brooklyn.entity.proxying.EntitySpec; import brooklyn.management.classloading.BrooklynClassLoadingContext; public interface AssemblyTemplateSpecInstantiator extends AssemblyTemplateInstantiator { EntitySpec<?> createSpec(AssemblyTemplate template, CampPlatform platform, BrooklynClassLoadingContext loader, boolean autoUnwrapIfAppropriate); + EntitySpec<?> createNestedSpec(AssemblyTemplate template, CampPlatform platform, BrooklynClassLoadingContext itemLoader, Set<String> encounteredCatalogTypes); + } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fecec16d/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 2153ada..6b61b44 100644 --- a/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/brooklyn/catalog/internal/BasicBrooklynCatalog.java @@ -51,6 +51,7 @@ import brooklyn.management.internal.ManagementContextInternal; import brooklyn.policy.Policy; import brooklyn.policy.PolicySpec; import brooklyn.util.collections.MutableMap; +import brooklyn.util.collections.MutableSet; import brooklyn.util.exceptions.Exceptions; import brooklyn.util.guava.Maybe; import brooklyn.util.javalang.AggregateClassLoader; @@ -305,7 +306,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { switch (item.getCatalogItemType()) { case TEMPLATE: case ENTITY: - spec = createEntitySpec(plan, loader); + spec = createEntitySpec(loadedItem.getSymbolicName(), plan, loader); break; case POLICY: spec = createPolicySpec(plan, loader); @@ -335,7 +336,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } @SuppressWarnings("unchecked") - private <T, SpecT> SpecT createEntitySpec(DeploymentPlan plan, BrooklynClassLoadingContext loader) { + private <T, SpecT> SpecT createEntitySpec(String symbolicName, DeploymentPlan plan, BrooklynClassLoadingContext loader) { CampPlatform camp = BrooklynServerConfig.getCampPlatform(mgmt).get(); // TODO should not register new AT each time we instantiate from the same plan; use some kind of cache @@ -350,7 +351,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { try { AssemblyTemplateInstantiator instantiator = at.getInstantiator().newInstance(); if (instantiator instanceof AssemblyTemplateSpecInstantiator) { - return (SpecT) ((AssemblyTemplateSpecInstantiator)instantiator).createSpec(at, camp, loader, true); + return (SpecT) ((AssemblyTemplateSpecInstantiator)instantiator).createNestedSpec(at, camp, loader, MutableSet.of(symbolicName)); } throw new IllegalStateException("Unable to instantiate YAML; incompatible instantiator "+instantiator+" for "+at); } catch (Exception e) { @@ -529,7 +530,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { String versionedId = CatalogUtils.getVersionedId(catalogSymbolicName, catalogVersion); BrooklynClassLoadingContext loader = CatalogUtils.newClassLoadingContext(mgmt, versionedId, libraries); - AbstractBrooklynObjectSpec<?, ?> spec = createSpec(plan, loader); + AbstractBrooklynObjectSpec<?, ?> spec = createSpec(catalogSymbolicName, plan, loader); CatalogItemDtoAbstract<?, ?> dto = createItemBuilder(spec, catalogSymbolicName, catalogVersion) .libraries(libraries) @@ -543,11 +544,11 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { return dto; } - private AbstractBrooklynObjectSpec<?,?> createSpec(DeploymentPlan plan, BrooklynClassLoadingContext loader) { + private AbstractBrooklynObjectSpec<?,?> createSpec(String symbolicName, DeploymentPlan plan, BrooklynClassLoadingContext loader) { if (isPolicyPlan(plan)) { return createPolicySpec(plan, loader); } else { - return createEntitySpec(plan, loader); + return createEntitySpec(symbolicName, plan, loader); } } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fecec16d/core/src/test/java/brooklyn/camp/lite/TestAppAssemblyInstantiator.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/brooklyn/camp/lite/TestAppAssemblyInstantiator.java b/core/src/test/java/brooklyn/camp/lite/TestAppAssemblyInstantiator.java index e63bd1f..23d85f2 100644 --- a/core/src/test/java/brooklyn/camp/lite/TestAppAssemblyInstantiator.java +++ b/core/src/test/java/brooklyn/camp/lite/TestAppAssemblyInstantiator.java @@ -27,6 +27,7 @@ import io.brooklyn.camp.spi.collection.ResolvableLink; import io.brooklyn.camp.spi.instantiate.BasicAssemblyTemplateInstantiator; import java.util.Map; +import java.util.Set; import brooklyn.camp.brooklyn.api.AssemblyTemplateSpecInstantiator; import brooklyn.camp.brooklyn.api.HasBrooklynManagementContext; @@ -82,4 +83,9 @@ public class TestAppAssemblyInstantiator extends BasicAssemblyTemplateInstantiat app.configure(ConfigBag.newInstance().putAll((Map)bc).getAllConfigAsConfigKeyMap()); } + @Override + public EntitySpec<?> createNestedSpec(AssemblyTemplate template, CampPlatform platform, BrooklynClassLoadingContext itemLoader, Set<String> encounteredCatalogTypes) { + return createSpec(template, platform, itemLoader, true); + } + } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fecec16d/usage/camp/src/main/java/io/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/main/java/io/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java b/usage/camp/src/main/java/io/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java index d03502e..17949d8 100644 --- a/usage/camp/src/main/java/io/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java +++ b/usage/camp/src/main/java/io/brooklyn/camp/brooklyn/spi/creation/BrooklynAssemblyTemplateInstantiator.java @@ -171,7 +171,10 @@ public class BrooklynAssemblyTemplateInstantiator implements AssemblyTemplateSpe String brooklynType = entityResolver.getBrooklynType(); CatalogItem<Entity, EntitySpec<?>> item = entityResolver.getCatalogItem(); - boolean firstOccurrence = encounteredCatalogTypes.add(brooklynType); + //Take the symoblicName 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 = (item == null || encounteredCatalogTypes.add(item.getSymbolicName())); boolean recursiveButTryJava = !firstOccurrence; if (log.isTraceEnabled()) log.trace("Building CAMP template services: type="+brooklynType+"; item="+item+"; loader="+entityResolver.loader+"; encounteredCatalogTypes="+encounteredCatalogTypes); @@ -223,7 +226,7 @@ public class BrooklynAssemblyTemplateInstantiator implements AssemblyTemplateSpe return null; } try { - return resolveYamlSpec(mgmt, encounteredCatalogTypes, yaml, itemLoader); + return createNestedSpec(mgmt, encounteredCatalogTypes, yaml, itemLoader); } finally { try { yaml.close(); @@ -242,10 +245,10 @@ public class BrooklynAssemblyTemplateInstantiator implements AssemblyTemplateSpe Reader input = new StringReader(yaml); BrooklynClassLoadingContext itemLoader = CatalogUtils.newClassLoadingContext(mgmt, item); - return resolveYamlSpec(mgmt, encounteredCatalogTypes, input, itemLoader); + return createNestedSpec(mgmt, encounteredCatalogTypes, input, itemLoader); } - private EntitySpec<?> resolveYamlSpec(ManagementContext mgmt, + private EntitySpec<?> createNestedSpec(ManagementContext mgmt, Set<String> encounteredCatalogTypes, Reader input, BrooklynClassLoadingContext itemLoader) { CampPlatform platform = BrooklynServerConfig.getCampPlatform(mgmt).get(); @@ -257,13 +260,21 @@ public class BrooklynAssemblyTemplateInstantiator implements AssemblyTemplateSpe } finally { BrooklynLoaderTracker.unsetLoader(itemLoader); } + return createNestedSpec(at, platform, itemLoader, encounteredCatalogTypes); + } + @Override + public EntitySpec<?> createNestedSpec( + AssemblyTemplate template, + CampPlatform platform, + BrooklynClassLoadingContext itemLoader, + Set<String> encounteredCatalogTypes) { // In case we want to allow multiple top-level entities in a catalog we need to think // about what it would mean to subsequently call buildChildrenEntitySpecs on the list of top-level entities! try { - AssemblyTemplateInstantiator ati = at.getInstantiator().newInstance(); + AssemblyTemplateInstantiator ati = template.getInstantiator().newInstance(); if (ati instanceof BrooklynAssemblyTemplateInstantiator) { - List<EntitySpec<?>> specs = ((BrooklynAssemblyTemplateInstantiator)ati).buildTemplateServicesAsSpecsImpl(itemLoader, at, platform, encounteredCatalogTypes); + List<EntitySpec<?>> specs = ((BrooklynAssemblyTemplateInstantiator)ati).buildTemplateServicesAsSpecsImpl(itemLoader, template, platform, encounteredCatalogTypes); if (specs.size() > 1) { throw new UnsupportedOperationException("Only supporting single service in catalog item currently: got "+specs); } http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/fecec16d/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java ---------------------------------------------------------------------- diff --git a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java index 94c3316..5724351 100644 --- a/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java +++ b/usage/camp/src/test/java/io/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java @@ -34,6 +34,7 @@ import org.testng.annotations.Test; import brooklyn.catalog.BrooklynCatalog; import brooklyn.catalog.CatalogItem; +import brooklyn.catalog.internal.CatalogUtils; import brooklyn.entity.Entity; import brooklyn.entity.basic.BasicEntity; import brooklyn.management.osgi.OsgiStandaloneTest; @@ -368,6 +369,57 @@ public class CatalogYamlEntityTest extends AbstractYamlTest { assertTrue(icon != null); icon.close(); } + + @Test + public void testMissingTypeDoesNotRecurse() { + String symbolicName = "my.catalog.app.id.basic"; + addCatalogItem( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION, + "", + "services:", + "- type: brooklyn.entity.basic.BasicEntity"); + + try { + addCatalogItem( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION + "-update", + "", + "services:", + "- type: " + symbolicName); + fail("Catalog addition expected to fail due to non-existent java type " + symbolicName); + } catch (IllegalStateException e) { + assertEquals(e.getMessage(), "Recursive reference to " + symbolicName + " (and cannot be resolved as a Java type)"); + } + } + + @Test + public void testVersionedTypeDoesNotRecurse() { + String symbolicName = "my.catalog.app.id.basic"; + addCatalogItem( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION, + "", + "services:", + "- type: brooklyn.entity.basic.BasicEntity"); + + String versionedId = CatalogUtils.getVersionedId(symbolicName, TEST_VERSION); + try { + addCatalogItem( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION + "-update", + "", + "services:", + "- type: " + versionedId); + fail("Catalog addition expected to fail due to non-existent java type " + versionedId); + } catch (IllegalStateException e) { + assertEquals(e.getMessage(), "Recursive reference to " + versionedId + " (and cannot be resolved as a Java type)"); + } + } private void registerAndLaunchAndAssertSimpleEntity(String symbolicName, String serviceType) throws Exception { addCatalogOSGiEntity(symbolicName, serviceType);
