Updates for the following review comments. Working on review comments for catalog-update PR
https://github.com/apache/brooklyn-server/pull/338#discussion_r94612120 Unused import in AbstractBrooklynObjectSpec.java - done in merge https://github.com/apache/brooklyn-server/pull/338#discussion_r94752687 Keep for persistence backwards compatibility - surely I take this into account already? See testRebindWithCatalogAndAppRebindCatalogItemIds, and BrooklynMementoPersisterToObjectStore.getCatalogItemIds(). https://github.com/apache/brooklyn-server/pull/338#discussion_r94762969 Not called on xstream deserialize, could be null - take into account in following code. - Introduced getCatalogItemIdStack(). What about the other pre-existing fields that also have initializers? https://github.com/apache/brooklyn-server/pull/338#discussion_r94612579 public SpecT catalogItemId(String val) { Should we deprecate this one? When is this one used vs nestCatalogItemId? I'd think (before looking at the code in details) that this shouldn't be called at all, we should only be "nesting" catalog items. - deprecated https://github.com/apache/brooklyn-server/pull/338/files#r94615424 Don't see this used, do you think we should keep it? - replaced it with protected SpecT catalogItemIdStack(Collection<String> catalogItemIdStack), used in AbstractBrooklynObjectSpec#copyFrom(). https://github.com/apache/brooklyn-server/pull/338/files#r94609346 missing word - fixed https://github.com/apache/brooklyn-server/pull/338/files#r94613348 It's not a mere reference but R3 extending R2, etc. - fixed https://github.com/apache/brooklyn-server/pull/338#discussion_r94618943 public SpecT catalogItemIdIfNotNull(String val) { I'd deprecate it, even if marked as @Beta. - done https://github.com/apache/brooklyn-server/pull/338/files#r94616318 I don't associate "nest" with anything in this context, perhaps stackCatalogItemId would be more appropriate? - changed to "stackCatalogItemId". https://github.com/apache/brooklyn-server/pull/338#discussion_r94616812 public final String getCatalogItemId() { Suggest deprecating this one and creating getOuterCatalogItemId, getInnerCatalogItemId. - created getOuterCatalogItemId; getInner not needed as there is getCatalogItemHierarchy https://github.com/apache/brooklyn-server/pull/338#discussion_r94753362 As a fallback return catalogItemId. - Seems preferable not to retain catalogItemId if we can get away without it, see comment on https://github.com/apache/brooklyn-server/pull/338#discussion_r94752687 above. https://github.com/apache/brooklyn-server/pull/338#discussion_r94617757 Can you add as a clarification - Added. https://github.com/apache/brooklyn-server/pull/338#discussion_r94618204 "Super" (here and same name in other classes) could be misinterpreted - changed name to getCatalogItemHierarchy Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/78cfeda4 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/78cfeda4 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/78cfeda4 Branch: refs/heads/master Commit: 78cfeda484002f7a483c81ccd7cd029210688afd Parents: c609497 Author: Geoff Macartney <[email protected]> Authored: Mon Feb 20 16:58:08 2017 +0000 Committer: Geoff Macartney <[email protected]> Committed: Thu Apr 20 11:20:36 2017 +0100 ---------------------------------------------------------------------- .../internal/AbstractBrooklynObjectSpec.java | 222 ++++++++++++------- .../brooklyn/spi/creation/CampResolver.java | 2 +- .../catalog/CatalogOsgiYamlEntityTest.java | 2 +- .../catalog/CatalogOsgiYamlTemplateTest.java | 2 +- .../catalog/CatalogYamlLocationTest.java | 4 +- .../catalog/CatalogYamlTemplateTest.java | 4 +- .../internal/CatalogItemDtoAbstract.java | 2 - .../core/mgmt/EntityManagementUtils.java | 2 +- .../core/mgmt/persist/XmlMementoSerializer.java | 2 +- .../core/objs/proxy/InternalEntityFactory.java | 8 +- .../objs/proxy/InternalLocationFactory.java | 4 +- .../core/objs/proxy/InternalPolicyFactory.java | 8 +- .../typereg/AbstractTypePlanTransformer.java | 2 +- .../rest/transform/LocationTransformer.java | 4 +- 14 files changed, 160 insertions(+), 108 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java ---------------------------------------------------------------------- diff --git a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java index c659782..16b4175 100644 --- a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java @@ -23,11 +23,11 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.io.Serializable; import java.lang.reflect.Modifier; import java.util.ArrayDeque; +import java.util.Collection; import java.util.Collections; import java.util.Deque; import java.util.List; import java.util.Map; -import java.util.Queue; import java.util.Set; import org.apache.brooklyn.api.mgmt.EntityManager; @@ -54,21 +54,21 @@ import com.google.common.collect.Maps; * <p> * In addition to the contract defined by the code, * subclasses should provide a public static <code>create(Class)</code> - * method to create an instance of the spec for the target type indicated by the argument. + * method to create an instance of the spec for the target type indicated by the argument. * <p> * The spec is then passed to type-specific methods, * e.g. {@link EntityManager#createEntity(org.apache.brooklyn.api.entity.EntitySpec)} * to create a managed instance of the target type. */ -public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrooklynObjectSpec<T,SpecT>> implements Serializable { +public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrooklynObjectSpec<T, SpecT>> implements Serializable { private static final long serialVersionUID = 3010955277740333030L; private static final Logger log = LoggerFactory.getLogger(AbstractBrooklynObjectSpec.class); - + private final Class<? extends T> type; private String displayName; - private Deque<String> catalogItemIdStack = new ArrayDeque<>(); + private Deque<String> catalogItemIdStack; private Set<Object> tags = MutableSet.of(); private List<SpecParameter<?>> parameters = ImmutableList.of(); @@ -79,7 +79,7 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly checkValidType(type); this.type = type; } - + @SuppressWarnings("unchecked") protected SpecT self() { return (SpecT) this; @@ -88,68 +88,95 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly @Override public String toString() { return MoreObjects.toStringHelper(this).omitNullValues() - .add("type", type) - .add("displayName", displayName) - .toString(); + .add("type", type) + .add("displayName", displayName) + .toString(); } protected abstract void checkValidType(Class<? extends T> type); - + public SpecT displayName(String val) { displayName = val; return self(); } - - /** Set the catalog item ID that defined this object, also used for searching for type and resources referenced */ - // since https://issues.apache.org/jira/browse/BROOKLYN-445 this must no longer be used to indicate - // a caller-context catalog item that should be used for search purposes; - // if that behaviour is desired, the child should be refactored to be its own item in the catalog BOM - // (or TODO we add a separate field to record other catalog item IDs that could be applied for searching, see below) + + /** + * Set the catalog item ID that defined this object, also used for searching for type and resources referenced + * since https://issues.apache.org/jira/browse/BROOKLYN-445 this must no longer be used to indicate + * a caller-context catalog item that should be used for search purposes; + * if that behaviour is desired, the child should be refactored to be its own item in the catalog BOM + * (or TODO we add a separate field to record other catalog item IDs that could be applied for searching, see below) + */ public SpecT catalogItemId(String val) { - catalogItemIdStack.clear(); - return nestCatalogItemId(val); + getCatalogItemIdStack().clear(); + return stackCatalogItemId(val); } - public SpecT catalogItemIds(List<String> ids) { - catalogItemIdStack.clear(); - catalogItemIdStack.addAll(ids); + protected SpecT catalogItemIdStack(Collection<String> catalogItemIdStack) { + this.catalogItemIdStack = null; + getCatalogItemIdStack().addAll(catalogItemIdStack); return self(); } /** + * @deprecated since 0.11.0, use {@link #stackCatalogItemId(String)} instead + */ + @Deprecated + @Beta + public SpecT catalogItemIdIfNotNull(String val) { + if (val!=null) { + stackCatalogItemId(val); + } + return self(); + } + + private Deque<String> getCatalogItemIdStack() { + if (catalogItemIdStack == null) { + catalogItemIdStack = new ArrayDeque<>(); + } + return catalogItemIdStack; + } + + /** * Adds (stacks) the catalog item id of a wrapping specification. * Does nothing if the value is null. - * - * Used when we to collect nested item ID's so that *all* can be searched. - * e.g. if R3 references R2 which references R1 any one of these might supply config keys + * <p> + * Used when we want to collect nested item ID's so that *all* can be searched. + * e.g. if R3 extends R2 which extends R1 any one of these might supply config keys * referencing resources or types in their local bundles. */ @Beta - public SpecT nestCatalogItemId(String val) { - if (null != val && (catalogItemIdStack.isEmpty() || !catalogItemIdStack.element().equals(val))) { - catalogItemIdStack.addFirst(val); + public SpecT stackCatalogItemId(String val) { + if (null != val && (getCatalogItemIdStack().isEmpty() || !getCatalogItemIdStack().element().equals(val))) { + getCatalogItemIdStack().addFirst(val); } return self(); } - - public SpecT tag(Object tag) { tags.add(tag); return self(); } - /** adds the given tags */ + /** + * adds the given tags + */ public SpecT tags(Iterable<Object> tagsToAdd) { return tagsAdd(tagsToAdd); } - /** adds the given tags */ + + /** + * adds the given tags + */ public SpecT tagsAdd(Iterable<Object> tagsToAdd) { Iterables.addAll(this.tags, tagsToAdd); return self(); } - /** replaces tags with the given */ + + /** + * replaces tags with the given + */ public SpecT tagsReplace(Iterable<Object> tagsToReplace) { this.tags.clear(); Iterables.addAll(this.tags, tagsToReplace); @@ -166,13 +193,16 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly // it is a CatalogConfig or merely a config key, maybe introducing displayable, or even priority // (but note part of the reason for CatalogConfig.priority is that java reflection doesn't preserve field order) . // see also comments on the camp SpecParameterResolver. - + // probably the thing to do is deprecate the ambiguous method in favour of an explicit @Beta public SpecT parameters(Iterable<? extends SpecParameter<?>> parameters) { return parametersReplace(parameters); } - /** adds the given parameters, new ones first so they dominate subsequent ones */ + + /** + * adds the given parameters, new ones first so they dominate subsequent ones + */ @Beta public SpecT parametersAdd(Iterable<? extends SpecParameter<?>> parameters) { // parameters follows immutable pattern, unlike the other fields @@ -181,11 +211,14 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly current.removeAll(params); return parametersReplace(ImmutableList.<SpecParameter<?>>builder() - .addAll(params) - .addAll(current) - .build()); + .addAll(params) + .addAll(current) + .build()); } - /** replaces parameters with the given */ + + /** + * replaces parameters with the given + */ @Beta public SpecT parametersReplace(Iterable<? extends SpecParameter<?>> parameters) { this.parameters = ImmutableList.copyOf(checkNotNull(parameters, "parameters")); @@ -193,37 +226,47 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly } /** - * @return The type (often an interface) this spec represents and which will be instantiated from it + * @return The type (often an interface) this spec represents and which will be instantiated from it */ public Class<? extends T> getType() { return type; } - + /** * @return The display name of the object */ public final String getDisplayName() { return displayName; } - + + /** + * @deprecated since 0.11.0, use getOuterCatalogItemId or getInnerCatalogItemIds as appropriate + */ + @Deprecated public final String getCatalogItemId() { - if (catalogItemIdStack.size() != 0) { - return catalogItemIdStack.getFirst(); + return getOuterCatalogItemId(); + } + + public final String getOuterCatalogItemId() { + if (getCatalogItemIdStack().size() != 0) { + return getCatalogItemIdStack().getFirst(); } return null; } - /** - * An immutable list of ids of this object's catalog item and its defining catalog items. - * e.g. if the catalog item is defined as - * <pre> - * items: - * - id: X - * item: Y - * </pre> - * then the list will contain X, Y. - */ - public final List<String> getCatalogItemSuperIds() { + + /** + * An immutable list of ids of this object's catalog item and its defining catalog items. + * Wrapping items are first in the list (i.e. wrapping items precede wrapped items), + * for example, if the catalog item is defined as + * <pre> + * items: + * - id: X + * item: Y + * </pre> + * then the list will contain X, Y. + */ + public final List<String> getCatalogItemIdHierarchy() { return ImmutableList.copyOf(catalogItemIdStack); } @@ -231,7 +274,9 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly return ImmutableSet.copyOf(tags); } - /** A list of configuration options that the entity supports. */ + /** + * A list of configuration options that the entity supports. + */ public final List<SpecParameter<?>> getParameters() { //Could be null after rebind if (parameters != null) { @@ -246,38 +291,43 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly try { implClazz.getConstructor(new Class[0]); } catch (NoSuchMethodException e) { - throw new IllegalStateException("Implementation "+implClazz+" must have a no-argument constructor"); + throw new IllegalStateException("Implementation " + implClazz + " must have a no-argument constructor"); } catch (SecurityException e) { throw Exceptions.propagate(e); } - - if (implClazz.isInterface()) throw new IllegalStateException("Implementation "+implClazz+" is an interface, but must be a non-abstract class"); - if (Modifier.isAbstract(implClazz.getModifiers())) throw new IllegalStateException("Implementation "+implClazz+" is abstract, but must be a non-abstract class"); + + if (implClazz.isInterface()) + throw new IllegalStateException("Implementation " + implClazz + " is an interface, but must be a non-abstract class"); + if (Modifier.isAbstract(implClazz.getModifiers())) + throw new IllegalStateException("Implementation " + implClazz + " is abstract, but must be a non-abstract class"); } - + // TODO Duplicates method in BasicEntityTypeRegistry protected final void checkIsImplementation(Class<?> val, Class<? super T> requiredInterface) { - if (!requiredInterface.isAssignableFrom(val)) throw new IllegalStateException("Implementation "+val+" does not implement "+requiredInterface.getName()); - if (val.isInterface()) throw new IllegalStateException("Implementation "+val+" is an interface, but must be a non-abstract class"); - if (Modifier.isAbstract(val.getModifiers())) throw new IllegalStateException("Implementation "+val+" is abstract, but must be a non-abstract class"); + if (!requiredInterface.isAssignableFrom(val)) + throw new IllegalStateException("Implementation " + val + " does not implement " + requiredInterface.getName()); + if (val.isInterface()) + throw new IllegalStateException("Implementation " + val + " is an interface, but must be a non-abstract class"); + if (Modifier.isAbstract(val.getModifiers())) + throw new IllegalStateException("Implementation " + val + " is abstract, but must be a non-abstract class"); } - + protected SpecT copyFrom(SpecT otherSpec) { return displayName(otherSpec.getDisplayName()) .configure(otherSpec.getConfig()) .configure(otherSpec.getFlags()) .tags(otherSpec.getTags()) - .catalogItemId(otherSpec.getCatalogItemId()) + .catalogItemIdStack(otherSpec.getCatalogItemIdHierarchy()) .parameters(otherSpec.getParameters()); } @Override public boolean equals(Object obj) { - if (obj==null) return false; + if (obj == null) return false; if (!obj.getClass().equals(getClass())) return false; - AbstractBrooklynObjectSpec<?,?> other = (AbstractBrooklynObjectSpec<?,?>)obj; + AbstractBrooklynObjectSpec<?, ?> other = (AbstractBrooklynObjectSpec<?, ?>) obj; if (!Objects.equal(getDisplayName(), other.getDisplayName())) return false; - if (!Objects.equal(getCatalogItemId(), other.getCatalogItemId())) return false; + if (!Objects.equal(getCatalogItemIdHierarchy(), other.getCatalogItemIdHierarchy())) return false; if (!Objects.equal(getType(), other.getType())) return false; if (!Objects.equal(getTags(), other.getTags())) return false; if (!Objects.equal(getConfig(), other.getConfig())) return false; @@ -285,30 +335,32 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly if (!Objects.equal(getParameters(), other.getParameters())) return false; return true; } - + @Override public int hashCode() { - return Objects.hashCode(getCatalogItemId(), getDisplayName(), getType(), getTags()); + return Objects.hashCode(getCatalogItemIdHierarchy(), getDisplayName(), getType(), getTags()); } - /** strings inserted as flags, config keys inserted as config keys; - * if you want to force one or the other, create a ConfigBag and convert to the appropriate map type */ - public SpecT configure(Map<?,?> val) { - if (val==null) { - log.warn("Null supplied when configuring "+this); - log.debug("Source for null supplied when configuring "+this, new Throwable("Source for null supplied when configuring "+this)); + /** + * strings inserted as flags, config keys inserted as config keys; + * if you want to force one or the other, create a ConfigBag and convert to the appropriate map type + */ + public SpecT configure(Map<?, ?> val) { + if (val == null) { + log.warn("Null supplied when configuring " + this); + log.debug("Source for null supplied when configuring " + this, new Throwable("Source for null supplied when configuring " + this)); return self(); } - for (Map.Entry<?, ?> entry: val.entrySet()) { - if (entry.getKey()==null) throw new NullPointerException("Null key not permitted"); + for (Map.Entry<?, ?> entry : val.entrySet()) { + if (entry.getKey() == null) throw new NullPointerException("Null key not permitted"); if (entry.getKey() instanceof CharSequence) flags.put(entry.getKey().toString(), entry.getValue()); else if (entry.getKey() instanceof ConfigKey<?>) - config.put((ConfigKey<?>)entry.getKey(), entry.getValue()); + config.put((ConfigKey<?>) entry.getKey(), entry.getValue()); else if (entry.getKey() instanceof HasConfigKey<?>) - config.put(((HasConfigKey<?>)entry.getKey()).getConfigKey(), entry.getValue()); + config.put(((HasConfigKey<?>) entry.getKey()).getConfigKey(), entry.getValue()); else { - log.warn("Spec "+this+" ignoring unknown config key "+entry.getKey()); + log.warn("Spec " + this + " ignoring unknown config key " + entry.getKey()); } } return self(); @@ -318,7 +370,7 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly flags.put(checkNotNull(key, "key").toString(), val); return self(); } - + public <V> SpecT configure(ConfigKey<V> key, V val) { config.put(checkNotNull(key, "key"), val); return self(); @@ -353,11 +405,13 @@ public abstract class AbstractBrooklynObjectSpec<T,SpecT extends AbstractBrookly return self(); } - /** Clears the config map, removing any config previously set. */ + /** + * Clears the config map, removing any config previously set. + */ public void clearConfig() { config.clear(); } - + /** * @return Read-only construction flags * @see SetFromFlag declarations on the policy type http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java index 39f77e0..9ec8fd8 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/creation/CampResolver.java @@ -115,7 +115,7 @@ class CampResolver { throw new IllegalStateException("Creating spec from "+item+", got "+spec.getType()+" which is incompatible with expected "+expectedType); } - spec.nestCatalogItemId(item.getId()); + spec.stackCatalogItemId(item.getId()); if (!spec.getFlags().containsKey("iconUrl") && item.getIconUrl()!=null) { spec.configure("iconUrl", item.getIconUrl()); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java index 102f476..ffc81b5 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlEntityTest.java @@ -708,7 +708,7 @@ public class CatalogOsgiYamlEntityTest extends AbstractYamlTest { BrooklynTypeRegistry registry = mgmt().getTypeRegistry(); RegisteredType item = registry.get(symbolicName, TEST_VERSION); AbstractBrooklynObjectSpec<?, ?> spec = registry.createSpec(item, null, null); - assertEquals(spec.getCatalogItemId(), ver(symbolicName)); + assertEquals(spec.getOuterCatalogItemId(), ver(symbolicName)); deleteCatalogEntity(symbolicName); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlTemplateTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlTemplateTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlTemplateTest.java index 8b2a561..4ecfcf2 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlTemplateTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogOsgiYamlTemplateTest.java @@ -87,7 +87,7 @@ public class CatalogOsgiYamlTemplateTest extends AbstractYamlTest { EntitySpec<?> child = Iterables.getOnlyElement( spec.getChildren() ); Assert.assertEquals(child.getType().getName(), SIMPLE_ENTITY_TYPE); - Assert.assertEquals(child.getCatalogItemId(), "t1:"+TEST_VERSION); + Assert.assertEquals(child.getOuterCatalogItemId(), "t1:"+TEST_VERSION); } private RegisteredType makeItem(String symbolicName, String templateType) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java index 01fb484..dc017d3 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlLocationTest.java @@ -176,10 +176,10 @@ public class CatalogYamlLocationTest extends AbstractYamlTest { LocationSpec<? extends Location> spec1 = mgmt().getLocationRegistry().getLocationSpec(def1).get(); LocationSpec<? extends Location> spec2 = mgmt().getLocationRegistry().getLocationSpec(def2).get(); - assertEquals(spec1.getCatalogItemId(), "loc1:0.1.2"); + assertEquals(spec1.getOuterCatalogItemId(), "loc1:0.1.2"); assertEquals(spec1.getDisplayName(), "My Loc 1"); assertContainsAll(spec1.getFlags(), ImmutableMap.of("mykey1", "myval1", "mykey1b", "myval1b")); - assertEquals(spec2.getCatalogItemId(), "loc2:0.1.2"); + assertEquals(spec2.getOuterCatalogItemId(), "loc2:0.1.2"); assertEquals(spec2.getDisplayName(), "My Loc 2"); assertContainsAll(spec2.getFlags(), ImmutableMap.of("mykey1", "myvalOverridden", "mykey1b", "myval1b", "mykey2", "myval2")); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlTemplateTest.java ---------------------------------------------------------------------- diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlTemplateTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlTemplateTest.java index cb585e0..f128abc 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlTemplateTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlTemplateTest.java @@ -208,7 +208,7 @@ public class CatalogYamlTemplateTest extends AbstractYamlTest { EntitySpec<?> child = Iterables.getOnlyElement( spec.getChildren() ); Assert.assertEquals(child.getType().getName(), TestEntity.class.getName()); - Assert.assertEquals(child.getCatalogItemId(), "t1:"+TEST_VERSION); + Assert.assertEquals(child.getOuterCatalogItemId(), "t1:"+TEST_VERSION); } @Test @@ -247,7 +247,7 @@ public class CatalogYamlTemplateTest extends AbstractYamlTest { Assert.assertEquals(spec.getChildren().size(), 0); Assert.assertEquals(spec.getType(), BasicApplication.class); Assert.assertEquals(ConfigBag.newInstance(spec.getConfig()).getStringKey("foo"), "boo"); - Assert.assertEquals(spec.getCatalogItemId(), "app1r:1"); + Assert.assertEquals(spec.getOuterCatalogItemId(), "app1r:1"); } private RegisteredType addCatalogItem(String symbolicName, String templateType) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java index 82a210f..dac5af8 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/CatalogItemDtoAbstract.java @@ -46,8 +46,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; -// TODO add support for nested catalog items, implement nestCatalogItemId in terms of symbolicName/Version -// TODO also getCatalogItemSuperIds. public abstract class CatalogItemDtoAbstract<T, SpecT> extends AbstractBrooklynObject implements CatalogItem<T, SpecT> { private static Logger LOG = LoggerFactory.getLogger(CatalogItemDtoAbstract.class); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/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 70ce4fb..d222151 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 @@ -259,7 +259,7 @@ public class EntityManagementUtils { wrappedChild.parametersAdd(wrapperParent.getParameters()); } - wrappedChild.nestCatalogItemId(wrapperParent.getCatalogItemId()); + wrappedChild.stackCatalogItemId(wrapperParent.getOuterCatalogItemId()); // NB: this clobber's child config wherever they conflict; might prefer to deeply merge maps etc // (or maybe even prevent the merge in these cases; http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java index 16beb4e..12b2c3a 100644 --- a/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java +++ b/core/src/main/java/org/apache/brooklyn/core/mgmt/persist/XmlMementoSerializer.java @@ -455,7 +455,7 @@ public class XmlMementoSerializer<T> extends XmlSerializer<T> implements Memento public void marshal(Object source, HierarchicalStreamWriter writer, MarshallingContext context) { if (source == null) return; AbstractBrooklynObjectSpec<?, ?> spec = (AbstractBrooklynObjectSpec<?, ?>) source; - String catalogItemId = spec.getCatalogItemId(); + String catalogItemId = spec.getOuterCatalogItemId(); if (Strings.isNonBlank(catalogItemId)) { // write this field first, so we can peek at it when we read writer.startNode("catalogItemId"); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java index 77dfcf4..cc1dd8c 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalEntityFactory.java @@ -246,8 +246,8 @@ public class InternalEntityFactory extends InternalFactory { if (spec.getDisplayName()!=null) ((AbstractEntity)entity).setDisplayName(spec.getDisplayName()); - if (spec.getCatalogItemId()!=null) { - ((AbstractEntity)entity).setCatalogItemIds(spec.getCatalogItemSuperIds()); + if (spec.getOuterCatalogItemId()!=null) { + ((AbstractEntity)entity).setCatalogItemIds(spec.getCatalogItemIdHierarchy()); } entity.tags().addTags(spec.getTags()); @@ -274,13 +274,13 @@ public class InternalEntityFactory extends InternalFactory { private void addSpecParameters(EntitySpec<?> spec, EntityDynamicType edType) { // if coming from a catalog item, parsed by CAMP, then the spec list of parameters is canonical, // the parent item has had its config keys set as parameters here with those non-inheritable - // via type definition removed, so wipe those on the EDT to make sure non-inheritable ones are removed; + // via type definition removed, so wipe those on the EDT to make sure non-inheritable ones are removed; // OTOH if item is blank, it was set as a java type, not inheriting it, // and the config keys on the dynamic type are the correct ones to use, and usually there is nothing in spec.parameters, // except what is being added programmatically. // (this logic could get confused if catalog item ID referred to some runtime-inherited context, // but those semantics should no longer be used -- https://issues.apache.org/jira/browse/BROOKLYN-445) - if (Strings.isNonBlank(spec.getCatalogItemId())) { + if (Strings.isNonBlank(spec.getOuterCatalogItemId())) { edType.clearConfigKeys(); } for (SpecParameter<?> param : spec.getParameters()) { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java index 57098b3..5a70257 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalLocationFactory.java @@ -117,8 +117,8 @@ public class InternalLocationFactory extends InternalFactory { if (spec.getDisplayName()!=null) ((AbstractLocation)loc).setDisplayName(spec.getDisplayName()); - if (spec.getCatalogItemId()!=null) { - ((AbstractLocation)loc).setCatalogItemIds(spec.getCatalogItemSuperIds()); + if (spec.getOuterCatalogItemId()!=null) { + ((AbstractLocation)loc).setCatalogItemIds(spec.getCatalogItemIdHierarchy()); } loc.tags().addTags(spec.getTags()); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java index fd444a0..11c27ac 100644 --- a/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java +++ b/core/src/main/java/org/apache/brooklyn/core/objs/proxy/InternalPolicyFactory.java @@ -106,8 +106,8 @@ public class InternalPolicyFactory extends InternalFactory { if (spec.getDisplayName()!=null) { ((AbstractPolicy)pol).setDisplayName(spec.getDisplayName()); } - if (spec.getCatalogItemId()!=null) { - ((AbstractPolicy)pol).setCatalogItemIds(spec.getCatalogItemSuperIds()); + if (spec.getOuterCatalogItemId()!=null) { + ((AbstractPolicy)pol).setCatalogItemIds(spec.getCatalogItemIdHierarchy()); } pol.tags().addTags(spec.getTags()); @@ -147,8 +147,8 @@ public class InternalPolicyFactory extends InternalFactory { if (spec.getDisplayName()!=null) ((AbstractEnricher)enricher).setDisplayName(spec.getDisplayName()); - if (spec.getCatalogItemId()!=null) { - ((AbstractEnricher)enricher).setCatalogItemIds(spec.getCatalogItemSuperIds()); + if (spec.getOuterCatalogItemId()!=null) { + ((AbstractEnricher)enricher).setCatalogItemIds(spec.getCatalogItemIdHierarchy()); } enricher.tags().addTags(spec.getTags()); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java b/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java index 1227b42..46b41d6 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/AbstractTypePlanTransformer.java @@ -103,7 +103,7 @@ public abstract class AbstractTypePlanTransformer implements BrooklynTypePlanTra @Override protected Object visitSpec() { try { AbstractBrooklynObjectSpec<?, ?> result = createSpec(type, context); - result.nestCatalogItemId(type.getId()); + result.stackCatalogItemId(type.getId()); return result; } catch (Exception e) { throw Exceptions.propagate(e); } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/78cfeda4/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java ---------------------------------------------------------------------- diff --git a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java index c612c6f..2ad3608 100644 --- a/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java +++ b/rest/rest-resources/src/main/java/org/apache/brooklyn/rest/transform/LocationTransformer.java @@ -83,7 +83,7 @@ public class LocationTransformer { } } - String id = Strings.isNonBlank(optionalExplicitId) ? optionalExplicitId : spec!=null && Strings.isNonBlank(spec.getCatalogItemId()) ? spec.getCatalogItemId() : null; + String id = Strings.isNonBlank(optionalExplicitId) ? optionalExplicitId : spec!=null && Strings.isNonBlank(spec.getOuterCatalogItemId()) ? spec.getOuterCatalogItemId() : null; URI selfUri = serviceUriBuilder(ub, LocationApi.class, "get").build(id); CatalogLocationSummary catalogSummary = null; @@ -100,7 +100,7 @@ public class LocationTransformer { return new LocationSummary( id, Strings.isNonBlank(name) ? name : spec!=null ? spec.getDisplayName() : null, - Strings.isNonBlank(specString) ? specString : spec!=null ? spec.getCatalogItemId() : null, + Strings.isNonBlank(specString) ? specString : spec!=null ? spec.getOuterCatalogItemId() : null, null, copyConfig(config, level), catalogSummary,
