From 5bc7546b9e2f5af743fad41533322c7804dd8e1b Mon Sep 17 00:00:00 2001 From: Aled Sage Date: Fri, 7 Sep 2018 19:50:39 +0100 Subject: [PATCH] Fix DSL recursive-reference detection --- .../spi/dsl/BrooklynDslDeferredSupplier.java | 3 +- .../spi/dsl/methods/DslComponent.java | 12 ++-- .../camp/brooklyn/spi/dsl/DslYamlTest.java | 61 +++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java index f7cf4cc057..484aacbd76 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java @@ -131,8 +131,7 @@ public final T get() { @Override public abstract Task newTask(); - protected void checkAndTagForRecursiveReference(Entity targetEntity) { - String tag = toString(); + protected void checkAndTagForRecursiveReference(Entity targetEntity, String tag) { Task ancestor = Tasks.current(); if (ancestor!=null) { ancestor = ancestor.getSubmittedByTask(); diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java index f542ff2168..1040fa954e 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/methods/DslComponent.java @@ -585,7 +585,8 @@ public Object call() throws Exception { } // this is always run in a new dedicated task (possibly a fake task if immediate), so no need to clear - checkAndTagForRecursiveReference(targetEntity); + String tag = "DSL:entity('"+targetEntity.getId()+"').config('"+keyName+"')"; + checkAndTagForRecursiveReference(targetEntity, tag); String keyNameS = resolveKeyName(true); ConfigKey key = targetEntity.getEntityType().getConfigKey(keyNameS); @@ -777,11 +778,12 @@ public Task newTask() { public Object call() throws Exception { Entity targetEntity = component.get(); - // this is always run in a new dedicated task (possibly a fake task if immediate), so no need to clear - checkAndTagForRecursiveReference(targetEntity); - int indexI = resolveIndex(immediate); + // this is always run in a new dedicated task (possibly a fake task if immediate), so no need to clear + String tag = "DSL:entity('"+targetEntity.getId()+"').location('"+indexI+"')"; + checkAndTagForRecursiveReference(targetEntity, tag); + // TODO Try repeatedly if no location(s)? Collection locations = getLocations(targetEntity); if (locations.size() < (indexI + 1)) { @@ -811,7 +813,7 @@ private int resolveIndex(boolean immediately) { .as(Integer.class) .context(findExecutionContext(this)) .immediately(immediately) - .description("Resolving indx from " + index) + .description("Resolving index from " + index) .get(); return result; } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java index 71201502aa..57fee747cc 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslYamlTest.java @@ -38,8 +38,11 @@ import org.apache.brooklyn.core.entity.EntityInternal; import org.apache.brooklyn.core.sensor.Sensors; import org.apache.brooklyn.core.test.entity.TestApplication; +import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.entity.group.DynamicCluster; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.entity.stock.BasicEntity; +import org.apache.brooklyn.entity.stock.BasicStartable; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.exceptions.CompoundRuntimeException; @@ -857,6 +860,64 @@ public void testDslTemplateOverDsl() throws Exception { assertEquals(getConfigEventually(app, DEST), "hello world"); } + @Test + public void testDslRecursiveFails() throws Exception { + final Entity app = createAndStartApplication( + "services:", + "- type: " + BasicApplication.class.getName(), + " brooklyn.config:", + " dest: $brooklyn:config(\"dest\")"); + try { + getConfigEventually(app, DEST); + Asserts.shouldHaveFailedPreviously(); + } catch (Exception e) { + Asserts.expectedFailureContains(e, "Recursive reference DSL:entity('", "').config('dest')"); + } + } + + @Test + public void testDslRecursiveFails2() throws Exception { + final Entity app = createAndStartApplication( + "services:", + "- type: " + BasicApplication.class.getName(), + " brooklyn.config:", + " val: $brooklyn:config(\"dest\")", + " dest: $brooklyn:config(\"val\")"); + try { + getConfigEventually(app, DEST); + Asserts.shouldHaveFailedPreviously(); + } catch (Exception e) { + Asserts.expectedFailureContains(e, "Recursive reference DSL:entity('", "').config('val')"); + } + } + + @Test + public void testDslFromParentConfigNotRecursive() throws Exception { + // Broke this in https://github.com/apache/brooklyn-server/pull/971. + // It thought that the '$brooklyn:parent().config("test.value")' was the + // same each time, rather than 'parent' resolving differently each time. + + final Entity app = createAndStartApplication( + "services:", + "- type: " + BasicApplication.class.getName(), + " brooklyn.config:", + " test.value: 0", + " brooklyn.children:", + " - type: "+BasicStartable.class.getName(), + " brooklyn.config:", + " test.value: $brooklyn:parent().config(\"test.value\")", + " brooklyn.children:", + " - type: "+DynamicCluster.class.getName(), + " brooklyn.config:", + " cluster.initial.size: $brooklyn:parent().config(\"test.value\")", + " memberSpec:", + " $brooklyn:entitySpec:", + " type: "+TestEntity.class.getName()); + + BasicStartable child = (BasicStartable) Iterables.getOnlyElement(app.getChildren()); + DynamicCluster cluster = (DynamicCluster) Iterables.getOnlyElement(child.getChildren()); + assertEquals(cluster.config().get(DynamicCluster.INITIAL_SIZE), Integer.valueOf(0)); + } private static T getConfigEventually(final Entity entity, final ConfigKey configKey) throws Exception { // Use an executor, in case config().get() blocks forever, waiting for the config value.