unleash-client-java icon indicating copy to clipboard operation
unleash-client-java copied to clipboard

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

Open jeffque opened this issue 3 years ago • 0 comments

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

ambiguous method call, both functional interfaces match

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

jeffque avatar Oct 17 '22 18:10 jeffque