diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java index ac11898513..35c074d9a2 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityOsgiTypeRegistryTest.java @@ -20,6 +20,7 @@ import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.core.catalog.internal.BasicBrooklynCatalog; +import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.core.typereg.RegisteredTypePredicates; import org.apache.brooklyn.entity.stock.BasicEntity; import org.apache.brooklyn.test.Asserts; @@ -55,12 +56,44 @@ public void testAddTypes() throws Exception { Asserts.assertEquals(item, Iterables.getOnlyElement(itemsInstalled), "Wrong item; installed: "+itemsInstalled); } - @Test // test broken in super works here - // TODO" comment at https://issues.apache.org/jira/browse/BROOKLYN-343 + @Test // test disabled as "broken" in super but works here public void testSameCatalogReferences() { super.testSameCatalogReferences(); } + + @Test + public void testUpdatingItemAllowedIfEquivalentUnderRewrite() { + String symbolicName = "my.catalog.app.id.duplicate"; + // forward reference supported here (but not in super) + // however the plan is rewritten meaning a second install requires special comparison + // (RegisteredTypes "equivalent plan" methods) + addForwardReferencePlan(symbolicName); + + // delete one but not the other to prevent resolution and thus rewrite until later validation phase, + // thus initial addition will compare unmodified plan from here against modified plan added above; + // replacement will then succeed only if we've correctly recorded equivalence tags + deleteCatalogEntity("forward-referenced-entity"); + addForwardReferencePlan(symbolicName); + } + + protected void addForwardReferencePlan(String symbolicName) { + addCatalogItems( + "brooklyn.catalog:", + " id: " + symbolicName, + " version: " + TEST_VERSION, + " itemType: entity", + " items:", + " - id: " + symbolicName, + " itemType: entity", + " item:", + " type: forward-referenced-entity", + " - id: " + "forward-referenced-entity", + " itemType: entity", + " item:", + " type: " + TestEntity.class.getName()); + } + // also runs many other tests from super, here using the osgi/type-registry appraoch } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java index 9d907d9083..4633817b4d 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/catalog/CatalogYamlEntityTest.java @@ -571,7 +571,6 @@ public void testSameCatalogReferences() { " spec: ", " $brooklyn:entitySpec:", " type: referenced-entity"); - } @Test diff --git a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java index 9eea402faa..affb4e7314 100644 --- a/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java +++ b/core/src/main/java/org/apache/brooklyn/core/catalog/internal/BasicBrooklynCatalog.java @@ -907,12 +907,16 @@ private void collectCatalogItemsFromItemMetadataBlock(String sourceYaml, Managed sourcePlanYaml = planInterpreter.itemYaml; } + BasicTypeImplementationPlan plan = new BasicTypeImplementationPlan(format, sourcePlanYaml); BasicRegisteredType type = (BasicRegisteredType) RegisteredTypes.newInstance( RegisteredTypeKind.UNRESOLVED, - symbolicName, version, new BasicTypeImplementationPlan(format, sourcePlanYaml), + symbolicName, version, plan, superTypes, aliases, tags, containingBundle==null ? null : containingBundle.getVersionedName().toString(), MutableList.copyOf(libraryBundles), displayName, description, catalogIconUrl, catalogDeprecated, catalogDisabled); + RegisteredTypes.notePlanEquivalentToThis(type, plan); + // record original source in case it was changed + RegisteredTypes.notePlanEquivalentToThis(type, new BasicTypeImplementationPlan(format, sourceYaml)); ((BasicBrooklynTypeRegistry) mgmt.getTypeRegistry()).addToLocalUnpersistedTypeRegistry(type, force); @@ -1581,7 +1585,7 @@ public ReferenceWithError resolve(RegisteredType typeToValidate) } if (!Objects.equal(guesser.getPlanYaml(), yaml)) { - RegisteredTypes.changePlan(resultT, + RegisteredTypes.changePlanNotingEquivalent(resultT, new BasicTypeImplementationPlan(null /* CampTypePlanTransformer.FORMAT */, guesser.getPlanYaml())); changedSomething = true; } diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java index bd2b056e1c..60142a7293 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/BasicBrooklynTypeRegistry.java @@ -367,7 +367,7 @@ private boolean assertSameEnoughToAllowReplacing(RegisteredType oldType, Registe } if (Objects.equals(oldType.getContainingBundle(), type.getContainingBundle())) { // if named bundles equal then contents must be the same (due to bundle checksum); bail out early - if (!oldType.getPlan().equals(type.getPlan())) { + if (!samePlan(oldType, type)) { String msg = "Cannot add "+type+" to catalog; different plan in "+oldType+" from same bundle "+ type.getContainingBundle()+" is already present"; log.debug(msg+"\n"+ @@ -383,7 +383,7 @@ private boolean assertSameEnoughToAllowReplacing(RegisteredType oldType, Registe } // different bundles, either anonymous or same item in two named bundles - if (!oldType.getPlan().equals(type.getPlan())) { + if (!samePlan(oldType, type)) { // if plan is different, fail String msg = "Cannot add "+type+" in "+type.getContainingBundle()+" to catalog; different plan in "+oldType+" from bundle "+ oldType.getContainingBundle()+" is already present (throwing)"; @@ -425,6 +425,10 @@ private boolean assertSameEnoughToAllowReplacing(RegisteredType oldType, Registe + "item is already present in different bundle "+oldType.getContainingBundle()); } + private boolean samePlan(RegisteredType oldType, RegisteredType type) { + return RegisteredTypes.arePlansEquivalent(oldType, type); + } + @Beta // API stabilising public void delete(VersionedName type) { RegisteredType registeredTypeRemoved = localRegisteredTypes.remove(type.toString()); diff --git a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypes.java b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypes.java index 6519edfa76..360df0294c 100644 --- a/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypes.java +++ b/core/src/main/java/org/apache/brooklyn/core/typereg/RegisteredTypes.java @@ -53,6 +53,7 @@ import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.guava.Maybe.Absent; +import org.apache.brooklyn.util.stream.Streams; import org.apache.brooklyn.util.text.NaturalOrderComparator; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.text.VersionComparator; @@ -62,6 +63,7 @@ import com.google.common.annotations.Beta; import com.google.common.base.Function; +import com.google.common.base.Objects; import com.google.common.base.Predicate; import com.google.common.base.Predicates; import com.google.common.collect.ComparisonChain; @@ -599,11 +601,57 @@ public static String getIconUrl(BrooklynObject object) { return item.getIconUrl(); } + /** @deprecated since 0.12.0 see {@link #changePlanNotingEquivalent(RegisteredType, TypeImplementationPlan)} */ + @Deprecated public static RegisteredType changePlan(RegisteredType type, TypeImplementationPlan plan) { ((BasicRegisteredType)type).implementationPlan = plan; return type; } + + /** Changes the plan set on the given type, returning it, + * and also recording that comparisons checking {@link #arePlansEquivalent(RegisteredType, RegisteredType)} + * should consider the two plans equivalent. + */ + @Beta // would prefer not to need this, but currently it is needed due to how resolver rewrites plan + // and we then check plan equality when considering a re-install, else we spuriously fail on + // identical re-installs (eg with dns-etc-hosts-generator) + public static RegisteredType changePlanNotingEquivalent(RegisteredType type, TypeImplementationPlan plan) { + RegisteredTypes.notePlanEquivalentToThis(type, type.getPlan()); + RegisteredTypes.notePlanEquivalentToThis(type, plan); + ((BasicRegisteredType)type).implementationPlan = plan; + return type; + } + + private static String tagForEquivalentPlan(String input) { + // plans may be trimmed by yaml parser so do that before checking equivalence + // it does mean a format change will be ignored + return "equivalent-plan("+Streams.getMd5Checksum(Streams.newInputStreamWithContents(input.trim()))+")"; + } + + @Beta + public static void notePlanEquivalentToThis(RegisteredType type, TypeImplementationPlan plan) { + Object data = plan.getPlanData(); + if (data==null) throw new IllegalStateException("No plan data for "+plan+" noted equivalent to "+type); + if (!(data instanceof String)) throw new IllegalStateException("Expected plan for equivalent to "+type+" to be a string; was "+data); + ((BasicRegisteredType)type).tags.add(tagForEquivalentPlan((String)data)); + } + @Beta + public static boolean arePlansEquivalent(RegisteredType type1, RegisteredType type2) { + String plan1 = getImplementationDataStringForSpec(type1); + String plan2 = getImplementationDataStringForSpec(type2); + if (Strings.isNonBlank(plan1) && Strings.isNonBlank(plan2)) { + String p2tag = tagForEquivalentPlan(plan2); + String p1tag = tagForEquivalentPlan(plan1); + // allow same plan under trimming, + // or any recorded tag in either direction + if (Objects.equal(p1tag, p2tag)) return true; + if (type1.getTags().contains(p2tag)) return true; + if (type2.getTags().contains(p1tag)) return true; + } + return Objects.equal(type1.getPlan(), type2.getPlan()); + } + public static boolean isTemplate(RegisteredType type) { if (type==null) return false; return type.getTags().contains(BrooklynTags.CATALOG_TEMPLATE); diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java index 0a91d5994e..abdd1ed500 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/resources/CatalogResourceTest.java @@ -175,10 +175,8 @@ public void testRegisterCustomEntityTopLevelSyntaxWithBundleWhereEntityIsFromCor assertEquals(item.getIconUrl(), "classpath:/org/apache/brooklyn/test/osgi/entities/icon.gif"); // an InterfacesTag should be created for every catalog item - assertEquals(entityItem.getTags().size(), 1); - Object tag = entityItem.getTags().iterator().next(); - @SuppressWarnings("unchecked") - List actualInterfaces = ((Map>) tag).get("traits"); + Map> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class)); + List actualInterfaces = traitsMapTag.get("traits"); List> expectedInterfaces = Reflections.getAllInterfaces(TestEntity.class); assertEquals(actualInterfaces.size(), expectedInterfaces.size()); for (Class expectedInterface : expectedInterfaces) { @@ -813,10 +811,9 @@ public void testOsgiBundleWithBom() throws Exception { assertEquals(item.getIconUrl(), "classpath:/org/apache/brooklyn/test/osgi/entities/icon.gif"); // an InterfacesTag should be created for every catalog item - assertEquals(entityItem.getTags().size(), 1); - Object tag = entityItem.getTags().iterator().next(); @SuppressWarnings("unchecked") - List actualInterfaces = ((Map>) tag).get("traits"); + Map> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class)); + List actualInterfaces = traitsMapTag.get("traits"); List> expectedInterfaces = Reflections.getAllInterfaces(TestEntity.class); assertEquals(actualInterfaces.size(), expectedInterfaces.size()); for (Class expectedInterface : expectedInterfaces) { @@ -889,10 +886,8 @@ public void testOsgiBundleWithBomNotInBrooklynNamespace() throws Exception { assertEquals(item.getIconUrl(), "classpath:" + iconPath); // an InterfacesTag should be created for every catalog item - assertEquals(entityItem.getTags().size(), 1); - Object tag = entityItem.getTags().iterator().next(); - @SuppressWarnings("unchecked") - List actualInterfaces = ((Map>) tag).get("traits"); + Map> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class)); + List actualInterfaces = traitsMapTag.get("traits"); List expectedInterfaces = ImmutableList.of(Entity.class.getName(), BrooklynObject.class.getName(), Identifiable.class.getName(), Configurable.class.getName()); assertTrue(actualInterfaces.containsAll(expectedInterfaces), "actual="+actualInterfaces);