[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user asfgit closed the pull request at: https://github.com/apache/brooklyn-server/pull/982 ---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214705115
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
\"world\"", ","));
+try {
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
world", ",");
+Assert.fail("Should have thrown");
+} catch (Exception e) { /* expected */ }
+
+Assert.assertEquals(MutableList.of("hello", "world"),
--- End diff --
fixed, in light of comment below
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214704867
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"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\""));
--- End diff --
yeah - in light of this i've updated this and the previous
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214703069
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
\"world\"", ","));
+try {
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
world", ",");
+Assert.fail("Should have thrown");
+} catch (Exception e) { /* expected */ }
+
+Assert.assertEquals(MutableList.of("hello", "world"),
--- End diff --
yeah ... i just copy-pasted other examples; sometimes when i encounter this
i change it if it's irritating, but in this case i didn't (yet).
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214702501
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -501,6 +501,6 @@ public boolean equals(Object obj) {
/** Finds the {@link Absent#getException()} if {@link #isAbsent()}, or
null */
public static RuntimeException getException(Maybe t) {
-return ((Maybe.Absent)t).getException();
+return t instanceof Maybe.Absent ?
((Maybe.Absent)t).getException() : null;
--- End diff --
(note the javadoc here, which is now accurate)
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214702367
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -365,7 +365,7 @@ public RuntimeException getException() {
});
}
public static Maybe changeExceptionSupplier(Maybe
original, Function,Supplier> transform) {
-if (original.isPresent()) return original;
+if (original==null || original.isPresent()) return original;
--- End diff --
semantics of method were to change exception supplier _if_ it's absent, and
do nothing if it's present. logical extension of that is also to do nothing if
it's null IMO. if we were to "fail fast" we are applying a particular dogma
that says a `Maybe` shouldn't be null, which might be helpful in some
situations but it gets in the way in others. if the `Maybe` being null is a
problem the user will likely discover that, and it's not much helpful if we
discover that slightly earlier. more helpful i think to allow callers who
might have a null Maybe.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214700788
--- Diff:
core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java ---
@@ -87,13 +89,23 @@ public boolean apply(@Nullable Method input) {
Method accessibleMethod =
Reflections.findAccessibleMethod(method).get();
try {
Type paramType = method.getGenericParameterTypes()[0];
-Object coercedArgument = TypeCoercions.coerce(argument,
TypeToken.of(paramType));
-return Maybe.of(accessibleMethod.invoke(instance,
coercedArgument));
+Maybe coercedArgumentM =
TypeCoercions.tryCoerce(argument, TypeToken.of(paramType));
+RuntimeException exception =
Maybe.getException(coercedArgumentM);
--- End diff --
The javadoc on `Maybe.getException` claimed it would return null for
present items; I relied on this and then discovered the error so updated the
code to bring it in line with the javadoc.
Can change back if you like but personally I think it's useful to be able
to get `null` back as an exception, saves a little bit of work sometimes, as in
this case: because we set `exception` manually in the boxing case we'd need
one more line than you suggest.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214612451
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"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 ]"));
+// 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,\"\""));
+}
+
+@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.of(MutableMap.of("a",
1),MutableMap.of("b", 2)),
+JavaStringEscapes.tryUnwrapJsonishList("[ a : 1, b : 2
]").get());
+
+Assert.assertEquals(MutableList.of(1, 2, "buckle my shoe",
true, "true", null, "null", ","),
--- End diff --
Can't see any tests for doubles.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214640708
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
\"world\"", ","));
+try {
+JavaStringEscapes.unwrapQuotedJavaStringList("\"hello\",
world", ",");
+Assert.fail("Should have thrown");
+} catch (Exception e) { /* expected */ }
+
+Assert.assertEquals(MutableList.of("hello", "world"),
--- End diff --
These assert calls are the wrong way round - first argument is `actual`,
second argument is `expected` for testng.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214615588
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -365,7 +365,7 @@ public RuntimeException getException() {
});
}
public static Maybe changeExceptionSupplier(Maybe
original, Function,Supplier> transform) {
-if (original.isPresent()) return original;
+if (original==null || original.isPresent()) return original;
--- End diff --
I think we should fail-fast if you pass in null. The caller has asked to
change the exception supplier, and we've ignored their request without telling
them of the problem.
This kind of null check just leads to more `NullPointerException`s later in
the code paths, or more null checks later (which coders/reviewers would not
expect to have to do when using `Maybe` or `Optional`).
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214611925
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---
@@ -315,12 +320,121 @@ else if (c=='\"') {
throw new IllegalArgumentException("String '"+s+"' is not a
valid Java string (unterminated string)");
}
+/** @deprecated since 1.0.0, use {@link
#unwrapJsonishListStringIfPossible(String)} (old semantics)
+ * or {@link #unwrapJsonishListStringIfPossible(String)}
(improved) */
+public static List unwrapJsonishListIfPossible(String
input) {
+return unwrapJsonishListStringIfPossible(input);
+}
+
+/** converts a comma separated list in a single string to a list
of json primitives or maps,
+ * falling back to returning the input.
+ *
+ * specifically:
+ * 1) if of form [ X ] (in brackets after trim),
parse as YAML;
+ * if parse succeeds return the result, or if parse fails,
return {@link Maybe#absent()}.
+ * 2) if not of form [ X ], wrap in brackets and
parse as YAML,
+ * and if that succeeds and is a list, return the result.
+ * 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.
+ * 4) if such tokens are not found, return {@link
Maybe#absent()}.
+ *
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+public static Maybe> tryUnwrapJsonishList(String
input) {
+if (input==null) return Maybe.absent("null input cannot unwrap
to a list");
+String inputT = input.trim();
+
+String inputYaml = null;
+if (!inputT.startsWith("[") && !inputT.endsWith("]")) {
+inputYaml = "[" + inputT + "]";
+}
+if (inputT.startsWith("[") && inputT.endsWith("]")) {
+inputYaml = inputT;
+}
+if (inputYaml!=null) {
+try {
+Object r = Iterables.getOnlyElement(
Yamls.parseAll(inputYaml) );
+if (r instanceof List) {
+@SuppressWarnings("unchecked")
+List result = (List)r;
+return Maybe.of(result);
+}
+} catch (Exception e) {}
+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");
+}
+}
+
+List result = MutableList.of();
+
+// double quote: ^ \s* " ([not quote or backslash] or
[backslash any-char])* " \s* (, or $)
+Pattern dq =
Pattern.compile("^\\s*(\"([^\"]|[.])*\")\\s*(,|$)");
+// could also support this, but we need new unescape routines
+//// single quote: ^ \s* ' ([not quote or backslash] or
[backslash any-char])* ' \s* (, or $)
+//Pattern sq =
Pattern.compile("^\\s*'([^\']|[.])'*\\s*(,|$)");
+// no quote: ^ \s* (empty, or [not ' or " or space] ([not ,
or "]* [not , or " or space])?) \s* (, or $)
+Pattern nq =
Pattern.compile("^\\s*(|[^,\"\\s]([^,\"]*[^,\"\\s])?)\\s*(,|$)");
+
+int removedChars = 0;
+while (true) {
+Object ri;
+Matcher m = dq.matcher(input);
+if (m.find()) {
+try {
+ri = unwrapJavaString(m.group(1));
+} catch (Exception e) {
+Exceptions.propagateIfFatal(e);
+return Maybe.absent("Could not match valid quote
pattern" +
+(removedChars>0 ? " at position "+removedChars
: "")
++ ": "+ Exceptions.collapseText(e));
+}
+} else {
+m = nq.matcher(input);
+if (m.find()) {
+String w = m.group(1);
+
+
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214640867
--- Diff:
utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java
---
@@ -115,4 +117,80 @@ public void testJavaLists() {
JavaStringEscapes.unwrapJsonishListIfPossible("\"\",,x,\"\""));
}
+@Test
+public void testJavaListString() {
+Assert.assertEquals(MutableList.of("hello", "world"),
+JavaStringEscapes.unwrapQuotedJavaStringList("\"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\""));
--- End diff --
Minor: I'd follow the java convention (and the Brooklyn convention) for
indents: 8 spaces. The 4 spaces looks too much like a new code block, rather
than a continuation of the previous line.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214612997
--- Diff:
core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java ---
@@ -87,13 +89,23 @@ public boolean apply(@Nullable Method input) {
Method accessibleMethod =
Reflections.findAccessibleMethod(method).get();
try {
Type paramType = method.getGenericParameterTypes()[0];
-Object coercedArgument = TypeCoercions.coerce(argument,
TypeToken.of(paramType));
-return Maybe.of(accessibleMethod.invoke(instance,
coercedArgument));
+Maybe coercedArgumentM =
TypeCoercions.tryCoerce(argument, TypeToken.of(paramType));
+RuntimeException exception =
Maybe.getException(coercedArgumentM);
--- End diff --
Looks wrong - this will cast `coercedArgumentM` to `Maybe.absent`, but on
the line below you do `coercedArgumentM.isPresent`. So presumably the call to
`getException` will sometimes throw a `ClassCastException`?
Ah, I see you've changed the semantics of `getException` to return null if
it wasn't an exception.
Why change it? I liked the way that asking for the exception when `present`
was a programming error - there will never be an exception when present. It
looks like you can easily avoid that by removing this line, and changing the if
statement below to just `if (coercedArgumentM.isAbsent()) {`.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214610344
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---
@@ -315,12 +320,121 @@ else if (c=='\"') {
throw new IllegalArgumentException("String '"+s+"' is not a
valid Java string (unterminated string)");
}
+/** @deprecated since 1.0.0, use {@link
#unwrapJsonishListStringIfPossible(String)} (old semantics)
+ * or {@link #unwrapJsonishListStringIfPossible(String)}
(improved) */
+public static List unwrapJsonishListIfPossible(String
input) {
+return unwrapJsonishListStringIfPossible(input);
+}
+
+/** converts a comma separated list in a single string to a list
of json primitives or maps,
+ * falling back to returning the input.
+ *
+ * specifically:
+ * 1) if of form [ X ] (in brackets after trim),
parse as YAML;
+ * if parse succeeds return the result, or if parse fails,
return {@link Maybe#absent()}.
+ * 2) if not of form [ X ], wrap in brackets and
parse as YAML,
+ * and if that succeeds and is a list, return the result.
+ * 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.
+ * 4) if such tokens are not found, return {@link
Maybe#absent()}.
+ *
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+public static Maybe> tryUnwrapJsonishList(String
input) {
+if (input==null) return Maybe.absent("null input cannot unwrap
to a list");
+String inputT = input.trim();
+
+String inputYaml = null;
+if (!inputT.startsWith("[") && !inputT.endsWith("]")) {
+inputYaml = "[" + inputT + "]";
+}
+if (inputT.startsWith("[") && inputT.endsWith("]")) {
+inputYaml = inputT;
+}
+if (inputYaml!=null) {
+try {
+Object r = Iterables.getOnlyElement(
Yamls.parseAll(inputYaml) );
+if (r instanceof List) {
+@SuppressWarnings("unchecked")
+List result = (List)r;
+return Maybe.of(result);
+}
+} catch (Exception e) {}
--- End diff --
Strong preference to never have empty catch blocks. I'd do:
```
catch(Exception e) {
Exceptions.propagateIfFatal(e);
// Logic below decides whether to return absent or to keep trying
}
```
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214619292
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/guava/Maybe.java ---
@@ -501,6 +501,6 @@ public boolean equals(Object obj) {
/** Finds the {@link Absent#getException()} if {@link #isAbsent()}, or
null */
public static RuntimeException getException(Maybe t) {
-return ((Maybe.Absent)t).getException();
+return t instanceof Maybe.Absent ?
((Maybe.Absent)t).getException() : null;
--- End diff --
As per earlier comment, I'd leave it as throwing an exception if you try to
get the exception of a `present`.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214642777
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---
@@ -315,12 +320,121 @@ else if (c=='\"') {
throw new IllegalArgumentException("String '"+s+"' is not a
valid Java string (unterminated string)");
}
+/** @deprecated since 1.0.0, use {@link
#unwrapJsonishListStringIfPossible(String)} (old semantics)
+ * or {@link #unwrapJsonishListStringIfPossible(String)}
(improved) */
+public static List unwrapJsonishListIfPossible(String
input) {
+return unwrapJsonishListStringIfPossible(input);
+}
+
+/** converts a comma separated list in a single string to a list
of json primitives or maps,
+ * falling back to returning the input.
+ *
+ * specifically:
+ * 1) if of form [ X ] (in brackets after trim),
parse as YAML;
+ * if parse succeeds return the result, or if parse fails,
return {@link Maybe#absent()}.
+ * 2) if not of form [ X ], wrap in brackets and
parse as YAML,
+ * and if that succeeds and is a list, return the result.
+ * 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.
+ * 4) if such tokens are not found, return {@link
Maybe#absent()}.
+ *
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+public static Maybe> tryUnwrapJsonishList(String
input) {
+if (input==null) return Maybe.absent("null input cannot unwrap
to a list");
+String inputT = input.trim();
+
+String inputYaml = null;
+if (!inputT.startsWith("[") && !inputT.endsWith("]")) {
+inputYaml = "[" + inputT + "]";
+}
+if (inputT.startsWith("[") && inputT.endsWith("]")) {
+inputYaml = inputT;
+}
+if (inputYaml!=null) {
+try {
+Object r = Iterables.getOnlyElement(
Yamls.parseAll(inputYaml) );
+if (r instanceof List) {
+@SuppressWarnings("unchecked")
+List result = (List)r;
+return Maybe.of(result);
+}
+} catch (Exception e) {}
+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");
+}
+}
+
+List result = MutableList.of();
+
+// double quote: ^ \s* " ([not quote or backslash] or
[backslash any-char])* " \s* (, or $)
+Pattern dq =
Pattern.compile("^\\s*(\"([^\"]|[.])*\")\\s*(,|$)");
+// could also support this, but we need new unescape routines
+//// single quote: ^ \s* ' ([not quote or backslash] or
[backslash any-char])* ' \s* (, or $)
+//Pattern sq =
Pattern.compile("^\\s*'([^\']|[.])'*\\s*(,|$)");
+// no quote: ^ \s* (empty, or [not ' or " or space] ([not ,
or "]* [not , or " or space])?) \s* (, or $)
+Pattern nq =
Pattern.compile("^\\s*(|[^,\"\\s]([^,\"]*[^,\"\\s])?)\\s*(,|$)");
+
+int removedChars = 0;
+while (true) {
+Object ri;
+Matcher m = dq.matcher(input);
+if (m.find()) {
+try {
+ri = unwrapJavaString(m.group(1));
+} catch (Exception e) {
+Exceptions.propagateIfFatal(e);
+return Maybe.absent("Could not match valid quote
pattern" +
+(removedChars>0 ? " at position "+removedChars
: "")
++ ": "+ Exceptions.collapseText(e));
+}
+} else {
+m = nq.matcher(input);
+if (m.find()) {
+String w = m.group(1);
+
+
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214607884
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---
@@ -315,12 +320,121 @@ else if (c=='\"') {
throw new IllegalArgumentException("String '"+s+"' is not a
valid Java string (unterminated string)");
}
+/** @deprecated since 1.0.0, use {@link
#unwrapJsonishListStringIfPossible(String)} (old semantics)
+ * or {@link #unwrapJsonishListStringIfPossible(String)}
(improved) */
--- End diff --
It looks like the two methods being suggested (for old semantics and
improved) are identical - what should they be? Should it be pointing folk at
`tryUnwrapJsonishList(String)` as well?
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214554995
--- Diff:
core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java ---
@@ -149,19 +153,28 @@ public ConfigConstraints(T brooklynObject) {
return violating;
}
-@SuppressWarnings("unchecked")
boolean isValueValid(ConfigKey configKey, V value) {
+return !checkValueValid(configKey, value).hasError();
+}
+
+/** returns reference to null without error if valid; otherwise
returns reference to predicate and a good error message */
+@SuppressWarnings("unchecked")
+ ReferenceWithError> checkValueValid(ConfigKey
configKey, V value) {
--- End diff --
Naming convention in guava is that `checkXyz` methods will throw an
exception if the check fails (e.g.
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/base/Preconditions.html).
I'd prefer the name `validateValue(...)`.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214608331
--- Diff:
utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java ---
@@ -315,12 +320,121 @@ else if (c=='\"') {
throw new IllegalArgumentException("String '"+s+"' is not a
valid Java string (unterminated string)");
}
+/** @deprecated since 1.0.0, use {@link
#unwrapJsonishListStringIfPossible(String)} (old semantics)
+ * or {@link #unwrapJsonishListStringIfPossible(String)}
(improved) */
+public static List unwrapJsonishListIfPossible(String
input) {
+return unwrapJsonishListStringIfPossible(input);
+}
+
+/** converts a comma separated list in a single string to a list
of json primitives or maps,
+ * falling back to returning the input.
+ *
+ * specifically:
+ * 1) if of form [ X ] (in brackets after trim),
parse as YAML;
+ * if parse succeeds return the result, or if parse fails,
return {@link Maybe#absent()}.
+ * 2) if not of form [ X ], wrap in brackets and
parse as YAML,
+ * and if that succeeds and is a list, return the result.
+ * 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 '
--- End diff --
Could do we rewording. This line introduces `C` - should it be `B`? There
was no mention of C previously.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/982#discussion_r214606998
--- Diff:
core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java ---
@@ -251,9 +255,23 @@ public ResolverBuilderPretype(Object v) {
else return Maybe.absent("No default value set");
}
-/** causes nested structures (maps, lists) to be descended and nested
unresolved values resolved */
+/** causes nested structures (maps, lists) to be descended and nested
unresolved values resolved.
+ * for legacy reasons this sets deepTraversalUsesRootType.
+ * @deprecated use {@link #deep(boolean, boolean)} */
public ValueResolver deep(boolean forceDeep) {
+return deep(true, true);
+}
+/** causes nested structures (maps, lists) to be descended and nested
unresolved values resolved.
+ * 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.
--- End diff --
Is it not the other way round: "those with generics mean false, and those
without mean true"?
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
GitHub user ahgittin opened a pull request: https://github.com/apache/brooklyn-server/pull/982 Improve coercions esp with generics previously when we coerced or resolved values even with config keys, we did not reliably obey generics. now we do. preserves previous behaviour except now can be a bit stricter when convering, e.g. `ConfigKey>` now is only allowed to contain integers (but strings will be converted; previously it could contain strings and wouldn't even attempt to convert them) You can merge this pull request into a Git repository by running: $ git pull https://github.com/ahgittin/brooklyn-server misc-coercions Alternatively you can review and apply these changes as the patch at: https://github.com/apache/brooklyn-server/pull/982.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #982 commit b8f6264067934f694ab82cf87d7bc1ddb8ff130a Author: Alex Heneveld Date: 2018-08-30T20:08:01Z better message when trying to use a method to coerce commit ddc6c71489926ee1d6d5c1d6d51697a7b5d18495 Author: Alex Heneveld Date: 2018-08-30T20:08:24Z better message in some cases when constraint validation fails commit 89c692e4951deb17cbca05567dea74ce2dd78860 Author: Alex Heneveld Date: 2018-08-30T23:24:32Z coercing from string to list can now handle objects now have two methods for parsing a list as a string, depending whether it wants object or string commit 8d081d01a3bb6e0e48ba64f13a6d4485d9d3cad8 Author: Alex Heneveld Date: 2018-08-31T00:13:37Z no longer coerce things if generic parametrized types don't match previously we would return e.g. a List not containing T if List was requested. this was deprecated and we would warn, but now we are stricter. commit 3e57b14b220bd7a994a9143d83bc123879086aff Author: Alex Heneveld Date: 2018-08-31T09:28:57Z respect generics better when coercing previous commit used yaml parse even for things like List; 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. commit 312131c8b4032d2c78831af825b96a05e36bce6b Author: Alex Heneveld Date: 2018-08-31T10:15:34Z preserve generics info when resolving config keys, more places previously we converted to Class in places, losing generic info for resolution commit e8fa71e03b5b42cedfa15ca4df18324cafe122cc Author: Alex Heneveld Date: 2018-08-31T12:49:48Z improve coercion of items with generics, esp config keys previously config keys did not properly keep generic information for their contents; now this information is preserved and the results coerced as per the types. also enhances the ValueResolver to have clearer semantics when coercing generics in maps/iterables. ---
