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 5729810bcd7cbba079091b91f57c999f62e3a160 Author: Alex Heneveld <[email protected]> AuthorDate: Mon Dec 12 13:46:03 2022 +0000 better deserialization for adjunct specs was picky about use of registered types vs classes, brooklyn.config vs flags; now jackson deserialization handles most of those, understanding specs and registered spec types, with lots of tests. other minor tweaks for use of specs eg in wrapped values. also fixes "uniqueTag" in addPolicy step so that it bases on retention hash, and retention hash takes a hash of steps if there is no name, so it is consistent --- .../org/apache/brooklyn/api/entity/EntitySpec.java | 4 +- .../api/internal/AbstractBrooklynObjectSpec.java | 42 +++++++++------- .../org/apache/brooklyn/api/policy/PolicySpec.java | 16 +++--- .../apache/brooklyn/api/sensor/EnricherSpec.java | 12 +++-- .../brooklyn/WorkflowApplicationModelTest.java | 39 +++++++++++---- .../brooklyn/spi/dsl/DslSerializationTest.java | 57 +++++++++++++++++++++- .../catalog/internal/BasicBrooklynCatalog.java | 36 +++++++++----- .../resolve/jackson/AsPropertyIfAmbiguous.java | 53 +++++++++++++++++++- ...BrooklynRegisteredTypeJacksonSerialization.java | 5 ++ .../resolve/jackson/CommonTypesSerialization.java | 9 +++- .../jackson/WrappedValuesSerialization.java | 30 +++++++++--- .../core/workflow/WorkflowExecutionContext.java | 2 +- .../steps/appmodel/AddPolicyWorkflowStep.java | 15 ++++-- .../util/json/BrooklynJacksonSerializerTest.java | 57 ++++++++++++++++------ 14 files changed, 296 insertions(+), 81 deletions(-) diff --git a/api/src/main/java/org/apache/brooklyn/api/entity/EntitySpec.java b/api/src/main/java/org/apache/brooklyn/api/entity/EntitySpec.java index fdf645df53..f47fb6fc54 100644 --- a/api/src/main/java/org/apache/brooklyn/api/entity/EntitySpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/entity/EntitySpec.java @@ -123,7 +123,7 @@ public class EntitySpec<T extends Entity> extends AbstractBrooklynObjectSpec<T,E private final List<EntitySpec<?>> children = Lists.newArrayList(); private final List<Entity> members = Lists.newArrayList(); private final List<Group> groups = Lists.newArrayList(); - private volatile boolean immutable; + private volatile Boolean immutable; // Kept for backwards compatibility of persisted state private List<Policy> policies; @@ -422,7 +422,7 @@ public class EntitySpec<T extends Entity> extends AbstractBrooklynObjectSpec<T,E } private void checkMutable() { - if (immutable) throw new IllegalStateException("Cannot modify immutable entity spec "+this); + if (Boolean.TRUE.equals(immutable)) throw new IllegalStateException("Cannot modify immutable entity spec "+this); } } diff --git a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java index cb8679614d..c1c1253017 100644 --- a/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/internal/AbstractBrooklynObjectSpec.java @@ -18,18 +18,17 @@ */ package org.apache.brooklyn.api.internal; -import static com.google.common.base.Preconditions.checkNotNull; - -import java.io.Serializable; -import java.lang.reflect.Modifier; -import java.util.Collection; -import java.util.Collections; -import java.util.List; -import java.util.Map; -import java.util.Set; - +import com.fasterxml.jackson.annotation.JsonAnySetter; +import com.fasterxml.jackson.annotation.JsonInclude; import com.fasterxml.jackson.annotation.JsonSetter; +import com.google.common.annotations.Beta; +import com.google.common.base.MoreObjects; +import com.google.common.base.Objects; import com.google.common.base.Predicate; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.collect.Maps; import org.apache.brooklyn.api.mgmt.EntityManager; import org.apache.brooklyn.api.mgmt.Task; import org.apache.brooklyn.api.objs.BrooklynObject; @@ -42,13 +41,11 @@ import org.apache.brooklyn.util.exceptions.Exceptions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import com.google.common.annotations.Beta; -import com.google.common.base.MoreObjects; -import com.google.common.base.Objects; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; -import com.google.common.collect.Iterables; -import com.google.common.collect.Maps; +import java.io.Serializable; +import java.lang.reflect.Modifier; +import java.util.*; + +import static com.google.common.base.Preconditions.checkNotNull; /** * Defines a spec for creating a {@link BrooklynObject}. @@ -61,13 +58,14 @@ import com.google.common.collect.Maps; * e.g. {@link EntityManager#createEntity(org.apache.brooklyn.api.entity.EntitySpec)} * to create a managed instance of the target type. */ +@JsonInclude(JsonInclude.Include.NON_EMPTY) public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrooklynObjectSpec<T, SpecT>> implements Serializable { private static final long serialVersionUID = 3010955277740333030L; private static final Logger log = LoggerFactory.getLogger(AbstractBrooklynObjectSpec.class); - private final Class<? extends T> type; + private Class<? extends T> type; private String displayName; private String catalogItemId; private Collection<String> catalogItemIdSearchPath = MutableSet.of(); @@ -78,6 +76,9 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl protected final Map<String, Object> flags = Maps.newLinkedHashMap(); protected final Map<ConfigKey<?>, Object> config = Maps.newLinkedHashMap(); + // jackson deserializer only + protected AbstractBrooklynObjectSpec() {} + protected AbstractBrooklynObjectSpec(Class<? extends T> type) { checkValidType(type); this.type = type; @@ -475,4 +476,9 @@ public abstract class AbstractBrooklynObjectSpec<T, SpecT extends AbstractBrookl configure(val); } + @JsonAnySetter + private void jsonSetConfig(String flag, Object value) { + configure(flag, value); + } + } diff --git a/api/src/main/java/org/apache/brooklyn/api/policy/PolicySpec.java b/api/src/main/java/org/apache/brooklyn/api/policy/PolicySpec.java index 6182629caf..9f8d5f4101 100644 --- a/api/src/main/java/org/apache/brooklyn/api/policy/PolicySpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/policy/PolicySpec.java @@ -18,10 +18,10 @@ */ package org.apache.brooklyn.api.policy; -import java.util.Map; - import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; +import java.util.Map; + /** * Gives details of a policy to be created. It describes the policy's configuration, and is * reusable to create multiple policies with the same configuration. @@ -58,20 +58,24 @@ public class PolicySpec<T extends Policy> extends AbstractBrooklynObjectSpec<T,P public static <T extends Policy> PolicySpec<T> create(Map<?,?> config, Class<T> type) { return PolicySpec.create(type).configure(config); } - + + // jackson constructor only + protected PolicySpec() { + super(); + } protected PolicySpec(Class<T> type) { super(type); } - + @Override protected void checkValidType(Class<? extends T> type) { checkIsImplementation(type, Policy.class); checkIsNewStyleImplementation(type); } - + public PolicySpec<T> uniqueTag(String uniqueTag) { flags.put("uniqueTag", uniqueTag); return this; } - + } diff --git a/api/src/main/java/org/apache/brooklyn/api/sensor/EnricherSpec.java b/api/src/main/java/org/apache/brooklyn/api/sensor/EnricherSpec.java index 8e02fb85d7..0ba6de4641 100644 --- a/api/src/main/java/org/apache/brooklyn/api/sensor/EnricherSpec.java +++ b/api/src/main/java/org/apache/brooklyn/api/sensor/EnricherSpec.java @@ -18,13 +18,13 @@ */ package org.apache.brooklyn.api.sensor; -import java.util.Map; - import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; import org.apache.brooklyn.api.mgmt.Task; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.config.ConfigKey.HasConfigKey; +import java.util.Map; + /** * Gives details of an enricher to be created. It describes the enricher's configuration, and is * reusable to create multiple enrichers with the same configuration. @@ -60,7 +60,11 @@ public class EnricherSpec<T extends Enricher> extends AbstractBrooklynObjectSpec public static <T extends Enricher> EnricherSpec<T> create(Map<?,?> config, Class<T> type) { return EnricherSpec.create(type).configure(config); } - + + // jackson constructor only + protected EnricherSpec() { + super(); + } protected EnricherSpec(Class<? extends T> type) { super(type); } @@ -70,7 +74,7 @@ public class EnricherSpec<T extends Enricher> extends AbstractBrooklynObjectSpec checkIsImplementation(type, Enricher.class); checkIsNewStyleImplementation(type); } - + public EnricherSpec<T> uniqueTag(String uniqueTag) { flags.put("uniqueTag", uniqueTag); return this; diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowApplicationModelTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowApplicationModelTest.java index 3d57e6e5f9..868ae1b80a 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowApplicationModelTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowApplicationModelTest.java @@ -25,7 +25,9 @@ import org.apache.brooklyn.api.entity.EntityLocal; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.location.Location; import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.policy.Policy; import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.resolve.jackson.CommonTypesSerialization; import org.apache.brooklyn.core.test.policy.TestPolicy; import org.apache.brooklyn.core.workflow.WorkflowBasicTest; import org.apache.brooklyn.core.workflow.WorkflowEffector; @@ -40,10 +42,14 @@ import org.apache.brooklyn.util.core.config.ConfigBag; 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.StringEscapes; import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.time.Duration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.testng.annotations.Test; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Map; @@ -53,6 +59,8 @@ import java.util.stream.Collectors; public class WorkflowApplicationModelTest extends AbstractYamlTest { + private static final Logger LOG = LoggerFactory.getLogger(WorkflowApplicationModelTest.class); + protected void loadTypes() { WorkflowBasicTest.addWorkflowStepTypes(mgmt()); } @@ -245,21 +253,32 @@ public class WorkflowApplicationModelTest extends AbstractYamlTest { @Test public void testAddAndDeletePolicyAndOtherAdjuncts() { loadTypes(); - doTestAddAndDeletePolicyAndOtherAdjuncts(true, true); - doTestAddAndDeletePolicyAndOtherAdjuncts(true, false); - doTestAddAndDeletePolicyAndOtherAdjuncts(false, true); - doTestAddAndDeletePolicyAndOtherAdjuncts(false, false); + WorkflowBasicTest.addRegisteredTypeSpec(mgmt(), "test-policy", TestPolicy.class, Policy.class); + + // to test a specific configuration +// doTestAddAndDeletePolicyAndOtherAdjuncts("test-policy", false, true, false); + + Arrays.asList(TestPolicy.class.getName(), "test-policy").forEach(type -> + Arrays.asList(true, false).forEach(shorthand -> + Arrays.asList(true, false).forEach(explicitUniqueTag -> + Arrays.asList(true, false).forEach(nestInBrooklynConfig -> { + LOG.info("Testing with "+type+" "+shorthand+" "+explicitUniqueTag+" "+nestInBrooklynConfig); + doTestAddAndDeletePolicyAndOtherAdjuncts(type, shorthand, explicitUniqueTag, nestInBrooklynConfig); + } + )))); } - void doTestAddAndDeletePolicyAndOtherAdjuncts(boolean shorthand, boolean explicitUniqueTag) { + void doTestAddAndDeletePolicyAndOtherAdjuncts(String type, boolean shorthand, boolean explicitUniqueTag, Boolean nestInBrooklynConfig) { BasicApplication app = mgmt().getEntityManager().createEntity(EntitySpec.create(BasicApplication.class)); Object step1 = shorthand ? "add-policy " + TestPolicy.class.getName() + (explicitUniqueTag ? " unique-tag my-policy" : "") : "\n"+Strings.indent(2, Strings.lines( "type: add-policy", "blueprint:", - " type: "+TestPolicy.class.getName(), - " "+TestPolicy.CONF_NAME.getName()+": my-policy-name", - explicitUniqueTag ? " uniqueTag: my-policy" : "")); + " type: "+type, + explicitUniqueTag ? " uniqueTag: my-policy" : "", + nestInBrooklynConfig ? " brooklyn.config:" : "", + (nestInBrooklynConfig ? " " : "") + " "+TestPolicy.CONF_NAME.getName()+": my-policy-name", + "")); WorkflowExecutionContext w1 = WorkflowBasicTest.runWorkflow(app, Strings.lines( "steps:", "- "+step1, @@ -282,13 +301,13 @@ public class WorkflowApplicationModelTest extends AbstractYamlTest { String ut = Iterables.getOnlyElement(app.policies()).getUniqueTag(); if (explicitUniqueTag) Asserts.assertEquals(ut, "my-policy"); - else Asserts.assertEquals(ut, w1.getWorkflowId()+"-1"); + else Asserts.assertEquals(ut, w1.getRetentionHash()+" - 1"); Asserts.assertEquals(t1, ut); Asserts.assertEquals(t2, ut); WorkflowExecutionContext w2 = WorkflowBasicTest.runWorkflow(app, Strings.lines( "steps:", - " - step: delete-policy "+ut + " - step: delete-policy "+ StringEscapes.JavaStringEscapes.wrapJavaString(ut) ), null); w2.getTask(false).get().getUnchecked(); diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslSerializationTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslSerializationTest.java index 37a598b03d..3ee4bda07c 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslSerializationTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/DslSerializationTest.java @@ -22,6 +22,10 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Optional; import java.util.Map; import java.util.function.Supplier; + +import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; import org.apache.brooklyn.camp.brooklyn.AbstractYamlTest; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent; @@ -31,6 +35,8 @@ import org.apache.brooklyn.core.resolve.jackson.MapperTestFixture; import org.apache.brooklyn.core.resolve.jackson.WrappedValue; import org.apache.brooklyn.core.resolve.jackson.WrappedValuesSerializationTest; import org.apache.brooklyn.core.resolve.jackson.WrappedValuesSerializationTest.ObjectWithWrappedValueString; +import org.apache.brooklyn.core.workflow.WorkflowBasicTest; +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.core.task.BasicExecutionContext; @@ -76,7 +82,7 @@ public class DslSerializationTest extends AbstractYamlTest implements MapperTest } public ObjectMapper mapper() { - return BeanWithTypeUtils.newMapper(mgmt(), false, null, true); + return BeanWithTypeUtils.newMapper(mgmt(), true, null, true); } @Test @@ -174,4 +180,53 @@ public class DslSerializationTest extends AbstractYamlTest implements MapperTest Asserts.assertNotNull(impl.x); Asserts.assertEquals(resolve(impl.x, String.class).get(), "foo"); } + + // also see org.apache.brooklyn.rest.util.json.BrooklynJacksonSerializerTest.SpecHolder + public static class ObjectWithWrappedValueSpec extends WrappedValue.WrappedValuesInitialized { + public WrappedValue<EntitySpec<?>> sw; + public EntitySpec<?> su; + + public WrappedValue<AbstractBrooklynObjectSpec<?,?>> asw; + public AbstractBrooklynObjectSpec<?,?> asu; + } + + @Test + public void testDeserializeSpec() throws Exception { + // also see org.apache.brooklyn.rest.util.json.BrooklynJacksonSerializerTest.SpecHolder + + BrooklynDslCommon.registerSerializationHooks(); + + WorkflowBasicTest.addRegisteredTypeSpec(mgmt(), "basic-startable", BasicStartable.class, Entity.class); + ObjectWithWrappedValueSpec impl; + + impl = deser(json("sw: { type: basic-startable }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.sw); + Asserts.assertEquals(impl.sw.get().getType(), BasicStartable.class); + + impl = deser(json("sw: { type: "+BasicStartable.class.getName()+" }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.sw); + Asserts.assertEquals(impl.sw.get().getType(), BasicStartable.class); + + impl = deser(json("asw: { type: basic-startable }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.asw); + Asserts.assertEquals(impl.asw.get().getType(), BasicStartable.class); + + impl = deser(json("asw: { type: "+BasicStartable.class.getName()+" }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.asw); + // with non-concrete type, we used to get a map in wrapped value, but now we try the generic type +// Asserts.assertInstanceOf(impl.asw.get(), Map.class); + Asserts.assertEquals(impl.asw.get().getType(), BasicStartable.class); + + impl = deser(json("su: { type: basic-startable }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.su); + + impl = deser(json("su: { type: "+BasicStartable.class.getName()+" }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.su); + + impl = deser(json("asu: { type: basic-startable }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.asu); + + impl = deser(json("asu: { type: "+BasicStartable.class.getName()+" }"), ObjectWithWrappedValueSpec.class); + Asserts.assertNotNull(impl.asu); + } } 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 69e0ff4c24..f28f819b64 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 @@ -1737,7 +1737,7 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { } private static boolean isDubiousBeanType(Object t) { - return t instanceof Map || t instanceof Collection || (t instanceof BrooklynObject && !(t instanceof Feed)); + return t instanceof Map || t instanceof Collection || (t instanceof BrooklynObject && !(t instanceof Feed)) || (t instanceof AbstractBrooklynObjectSpec); } /** records the type this catalog is currently trying to resolve items being added to the catalog, if it is trying to resolve. @@ -1993,7 +1993,8 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { // more importantly bean to allow arbitrary types to be added to catalog RegisteredType resultT = null; - + boolean recheckNeededBecauseChangedOrUncertain = false; + Object resultO = null; if (resultO==null && boType!=null) try { // try spec instantiation if we know the BO Type (no point otherwise) @@ -2014,12 +2015,26 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { // try it as a bean resultT = RegisteredTypes.copyResolved(RegisteredTypeKind.BEAN, typeToValidate, allowChangingKind); try { - resultO = ((BasicBrooklynTypeRegistry)mgmt.getTypeRegistry()).createBean(resultT, constraint, superJ); - if (typeToValidate.getKind()!=RegisteredTypeKind.BEAN && isDubiousBeanType(resultO)) { - // 2022-05 previously we would set this, and it would get re-resolved better later on; but now we don't; if it's a dubious bean - // (ie a map or collection) you have to specify that is a bean + resultO = mgmt.getTypeRegistry().createBean(resultT, constraint, superJ); + if (resultO instanceof AbstractBrooklynObjectSpec) { resultO = null; - throw new IllegalStateException("Dubious resolution of "+typeToValidate+" as "+resultO.getClass().getName()+" "+resultO+"; if this is intended, specify kind as bean"); + throw new IllegalStateException("Dubious resolution of "+typeToValidate+" as "+resultO.getClass().getName()+" (spec not bean)"); + } + if (isDubiousBeanType(resultO)) { + // 2022-05 previously we would always infer bean type, but now if it's a "dubious bean" you have the specify that it is a bean; + // if not, we mark it as dubious here, and we re-resolve later on. + // 2022-11 now we always try re-resolving later if it's a dubious bean type, so that we don't accept maps where caller has indicated a type, + // and that type might change (eg NestedRefsCatalogYamlTest) + if (typeToValidate.getKind()!=RegisteredTypeKind.BEAN) { + recheckNeededBecauseChangedOrUncertain = true; + resultO = null; + throw new IllegalStateException("Dubious resolution of " + typeToValidate + " as " + resultO.getClass().getName() + " " + resultO + "; if this is intended, specify kind as bean"); + } + if (allowChangingKind) { + recheckNeededBecauseChangedOrUncertain = true; + resultO = null; + throw new IllegalStateException("Uncertain resolution of " + typeToValidate + " as " + resultO.getClass().getName() + " " + resultO + "; will try again"); + } } } catch (Exception e) { Exceptions.propagateIfFatal(e); @@ -2063,7 +2078,6 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { // try this even for templates; errors in them will be ignored by validator // but might be interesting to someone calling resolve directly - boolean changedSomething = false; // reset resultT and change things as needed based on guesser resultT = typeToValidate; if (boType==null) { @@ -2075,17 +2089,17 @@ public class BasicBrooklynCatalog implements BrooklynCatalog { // didn't know type before, retry now that we know the type resultT = RegisteredTypes.copyResolved(RegisteredTypeKind.SPEC, resultT, allowChangingKind); RegisteredTypes.addSuperTypes(resultT, supers); - changedSomething = true; + recheckNeededBecauseChangedOrUncertain = true; } } if (!Objects.equal(guesser.getPlanYaml(), yaml)) { RegisteredTypes.changePlanNotingEquivalent(resultT, new BasicTypeImplementationPlan(typeToValidate.getPlan().getPlanFormat(), guesser.getPlanYaml())); - changedSomething = true; + recheckNeededBecauseChangedOrUncertain = true; } - if (changedSomething) { + if (recheckNeededBecauseChangedOrUncertain) { log.debug("Re-resolving "+resultT+" following detection of change"); // try again with new plan or supertype info return validateResolve(resultT, constraint, false); diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/AsPropertyIfAmbiguous.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/AsPropertyIfAmbiguous.java index f9ab39d210..6cc34dd269 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/AsPropertyIfAmbiguous.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/AsPropertyIfAmbiguous.java @@ -34,6 +34,11 @@ import com.fasterxml.jackson.databind.jsontype.TypeIdResolver; import com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer; import com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeSerializer; import com.fasterxml.jackson.databind.util.TokenBuffer; +import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; +import org.apache.brooklyn.api.objs.BrooklynObject; +import org.apache.brooklyn.api.objs.BrooklynObjectType; +import org.apache.brooklyn.api.sensor.Feed; +import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; @@ -186,7 +191,7 @@ public class AsPropertyIfAmbiguous { if (_idResolver instanceof HasBaseType) { JavaType baseType = ((HasBaseType) _idResolver).getBaseType(); if (baseType != null ) { - if (hasTypePropertyNameAsField(baseType)) { + if (hasTypePropertyNameAsField(baseType) && !AbstractBrooklynObjectSpec.class.isAssignableFrom(baseType.getRawClass())) { // look for an '@' type // return cloneWithNewTypePropertyName(CONFLICTING_TYPE_NAME_PROPERTY_TRANSFORM.apply(_typePropertyName)).deserializeTypedFromObject(p, ctxt); // now we always look for @ first, in case the type is not known but that field is present; but if we know 'type' is a bean field, don't allow it to be used @@ -278,6 +283,52 @@ public class AsPropertyIfAmbiguous { boolean disallowed = false; if (ambiguousName) { JavaType tt = _idResolver.typeFromId(ctxt, typeId); + if (BrooklynObject.class.isAssignableFrom(tt.getRawClass()) && !Feed.class.isAssignableFrom(tt.getRawClass())) { + Boolean wantsSpec = null; + Boolean wantsBO = null; + + JavaType baseType = null; + if (_idResolver instanceof HasBaseType) { + baseType = ((HasBaseType) _idResolver).getBaseType(); + if (baseType != null) { + wantsSpec = AbstractBrooklynObjectSpec.class.isAssignableFrom(baseType.getRawClass()); + wantsBO = BrooklynObject.class.isAssignableFrom(baseType.getRawClass()); + } + } + + if (Boolean.TRUE.equals(wantsSpec)) { + if (tt instanceof BrooklynJacksonType && BrooklynTypeRegistry.RegisteredTypeKind.SPEC.equals(((BrooklynJacksonType)tt).getRegisteredType().getKind())) { + // if it's a spec registered type, we should load it, like normal + // (no-op) + } else { + // if it's a class then we need to (1) infer the BOSpec type, then (2) re-read the type and set that as the field + typeId = BrooklynObjectType.of(tt.getRawClass()).getSpecType().getName(); + tt = null; + if (tb == null) { + tb = ctxt.bufferForInputBuffering(p); + } + tb.writeFieldName(name); + tb.copyCurrentStructure(p); + } + } else if (Boolean.TRUE.equals(wantsBO)) { + // if caller wants a BO we just read it normally, whether loading from an ID or created a (non-entity) instance such as a feed + // no-op + + } else if (!(tt instanceof BrooklynJacksonType) && BrooklynObjectType.of(tt.getRawClass()).getInterfaceType().equals(tt.getRawClass())) { + // if caller hasn't explicitly asked for a BO, and a base BO type (eg Entity) is specified, probably we are loading from an ID + // by specifying Entity class exactly (not a sub-type interface and not registered type) we allow re-instantiation using ID + // no-op + + } else { + // caller hasn't explicitly asked for a BO, and it isn't a recognized pattern, so in this case we do not load the type; + // will probably remain as a map, unless (type) is specified + + if (LOG.isTraceEnabled()) LOG.trace("Ambiguous request for "+baseType+" / "+tt+"; allowing"); + tt = null; + disallowed = true; + } + } + if (tt!=null && hasTypePropertyNameAsField(tt)) { // if there is a property called 'type' then caller should use @type. disallowed = true; diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerialization.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerialization.java index 31e0cab4c7..a3a1d7a56a 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerialization.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerialization.java @@ -40,6 +40,7 @@ import com.fasterxml.jackson.databind.module.SimpleModule; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.mgmt.classloading.BrooklynClassLoadingContext; import org.apache.brooklyn.api.objs.BrooklynObject; +import org.apache.brooklyn.api.typereg.BrooklynTypeRegistry; import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext; import org.apache.brooklyn.core.resolve.jackson.AsPropertyIfAmbiguous.AsPropertyButNotIfFieldConflictTypeDeserializer; @@ -84,6 +85,10 @@ public class BrooklynRegisteredTypeJacksonSerialization { // but instead treat as error throw new NullPointerException("Requested to deserialize Brooklyn type "+type+" without a management context"); } + if (type.getRegisteredType().getKind().equals(BrooklynTypeRegistry.RegisteredTypeKind.SPEC)) { + return mgmt.getTypeRegistry().createSpec(type.getRegisteredType(), null, null); + } + return mgmt.getTypeRegistry().createBean(type.getRegisteredType(), null, null); } } 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 c5d091b007..679bddec6f 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 @@ -32,6 +32,7 @@ import com.fasterxml.jackson.databind.ser.BeanPropertyWriter; import com.fasterxml.jackson.databind.ser.BeanSerializerModifier; import com.google.common.base.Predicate; import com.google.common.reflect.TypeToken; +import org.apache.brooklyn.api.internal.AbstractBrooklynObjectSpec; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.objs.BrooklynObject; import org.apache.brooklyn.api.objs.BrooklynObjectType; @@ -39,9 +40,11 @@ import org.apache.brooklyn.api.objs.Configurable; import org.apache.brooklyn.config.ConfigKey; import org.apache.brooklyn.core.config.ConfigKeys; import org.apache.brooklyn.core.objs.BrooklynObjectInternal; +import org.apache.brooklyn.core.typereg.TypePlanTransformers; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.core.flags.BrooklynTypeNameResolution; import org.apache.brooklyn.util.core.flags.FlagUtils; +import org.apache.brooklyn.util.core.flags.TypeCoercions; import org.apache.brooklyn.util.core.predicates.DslPredicates; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.javalang.Boxing; @@ -214,13 +217,17 @@ public class CommonTypesSerialization { } else if (value.getClass().equals(Object.class)) { return newEmptyInstance(); } else { - throw new IllegalStateException(getType()+" should be supplied as string or map with 'type' and 'value'; instead had " + value); + return deserializeOther(value); } } catch (Exception e) { throw Exceptions.propagate(e); } } + public T deserializeOther(Object value) { + throw new IllegalStateException(getType()+" should be supplied as string or map with 'type' and 'value'; instead had " + value); + } + @Override public T deserialize(JsonParser p, DeserializationContext ctxt, T intoValue) throws IOException, JsonProcessingException { return copyInto(deserialize(p, ctxt), intoValue); diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/WrappedValuesSerialization.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/WrappedValuesSerialization.java index 6123497169..8a05b79a64 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/WrappedValuesSerialization.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/WrappedValuesSerialization.java @@ -95,23 +95,39 @@ public class WrappedValuesSerialization { List<Exception> exceptions = MutableList.of(); try { TokenBuffer b = BrooklynJacksonSerializationUtils.createBufferForParserCurrentObject(p, ctxt); + JavaType genericType = null; try { // this should work for primitives, objects, and suppliers (which will declare type) // only time it won't is where generics are used to drop the type declaration during serialization - JavaType genericType = getGenericType(typeDeserializer); - if (genericType != null) { - return ctxt.findNonContextualValueDeserializer(genericType).deserialize(b.asParserOnFirstToken(), ctxt); - } + genericType = getGenericType(typeDeserializer); } catch (Exception e) { exceptions.add(e); } + if (genericType != null) { + try { + // this uses our type deserializer, will try type instantiation from a declared type and/or expected type of the generics + return ctxt.findRootValueDeserializer(genericType).deserialize( + b.asParserOnFirstToken(), ctxt); + } catch (Exception e) { + exceptions.add(e); + } + } + + if (genericType!=null) { + try { + // this does _not_ use our type deserializer; will try type instantiation from the expected type of the generics however + return ctxt.findNonContextualValueDeserializer(genericType).deserialize( + createParserFromTokenBufferAndParser(b, p), ctxt); + } catch (Exception e) { + exceptions.add(e); + } + } + // fall back to just using object try { return ctxt.findRootValueDeserializer(ctxt.constructType(Object.class)).deserialize( -// b.asParserOnFirstToken() - createParserFromTokenBufferAndParser(b, p) - , ctxt); + createParserFromTokenBufferAndParser(b, p), ctxt); } catch (Exception e) { exceptions.add(e); } diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java index 2c2327283c..2e9e0581d8 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExecutionContext.java @@ -671,7 +671,7 @@ public class WorkflowExecutionContext { public String getRetentionHash() { if (retention!=null && Strings.isNonBlank(retention.hash)) return retention.hash; if (Strings.isNonBlank(getName())) return getName(); - return "anonymous-workflow"; + return "anonymous-workflow-"+Math.abs(getStepsDefinition().hashCode()); } public void updateRetentionFrom(WorkflowRetentionAndExpiration.WorkflowRetentionSettings other) { diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/steps/appmodel/AddPolicyWorkflowStep.java b/core/src/main/java/org/apache/brooklyn/core/workflow/steps/appmodel/AddPolicyWorkflowStep.java index 2855b0b414..2ec1ab42eb 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/steps/appmodel/AddPolicyWorkflowStep.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/steps/appmodel/AddPolicyWorkflowStep.java @@ -41,6 +41,7 @@ import org.apache.brooklyn.util.core.flags.TypeCoercions; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.text.StringEscapes; +import org.apache.brooklyn.util.text.Strings; import org.apache.brooklyn.util.yaml.Yamls; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -95,14 +96,17 @@ public class AddPolicyWorkflowStep extends WorkflowStepDefinition { if (!(blueprint instanceof String)) blueprint = BeanWithTypeUtils.newYamlMapper(null, false, null, false).writeValueAsString(blueprint); Object yo = Iterables.getOnlyElement(Yamls.parseAll((String) blueprint)); - // coercion does this at: org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon.registerSpecCoercionAdapter + + // coercion does this _for entities only_ at: org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon.registerSpecCoercionAdapter; + // other types follow jackson deserialization Maybe<AbstractBrooklynObjectSpec> spec0 = TypeCoercions.tryCoerce(yo, AbstractBrooklynObjectSpec.class); if (spec0.isAbsent()) { + inst = TypeCoercions.tryCoerce(yo, EntityAdjunct.class) // .get(); // prefer this exception - .orNull(); // or prefer original expression (line below) + .orNull(); // or prefer other error if (inst == null) { - // try camp spec instantiation; and prefer this error. this is the only way if loading a registered type of kind spec; + // try camp spec instantiation; and prefer this error. this used to be the only way if loading a registered type of kind spec; // but it is stricter about requiring arguments in `brooklyn.config` spec = context.getManagementContext().getTypeRegistry().createSpecFromPlan(null, blueprint, null, AbstractBrooklynObjectSpec.class); @@ -119,11 +123,12 @@ public class AddPolicyWorkflowStep extends WorkflowStepDefinition { BrooklynObject result; boolean uniqueTagSet = input.containsKey(UNIQUE_TAG.getName()); - String uniqueTag = uniqueTagSet ? context.getInput(UNIQUE_TAG) : context.getWorkflowStepReference(); + String uniqueTag = uniqueTagSet ? context.getInput(UNIQUE_TAG) : + Strings.firstNonBlank(context.getWorkflowExectionContext().getName(), context.getWorkflowExectionContext().getRetentionHash())+" - "+(context.getStepIndex()+1); // set the unique tag to the workflow step if not already set, to ensure idempotency if (spec!=null) { if (uniqueTagSet || !spec.getFlags().containsKey("uniqueTag")) { - FlagUtils.setFieldFromFlag(spec, "uniqueTag", uniqueTag); + spec.configure("uniqueTag", uniqueTag); } result = EntityAdjuncts.addAdjunct(entity, spec); } else { diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java index 4b733af65a..d3882c1943 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java @@ -457,14 +457,17 @@ public class BrooklynJacksonSerializerTest { Asserts.assertEquals(tt2.toString(), "java.util.List<java.util.List<java.lang.String>>"); } + // also see DslSerializationTest.ObjectWithWrappedValueSpec static class SpecHolder { String label; EntitySpec<?> spec; WrappedValue<EntitySpec<?>> specT; WrappedValue<Object> specU; } + @Test public void testEntitySpecSerialization() throws JsonProcessingException { + // also see DslSerializationTest.ObjectWithWrappedValueSpec ObjectMapper mapperY = BeanWithTypeUtils.newYamlMapper(null, true, null, true); ObjectMapper mapperJ = BeanWithTypeUtils.newMapper(null, true, null, true); @@ -473,8 +476,12 @@ public class BrooklynJacksonSerializerTest { "specT:", " $brooklyn:entitySpec:", " type: "+ TestEntity.class.getName())); - // above creates a map because the DSL isn't recognised with non-management mapper - Asserts.assertInstanceOf(in.specT.get(), Map.class); + +// Asserts.assertInstanceOf(in.specT.get(), Map.class); + // used to do above, create a map because the DSL isn't recognised with non-management mapper; now it sets it as a flag + Asserts.assertInstanceOf(in.specT.get(), EntitySpec.class); + Asserts.assertNull(in.specT.get().getType()); + Asserts.assertEquals(in.specT.get().getFlags().get("$brooklyn:entitySpec"), MutableMap.of("type", TestEntity.class.getName())); String out; SpecHolder in2; @@ -495,19 +502,25 @@ public class BrooklynJacksonSerializerTest { in.specU = WrappedValue.of( EntitySpec.create(TestEntity.class) ); out = mapperJ.writerFor(Object.class).writeValueAsString(in); // not strongly typed typed - type is contained, but we get a map which still we need to coerce - Asserts.assertStringContains(out, EntitySpec.class.getName()); + Asserts.assertStringContains(out, "\"(type)\":\""+EntitySpec.class.getName()+"\""); + Asserts.assertStringContains(out, "\"type\":\""+TestEntity.class.getName()+"\""); + in2 = mapperJ.readerFor(SpecHolder.class).readValue(out); - // and we get a map - Map map; - map = (Map) in2.specU.get(); - Asserts.assertEquals( TypeCoercions.coerce(map, EntitySpec.class).getType(), TestEntity.class); + // and we used to get a map +// Map map = (Map) in2.specU.get(); +// Asserts.assertEquals( TypeCoercions.coerce(map, EntitySpec.class).getType(), TestEntity.class); + // but now the type is populated + Asserts.assertEquals( ((EntitySpec)in2.specU.get()).getType(), TestEntity.class); + // and same with yaml out = mapperY.writerFor(Object.class).writeValueAsString(in); - Asserts.assertStringContains(out, EntitySpec.class.getName()); +// Asserts.assertStringContains(out, "\"(type)\":\""+EntitySpec.class.getName()+"\""); + Asserts.assertStringContains(out, "!<"+EntitySpec.class.getName()+">"); + Asserts.assertStringContains(out, "type: "+TestEntity.class.getName()); in2 = mapperY.readerFor(SpecHolder.class).readValue(out); - map = (Map) in2.specU.get(); - Asserts.assertEquals( TypeCoercions.coerce(map, EntitySpec.class).getType(), TestEntity.class); - +// map = (Map) in2.specU.get(); +// Asserts.assertEquals( TypeCoercions.coerce(map, EntitySpec.class).getType(), TestEntity.class); + Asserts.assertEquals( ((EntitySpec)in2.specU.get()).getType(), TestEntity.class); ManagementContext mgmt = LocalManagementContextForTests.newInstance(); try { @@ -515,7 +528,10 @@ public class BrooklynJacksonSerializerTest { BiConsumer<List<SpecHolder>,Boolean> test = (inL, expectMap) -> { SpecHolder inX = inL.iterator().next(); - Asserts.assertEquals(inX.specT.get().getType(), TestEntity.class); + if (!expectMap) { + Asserts.assertEquals(inX.specT.get().getType(), TestEntity.class); + } + // management mappers don't have the map artifice (unless type is unknown and doing deep conversion) Object sp = inX.specU.get(); if (Boolean.TRUE.equals(expectMap)) Asserts.assertInstanceOf(sp, Map.class); @@ -529,11 +545,24 @@ public class BrooklynJacksonSerializerTest { // and in a list, deep and shallow conversion both work test.accept( BeanWithTypeUtils.convertDeeply(mgmt, Arrays.asList(in), new TypeToken<List<SpecHolder>>() {}, true, null, true), - /* deep conversion serializes and for untyped key it doesn't know the type to deserialize, until it is explicitly coerced, so we get a map */ true ); +// /* deep conversion serializes and for untyped key it doesn't know the type to deserialize, until it is explicitly coerced, so we get a map */ true + false /* now it does know the type */ + ); test.accept( BeanWithTypeUtils.convertShallow(mgmt, Arrays.asList(in), new TypeToken<List<SpecHolder>>() {}, true, null, true), /* shallow conversion should preserve the object */ false ); test.accept( BeanWithTypeUtils.convert(mgmt, Arrays.asList(in), new TypeToken<List<SpecHolder>>() {}, true, null, true), - /* overall convert tries deep first */ true ); +// /* overall convert tries deep first */ true + false + ); + + // but if it's a wrapped _object_ then we get a map back + List<Map<String, Map<String, String>>> in3 = Arrays.asList(MutableMap.of("specU", MutableMap.of("type", TestEntity.class.getName()))); + test.accept( BeanWithTypeUtils.convertDeeply(mgmt, in3, new TypeToken<List<SpecHolder>>() {}, true, null, true), + true); + test.accept( BeanWithTypeUtils.convertShallow(mgmt, in3, new TypeToken<List<SpecHolder>>() {}, true, null, true), + true); + test.accept( BeanWithTypeUtils.convert(mgmt, in3, new TypeToken<List<SpecHolder>>() {}, true, null, true), + true); } finally { Entities.destroyAll(mgmt); }
