OptionalBinder and Names.bindProperties

[Joshua Moore-Oliva]

First off - I am only a few days into guice, so I apologize if my questions are dumb - I have tried to compensate with what I hope are clear examples.

I am attempting to make liberal use of OptionalBinder in my code to allow some default values to be specified and then potentially overriden by configuration. I have run into a cascading series of issues. First, lets start with what works in a simple case that currently works (but does not have the default value functionality I hope to achieve with OptionalBinder):

public class Simple {

    private final int simpleInt;

    private final String simpleStr;

    @Inject

    public Simple(@Named("simpleInt") int simpleInt, @Named("simpleStr") String simpleStr) {

        this.simpleInt = simpleInt;

        this.simpleStr = simpleStr;

    }

    public int getSimpleInt() {

        return simpleInt;

    }

    public String getSimpleStr() {

        return simpleStr;

    }

}

If I wish to wire up the value of simpleVal from configuration, I can use Names.bindProperties (which was recommended in the FAQ)

public class SimpleTest {

    @Test

    public void testGetSimpleVal() throws Exception {

        Injector inj = Guice.createInjector(getTestModule());

        Simple s = inj.getInstance(Simple.class);

        assertEquals(1, s.getSimpleInt());

        assertEquals("default", s.getSimpleStr());

    }

    private Module getTestModule() {

        return new AbstractModule() {

            @Override protected void configure() {

                bind(Simple.class);

                Properties p = new Properties();

                p.setProperty("simpleInt", "1");

                p.setProperty("simpleStr", "default");

                Names.bindProperties(binder(), p);

            }

        };

    }

}

So far, so good - Names.bindProperties is great, it even converts my strings to ints ("1" -> 1 for simpleInt)

Now I attempt to set up default values for simpleInt and simpleStr using OptionalBinder. I do this by returning another Module with default bindings. (I assume I should do this on a package not class level, but it keeps the example simple).

public class Simple {

    private final int simpleInt;

    private final String simpleStr;

    @Inject

    public Simple(@Named("simpleInt") int simpleInt, @Named("simpleStr") String simpleStr) {

        this.simpleInt = simpleInt;

        this.simpleStr = simpleStr;

    }

    public int getSimpleInt() {

        return simpleInt;

    }

    public String getSimpleStr() {

        return simpleStr;

    }

    public static AbstractModule getDefaultsModule() {

        return new AbstractModule() {

            @Override protected void configure() {

                 OptionalBinder.newOptionalBinder(binder(), Key.get(Integer.class, Names.named("simpleInt" )))

                        .setDefault().toInstance(12345);

                //OptionalBinder.newOptionalBinder(binder(), Key.get(String.class, Names.named("simpleInt" )))

                //        .setDefault().toInstance("12345");

                OptionalBinder.newOptionalBinder(binder(), Key.get(String.class, Names.named("simpleStr")))

                        .setDefault().toInstance("Simple class default");

            }

        };

    }

}

public class SimpleTest {

    @Test

    public void testGetSimpleVal() throws Exception {

        Injector inj = Guice.createInjector(Simple.getDefaultsModule(), getTestModule());

        Simple s = inj.getInstance(Simple.class);

        assertEquals(1, s.getSimpleInt());

        assertEquals("default", s.getSimpleStr());

    }

    private Module getTestModule() {

        return new AbstractModule() {

            @Override protected void configure() {

                bind(Simple.class);

                Properties p = new Properties();

                p.setProperty("simpleInt", "1");

                p.setProperty("simpleStr", "default");

                Names.bindProperties(binder(), p);

            }

        };

    }

}

At this point, I get the following error

1) A binding to java.lang.String annotated with @com.google.inject.name.Named(value=simpleStr) was already configured at com.baselayer.device.scratch.Simple$1.configure(Simple.java:42).

  at com.baselayer.device.scratch.SimpleTest$1.configure(SimpleTest.java:33)

Looking into the source code for Names.bindProperties, I see this is because it does not use setBinding, but rather normal non-optional .toInstance.

binder.bind(Key.get(String.class, new NamedImpl(propertyName))).toInstance(value);

At this point, I assume there is some good reason that you must use setBindings, though I will make the suggestion that as a new user of guice, I would expect a normal non-optional .toInstance to override a .setDefault.

So, shamelessly copying the source code of Names.bindProperties, I created my own replacement test module as follows:

    private Module getTestModule() {

        return new AbstractModule() {

            @Override protected void configure() {

                bind(Simple.class);

                Properties p = new Properties();

                p.setProperty("simpleInt", "1");

                p.setProperty("simpleStr", "default");

                //Names.bindProperties(binder(), p);

                //The following code replaces Names.bindProperties

                Binder skippedBinder = binder().skipSources(Names.class);

                for (Enumeration<?> e = p.propertyNames(); e.hasMoreElements(); ) {

                    String propertyName = (String) e.nextElement();

                    String value = p.getProperty(propertyName);

                    OptionalBinder.newOptionalBinder(skippedBinder, Key.get(String.class, Names.named(propertyName))).setBinding().toInstance(value);

                }

            }

        };

    }

However - unlike Names.bindProperties, the String binding will not stick to simpleInt.  The very useful auto-mutation of String to Int that occurs with non-optional .toInstance does not appear with .setBinding. Either I get 

1) No implementation for java.lang.Integer annotated with @com.google.inject.name.Named(value=simpleInt) was bound.

  while locating java.lang.Integer annotated with @com.google.inject.name.Named(value=simpleInt)

    for parameter 0 at com.baselayer.device.scratch.Simple.<init>(Simple.java:20)

  at com.baselayer.device.scratch.SimpleTest$1.configure(SimpleTest.java:27)

If I do not have any .setDefault bound for simpleInt in Simple.getDefaultsModule (neither Integer.class or String.class), or I get

Failed tests:   testGetSimpleVal(com.baselayer.device.scratch.SimpleTest): expected:<1> but was:<12345>

If I had bound an Integer in Simple.getDefaultsModule with

OptionalBinder.newOptionalBinder(binder(), Key.get(Integer.class, Names.named("simpleInt" )))

                        .setDefault().toInstance(12345);

Finally - If I use 

OptionalBinder.newOptionalBinder(binder(), Key.get(String.class, Names.named("simpleInt" )))

                        .setDefault().toInstance("12345");

But do NOT overwrite it with setBinding(), then I get the following error

1) No implementation for java.lang.Integer annotated with @com.google.inject.name.Named(value=simpleInt) was bound.

  while locating java.lang.Integer annotated with @com.google.inject.name.Named(value=simpleInt)

    for parameter 0 at com.baselayer.device.scratch.Simple.<init>(Simple.java:20)

  at com.baselayer.device.scratch.SimpleTest$1.configure(SimpleTest.java:30) (via modules: com.google.inject.util.Modules$OverrideModule -> com.baselayer.device.scratch.SimpleTest$1)

It appears that guice will bind a String to an int with the normal, non-optional .toInstance, but .setDefault  .toInstance refuses to bind a String to an int with the OptionalBinder. (but .setBinding .toInstance will set a String to an Integer, as long as a separate specific Integer binding does not already exist).

If .setDefault and .setBinding .toInstance had the same semantics as normal, non-optional .toInstance, I could make my own Names.bindProperties and use that everywhere instead (with the intent being that if any default was already set, I would overwrite it)

However, the only thing I have tried that works is to attempt to bind to every single instance (ensuring that the exact type has been bound), something like this for getTestModule()

    private Module getTestModule() {

        return new AbstractModule() {

            @Override protected void configure() {

                bind(Simple.class);

                Properties p = new Properties();

                p.setProperty("simpleInt", "1");

                p.setProperty("simpleStr", "default");

                //Names.bindProperties(binder(), p);

                Binder skippedBinder = binder().skipSources(Names.class);

                for (Enumeration<?> e = p.propertyNames(); e.hasMoreElements(); ) {

                    String propertyName = (String) e.nextElement();

                    String value = p.getProperty(propertyName);

                    OptionalBinder.newOptionalBinder(skippedBinder, Key.get(String.class, Names.named(propertyName))).setBinding().toInstance(value);

                    try {

                        int i = Integer.parseInt(value);

                        OptionalBinder.newOptionalBinder(skippedBinder, Key.get(Integer.class, Names.named(propertyName))).setBinding().toInstance(i);

                    } catch (NumberFormatException asdf) {

                    }

                }

            }

        };

    }

In order to bind a configuration file's properties to @Named instances, I need to attempt to convert the String to each possible type if I wish to load dynamically without some kind of type metadata in the configuration file as well to know which type I should target the binding to.

Having outlined all this, a few questions

1) Is it feasible for the behaviour of OptionalBinder to allow a .toInstance to override a .setDefault? If so then things like Names.bindProperties could (maybe) just work as is. If not, why? (I assume there is a good reason since .to was explicitly disallowed as an override for .setDefault in the documentation)

2) What is the intended behaviour of Names.bindProperties.  Are strings supposed to be able to inject onto a Integer? Or only inject if a specific Integer injection doesn't already exist? (Are users supposed to be able to inject different String and Integer toInstance values for a Named annotation?)  Is this behaviour I am experiencing via .setDefault something that hasn't been fully thought through, or is there a ruleset for when some types are supposed to ovverride other types when a more specific mapping doesn't already exist for the annotation?

        Experimenting with Modules.override it seems that normal non-optional .toInstance starts behaving similarly if the defaults module binds both an Integer and a String, and the overriding module just binds a string (As Names.bindProperties).

3) Are there any downsides to just OptionalBinding any configuration file property to a variety of common primitive types to ensure that all default injection parameters gets overridden by configuration?

4) If I just want default values (without the Optional<Foo> support), should I be using Modules.override instead?

5) Is setDefault working as intended, given that is has different semantics to .setBinding (.setDefault .toInstance String.class will not inject a String to an Integer in the absence of a Integer injection, but .setBinding .toInstance will just like normal non-optional .toInstance)

Thanks for reading if you made it this far in my rocky start to DI in java :)

Josh

[Laszlo Ferenczi]

Hi Josh,

Using the binding mechanism in guice might be a bit of an overkill for simple property bindings, however you have a number of alternatives:

- Properties itself allows for defaults handling, create a property file with the defaults and create one for the application and merge them in the properties object. Binding the resulting properties will have the defaults.

- Bind the properties object itself and manually fetch the values from it

- Specify defaults in the code, only override when you get a meaningful value injected

A more complex way would be to roll your own configuration management. 

In our projects we use an in-house developed ConfigManager object which is similar to the Properties object but it provides typesafe getters for various primitive types. A generic TypeListener allows the injections of config values into the object. (conceptually very similar of what netflix's governator does)

Obviously you can use the governator library also, it'll do the trick (and many others). I believe apache onami has a solution for configuration too.

Hope this helps.

--
L

--
You received this message because you are subscribed to the Google Groups "google-guice" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-guice...@googlegroups.com.
To post to this group, send email to google...@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/0c3bb455-d65d-4ed4-bf7d-e84d5f84f7af%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Joshua Moore-Oliva]

Thanks, that helped me ramp up on the guice ecosystem and configuration.

However - the fact that setDefault has different binding coercion semantics to setBinding smells like a bug to me. Is there any reason that its current behaviour is the correct one?  If not I might file a bug report.

[Laszlo Ferenczi]

Josh,

Well, let's see some official opinion on this matter, i'm just a user of guice. 

--
L

To view this discussion on the web visit https://groups.google.com/d/msgid/google-guice/a88525ad-a952-4364-9e30-d6bce7ea173a%40googlegroups.com.

[Joshua Moore-Oliva]

While I was creating a simpler test case for the bug report, it appears that .setDefault and .setBinding behave consistently wrt each other, but violate rule/step #5 in BindingResolution  https://github.com/google/guice/wiki/BindingResolution .

I filed an issue for my own personal curiosity here: https://github.com/google/guice/issues/901 

Will be interesting to see the official opinion.

Thanks for your help!