My favorites | Sign in
Project Home Downloads Wiki Issues Source
READ-ONLY: This project has been archived. For more information see this post.
Search
for
  Advanced search   Search tips   Subscriptions
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
Status:  Fixed
Owner:  kandpwel...@gmail.com
Closed:  Dec 2011
Cc:  codeto...@gmail.com


 
Project Member Reported by kandpwel...@gmail.com, Nov 21, 2011
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
Doh!  Guava Predicate.apply() contract requires return type of boolean primitive.  So the real issue here should be what is the expected behavior?
  * Throw an exception?
  * Prevent mocking of Boolean-return methods?
  * Prevent mocking of Boolean-return methods that are annotated by Guava @Nullable annotation?
  * Treat "null" return from method as return "false" from apply
  * other
Nov 21, 2011
Project Member #2 kandpwel...@gmail.com
(No comment was entered for this change.)
Cc: codeto...@gmail.com
Nov 22, 2011
Project Member #4 kandpwel...@gmail.com
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
Project Member #5 kandpwel...@gmail.com
(No comment was entered for this change.)
Labels: Usability
Nov 23, 2011
Project Member #6 kandpwel...@gmail.com
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
Project Member #7 codeto...@gmail.com
My vote is for the first proposal in comment 4.
Nov 25, 2011
Project Member #8 codeto...@gmail.com
Will MethodFunction.apply() be a problem for nullable Integer, Float, etc ? Is this a general problem with auto-boxing?
Nov 29, 2011
Project Member #9 kandpwel...@gmail.com
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
Project Member #10 kandpwel...@gmail.com
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
Project Member #11 kandpwel...@gmail.com
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
Project Member #12 kandpwel...@gmail.com
Changing priority down a notch, since Guava Predicate NPE is not inherently a Funcito problem
Labels: -Priority-Critical Priority-High
Dec 6, 2011
Project Member #13 kandpwel...@gmail.com
I went ahead, per comment 11.  Comments?
Status: CodeComplete
Dec 8, 2011
Project Member #14 kandpwel...@gmail.com
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
Project Member #15 codeto...@gmail.com
+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
Project Member #16 kandpwel...@gmail.com
done
Status: CodeComplete
Dec 12, 2011
Project Member #17 kandpwel...@gmail.com
closing
Status: Fixed

Powered by Google Project Hosting