You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by ahgittin <gi...@git.apache.org> on 2018/08/31 15:15:06 UTC
[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<Set<Integer>>` 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 <al...@...>
Date: 2018-08-30T20:08:01Z
better message when trying to use a method to coerce
commit ddc6c71489926ee1d6d5c1d6d51697a7b5d18495
Author: Alex Heneveld <al...@...>
Date: 2018-08-30T20:08:24Z
better message in some cases when constraint validation fails
commit 89c692e4951deb17cbca05567dea74ce2dd78860
Author: Alex Heneveld <al...@...>
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 <al...@...>
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<T> was requested.
this was deprecated and we would warn, but now we are stricter.
commit 3e57b14b220bd7a994a9143d83bc123879086aff
Author: Alex Heneveld <al...@...>
Date: 2018-08-31T09:28:57Z
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.
commit 312131c8b4032d2c78831af825b96a05e36bce6b
Author: Alex Heneveld <al...@...>
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 <al...@...>
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.
----
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Posted by aledsage <gi...@git.apache.org>.
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<String> 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.
+ * <p>
+ * specifically:
+ * <li> 1) if of form <code>[ X ]</code> (in brackets after trim), parse as YAML;
+ * if parse succeeds return the result, or if parse fails, return {@link Maybe#absent()}.
+ * <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.
+ * <li> 4) if such tokens are not found, return {@link Maybe#absent()}.
+ * <p>
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+ public static Maybe<List<Object>> 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<Object> result = (List<Object>)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<Object> 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);
+
+ ri = w;
+ if (w.matches("[0-9]*.[0-9]+")) {
--- End diff --
It was a bit tricky to actually get into this code block! Sensible-looking things are pretty much always covered by the `Yamls.parseAll`. An example failing test is:
```
Assert.assertEquals(JavaStringEscapes.tryUnwrapJsonishList("[1, 123").get(),
MutableList.of("[1", 123));
```
This gives `Double.valueOf(123)` as the second value in the list.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Posted by aledsage <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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<String> 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.
+ * <p>
+ * specifically:
+ * <li> 1) if of form <code>[ X ]</code> (in brackets after trim), parse as YAML;
+ * if parse succeeds return the result, or if parse fails, return {@link Maybe#absent()}.
+ * <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.
+ * <li> 4) if such tokens are not found, return {@link Maybe#absent()}.
+ * <p>
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+ public static Maybe<List<Object>> 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<Object> result = (List<Object>)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
Posted by ahgittin <gi...@git.apache.org>.
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
Posted by ahgittin <gi...@git.apache.org>.
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
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/982
---
[GitHub] brooklyn-server issue #982: Improve coercions esp with generics
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the issue:
https://github.com/apache/brooklyn-server/pull/982
thx, great review @aledsage . addressed all comments except those re permitting `null` as noted above.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Posted by aledsage <gi...@git.apache.org>.
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")
<V> boolean isValueValid(ConfigKey<V> 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")
+ <V> ReferenceWithError<Predicate<?>> checkValueValid(ConfigKey<V> 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
Posted by aledsage <gi...@git.apache.org>.
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<String> 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.
+ * <p>
+ * specifically:
+ * <li> 1) if of form <code>[ X ]</code> (in brackets after trim), parse as YAML;
+ * if parse succeeds return the result, or if parse fails, return {@link Maybe#absent()}.
+ * <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 '
--- 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
Posted by aledsage <gi...@git.apache.org>.
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<T> 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
Posted by aledsage <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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 <T> Maybe<T> changeExceptionSupplier(Maybe<T> original, Function<AnyExceptionSupplier<?>,Supplier<? extends RuntimeException>> 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
Posted by ahgittin <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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
Posted by ahgittin <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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.<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", ","),
--- End diff --
Can't see any tests for doubles.
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Posted by ahgittin <gi...@git.apache.org>.
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 <T> Maybe<T> changeExceptionSupplier(Maybe<T> original, Function<AnyExceptionSupplier<?>,Supplier<? extends RuntimeException>> 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
Posted by ahgittin <gi...@git.apache.org>.
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
Posted by aledsage <gi...@git.apache.org>.
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<String> 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.
+ * <p>
+ * specifically:
+ * <li> 1) if of form <code>[ X ]</code> (in brackets after trim), parse as YAML;
+ * if parse succeeds return the result, or if parse fails, return {@link Maybe#absent()}.
+ * <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.
+ * <li> 4) if such tokens are not found, return {@link Maybe#absent()}.
+ * <p>
+ * @see #unwrapOptionallyQuotedJavaStringList(String)
+ **/
+ public static Maybe<List<Object>> 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<Object> result = (List<Object>)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<Object> 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);
+
+ ri = w;
+ if (w.matches("[0-9]*.[0-9]+")) {
--- End diff --
Should this not be an escaped decimal place rather than any char (`.`)? Are you meaning to deal with `e` as well? Is `.4` a valid double in yaml (rather than having to write `0.4`)?
---
[GitHub] brooklyn-server pull request #982: Improve coercions esp with generics
Posted by aledsage <gi...@git.apache.org>.
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()) {`.
---