Improve coercions esp with generics #982

ahgittin · 2018-08-31T15:15:02Z

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)

now have two methods for parsing a list as a string, depending whether it wants object or string

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.

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.

previously we converted to Class in places, losing generic info for resolution

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.

aledsage

Looks really good. Only minor comments, but some of them are worth addressing before merging.

I've also played around with a couple more unit tests, which pass. Easiest if I just submit a separate PR for those after this is merged.

aledsage · 2018-09-02T22:29:18Z

core/src/main/java/org/apache/brooklyn/core/config/ConfigConstraints.java

+
+    /** 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) {


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(...).

aledsage · 2018-09-03T08:13:52Z

core/src/main/java/org/apache/brooklyn/util/core/task/ValueResolver.java

+     * 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. 


Is it not the other way round: "those with generics mean false, and those without mean true"?

aledsage · 2018-09-03T08:17:25Z

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) */


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?

aledsage · 2018-09-03T08:19:11Z

utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java

+         * <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 ' 


Could do we rewording. This line introduces C - should it be B? There was no mention of C previously.

aledsage · 2018-09-03T08:26:46Z

utils/common/src/main/java/org/apache/brooklyn/util/text/StringEscapes.java

+                        List<Object> result = (List<Object>)r;
+                        return Maybe.of(result);
+                    }
+                } catch (Exception e) {}


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 }

aledsage · 2018-09-03T08:37:06Z

core/src/main/java/org/apache/brooklyn/util/core/flags/MethodCoercions.java

-                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);


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()) {.

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.

aledsage · 2018-09-03T08:47:10Z

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;


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 NullPointerExceptions 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).

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.

aledsage · 2018-09-03T09:00:30Z

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;


As per earlier comment, I'd leave it as throwing an exception if you try to get the exception of a present.

(note the javadoc here, which is now accurate)

aledsage · 2018-09-03T10:18:51Z

utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java

+            Assert.fail("Should have thrown");
+        } catch (Exception e) { /* expected */ }
+
+        Assert.assertEquals(MutableList.of("hello", "world"),


These assert calls are the wrong way round - first argument is actual, second argument is expected for testng.

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).

fixed, in light of comment below

aledsage · 2018-09-03T10:19:34Z

utils/common/src/test/java/org/apache/brooklyn/util/text/StringEscapesTest.java

+        } catch (Exception e) { /* expected */ }
+
+        Assert.assertEquals(MutableList.of("hello", "world"),
+            JavaStringEscapes.unwrapJsonishListStringIfPossible("\"hello\", \"world\""));


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.

yeah - in light of this i've updated this and the previous

ahgittin · 2018-09-03T14:23:53Z

thx, great review @aledsage . addressed all comments except those re permitting null as noted above.

aledsage

Thanks @ahgittin - looks great, merging now.

ahgittin added 7 commits August 30, 2018 21:08

better message when trying to use a method to coerce

b8f6264

better message in some cases when constraint validation fails

ddc6c71

coercing from string to list can now handle objects

89c692e

now have two methods for parsing a list as a string, depending whether it wants object or string

no longer coerce things if generic parametrized types don't match

8d081d0

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.

respect generics better when coercing

3e57b14

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.

preserve generics info when resolving config keys, more places

312131c

previously we converted to Class in places, losing generic info for resolution

ahgittin force-pushed the misc-coercions branch from e8fa71e to 8e48531 Compare August 31, 2018 15:39

aledsage suggested changes Sep 3, 2018

View reviewed changes

address code review comments

bae72fc

ahgittin force-pushed the misc-coercions branch from 6618d72 to bae72fc Compare September 3, 2018 14:26

aledsage approved these changes Sep 4, 2018

View reviewed changes

asfgit merged commit bae72fc into apache:master Sep 4, 2018

asfgit pushed a commit that referenced this pull request Sep 4, 2018

This closes #982

002be99

ahgittin mentioned this pull request Sep 5, 2018

restore the field subType for sub-element config keys #984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve coercions esp with generics #982

Improve coercions esp with generics #982

ahgittin commented Aug 31, 2018

aledsage left a comment

aledsage Sep 2, 2018

aledsage Sep 3, 2018

aledsage Sep 3, 2018

aledsage Sep 3, 2018

aledsage Sep 3, 2018

aledsage Sep 3, 2018

ahgittin Sep 3, 2018

aledsage Sep 3, 2018

ahgittin Sep 3, 2018

aledsage Sep 3, 2018

ahgittin Sep 3, 2018

aledsage Sep 3, 2018

ahgittin Sep 3, 2018

ahgittin Sep 3, 2018

aledsage Sep 3, 2018

ahgittin Sep 3, 2018

ahgittin commented Sep 3, 2018

aledsage left a comment

Navigation Menu

Improve coercions esp with generics #982

Improve coercions esp with generics #982

Conversation

ahgittin commented Aug 31, 2018

aledsage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahgittin commented Sep 3, 2018

aledsage left a comment

Choose a reason for hiding this comment