| Issue 13: | MethodPredicate apply throws NPE for methods with nullable Boolean return type | |
| 2 people starred this issue and may be notified of changes. | Back to list |
If actual method return value from a Boolean return type method is null, apply() causes a NPE. The return type from MethodPredicate.apply() should probably be Boolean.class instead.
Nov 21, 2011
Project Member
#1
kandpwel...@gmail.com
Nov 21, 2011
(No comment was entered for this change.)
Cc:
codeto...@gmail.com
Nov 22, 2011
So here's my proposal: * predicateFor(T val) only permits handling of primitive boolean return types, throws exception when tried on Boolean return type method * new method unsafePredicateFor(T val) permits handling of either primitive or Boolean, NPEs possible, caveat emptor (Java doc to explain) * possible addition: overloaded unsafePredicateFor(T val, boolean nullBecomes) handles Boolean return types by mapping null to the value of "nullBecomes" What do you think?
Nov 22, 2011
(No comment was entered for this change.)
Labels:
Usability
Nov 23, 2011
conversely, we could have: * safePredicateFor(T val) with the restrictions that it only handles primitive return * predicateFor(T val) as the unsafe alternative that throws NPE for a null Boolean * predicateFor(T val, boolean, treatNullAs) as the "safe" alternative that allows Boolean to be handled I'm trying to figure out how to keep the API as thin/terse as possible. I'm not sure which of these cases would be the "ordinary" case
Nov 25, 2011
My vote is for the first proposal in comment 4.
Nov 25, 2011
Will MethodFunction.apply() be a problem for nullable Integer, Float, etc ? Is this a general problem with auto-boxing?
Nov 29, 2011
I have been considering this problem some more, and I realize that the problem is inherent in any Java functional framework predicates where the apply() expects a primitive boolean return, such as Guava, and not specifically in the Funcito API. Take for example the following standard Guava Predicate definition and use, which also throws a NPE:
<code>
Predicate<Object> pred = new Predicate<Object>() {
@Override
public boolean apply(Object arg0) {
Boolean b = null;
return b;
}
};
pred.apply(new Object());
</code>
However, what Funcito does is abstract us away from the problem to a greater degree where it becomes easier to make the mistake of writing an unsafe Predicate. So to keep the Funcito API thin, I have an alternative suggestion to what I have suggested so far. We add a method to the MethodPredicate and MethodFunction (etc., for other frameworks) classes, either:
<T> MethodPredicate<T> nullsAre(boolean defaultValue);
<T,V> MethodFunction<T,V> nullsAre(V nonNullDefaultValue);
or
<T> MethodPredicate<T> defaultValue(boolean defaultValue);
<T,V> MethodFunction<T,V> defaultValue(V nonNullDefaultValue);
that performs a transformation on the MethodFunction/Predicate into another one that includes the null check and default value replacement. So in essence, the standard behavior without using the new methods is the caveat emptor since Guave (etc.) already has that risk built in. But using the extra convenience method will just be a way of simplifying the null checking since we don't want the Funcito user to have to go back and modify their wrapped methods. Also, note that the Function version is not strictly needed, since the apply() for Function returns an Object rather than a primitive. For Function, the extra method is 100% for convenience.
Anyway, standard usage would look like:
Predicate<SomeClass> pred = predicateFor(callsTo(SomeClass.class).myBoolMethod()).defaultValue(false);
Nov 29, 2011
Extra note to above: in order to make the above semantics work, I would have to change the return type of predicateFor() from Predicate<T> to MethodPredicate<T> (similar for Function<T,V>), which is assignment compatible with Predicate<T>. My only hesitation on that is that if the return type is not plain Predicate/Function, it may *feel* less user friendly to those who are using IDE auto-completion or browsing Javadoc to not immediately see that the MethodPredicate is assignment compatible. People might feel compelled to declare their predicates as MethodPredicate (etc.).
Nov 29, 2011
Now I'm kind of leaning back to a partial implementation of Comment 6: * predicateFor(T val) as the unsafe alternative that throws NPE for a null Boolean * predicateFor(T val, boolean, treatNullAs) as the "safe" alternative that allows Boolean to be handled It's the simplest, cleanest API, and it allows us to keep a return type of Predicate. And I would not even provide "safePredicateFor(T val)", which would complicate the Funcito internals to try to detect and manage it.
Dec 2, 2011
Changing priority down a notch, since Guava Predicate NPE is not inherently a Funcito problem
Labels:
-Priority-Critical Priority-High
Dec 6, 2011
I went ahead, per comment 11. Comments?
Status:
CodeComplete
Dec 8, 2011
I thought of a help to make the NPEs more understandable, by surrounding Predicate.apply()'s functionality in a try/catch and rethrowing an explanatory FuncitoException. Also, I would like to up our JUnit version to at least 4.9 (maybe 4.10) if you don't have an objection so we can take advanatage of some goodness (in particular, for this case and other exception-catching test cases, using the ExpectedException with @Rule. Much more advanced exception testing than @Exception(expects=...)
Status:
Started
Dec 8, 2011
+1 on try/catch. I don't understand if the point about JUnit applies to this issue? It is easy to change but jar versions should be its own issue. (I think we have one)
Dec 8, 2011
done
Status:
CodeComplete
Dec 12, 2011
closing
Status:
Fixed
|