record a checksum so that on plan rewrite we don't block reinstallation

ahgittin · ahgittin · commit bb9d1f89e495 · 2017-07-21T23:22:28.000+01:00
with test case that fails without the equivalent plan checks, but works with them
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
@@ -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
@@ -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
@@ -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.<OsgiBundleWithUrl>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<RegisteredType> 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
@@ -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
@@ -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
@@ -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<String> actualInterfaces = ((Map<String, List<String>>) tag).get("traits");
+        Map<String, List<String>> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class));
+        List<String> actualInterfaces = traitsMapTag.get("traits");
         List<Class<?>> 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<String> actualInterfaces = ((Map<String, List<String>>) tag).get("traits");
+        Map<String, List<String>> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class));
+        List<String> actualInterfaces = traitsMapTag.get("traits");
         List<Class<?>> 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<String> actualInterfaces = ((Map<String, List<String>>) tag).get("traits");
+        Map<String, List<String>> traitsMapTag = Iterables.getOnlyElement(Iterables.filter(entityItem.getTags(), Map.class));
+        List<String> actualInterfaces = traitsMapTag.get("traits");
         List<String> expectedInterfaces = ImmutableList.of(Entity.class.getName(), BrooklynObject.class.getName(), Identifiable.class.getName(), Configurable.class.getName());
         assertTrue(actualInterfaces.containsAll(expectedInterfaces), "actual="+actualInterfaces);
 

Original file line number	Diff line number	Diff line change
`@@ -571,7 +571,6 @@ public void testSameCatalogReferences() {`
`571`	`571`	`" spec: ",`
`572`	`572`	`" $brooklyn:entitySpec:",`
`573`	`573`	`" type: referenced-entity");`
`574`		`-`
`575`	`574`	`}`
`576`	`575`
`577`	`576`	`@Test`