respect generics better when coercing previous commit used yaml parse even for things like List<String>; now do string-list parse in that case. means the json adapters have access to the type token, but that means running them earlier and ensuring we do a re-coercion if needed.
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/3e57b14b Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/3e57b14b Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/3e57b14b Branch: refs/heads/master Commit: 3e57b14b220bd7a994a9143d83bc123879086aff Parents: 8d081d0 Author: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Authored: Fri Aug 31 10:28:57 2018 +0100 Committer: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Committed: Fri Aug 31 10:28:57 2018 +0100 ---------------------------------------------------------------------- .../brooklyn/util/core/flags/TypeCoercions.java | 4 +- .../util/core/internal/TypeCoercionsTest.java | 11 ++ .../brooklyn/util/javalang/Reflections.java | 12 ++ .../util/javalang/StackTraceSimplifier.java | 1 + .../coerce/CommonAdaptorTypeCoercions.java | 121 ++++++++++++------- .../javalang/coerce/TypeCoercerExtensible.java | 38 +++++- .../util/javalang/coerce/TypeCoercionsTest.java | 35 ------ .../brooklyn/util/text/StringEscapesTest.java | 4 +- 8 files changed, 142 insertions(+), 84 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/core/src/main/java/org/apache/brooklyn/util/core/flags/TypeCoercions.java ---------------------------------------------------------------------- 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 3b3f874..84b3b10 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 @@ -290,7 +290,9 @@ public class TypeCoercions { } Maybe<Object> result = Reflections.invokeConstructorFromArgs(clazz); if (result.isPresentAndNonNull() && supertype.isInstance(result.get())) { - return (T) result.get(); + @SuppressWarnings("unchecked") + T rT = (T) result.get(); + return rT; } else if (result.isPresent()) { throw new IllegalStateException("Object is not a " + supertype.getSimpleName()+": " + result.get()); } else { http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java ---------------------------------------------------------------------- diff --git a/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java b/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java index b6a20e5..a88effc 100644 --- a/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java +++ b/core/src/test/java/org/apache/brooklyn/util/core/internal/TypeCoercionsTest.java @@ -35,6 +35,7 @@ import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; import org.apache.brooklyn.core.entity.lifecycle.Lifecycle; +import org.apache.brooklyn.util.collections.MutableMap; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.ClassLoaderUtils; import org.apache.brooklyn.util.core.flags.TypeCoercions; @@ -325,6 +326,16 @@ public class TypeCoercionsTest { Assert.assertEquals(s, ImmutableMap.of("a", "1", "b", 2)); } + @SuppressWarnings("serial") + @Test + public void testYamlMapsDontGoTooFarWhenWantingListOfString() { + List<?> s = TypeCoercions.coerce("[ a: 1, b: 2 ]", List.class); + Assert.assertEquals(s, ImmutableList.of(MutableMap.of("a", 1), MutableMap.of("b", 2))); + + s = TypeCoercions.coerce("[ a: 1, b : 2 ]", new TypeToken<List<String>>() {}); + Assert.assertEquals(s, ImmutableList.of("a: 1", "b : 2")); + } + @Test public void testURItoStringCoercion() { String s = TypeCoercions.coerce(URI.create("http://localhost:1234/";), String.class); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java index be6e9a4..8718d69 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/Reflections.java @@ -31,6 +31,7 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; +import java.lang.reflect.TypeVariable; import java.net.URL; import java.util.ArrayList; import java.util.Arrays; @@ -61,6 +62,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; +import com.google.common.reflect.TypeToken; /** * Reflection utilities @@ -1140,4 +1142,14 @@ public class Reflections { public static boolean trySetAccessible(Method method) { return MethodAccessibleReflections.trySetAccessible(method); } + + public static TypeToken<?>[] getGenericParameterTypeTokens(TypeToken<?> t) { + Class<?> rawType = t.getRawType(); + TypeVariable<?>[] pT = rawType.getTypeParameters(); + TypeToken<?> pTT[] = new TypeToken<?>[pT.length]; + for (int i=0; i<pT.length; i++) { + pTT[i] = t.resolveType(pT[i]); + } + return pTT; + } } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/StackTraceSimplifier.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/StackTraceSimplifier.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/StackTraceSimplifier.java index 6425fac..20b2141 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/StackTraceSimplifier.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/StackTraceSimplifier.java @@ -45,6 +45,7 @@ public class StackTraceSimplifier { System.getProperty(DEFAULT_BLACKLIST_SYSTEM_PROPERTY_NAME, "java.," + "javax.," + + "jdk.," + "sun.," + "groovy.," + "org.codehaus.groovy.," + http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java index 3c31bbc..0842f19 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/CommonAdaptorTypeCoercions.java @@ -34,10 +34,13 @@ import java.util.concurrent.atomic.AtomicLong; import javax.annotation.Nullable; +import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.collections.QuorumCheck; import org.apache.brooklyn.util.collections.QuorumCheck.QuorumChecks; 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.net.Cidr; import org.apache.brooklyn.util.net.Networking; import org.apache.brooklyn.util.net.UserAndHostAndPort; @@ -54,6 +57,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.common.net.HostAndPort; +import com.google.common.reflect.TypeToken; public class CommonAdaptorTypeCoercions { @@ -76,6 +80,10 @@ public class CommonAdaptorTypeCoercions { public synchronized <A,B> Function<? super A,B> registerAdapter(Class<A> sourceType, Class<B> targetType, Function<? super A,B> fn) { return coercer.registerAdapter(sourceType, targetType, fn); } + /** Registers an adapter for use with type coercion. */ + public synchronized void registerAdapter(TryCoercer coerceFn) { + coercer.registerAdapter(coerceFn); + } @SuppressWarnings("rawtypes") public void registerStandardAdapters() { @@ -363,61 +371,92 @@ public class CommonAdaptorTypeCoercions { }); } - @SuppressWarnings("rawtypes") public void registerCollectionJsonAdapters() { - registerAdapter(String.class, List.class, new Function<String,List>() { - @Override - public List<Object> apply(final String input) { - return JavaStringEscapes.tryUnwrapJsonishList(input).orNull(); - } - }); - registerAdapter(String.class, Set.class, new Function<String,Set>() { - @Override - public Set<Object> apply(final String input) { - List<Object> l = JavaStringEscapes.tryUnwrapJsonishList(input).orNull(); - if (l==null) return null; - return MutableSet.copyOf(l).asUnmodifiable(); + registerAdapter(new CoerceStringToCollections()); + } + + /** Does a rough coercion of the string to the indicated Collection or Map type. + * Only looks at generics enough to choose the right parser. + * Expects the caller {@link TypeCoercerExtensible} to recurse inside the collection/map. + */ + public static class CoerceStringToCollections implements TryCoercer { + @SuppressWarnings("unchecked") + @Override + public <T> Maybe<T> tryCoerce(Object input, TypeToken<T> type) { + if (!(input instanceof String)) return null; + String inputS = (String)input; + + Class<? super T> rawType = type.getRawType(); + + if (Collection.class.isAssignableFrom(rawType)) { + TypeToken<?> parameters[] = Reflections.getGenericParameterTypeTokens(type); + Maybe<?> resultM = null; + Collection<?> result = null; + if (parameters.length==1 && CharSequence.class.isAssignableFrom(parameters[0].getRawType())) { + // for list of strings, use special parse + result = JavaStringEscapes.unwrapJsonishListStringIfPossible(inputS); + } else { + // any other type, use YAMLish parse + resultM = JavaStringEscapes.tryUnwrapJsonishList(inputS); + result = (Collection<?>) resultM.orNull(); + } + if (result==null) { + if (resultM!=null) return Maybe.Absent.castAbsent(resultM); + return null; + } + if (rawType.isAssignableFrom(MutableList.class)) { + return Maybe.of((T) MutableList.copyOf(result).asUnmodifiable()); + } + if (rawType.isAssignableFrom(MutableSet.class)) { + return Maybe.of((T) MutableSet.copyOf(result).asUnmodifiable()); + } + if (rawType.isInstance(result)) { + return Maybe.of((T) result); + } + // the type is not a collection we can deal with + return null; } - }); - registerAdapter(String.class, Map.class, new Function<String,Map>() { - @Override - public Map apply(final String input) { - Exception error = null; + + if (Map.class.isAssignableFrom(rawType)) { - // first try wrapping in braces if needed - if (!input.trim().startsWith("{")) { + Function<String,Maybe<Map<?,?>>> parseYaml = (in) -> { try { - return apply("{ "+input+" }"); + return Maybe.of(Yamls.getAs( Yamls.parseAll(in), Map.class )); } catch (Exception e) { Exceptions.propagateIfFatal(e); - // prefer this error - error = e; - // fall back to parsing without braces, e.g. if it's multiline + return Maybe.absent(new IllegalArgumentException("Cannot parse string as map with flexible YAML parsing; "+ + (e instanceof ClassCastException ? "yaml treats it as a string" : + (e instanceof IllegalArgumentException && Strings.isNonEmpty(e.getMessage())) ? e.getMessage() : + ""+e) )); } + }; + + Maybe<Map<?, ?>> r1 = null; + + // first try wrapping in braces if needed + if (!inputS.trim().startsWith("{")) { + r1 = parseYaml.apply("{ "+inputS+" }"); + if (r1.isPresent()) return (Maybe<T>) r1; + // fall back to parsing without braces, e.g. if it's multiline } - - try { - return Yamls.getAs( Yamls.parseAll(input), Map.class ); - } catch (Exception e) { - Exceptions.propagateIfFatal(e); - if (error!=null && input.indexOf('\n')==-1) { - // prefer the original error if it wasn't braced and wasn't multiline - e = error; - } - throw new IllegalArgumentException("Cannot parse string as map with flexible YAML parsing; "+ - (e instanceof ClassCastException ? "yaml treats it as a string" : - (e instanceof IllegalArgumentException && Strings.isNonEmpty(e.getMessage())) ? e.getMessage() : - ""+e) ); - } + + Maybe<Map<?, ?>> r2 = parseYaml.apply(inputS); + if (r2.isPresent()) return (Maybe<T>) r2; + + // absent - prefer the first error if it wasn't multiline + return (Maybe<T>) ((r1!=null && inputS.indexOf('\n')==-1) ? r1 : r2); // NB: previously we supported this also, when we did json above; // yaml support is better as it supports quotes (and better than json because it allows dropping quotes) // snake-yaml, our parser, also accepts key=value -- although i'm not sure this is strictly yaml compliant; // our tests will catch it if snake behaviour changes, and we can reinstate this // (but note it doesn't do quotes; see http://code.google.com/p/guava-libraries/issues/detail?id=412 for that): -// return ImmutableMap.copyOf(Splitter.on(",").trimResults().omitEmptyStrings().withKeyValueSeparator("=").split(input)); + // return ImmutableMap.copyOf(Splitter.on(",").trimResults().omitEmptyStrings().withKeyValueSeparator("=").split(input)); + } - }); + + // other types not supported here + return null; + } } - } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/main/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercerExtensible.java ---------------------------------------------------------------------- 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 a4b4846..64e91b4 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 @@ -27,7 +27,6 @@ import java.util.Map; import java.util.Set; import org.apache.brooklyn.util.exceptions.Exceptions; -import org.apache.brooklyn.util.exceptions.PropagatedRuntimeException; import org.apache.brooklyn.util.guava.AnyExceptionSupplier; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.guava.TypeTokens; @@ -149,7 +148,24 @@ public class TypeCoercerExtensible implements TypeCoercer { targetTypeToken = TypeTokens.getTypeToken(targetTypeToken, targetType); for (TryCoercer coercer : genericCoercers) { result = coercer.tryCoerce(value, targetTypeToken); - if (result!=null && result.isPresent()) return result; + + if (result!=null && result.isPresent()) { + // Check if need to unwrap again (e.g. if want List<Integer> and are given a String "1,2,3" + // then we'll have so far converted to List.of("1", "2", "3"). Call recursively. + // First check that value has changed, to avoid stack overflow! + if (!Objects.equal(value, result.get()) && !Objects.equal(value.getClass(), result.get().getClass()) && targetTypeToken.getType() instanceof ParameterizedType) { + Maybe<T> resultM = tryCoerce(result.get(), targetTypeToken); + if (resultM!=null) { + if (resultM.isPresent()) return resultM; + // if couldn't coerce parameterized types then back out of this coercer + result = resultM; + } + } else { + return result; + } + } + + // remember any error if we were first if (result!=null && firstError==null) firstError = result; } @@ -183,9 +199,16 @@ public class TypeCoercerExtensible implements TypeCoercer { // Could duplicate check for `result instanceof Collection` etc; but recursive call // will be fine as if that doesn't match we'll safely reach `targetType.isInstance(value)` // and just return the result. - return tryCoerce(resultT, targetTypeToken); + Maybe<T> resultM = tryCoerce(resultT, targetTypeToken); + if (resultM!=null) { + if (resultM.isPresent()) return resultM; + // if couldn't coerce parameterized types then back out of this coercer + // but remember the error if we were first + if (firstError==null) firstError = resultM; + } + } else { + return Maybe.of(resultT); } - return Maybe.of(resultT); } catch (Exception e) { Exceptions.propagateIfFatal(e); if (log.isDebugEnabled()) { @@ -206,8 +229,11 @@ public class TypeCoercerExtensible implements TypeCoercer { } } - //not found - if (firstError!=null) return firstError; + // not found + if (firstError!=null) { + // it might be nice to have more than just the first error but for now that's all we remember + return firstError; + } return Maybe.absent(new ClassCoercionException("Cannot coerce type "+value.getClass().getCanonicalName()+" to "+targetTypeToken+" ("+value+"): no adapter known")); } http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java ---------------------------------------------------------------------- diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java index 5e6ad9a..7296f79 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/javalang/coerce/TypeCoercionsTest.java @@ -447,41 +447,6 @@ public class TypeCoercionsTest { } } - // TODO Asserting this undesirable behaviur, to preserve backwards compatibility! - // Would much prefer that we fail-fast. However, I worry that some entity's java declared - // ConfigKey<List<Foo>>, and rely on erasure so they can pass in other representations and - // coerce it themsevles in some way. - // I checked things like JcloudsLocation.JCLOUDS_LOCATION_CUSTOMIZERS - those look ok. - // - // Expect to get a log.warn about that now. Could assert that using LogWatcher. - @Test - public void testListOfFromThrowingException() { - @SuppressWarnings("serial") - TypeToken<List<WithFromThrowingException>> typeToken = new TypeToken<List<WithFromThrowingException>>() {}; - List<String> rawVal = ImmutableList.of("myval"); - - List<WithFromThrowingException> val = coercer.coerce(rawVal, typeToken); - assertEquals(val, rawVal); - - List<WithFromThrowingException> val2 = coercer.tryCoerce(rawVal, typeToken).get(); - assertEquals(val2, rawVal); - } - - // See comment on testListOfFromThrowingException for why we're asserting this undesirable - // behaviour. - @Test - public void testMapOfFromThrowingException() { - @SuppressWarnings("serial") - TypeToken<Map<String, WithFromThrowingException>> typeToken = new TypeToken<Map<String, WithFromThrowingException>>() {}; - ImmutableMap<String, String> rawVal = ImmutableMap.of("mykey", "myval"); - - Map<String, WithFromThrowingException> val = coercer.coerce(rawVal, typeToken); - assertEquals(val, rawVal); - - Map<String, WithFromThrowingException> val2 = coercer.tryCoerce(rawVal, typeToken).get(); - assertEquals(val2, rawVal); - } - @Test public void testCoerceStringToNumber() { assertEquals(coerce("1", Number.class), Double.valueOf(1)); http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/3e57b14b/utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java ---------------------------------------------------------------------- diff --git a/utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java b/utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java index b23a2a7..8739484 100644 --- a/utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java +++ b/utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java @@ -161,7 +161,7 @@ public class StringEscapesTest { JavaStringEscapes.tryUnwrapJsonishList("\"hello\", world").get()); Assert.assertEquals(MutableList.of("hello", "world"), JavaStringEscapes.tryUnwrapJsonishList("[ \"hello\", world ]").get()); - Assert.assertEquals(MutableList.of(""), + Assert.assertEquals(MutableList.of(), JavaStringEscapes.tryUnwrapJsonishList(" ").get()); Assert.assertEquals(MutableList.of(""), JavaStringEscapes.tryUnwrapJsonishList("\"\"").get()); @@ -169,6 +169,8 @@ public class StringEscapesTest { JavaStringEscapes.tryUnwrapJsonishList(",,x,").get()); Assert.assertEquals(MutableList.of("","","x",""), JavaStringEscapes.tryUnwrapJsonishList("\"\",,x,\"\"").get()); + Assert.assertEquals(MutableList.<Object>of(MutableMap.of("a", 1),MutableMap.of("b", 2)), + JavaStringEscapes.tryUnwrapJsonishList("[ a : 1, b : 2 ]").get()); Assert.assertEquals(MutableList.<Object>of(1, 2, "buckle my shoe", true, "true", null, "null", ","), JavaStringEscapes.tryUnwrapJsonishList("1, 2, buckle my shoe, true, \"true\", null, \"null\", \",\"").get());
