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 09b005b2dc1ac3dc32342a145a6c9b4ec7028b0e Author: Alex Heneveld <[email protected]> AuthorDate: Mon Nov 7 14:31:27 2022 +0000 improve resolution of `type` key as either a field or the type to create --- .../core/effector/AddSensorInitializer.java | 9 +++ .../resolve/jackson/AsPropertyIfAmbiguous.java | 71 +++++++++++++++++++--- .../brooklyn/core/workflow/WorkflowSensor.java | 4 +- ...klynRegisteredTypeJacksonSerializationTest.java | 64 +++++++++++++++++-- .../core/test/BrooklynAppUnitTestSupport.java | 10 ++- 5 files changed, 141 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/brooklyn/core/effector/AddSensorInitializer.java b/core/src/main/java/org/apache/brooklyn/core/effector/AddSensorInitializer.java index c0f2df43b8..d56bbfcd3a 100644 --- a/core/src/main/java/org/apache/brooklyn/core/effector/AddSensorInitializer.java +++ b/core/src/main/java/org/apache/brooklyn/core/effector/AddSensorInitializer.java @@ -18,6 +18,8 @@ */ package org.apache.brooklyn.core.effector; +import com.fasterxml.jackson.annotation.JsonIgnore; +import com.fasterxml.jackson.annotation.JsonProperty; import com.google.common.annotations.Beta; import com.google.common.base.Preconditions; import com.google.common.reflect.TypeToken; @@ -89,7 +91,14 @@ public class AddSensorInitializer<T> extends EntityInitializers.InitializerPatte // kept for backwards deserialization compatibility private String name; private Duration period; + + @JsonIgnore // handle legacy deserialization carefully; this allows the property to be passed in but doesn't trigger requiring @type as key private String type; + @JsonProperty("type") + private void setType(String type) { + this.type = type; + } + private AttributeSensor<T> sensor; private ConfigBag params; // introduced in 1.1 for legacy compatibility 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 819721baba..74fcc2dcbb 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 @@ -21,6 +21,7 @@ package org.apache.brooklyn.core.resolve.jackson; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonTypeInfo.As; import com.fasterxml.jackson.core.JsonGenerator; +import com.fasterxml.jackson.core.JsonLocation; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonToken; import com.fasterxml.jackson.core.type.WritableTypeId; @@ -33,21 +34,28 @@ 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.util.collections.MutableSet; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.javalang.Reflections; import org.apache.brooklyn.util.text.Strings; import org.apache.commons.lang3.tuple.Pair; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.lang.reflect.AccessibleObject; import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; public class AsPropertyIfAmbiguous { + private static final Logger LOG = LoggerFactory.getLogger(AsPropertyIfAmbiguous.class); + private static Set<String> warnedAmbiguousTypeProperty = MutableSet.of(); + public static final String CONFLICTING_TYPE_NAME_PROPERTY_PREFIX = "@"; public interface HasBaseType { @@ -160,6 +168,12 @@ public class AsPropertyIfAmbiguous { return new AsPropertyButNotIfFieldConflictTypeDeserializer(_baseType, _idResolver, newTypePropertyName, _typeIdVisible, _defaultImpl, _inclusion); } + protected boolean hasTypePropertyNameAsField(JavaType type) { + // object has field with same name as the type property - don't treat the type property supplied here as the type + return presentAndNotJsonIgnored(Reflections.findFieldMaybe(type.getRawClass(), _typePropertyName)) + || // or object has getter with same name as the type property + presentAndNotJsonIgnored(Reflections.findMethodMaybe(type.getRawClass(), "get" + Strings.toInitialCapOnly(_typePropertyName))); + } @Override public Object deserializeTypedFromObject(JsonParser p, DeserializationContext ctxt) throws IOException { AsPropertyButNotIfFieldConflictTypeDeserializer target = this; @@ -168,11 +182,7 @@ public class AsPropertyIfAmbiguous { if (_idResolver instanceof HasBaseType) { JavaType baseType = ((HasBaseType) _idResolver).getBaseType(); if (baseType != null ) { - if (// object has field with same name as the type property - don't treat the type property supplied here as the type - presentAndNotJsonIgnored(Reflections.findFieldMaybe(baseType.getRawClass(), _typePropertyName)) - || // or object has getter with same name as the type property - presentAndNotJsonIgnored(Reflections.findMethodMaybe(baseType.getRawClass(), "get" + Strings.toInitialCapOnly(_typePropertyName))) - ) { + if (hasTypePropertyNameAsField(baseType)) { // look for an '@' type // return cloneWithNewTypePropertyName(CONFLICTING_TYPE_NAME_PROPERTY_PREFIX + _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 @@ -246,17 +256,57 @@ public class AsPropertyIfAmbiguous { private Pair<String,TokenBuffer> findTypeIdOrUnambiguous(JsonParser p, DeserializationContext ctxt, JsonToken t, TokenBuffer tb, boolean ignoreCase, boolean mustUseConflictingTypePrefix) throws IOException { String typeUnambiguous = CONFLICTING_TYPE_NAME_PROPERTY_PREFIX + _typePropertyName; + JsonLocation loc = p.currentTokenLocation(); //p.getCurrentLocation(); for (; t == JsonToken.FIELD_NAME; t = p.nextToken()) { final String name = p.currentName(); p.nextToken(); // to point to the value - // require unambiguous one is first if present; otherwise maintaining the parser and token buffer in the desired states is too hard - if (name.equals(typeUnambiguous) || (!mustUseConflictingTypePrefix && (name.equals(_typePropertyName) - || (ignoreCase && name.equalsIgnoreCase(_typePropertyName))))) { // gotcha! + // unambiguous property should precede ambiguous property name in cases where property name is required + // maintaining the parser and token buffer in the desired states to allow either anywhere is too hard + boolean unambiguousName = name.equals(typeUnambiguous); + boolean ambiguousName = !unambiguousName && (!mustUseConflictingTypePrefix && (name.equals(_typePropertyName) + || (ignoreCase && name.equalsIgnoreCase(_typePropertyName)))); + if (ambiguousName || unambiguousName) { // gotcha! // 09-Sep-2021, tatu: [databind#3271]: Avoid converting null to "null" String typeId = p.getValueAsString(); if (typeId != null) { - return Pair.of(typeId, tb); + boolean disallowed = false; + if (ambiguousName) { + JavaType tt = _idResolver.typeFromId(ctxt, typeId); + if (tt!=null && hasTypePropertyNameAsField(tt)) { + // if there is a property called 'type' then caller should use @type. + disallowed = true; + // unless we need a type to conform to coercion. + if (_idResolver instanceof HasBaseType) { + JavaType baseType = ((HasBaseType) _idResolver).getBaseType(); + if (baseType==null || baseType.getRawClass().equals(Object.class)) { + if (loc.getCharOffset()<=1) { + // 'type' should be treated as a normal key when an object is expected, if type it references has a field 'type', + // except if it is the first key in the definition, to facilitate messy places where we say 'type: xxx' as the definition + if (warnedAmbiguousTypeProperty.add(typeId)) { + LOG.warn("Ambiguous type property '" + _typePropertyName + "' used for '" + typeId + "' as first entry in definition; this looks like a type specification but this could also refer to the property; " + + "using for the former, but specification should have used '" + CONFLICTING_TYPE_NAME_PROPERTY_PREFIX + _typePropertyName + "' as key earlier in the map, " + + "or if setting the field is intended put an explicit '" + CONFLICTING_TYPE_NAME_PROPERTY_PREFIX + _typePropertyName + "' before it"); + } + disallowed = false; + } else { + // leave disallowed + } + } else if (baseType.isMapLikeType()) { + // leave disalloed + } else { + if (warnedAmbiguousTypeProperty.add(typeId)) { + LOG.warn("Ambiguous type property '" + _typePropertyName + "' used for '" + typeId + "'; a type specification is needed to comply with expectations, but this could also refer to the property; " + + "using for the former, but specification should have used " + CONFLICTING_TYPE_NAME_PROPERTY_PREFIX + _typePropertyName + " as key earlier in the map"); + } + disallowed = false; + } + } + } + } + if (!disallowed) { + return Pair.of(typeId, tb); + } } } if (tb == null) { @@ -264,6 +314,9 @@ public class AsPropertyIfAmbiguous { } tb.writeFieldName(name); tb.copyCurrentStructure(p); + + // advance so we no longer think we are at the beginning + loc = p.getCurrentLocation(); } return Pair.of(null, tb); } diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowSensor.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowSensor.java index 8e69f5ecdd..d70e0d5f26 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowSensor.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowSensor.java @@ -108,7 +108,8 @@ public final class WorkflowSensor<T> extends AbstractAddTriggerableSensor<T> imp protected AttributeSensor<T> sensor(Entity entity) { // overridden because sensor type defaults to object here; and we support 'sensor' EntityValueToSet s = lookupSensorReference(entity); - TypeToken<T> clazz = getType(entity, s.type, s.name); + // could assert compatibility with defined sensor, or tighten; but currently we don't + TypeToken<T> clazz = s.type == null ? (TypeToken)TypeToken.of(Object.class) : getType(entity, s.type, s.name); return Sensors.newSensor(clazz, s.name); } @@ -129,6 +130,7 @@ public final class WorkflowSensor<T> extends AbstractAddTriggerableSensor<T> imp if (s.entity!=null && !s.entity.equals(entity)) throw new IllegalArgumentException("Not permitted to specify different entity ("+s.entity+") for workflow-sensor on "+entity); String t2 = initParam(SENSOR_TYPE); + if (Object.class.getName().equals(t2)) t2 = null; if (s.type!=null && t2!=null && !t2.equals(s.type)) throw new IllegalArgumentException("Incompatible types "+s.type+" and "+t2); if (t2!=null) s.type = t2; diff --git a/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java b/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java index 30ea7877af..55d076014e 100644 --- a/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java +++ b/core/src/test/java/org/apache/brooklyn/core/resolve/jackson/BrooklynRegisteredTypeJacksonSerializationTest.java @@ -30,6 +30,7 @@ import org.apache.brooklyn.core.typereg.BasicBrooklynTypeRegistry; import org.apache.brooklyn.core.typereg.BasicTypeImplementationPlan; import org.apache.brooklyn.core.typereg.JavaClassNameTypePlanTransformer; import org.apache.brooklyn.core.typereg.RegisteredTypes; +import org.apache.brooklyn.core.workflow.WorkflowSensor; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.core.config.ConfigBag; @@ -37,6 +38,8 @@ import org.apache.brooklyn.util.time.Duration; import org.testng.Assert; import org.testng.annotations.Test; +import java.util.Map; + public class BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmtUnitTestSupport implements MapperTestFixture { // public because of use of JavaClassNameTypePlanTransformer @@ -77,8 +80,7 @@ public class BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmt @Test public void testDeserializeAlias() throws Exception { - BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt,"sample", "1", - new BasicTypeImplementationPlan(JavaClassNameTypePlanTransformer.FORMAT, SampleBean.class.getName())); + BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt,"sample", "1", SampleBean.class); Object impl = deser("{\"type\":\"sample\",\"x\":\"hello\"}"); Asserts.assertInstanceOf(impl, SampleBean.class); @@ -113,10 +115,7 @@ public class BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmt @Test public void testExtendedListBeanRegisteredType() throws Exception { - RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "list-extended", "1", - new BasicTypeImplementationPlan(BeanWithTypeUtils.FORMAT, - "type: " + ListExtended.class.getName() - )); + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "list-extended", "1", ListExtended.class); Object impl = mgmt().getTypeRegistry().create(rt, null, null); Asserts.assertInstanceOf(impl, ListExtended.class); @@ -141,6 +140,59 @@ public class BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmt Asserts.assertInstanceOf(impl, EntityInitializer.class); } + public static class SampleBeanWithType extends SampleBean { + public String type; + } + + @Test + public void testDeserializeEntityInitializerWithTypeField() throws Exception { + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class); + Object impl = deser("{\"type\":\""+ WorkflowSensor.class.getName()+"\"" + +",\"brooklyn.config\":{\"sensor\":{\"name\":\"mytestsensor\",\"type\":\"samplebean-with-type\"}}" + +"}"); + Asserts.assertInstanceOf(impl, WorkflowSensor.class); + } + + @Test + public void testDeserializeEntityInitializerWithTypeFieldNeededForDeserialization() throws Exception { + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class); + // 'type' field used to give type because the expected type does not expect 'type' + Object impl = deser("{\"x\":\"mytestsensor\",\"type\":\"samplebean-with-type\"}", SampleBean.class); + Asserts.assertInstanceOf(impl, SampleBeanWithType.class); + Asserts.assertEquals( ((SampleBean)impl).x, "mytestsensor"); + Asserts.assertEquals( ((SampleBeanWithType)impl).type, null); + } + + @Test + public void testDeserializeEntityInitializerWithTypeFieldUsedForDeserialization() throws Exception { + // 'type' field used to give type because it is the very first thing in an object, e.g. in a catalog definition + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class); + Object impl = deser("{\"type\":\"samplebean-with-type\",\"x\":\"mytestsensor\"}", Object.class); + Asserts.assertInstanceOf(impl, SampleBeanWithType.class); + Asserts.assertEquals( ((SampleBean)impl).x, "mytestsensor"); + Asserts.assertEquals( ((SampleBeanWithType)impl).type, null); + } + + @Test + public void testDeserializeEntityInitializerWithTypeFieldNotUsedForDeserialization() throws Exception { + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class); + Object impl = deser("{\"x\":\"mytestsensor\",\"type\":\"samplebean-with-type\"}", Object.class); + // above cases don't apply, we get a map with both set as fields + Asserts.assertInstanceOf(impl, Map.class); + Asserts.assertEquals( ((Map)impl).get("x"), "mytestsensor"); + Asserts.assertEquals( ((Map)impl).get("type"), "samplebean-with-type"); + } + + @Test + public void testDeserializeEntityInitializerWithUnambiguousTypeFieldUsedForDeserialization() throws Exception { + RegisteredType rt = BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "samplebean-with-type", "1", SampleBeanWithType.class); + Object impl = deser("{\"x\":\"mytestsensor\",\"@type\":\"samplebean-with-type\",\"type\":\"other\"}", Object.class); + // above cases don't apply, we get a map with both set as fields + Asserts.assertInstanceOf(impl, SampleBeanWithType.class); + Asserts.assertEquals( ((SampleBean)impl).x, "mytestsensor"); + Asserts.assertEquals( ((SampleBeanWithType)impl).type, "other"); + } + @Test public void testConfigBagSerialization() throws Exception { ConfigBag bag = ConfigBag.newInstance(); diff --git a/core/src/test/java/org/apache/brooklyn/core/test/BrooklynAppUnitTestSupport.java b/core/src/test/java/org/apache/brooklyn/core/test/BrooklynAppUnitTestSupport.java index 27514a2d7f..040250696a 100644 --- a/core/src/test/java/org/apache/brooklyn/core/test/BrooklynAppUnitTestSupport.java +++ b/core/src/test/java/org/apache/brooklyn/core/test/BrooklynAppUnitTestSupport.java @@ -22,6 +22,8 @@ import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.core.entity.BrooklynConfigKeys; +import org.apache.brooklyn.core.resolve.jackson.BeanWithTypeUtils; +import org.apache.brooklyn.core.resolve.jackson.BrooklynRegisteredTypeJacksonSerializationTest; import org.apache.brooklyn.core.test.entity.TestApplication; import org.apache.brooklyn.core.typereg.BasicTypeImplementationPlan; import org.apache.brooklyn.core.typereg.JavaClassNameTypePlanTransformer; @@ -61,7 +63,6 @@ public class BrooklynAppUnitTestSupport extends BrooklynMgmtUnitTestSupport { app = mgmt.getEntityManager().createEntity(appSpec); } - @SuppressWarnings("deprecation") public static RegisteredType addRegisteredTypeBean(ManagementContext mgmt, String symName, String version, RegisteredType.TypeImplementationPlan plan) { RegisteredType rt = RegisteredTypes.bean(symName, version, plan); Collection<Throwable> errors = mgmt.getCatalog().validateType(rt, null, true); @@ -69,4 +70,11 @@ public class BrooklynAppUnitTestSupport extends BrooklynMgmtUnitTestSupport { // the resolved type is added, not necessarily type above which will be unresolved return mgmt.getTypeRegistry().get(symName, version); } + + public static RegisteredType addRegisteredTypeBean(ManagementContext mgmt, String symName, String version, Class<?> clazz) { + return addRegisteredTypeBean(mgmt, symName, version, + new BasicTypeImplementationPlan(BeanWithTypeUtils.FORMAT, "type: " + clazz.getName())); + // should be identical: + //new BasicTypeImplementationPlan(JavaClassNameTypePlanTransformer.FORMAT, clazz.getName())); + } }
