address code review comments
Project: http://git-wip-us.apache.org/repos/asf/brooklyn-server/repo Commit: http://git-wip-us.apache.org/repos/asf/brooklyn-server/commit/bae72fc4 Tree: http://git-wip-us.apache.org/repos/asf/brooklyn-server/tree/bae72fc4 Diff: http://git-wip-us.apache.org/repos/asf/brooklyn-server/diff/bae72fc4 Branch: refs/heads/master Commit: bae72fc4ed1bc1813a4014e71955f51dad1e1994 Parents: 8e48531 Author: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Authored: Mon Sep 3 15:22:09 2018 +0100 Committer: Alex Heneveld <alex.henev...@cloudsoftcorp.com> Committed: Mon Sep 3 15:26:18 2018 +0100 ---------------------------------------------------------------------- .../brooklyn/core/config/ConfigConstraints.java | 6 +- .../brooklyn/util/core/task/ValueResolver.java | 2 +- .../brooklyn/util/text/StringEscapes.java | 17 +- .../brooklyn/util/text/StringEscapesTest.java | 175 +++++++++++-------- 4 files changed, 119 insertions(+), 81 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bae72fc4/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java b/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java index 91f4eab..17b7c9d 100644 --- a/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java +++ b/core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java @@ -89,7 +89,7 @@ public abstract class ConfigConstraints<T extends BrooklynObject> { } private static <T> void assertValid(ConfigConstraints<?> constrants, Object context, ConfigKey<T> key, T value) { - ReferenceWithError<Predicate<?>> validity = constrants.checkValueValid(key, value); + ReferenceWithError<Predicate<?>> validity = constrants.validateValue(key, value); if (validity.hasError()) { throw new ConstraintViolationException("Invalid value for " + key + " on " + context + " (" + value + "); it should satisfy "+validity.getWithoutError()); } @@ -154,12 +154,12 @@ public abstract class ConfigConstraints<T extends BrooklynObject> { } <V> boolean isValueValid(ConfigKey<V> configKey, V value) { - return !checkValueValid(configKey, value).hasError(); + return !validateValue(configKey, value).hasError(); } /** returns reference to null without error if valid; otherwise returns reference to predicate and a good error message */ @SuppressWarnings("unchecked") - <V> ReferenceWithError<Predicate<?>> checkValueValid(ConfigKey<V> configKey, V value) { + <V> ReferenceWithError<Predicate<?>> validateValue(ConfigKey<V> configKey, V value) { try { Predicate<? super V> po = configKey.getConstraint(); boolean valid; http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bae72fc4/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java index f39af09..b1c44c3 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java @@ -265,7 +265,7 @@ public class ValueResolver<T> implements DeferredSupplier<T>, Iterable<Maybe<Obj * if the second argument is true, the type specified here is used against non-map/iterable items * inside maps and iterables encountered. if false, any generic signature on the supplied type * is traversed to match contained items. if null (default), it is inferred from the type, - * those with generics mean true, and those without mean false. + * those with generics imply false, and those without imply true. * * see {@link Tasks#resolveDeepValue(Object, Class, ExecutionContext, String)} and * {@link Tasks#resolveDeepValueExactly(Object, TypeToken, ExecutionContext, String)}. */ http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bae72fc4/utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---------------------------------------------------------------------- diff --git a/utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java b/utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java index f3df2b2..98e183e 100644 --- a/utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java +++ b/utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java @@ -321,7 +321,7 @@ public class StringEscapes { } /** @deprecated since 1.0.0, use {@link #unwrapJsonishListStringIfPossible(String)} (old semantics) - * or {@link #unwrapJsonishListStringIfPossible(String)} (improved) */ + * or {@link #tryUnwrapJsonishList(String)} (improved) */ public static List<String> unwrapJsonishListIfPossible(String input) { return unwrapJsonishListStringIfPossible(input); } @@ -335,10 +335,10 @@ public class StringEscapes { * <ll> 2) if not of form <code>[ X ]</code>, wrap in brackets and parse as YAML, * and if that succeeds and is a list, return the result. * <li> 3) otherwise, expect comma-separated tokens which after trimming are of the form "A" or B, - * where "A" is a valid Java string or C is any string not starting with ' - * and not containing " or ,. return the list of those tokens, where A and B - * are their string value, and C as a primitive if it is a number or boolean or null, - * or else a string, including the empty string if empty. + * where "A" is a valid Java string or B is any string not containing any of the chars <code>",.</code> + * and not starting with <code>'</code>, and returns the list of those tokens, where A is + * returned as its string value, and B as a primitive if it is a number or boolean or null, + * or else a string (including the empty string if empty) * <li> 4) if such tokens are not found, return {@link Maybe#absent()}. * <p> * @see #unwrapOptionallyQuotedJavaStringList(String) @@ -362,7 +362,10 @@ public class StringEscapes { List<Object> result = (List<Object>)r; return Maybe.of(result); } - } catch (Exception e) {} + } catch (Exception e) { + Exceptions.propagateIfFatal(e); + // Otherwise ignore; logic below decides whether to return absent or to keep trying + } if (inputT.startsWith("[")) { // if supplied as yaml, don't allow failures return Maybe.absent("Supplied format looked like YAML but could not parse as YAML"); @@ -398,7 +401,7 @@ public class StringEscapes { String w = m.group(1); ri = w; - if (w.matches("[0-9]*.[0-9]+")) { + if (w.matches("[0-9]*\\.[0-9]+")) { try { ri = Double.parseDouble(w); } catch (Exception e) {} http://git-wip-us.apache.org/repos/asf/brooklyn-server/blob/bae72fc4/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 8739484..613bc6c 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 @@ -87,93 +87,126 @@ public class StringEscapesTest { @SuppressWarnings("deprecation") @Test public void testJavaListDeprecated() { - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", \"world\"", ",")); + Assert.assertEquals( + JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", \"world\"", ","), + MutableList.of("hello", "world")); try { JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", world", ","); Assert.fail("Should have thrown"); } catch (Exception e) { /* expected */ } - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListIfPossible("\"hello\", \"world\"")); - Assert.assertEquals(MutableList.of("hello"), - JavaStringEscapes.unwrapJsonishListIfPossible("hello")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListIfPossible("hello, world")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListIfPossible("\"hello\", world")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListIfPossible("[ \"hello\", world ]")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("\"hello\", \"world\""), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("hello"), + MutableList.of("hello")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("hello, world"), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("\"hello\", world"), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("[ \"hello\", world ]"), + MutableList.of("hello", "world")); // if can't parse e.g. because contains double quote, then returns original string as single element list - Assert.assertEquals(MutableList.of("hello\", \"world\""), - JavaStringEscapes.unwrapJsonishListIfPossible("hello\", \"world\"")); - Assert.assertEquals(MutableList.of(), - JavaStringEscapes.unwrapJsonishListIfPossible(" ")); - Assert.assertEquals(MutableList.of(""), - JavaStringEscapes.unwrapJsonishListIfPossible("\"\"")); - Assert.assertEquals(MutableList.of("x"), - JavaStringEscapes.unwrapJsonishListIfPossible(",,x,")); - Assert.assertEquals(MutableList.of("","x",""), - JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\"")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("hello\", \"world\""), + MutableList.of("hello\", \"world\"")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible(" "), + MutableList.of()); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("\"\""), + MutableList.of("")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible(",,x,"), + MutableList.of("x")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""), + MutableList.of("","x","")); } @Test public void testJavaListString() { - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", \"world\"", ",")); + Assert.assertEquals( + JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", \"world\"", ","), + MutableList.of("hello", "world")); try { JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\", world", ","); Assert.fail("Should have thrown"); } catch (Exception e) { /* expected */ } - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListStringIfPossible("\"hello\", \"world\"")); - Assert.assertEquals(MutableList.of("hello"), - JavaStringEscapes.unwrapJsonishListStringIfPossible("hello")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListStringIfPossible("hello, world")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListStringIfPossible("\"hello\", world")); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.unwrapJsonishListStringIfPossible("[ \"hello\", world ]")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("\"hello\", \"world\""), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("hello"), + MutableList.of("hello")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("hello, world"), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("\"hello\", world"), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("[ \"hello\", world ]"), + MutableList.of("hello", "world")); // if can't parse e.g. because contains double quote, then returns original string as single element list - Assert.assertEquals(MutableList.of("hello\", \"world\""), - JavaStringEscapes.unwrapJsonishListStringIfPossible("hello\", \"world\"")); - Assert.assertEquals(MutableList.of(), - JavaStringEscapes.unwrapJsonishListStringIfPossible(" ")); - Assert.assertEquals(MutableList.of(""), - JavaStringEscapes.unwrapJsonishListStringIfPossible("\"\"")); - Assert.assertEquals(MutableList.of("x"), - JavaStringEscapes.unwrapJsonishListStringIfPossible(",,x,")); - Assert.assertEquals(MutableList.of("","x",""), - JavaStringEscapes.unwrapJsonishListStringIfPossible("\"\",,x,\"\"")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("hello\", \"world\""), + MutableList.of("hello\", \"world\"")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible(" "), + MutableList.of()); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("\"\""), + MutableList.of("")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible(",,x,"), + MutableList.of("x")); + Assert.assertEquals( + JavaStringEscapes.unwrapJsonishListStringIfPossible("\"\",,x,\"\""), + MutableList.of("","x","")); } @Test public void testJavaListObject() { - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.tryUnwrapJsonishList("\"hello\", \"world\"").get()); - Assert.assertEquals(MutableList.of("hello"), - JavaStringEscapes.tryUnwrapJsonishList("hello").get()); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.tryUnwrapJsonishList("hello, world").get()); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.tryUnwrapJsonishList("\"hello\", world").get()); - Assert.assertEquals(MutableList.of("hello", "world"), - JavaStringEscapes.tryUnwrapJsonishList("[ \"hello\", world ]").get()); - Assert.assertEquals(MutableList.of(), - JavaStringEscapes.tryUnwrapJsonishList(" ").get()); - Assert.assertEquals(MutableList.of(""), - JavaStringEscapes.tryUnwrapJsonishList("\"\"").get()); - Assert.assertEquals(MutableList.of("","","x",""), - 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( + JavaStringEscapes.tryUnwrapJsonishList("\"hello\", \"world\"").get(), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("hello").get(), + MutableList.of("hello")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("hello, world").get(), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("\"hello\", world").get(), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("[ \"hello\", world ]").get(), + MutableList.of("hello", "world")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList(" ").get(), + MutableList.of()); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("\"\"").get(), + MutableList.of("")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList(",,x,").get(), + MutableList.of("","","x","")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("\"\",,x,\"\"").get(), + MutableList.of("","","x","")); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("[ a : 1, b : 2 ]").get(), + MutableList.<Object>of(MutableMap.of("a", 1),MutableMap.of("b", 2))); - 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()); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("1, 2.0, buckle my shoe, true, \"true\", null, \"null\", \",\"").get(), + MutableList.<Object>of(1, 2.0, "buckle my shoe", true, "true", null, "null", ",")); try { JavaStringEscapes.tryUnwrapJsonishList("\"hello").get(); @@ -186,11 +219,13 @@ public class StringEscapesTest { Asserts.expectedFailureContains(e, "position 1"); } - Assert.assertEquals(MutableList.of(MutableMap.of("a", "b"), "world"), - JavaStringEscapes.tryUnwrapJsonishList("[ { a: b }, world ]").get()); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("[ { a: b }, world ]").get(), + MutableList.of(MutableMap.of("a", "b"), "world")); - Assert.assertEquals(MutableList.of(MutableMap.of("a", MutableList.<Object>of("b", 2)), "world"), - JavaStringEscapes.tryUnwrapJsonishList("{ a: [ b, 2 ] }, world").get()); + Assert.assertEquals( + JavaStringEscapes.tryUnwrapJsonishList("{ a: [ b, 2 ] }, world").get(), + MutableList.of(MutableMap.of("a", MutableList.<Object>of("b", 2)), "world")); } }
