You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@brooklyn.apache.org by he...@apache.org on 2023/06/23 20:20:39 UTC
[brooklyn-server] 01/03: fix for NPE when condition gets a Maybe containing null, and disallow that for entity coercion
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 <al...@cloudsoft.io>
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...