Why use a BiFunction when the desired interface was a BiPredicate?

[jeffque]

Describe the feature request

In the Unleash interface, there are some methods with a BiFunction<String, UnleashContext, Boolean> argument. But the availables implementations for Unleash (DefaultUnleash, FakeUnleash) both have no safe measures against this function returning a null. Bot will yield a NullPointerException:

DefaultUnleash.java#L123-L127

        boolean enabled;
        UnleashContext enhancedContext = context.applyStaticFields(config);

        if (featureToggle == null) {
            enabled = fallbackAction.apply(toggleName, enhancedContext);

FakeUnleash.java#L43-L46:

    public boolean isEnabled(
            String toggleName, BiFunction<String, UnleashContext, Boolean> fallbackAction) {
        if (!features.containsKey(toggleName)) {
            return fallbackAction.apply(toggleName, UnleashContext.builder().build());

Also, it is more semantic to use Predicate<T> or similar instead of Function<T, Boolean> or similar.

Background

Using Predicate<T> instead of Function<T, Boolean> is a lot more semantic, and also avoids (some) random null pointer exceptions that the java type system allows.

If the null pointer exception happens for some reason, it will be a violation of contract of the API consumer, not of the API provider, as it is now.

Solution suggestions

Change the API to accept a BiPredicate instead of a BiFunction.

It is wildly expected that it shall be ok with the use of lambdas. It will break compatibility if someone passes directly an instance of BiFunction<String, UnleashContext, Boolean>, as the tests within here have broken.

Trying to accept both BiPredicate<String, UnleashContext> and BiFunction<String, UnleashContext, Boolean> will cause the compiler to get confused, as both will satisfy the lambda signature. For example, in DefaultUnleash will yield a compiler error:

    @Override
    public boolean isEnabled(
            final String toggleName, final UnleashContext context, final boolean defaultSetting) {
        return isEnabled(
                toggleName,
                context,
                (n, c) -> defaultSetting);
    }

This will require a manual action of the maintainer to specify that it is a BiPredicate that it is wanted:

    @Override
    public boolean isEnabled(
            final String toggleName, final UnleashContext context, final boolean defaultSetting) {
        return isEnabled(
                toggleName,
                context,
                (BiPredicate<String, UnleashContext>) (n, c) -> defaultSetting);
    }

[thomasheartman]

Hey, @jeffque 👋🏼 Thanks for opening the issue and for the thorough explanation 🙌🏼 From what I can tell, this seems like a very sensible change. However, as you mention in the PR description, this "might break some API consumers if they are using directly the interface BiFunction instead of a lambda". That means this would be a breaking change require a major version bump, so it requires a little more thought than it otherwise would.

I'm gonna take this and discuss it with some of the more Java-oriented of our devs and get back to you. Thanks again!