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
The following commit(s) were added to refs/heads/master by this push:
new 6401e02159 allow maps expected containing a type key
6401e02159 is described below
commit 6401e02159aae9737bb80cece4f09010b0ce9ffb
Author: Alex Heneveld <[email protected]>
AuthorDate: Fri Aug 11 17:50:31 2023 +0100
allow maps expected containing a type key
previously it would instantiate the type;
also adds a static option to permit object expected to allow that (but
disabled so we get helpful error messages)
---
.../resolve/jackson/AsPropertyIfAmbiguous.java | 40 +++++++-
.../util/core/xstream/SafeThrowableConverter.java | 3 -
.../brooklyn/util/core/xstream/XmlSerializer.java | 5 +-
...klynRegisteredTypeJacksonSerializationTest.java | 101 ++++++++++++++++++++-
4 files changed, 134 insertions(+), 15 deletions(-)
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 94405ce681..6824985ef2 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,7 +21,6 @@ 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;
@@ -45,7 +44,6 @@ 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;
@@ -68,6 +66,11 @@ public class AsPropertyIfAmbiguous {
/** @deprecated since 1.1 now use transform fn, and prefer wrapped in
parens */
public static final String CONFLICTING_TYPE_NAME_PROPERTY_PREFIX = "@";
+ // we can change this to false to allow e.g. deserialize "{type: unknown}"
as a map when an object is expected;
+ // however it is probably more useful to return that as an error, because
usually it is an error,
+ // and have a special way of permitting it in places
+ public static final boolean
THROW_ON_OBJECT_EXPECTED_AND_INVALID_TYPE_KEY_SUPPLIED = true;
+
public interface HasBaseType {
JavaType getBaseType();
}
@@ -310,6 +313,12 @@ public class AsPropertyIfAmbiguous {
}
private DiscoveredTypeAndCachedTokenBuffer
findTypeIdOrUnambiguous(JsonParser p, DeserializationContext ctxt, JsonToken t,
TokenBuffer tb, boolean ignoreCase, boolean mustUseConflictingTypePrefix)
throws IOException {
+ if (baseType()!=null &&
Map.class.isAssignableFrom(baseType().getRawClass())) {
+ // if a map is expected, don't try to do fancy type lookup;
+ // we ignore subclasses of maps anyway (eg see test for
FancyMap)
+ return new DiscoveredTypeAndCachedTokenBuffer(null, tb, false);
+ }
+
String typeUnambiguous1 =
CONFLICTING_TYPE_NAME_PROPERTY_TRANSFORM.apply(_typePropertyName);
String typeUnambiguous2 =
CONFLICTING_TYPE_NAME_PROPERTY_TRANSFORM_ALT.apply(_typePropertyName);
@@ -328,9 +337,30 @@ public class AsPropertyIfAmbiguous {
String typeId = p.getValueAsString();
if (typeId != null) {
boolean disallowed = false;
- if (ambiguousName) {
- JavaType tt = _idResolver.typeFromId(ctxt, typeId);
- if
(BrooklynObject.class.isAssignableFrom(tt.getRawClass()) &&
!Feed.class.isAssignableFrom(tt.getRawClass())) {
+
+ JavaType tt = null;
+ try {
+ tt = _idResolver.typeFromId(ctxt, typeId);
+ } catch (Exception e) {
+ Exceptions.propagateIfInterrupt(e);
+ if
(!THROW_ON_OBJECT_EXPECTED_AND_INVALID_TYPE_KEY_SUPPLIED && (baseType()==null
|| hasTypePropertyNameAsField(baseType()) ||
baseType().getRawClass().isAssignableFrom(Map.class))) {
+ // if we allow an object with a type key here,
don't throw
+
+ // 2023-08 previously we would throw here if
it was an ambiguous name; if an unambiguous name we didn't check the type,
+ // and it would throw later, but throwing now
is fine (if we want to throw)
+
+ } else {
+ throw Exceptions.propagate(e);
+ }
+ // incompatible type, or type not found; just
ignore for now
+ // (although the error might be useful)
+ tt = null;
+ disallowed = true;
+ }
+
+ if (ambiguousName && !disallowed) {
+
+ if (tt!=null &&
BrooklynObject.class.isAssignableFrom(tt.getRawClass()) &&
!Feed.class.isAssignableFrom(tt.getRawClass())) {
Boolean wantsSpec = null;
Boolean wantsBO = null;
diff --git
a/core/src/main/java/org/apache/brooklyn/util/core/xstream/SafeThrowableConverter.java
b/core/src/main/java/org/apache/brooklyn/util/core/xstream/SafeThrowableConverter.java
index fc3c84cad8..9097b15869 100644
---
a/core/src/main/java/org/apache/brooklyn/util/core/xstream/SafeThrowableConverter.java
+++
b/core/src/main/java/org/apache/brooklyn/util/core/xstream/SafeThrowableConverter.java
@@ -23,7 +23,6 @@ import com.thoughtworks.xstream.converters.Converter;
import com.thoughtworks.xstream.converters.MarshallingContext;
import com.thoughtworks.xstream.converters.UnmarshallingContext;
import com.thoughtworks.xstream.core.DefaultConverterLookup;
-import com.thoughtworks.xstream.core.util.QuickWriter;
import com.thoughtworks.xstream.io.HierarchicalStreamReader;
import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
import com.thoughtworks.xstream.io.path.PathTrackingWriter;
@@ -33,8 +32,6 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
-import java.net.Inet4Address;
-import java.net.UnknownHostException;
import java.util.function.Predicate;
/** This is a hacky way to try to recover in some non-serializable situations.
But it can and often does
diff --git
a/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
b/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
index 2484a2c6a3..d7b19cb27f 100644
---
a/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
+++
b/core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java
@@ -27,9 +27,7 @@ import com.google.common.collect.Maps;
import com.thoughtworks.xstream.XStream;
import com.thoughtworks.xstream.XStreamException;
import com.thoughtworks.xstream.converters.extended.JavaClassConverter;
-import com.thoughtworks.xstream.core.ClassLoaderReference;
import com.thoughtworks.xstream.core.DefaultConverterLookup;
-import com.thoughtworks.xstream.core.util.CompositeClassLoader;
import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
import com.thoughtworks.xstream.io.naming.NameCoder;
import com.thoughtworks.xstream.io.path.PathTracker;
@@ -89,7 +87,8 @@ public class XmlSerializer<T> {
converterLookup = new DefaultConverterLookup();
- xstream = new XStream(null, hierarchicalStreamDriver, new
ClassLoaderReference(new CompositeClassLoader()), (Mapper)null,
+ XStream xs1 = new XStream(); // use this to get the class loader
because its package isn't exposed
+ xstream = new XStream(null, hierarchicalStreamDriver,
xs1.getClassLoaderReference(), (Mapper)null,
type -> converterLookup.lookupConverterForType(type),
(converter,priority) ->
converterLookup.registerConverter(converter, priority)
) {
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 840371893b..8b5c03188b 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
@@ -27,10 +27,7 @@ import org.apache.brooklyn.core.config.ConfigKeys;
import org.apache.brooklyn.core.sensor.StaticSensor;
import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport;
import org.apache.brooklyn.core.test.BrooklynMgmtUnitTestSupport;
-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;
@@ -42,7 +39,9 @@ import org.apache.brooklyn.util.time.Duration;
import org.testng.Assert;
import org.testng.annotations.Test;
+import java.util.LinkedHashMap;
import java.util.Map;
+import java.util.Objects;
public class BrooklynRegisteredTypeJacksonSerializationTest extends
BrooklynMgmtUnitTestSupport implements MapperTestFixture {
@@ -76,7 +75,12 @@ public class BrooklynRegisteredTypeJacksonSerializationTest
extends BrooklynMgmt
public void testDeserializeUnknownTypeFails() throws
JsonProcessingException {
try {
Object x = BeanWithTypeUtils.newYamlMapper(mgmt, true, null,
true).readValue("type: DeliberatelyMissing", Object.class);
- Asserts.shouldHaveFailedPreviously("Should have failed due to
unknown type; instead got "+x);
+ if
(AsPropertyIfAmbiguous.THROW_ON_OBJECT_EXPECTED_AND_INVALID_TYPE_KEY_SUPPLIED) {
+ Asserts.shouldHaveFailedPreviously("Should have failed due to
unknown type; instead got " + x);
+ } else {
+ Asserts.assertInstanceOf(x, Map.class);
+ Asserts.assertSize((Map)x, 1);
+ }
} catch (Exception e) {
Asserts.expectedFailureContains(e, "DeliberatelyMissing");
}
@@ -232,6 +236,95 @@ public class
BrooklynRegisteredTypeJacksonSerializationTest extends BrooklynMgmt
Asserts.assertEquals(redeserObj.x, "hello");
}
+ static class FancyHolder {
+ Map map;
+ Object obj;
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ FancyHolder mapHolder = (FancyHolder) o;
+ return Objects.equals(map, mapHolder.map) && Objects.equals(obj,
mapHolder.obj);
+ }
+ }
+
+ static class FancyMap extends LinkedHashMap {
+ }
+
+ @Test
+ public void testSerializeAndDesMapWithTypeEntry() throws Exception {
+ // if a map is _expected_ at least we should ignore the field type
+ FancyHolder a = new FancyHolder();
+ a.map = MutableMap.of("type", "not_a_type");
+
+ {
+ String expectedSerialization =
"{\"map\":{\"type\":\"not_a_type\"}}";
+ Assert.assertEquals(ser(a, FancyHolder.class),
expectedSerialization);
+ Assert.assertEquals(deser(expectedSerialization,
FancyHolder.class), a);
+ }
+
+ {
+ FancyMap f = new FancyMap();
+ f.put("a",1);
+ // types ignored when serializing any subclass of map; we could
change if we want (but make MutableMap the default)
+ Assert.assertEquals(ser(f), "{\"a\":1}");
+ }
+
+ {
+ String expectedDeserialization = "{\"type\":\""+
BrooklynRegisteredTypeJacksonSerializationTest.FancyHolder.class.getName()
+"\"," +
+ "\"map\":{\"type\":\"not_a_type\"}}";
+ Assert.assertEquals(ser(a), expectedDeserialization);
+ Assert.assertEquals(deser(expectedDeserialization), a);
+ }
+
+ a.map = null;
+ a.obj = MutableMap.of("type",
BrooklynRegisteredTypeJacksonSerializationTest.FancyHolder.class.getName());
+ {
+ // it is expected that a map containing a type comes back as an
instance of that type if possible
+ String expectedSerialization = "{\"obj\":{\"type\":\"" +
BrooklynRegisteredTypeJacksonSerializationTest.FancyHolder.class.getName() +
"\"}}";
+ Assert.assertEquals(ser(a, FancyHolder.class),
expectedSerialization);
+ Asserts.assertThat(deser(expectedSerialization,
FancyHolder.class).obj, r -> r instanceof FancyHolder);
+
+ // an unknown field should undo that
+ ((Map) a.obj).put("unfield", "should_force_map");
+ Asserts.assertThat(deser(ser(a, FancyHolder.class),
FancyHolder.class), r -> {
+ Asserts.assertInstanceOf(r.obj, Map.class);
+ Asserts.assertEquals(((Map) r.obj).get("type"),
BrooklynRegisteredTypeJacksonSerializationTest.FancyHolder.class.getName());
+ Asserts.assertEquals(((Map) r.obj).get("unfield"),
"should_force_map");
+ return true;
+ });
+ }
+
+ if
(!AsPropertyIfAmbiguous.THROW_ON_OBJECT_EXPECTED_AND_INVALID_TYPE_KEY_SUPPLIED)
{
+ // all the below will throw if the above is true;
+ // see other tests that reference the constant above
+
+ // and an unknown field also forces a map
+ ((Map) a.obj).put("type", "not_a_type");
+ Asserts.assertThat(deser(ser(a, FancyHolder.class),
FancyHolder.class), r -> {
+ Asserts.assertInstanceOf(r.obj, Map.class);
+ Asserts.assertEquals(((Map) r.obj).get("type"), "not_a_type");
+ Asserts.assertEquals(((Map) r.obj).get("unfield"),
"should_force_map");
+ return true;
+ });
+ // also with unambiguous name
+ ((Map) a.obj).put("@type", "not_a_type");
+ Asserts.assertThat(deser(ser(a, FancyHolder.class),
FancyHolder.class), r -> {
+ Asserts.assertInstanceOf(r.obj, Map.class);
+ Asserts.assertEquals(((Map) r.obj).get("type"), "not_a_type");
+ Asserts.assertEquals(((Map) r.obj).get("unfield"),
"should_force_map");
+ return true;
+ });
+
+ // what if it's an Object where we don't know the type? probably
we should throw, we always have in the past, but we could allow
+ Map b = MutableMap.of("type", "not_a_type");
+ Asserts.assertEquals(ser(b), "{\"type\":\"not_a_type\"}");
+ Asserts.assertEquals(deser(ser(b)), MutableMap.of("type",
"not_a_type"));
+ }
+
+ }
+
@Test
public void testDeserializeEntityInitializerWithTypeField() throws
Exception {
BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt,
"samplebean-with-type", "1", SampleBeanWithType.class);
