This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit 11afb170840fb028ca9c383629e0d1208c858d31 Author: Alex Heneveld <[email protected]> AuthorDate: Wed Jun 21 15:22:22 2023 +0100 fix for NPE when condition gets a Maybe containing null, and disallow that for entity coercion slightly stricter and better logging for entities and other things that become null on failed deserialization, rather than throwing --- .../camp/brooklyn/WorkflowExpressionsYamlTest.java | 69 +++++++++++++++++++++- .../core/resolve/jackson/BeanWithTypeUtils.java | 2 +- .../resolve/jackson/CommonTypesSerialization.java | 2 +- .../brooklyn/util/core/flags/TypeCoercions.java | 5 ++ .../util/core/predicates/DslPredicates.java | 2 +- .../brooklyn/core/workflow/WorkflowBasicTest.java | 2 +- .../javalang/coerce/TypeCoercerExtensible.java | 10 +++- 7 files changed, 84 insertions(+), 8 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowExpressionsYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowExpressionsYamlTest.java index cd21a00522..d0fb7d02e2 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowExpressionsYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowExpressionsYamlTest.java @@ -19,18 +19,29 @@ package org.apache.brooklyn.camp.brooklyn; import com.google.common.collect.Iterables; +import com.google.common.reflect.TypeToken; import org.apache.brooklyn.api.effector.Effector; import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.mgmt.Task; import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.entity.EntityInternal; +import org.apache.brooklyn.core.resolve.jackson.BeanWithTypeUtils; import org.apache.brooklyn.core.sensor.Sensors; import org.apache.brooklyn.core.workflow.WorkflowBasicTest; import org.apache.brooklyn.core.workflow.WorkflowExecutionContext; import org.apache.brooklyn.core.workflow.steps.flow.LogWorkflowStep; +import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.entity.stock.BasicEntity; +import org.apache.brooklyn.entity.stock.BasicEntityImpl; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.test.ClassLogWatcher; +import org.apache.brooklyn.util.core.flags.TypeCoercions; +import org.apache.brooklyn.util.core.internal.TypeCoercionsTest; +import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.exceptions.Exceptions; +import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.text.Secret; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.time.Duration; @@ -39,6 +50,7 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; import java.util.concurrent.Callable; +import java.util.function.Function; public class WorkflowExpressionsYamlTest extends AbstractYamlTest { @@ -93,9 +105,13 @@ public class WorkflowExpressionsYamlTest extends AbstractYamlTest { return WorkflowBasicTest.runWorkflow(lastEntity, Strings.lines(workflowDefinition), "custom"); } - private Object invokeWorkflowOnLastEntity(String ...workflowDefinition) throws Exception { - WorkflowExecutionContext context = invocationWorkflowOnLastEntity(workflowDefinition); - return context.getTask(false).get().get(Duration.seconds(5)); + private Object invokeWorkflowOnLastEntity(String ...workflowDefinition) { + try { + WorkflowExecutionContext context = invocationWorkflowOnLastEntity(workflowDefinition); + return context.getTask(false).get().get(Duration.seconds(5)); + } catch (Exception e) { + throw Exceptions.propagate(e); + } } Entity waitForLastEntity() { @@ -225,4 +241,51 @@ public class WorkflowExpressionsYamlTest extends AbstractYamlTest { Asserts.assertEquals(invokeWorkflowStepsWithLogging(), "53cr37"); } + + @Test + public void testEntityConditionSucceeds() throws Exception { + createEntityWithWorkflowEffector("- return ignored"); + Function<String,Object> test = equalsCheckTarget -> invokeWorkflowOnLastEntity( + "steps:\n" + + " - let ent = $brooklyn:self()\n" + + " - let root = $brooklyn:root()\n" + + " - transform checkTargetS = "+equalsCheckTarget+" | to_string\n" + + " - log comparing ${ent.id} with ${checkTargetS}\n" + + " - step: return condition met\n" + + " condition:\n" + + " target: ${ent}\n" + + " equals: "+equalsCheckTarget+"\n" + + " - step: return condition not met"); + + Asserts.assertEquals(test.apply("xxx"), "condition not met"); + Asserts.assertEquals(test.apply("${root}"), "condition not met"); + Asserts.assertEquals(test.apply("${root.children[0]}"), "condition met"); + + + // checking equality to the ID does not work + // it could, fairly easily -- the ID *is* coerced to an entity; + // but it then fails because it is coerced to the _proxy_ which is *not* coerced to the real delegate + Asserts.assertEquals(test.apply("${ent.id}"), "condition not met"); + + // notes and minor tests on the above + // coercion of ID + Entity coercedFromId = Entities.submit(lastEntity, Tasks.of("test", () -> TypeCoercions.coerce(lastEntity.getId(), Entity.class))).get(); + Asserts.assertEquals(coercedFromId, lastEntity); + Maybe<BasicEntityImpl> coercedFromIdProxyToConcreteFails = Entities.submit(lastEntity, Tasks.of("test", () -> TypeCoercions.tryCoerce(lastEntity.getId(), BasicEntityImpl.class))).get(); + Asserts.assertThat(coercedFromIdProxyToConcreteFails, Maybe::isAbsent); + // under the covers above works using coercer 80-bean, which does + Entity coercedFromIdEntity = BeanWithTypeUtils.convert(mgmt(), lastEntity.getId(), TypeToken.of(Entity.class), true, null, true); + Asserts.assertEquals(coercedFromIdEntity, lastEntity); + + // some extra checks for coercion of unknown IDs -- conversion returns null, but tryConvert and coerce will not accept that + Entity coercedFromMissingIdRaw = BeanWithTypeUtils.convert(mgmt(), "xxx", TypeToken.of(Entity.class), true, null, true); + Asserts.assertNull(coercedFromMissingIdRaw); + + Maybe<Entity> coercedFromMissingId; + coercedFromMissingId = BeanWithTypeUtils.tryConvertOrAbsent(mgmt(), Maybe.of("xxx"), TypeToken.of(Entity.class), true, null, true); + Asserts.assertThat(coercedFromMissingId, Maybe::isAbsent); + + coercedFromMissingId = Entities.submit(lastEntity, Tasks.of("test", () -> TypeCoercions.tryCoerce("does_not_exist", Entity.class))).get(); + Asserts.assertThat(coercedFromMissingId, Maybe::isAbsent); + } } diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BeanWithTypeUtils.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BeanWithTypeUtils.java index 91835f23ee..9b2a6dde2b 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BeanWithTypeUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BeanWithTypeUtils.java @@ -270,7 +270,7 @@ public class BeanWithTypeUtils { } try { - return Maybe.of(convert(mgmt, o, type, allowRegisteredTypes, loader, allowJavaTypes)); + return Maybe.ofDisallowingNull(convert(mgmt, o, type, allowRegisteredTypes, loader, allowJavaTypes)); } catch (Exception e) { if (fallback!=null) return fallback; return Maybe.absent("BeanWithType cannot convert given input "+o+" to "+type, e); diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/CommonTypesSerialization.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/CommonTypesSerialization.java index 53b7e4bc53..fb5e553bec 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/CommonTypesSerialization.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/CommonTypesSerialization.java @@ -443,7 +443,7 @@ public class CommonTypesSerialization { return super.convertStringToObject(value, p, ctxt); } catch (Exception e) { Exceptions.propagateIfFatal(e); - LOG.warn("Reference to BrooklynObject "+value+" which is no longer available; replacing with 'null'"); + LOG.warn("Reference to BrooklynObject "+value+" which is unknown or no longer available; replacing with 'null'"); return null; } } diff --git a/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java b/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java index ed2e66f165..815363e270 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java @@ -367,6 +367,11 @@ public class TypeCoercions { if (BeanWithTypeUtils.isConversionRecommended(Maybe.of(input), type)) { try { Maybe<T> result = BeanWithTypeUtils.tryConvertOrAbsentUsingContext(Maybe.of(input), type); + if (result.isPresent() && result.get()==null) { + // normal for entities that are unknown; but when coercing we should not allow that + log.debug("Coercion of "+input+" to "+type+" returned null"); + return null; + } return result; } catch (Exception e) { return Maybe.absent(e); diff --git a/core/src/main/java/org/apache/brooklyn/util/core/predicates/DslPredicates.java b/core/src/main/java/org/apache/brooklyn/util/core/predicates/DslPredicates.java index f3d6673095..8dc29b47e3 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/predicates/DslPredicates.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/predicates/DslPredicates.java @@ -164,7 +164,7 @@ public class DslPredicates { Maybe<? extends Object> mma = TypeCoercions.tryCoerce(ma, mb.getClass()); if (mma.isPresent()) { // repeat equality check - if (mma.get().equals(mb) || mb.equals(mma.get())) return Maybe.of(true); + if (Objects.equals(mma.get(), mb) || Objects.equals(mb, mma.get())) return Maybe.of(true); } return Maybe.absent("coercion not supported in equality check, to "+mb.getClass()); } diff --git a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowBasicTest.java b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowBasicTest.java index e8da1bf0de..098e0c23a2 100644 --- a/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowBasicTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/workflow/WorkflowBasicTest.java @@ -478,5 +478,5 @@ public class WorkflowBasicTest extends BrooklynMgmtUnitTestSupport { w1.getTask(false).get().getUnchecked(), MutableList.of("a=b", "b=c")); } - + } diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercerExtensible.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercerExtensible.java index c9c03b1910..c1274810f7 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercerExtensible.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercerExtensible.java @@ -175,7 +175,15 @@ public class TypeCoercerExtensible implements TypeCoercer { } } - if (result!=null) errors.add(result); + if (result!=null) { + if (result.isAbsent()) errors.add(result); + else { + log.warn("Coercer " + coercer + " returned wrapped null when coercing " + value); + errors.add(Maybe.absent("coercion returned null")); + // arguably it should return null here +// return null; + } + } } //ENHANCEMENT could look in type hierarchy of both types for a conversion method...
