WIP: ConditionalBinder #256

elandau · 2015-12-11T18:07:39Z

The ConditionalBinder enables specifying multiple candidates for a binding with each having a condition that can be based on configuration or other state. ConditionalBinder follows an implementation very similar to Multibinder in that all bindings are created with custom annotations and are resolved at injector creation time.

For example,

public class SnacksModule extends AbstractModule {
    protected void configure() {
        ConditionalBinder<Snack> conditionalbinder
          = ConditionalBinder.newConditionalBinder(binder(), Snack.class);
        conditionalbinder.when(new ConditionalOnProperty("type", "twix")).toInstance(new Twix());
        conditionalbinder.when(new ConditionalOnProperty("type", "snickers")).toProvider(SnickersProvider.class);
        conditionalbinder.when(new ConditionalOnProperty("type", "skittles")).to(Skittles.class);
        conditionalbinder.whenNone().to(Carrots.class);
     }
}

The following situations are not valid and result in a CreationException error at injector creation (not injection) time. The exception will contain detailed information on the candidate bindings and where in code they were bound

Multiple matching candidates
No matched candidates
Multiple default bindings specified
Using conditional binding in Stage.PRODUCTION

Several approaches were considered for implementing this feature, including a Module annotation based approach. The benefits of this approach are,

Follows Guice conventions (such as Multibinder and MapBinder)
Does not require Module post processing (which would complicate usage)
Makes conditional processing injectable

Drawbacks to this approach are,

Does not work in Stage.PRODUCTION (results in unmatched conditionals being instantiated). Allowing this to run in production stage is possible but would add complexity that may not be necessary since we run only in the lazy Stage.DEVELOPMENT. We can revisit this if running in Stage.PRODUCTION becomes are requirement.
Unmatched conditionals result in unused bindings

…single type

tbak · 2015-12-14T23:07:54Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalBinder.java

+     * <p>Scoping elements independently supported per binding. Use the {@code in} method
+     * to specify a binding scope.
+     */
+    public abstract LinkedBindingBuilder<T> whenMatch(Conditional obj);


What about adding operators to compose Conditionals (and, or/any, not)? Implementing this explicitly (by wrapping into higher level Conditional object) would be cumbersome, due to the way injections are handled.

Guice's Matcher has an abstract implementation that includes 'and' and 'or'. We could follow the same pattern here.

I updated the PR with support for AND, OR and NOT operations on conditionals. I also simplified the API by removing the Matcher class and making dependencies required for evaluating a conditional directly injectable into the conditional via member injection.

…njection of any dependency needed by the conditional directly into the conditional. Add AND, OR, NOT operations on conditionals.

jhspaybar · 2015-12-15T06:07:00Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalBinder.java

+     * <p>Scoping elements independently supported per binding. Use the {@code in} method
+     * to specify a binding scope.
+     */
+    public abstract LinkedBindingBuilder<T> whenNoMatch();


Can't we do this with whenMatch(new DefaultBindingConditional()) or something like that to avoid the special case?

This seems like a very long and ambiguous way of specifying the default and is not in the sprit of a DSL. DefaultBindingConditional could mean several things, is it the default implementation of a conditional or is it the conditional to use as the default.

whenNone is just another conditional though, maybe call it whenMatch(new ConditionalOnNoOtherMatches())?

Either way, ConditionalOnNoOtherMatches() would require special handling since it would require knowledge of how all other conditionals have been processed. IMO it's much cleaner to just track this as an explicit special default case.

How do I do something like whenMatch(Conditional.and(new ConditionOnNoOtherMatches(), new ConditionalOnProfile("prod")) if this is kept as a special case?

…will then be the responsibility of a composite conditional (AndConditional, etc.) to pass the Injector to its children, giving them access to the injector.

…ls class. Rename 'and' to 'allOf', and 'or' to 'anyOf'.

qiangdavidliu · 2015-12-16T23:48:22Z

governator-core/src/main/java/com/netflix/governator/conditional/Conditional.java

+ */
+public interface Conditional {
+    /**
+     * Evaluate whether the condition is true.  evaluate() is only called once at injector


javadoc update

tcellucci · 2016-11-22T22:10:51Z

governator-core/src/main/java/com/netflix/governator/PropertiesPropertySource.java

 import com.google.inject.Module;
 import com.google.inject.Provides;
 import com.netflix.governator.spi.PropertySource;

-public class PropertiesPropertySource extends AbstractPropertySource {
+public final class PropertiesPropertySource extends AbstractPropertySource implements Module {


Gak. Properties is legacy Java with all synchronized methods. Why do we want this in 2016, and not a Map<String,String>?

tcellucci · 2016-11-22T22:12:04Z

governator-core/src/main/java/com/netflix/governator/PropertiesPropertySource.java

+        if (getClass() != obj.getClass())
+            return false;
+
+        throw new RuntimeException("Only one PropertiesModule may be installed");


IllegalStateException would be clearer; usually a bad idea to throw RuntimeException

tcellucci · 2016-11-22T22:17:55Z

governator-core/src/main/java/com/netflix/governator/conditional/AllOfConditional.java

+public class AllOfConditional implements Conditional {
+    private final List<Conditional> children;
+
+    public AllOfConditional(List<Conditional> children) {


should be able to take any Iterable or Collection as input, and add a varargs constructor too?

tcellucci · 2016-11-22T22:19:59Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalBinder.java

+import java.util.List;
+import java.util.Set;
+
+import com.google.common.collect.ImmutableList;


do we need to use these google collections classes any more (with Java 8)?

tcellucci · 2016-11-22T22:20:22Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalBinder.java

+
+    static <T> ConditionalBinder<T> newRealConditionalBinder(Binder binder, Key<T> key) {
+        if (binder.currentStage().equals(Stage.PRODUCTION)) {
+            throw new RuntimeException("ConditionalBinder may not be used in Stage.PRODUCTION.  Use Stage.DEVELOPMENT.");


IllegalStateException or IllegalArgumentException, take your pick

tcellucci · 2016-11-22T22:21:52Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalBinder.java

+        @Override
+        public List<Binding<?>> getCandidateElements() {
+            if (isInitialized()) {
+                return (List<Binding<?>>) (List<?>) candidateBindings; // safe because bindings is immutable.


double cast?

tcellucci · 2016-11-22T22:27:45Z

governator-core/src/main/java/com/netflix/governator/conditional/ConditionalElementImpl.java

+    }
+
+    @Override
+    public boolean equals(Object o) {


seems flawed that Condition is not part of the equality check, but is part of the object state and passed in via constructor. I would include at least an identity comparison for Condition here.

tcellucci · 2016-11-22T22:33:30Z

governator-core/src/main/java/com/netflix/governator/conditional/Conditionals.java

+/**
+ * Utility class for creating conditional
+ */
+public abstract class Conditionals {


no need for 'abstract', it's not instantiable (using java.util.Collections for comparison)

tcellucci · 2016-11-22T22:35:36Z

governator-core/src/test/java/com/netflix/governator/conditional/ConditionalBinderTest.java

+
+    @Singleton
+    static class SingletonFoo implements Foo {
+        static int injectedCount = 0;


tcellucci · 2016-11-22T22:40:56Z

I didn't see any unit tests that use the @conditional[Anything]; annotation-based conditionals are supported yes?

elandau added 2 commits December 11, 2015 09:52

New ConditionalBinder to simplify specifying multiple bindings for a …

8182c82

…single type

fix javadoc

0fac88c

tbak reviewed Dec 14, 2015
View reviewed changes

Simplify Conditional API by replacing the Matcher class with member i…

b0e47d0

…njection of any dependency needed by the conditional directly into the conditional. Add AND, OR, NOT operations on conditionals.

jhspaybar reviewed Dec 15, 2015
View reviewed changes

elandau added 2 commits December 15, 2015 14:47

Change Conditional#matches to accept the Injector as an argument. It …

c51ed37

…will then be the responsibility of a composite conditional (AndConditional, etc.) to pass the Injector to its children, giving them access to the injector.

Move and, or, and not conditional wrappers into a separate Conditiona…

efac56b

…ls class. Rename 'and' to 'allOf', and 'or' to 'anyOf'.

qiangdavidliu reviewed Dec 16, 2015
View reviewed changes

tcellucci reviewed Nov 22, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: ConditionalBinder #256

WIP: ConditionalBinder #256

elandau commented Dec 11, 2015

tbak Dec 14, 2015

elandau Dec 14, 2015

elandau Dec 15, 2015

jhspaybar Dec 15, 2015

elandau Dec 15, 2015

jhspaybar Dec 15, 2015

elandau Dec 15, 2015

jhspaybar Dec 16, 2015

qiangdavidliu Dec 16, 2015

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016 •

edited

Loading

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci Nov 22, 2016

tcellucci commented Nov 22, 2016 •

edited

Loading

WIP: ConditionalBinder #256

Are you sure you want to change the base?

WIP: ConditionalBinder #256

Conversation

elandau commented Dec 11, 2015

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

tcellucci Nov 22, 2016 • edited Loading

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

tcellucci commented Nov 22, 2016 • edited Loading

tcellucci Nov 22, 2016 •

edited

Loading

tcellucci commented Nov 22, 2016 •

edited

Loading