dagger icon indicating copy to clipboard operation
dagger copied to clipboard

Support Map bindings of some sort.

Open cgruber opened this issue 12 years ago • 9 comments

@swankjesse and I talked about this a while back. Some quests for predictable order in set bindings (see: #252) suggest the need for something more like map bindings.

The goal is to allow someone to do:

@Inject Map<SomeKeyType, SomeValueType> myStuff;

then contribute bindings in modules, such that the contributions (like a set binding) would be woven together into the above map.

They key issue is how to create a configuration API suitable for this that is (ideally) statically analyzable, and not horrible. The analyzability of the key is a secondary goal - we currently can't detect duplicate set contributions either, at compile time. But it's nice if we could do it.

Some API thoughts we had were


// limitation - @Key's value can only take a primitive types or static types (enums)
// and we have to choose it up-front.
// pro - can statically analyze the key to detect dupes, etc.
@Module(...)
class MyModule {
  @Provide(type=MAP_VALUE) @Key("foo") Plugin provideFooPlugin() { ... }
}

// pro: flexible, no custom type required, con: ugly as hell
// con: key not analyzable. 
@Module(...)
class MyModule {
  @Provide(type=MAP_VALUE) Map.Entry<String,Plugin> provideFooPlugin() { 
    new Map.Entry<>() {
       // provide the values via an inner impl.
    }
  }
}

// pro: flexible, triviial impl provided, con: name sucks. SUCKS!
// con: no static analysis on key.
@Module(...)
class MyModule {
  @Provide(type=MAP_VALUE) MappedValue<String,Plugin> provideFooPlugin() {
     new MappedValue<>(key, value)...
  }
}

// dislike this one - all the disadvantages of annotations, all the ugly, all in one. 
@Module(...)
class MyModule {
  @Provide(type=MAP_VALUE, key="Foo") provideFooPlugin() { ... }
}

These are just brain-storming fodder. Please...someone... anyone... do better.

cgruber avatar May 23 '13 23:05 cgruber

per issue #252, my main desire is for prioritized contributed bindings. I also like and would use Map bindings, and will think about it to see if this would work to solve prioritized, too.

codefromthecrypt avatar May 23 '13 23:05 codefromthecrypt

Updated the issue text slightly.

cgruber avatar May 23 '13 23:05 cgruber

I'm in favor of the first option, particularly for a configuration api, it is more important to me that the value can be complex than its key.

  @Provide(type=MAP_VALUE) @Key("foo") Plugin provideFooPlugin() { ... }

codefromthecrypt avatar May 23 '13 23:05 codefromthecrypt

I like the "worst" one the best:

  @Provides(type=MAP_VALUE, key="Foo") Handler provideFooPlugin() { ... }

Strings are enough for everything I've had to do. More sophisticated apps can use strings and create their own adapter bindings like this:

  @Provides Map<Identifier, Handler> provideFooPlugin(Map<String, Handler> stringMap) {
    // use stringMap to create the other map
  }

Upsides

  • No confusion between qualifier annotations and map key annotations
  • Statically checkable
  • Fewer types and less code

swankjesse avatar May 24 '13 06:05 swankjesse

All my current use cases are fine with "worst" fwiw. I also like the less types thing, and am ok with the extra key attribute on the Provides annotation only to permit map bindings, as javadoc can easily tell people "only when type is MAP"

On Thursday, May 23, 2013, Jesse Wilson wrote:

I like the "worst" one the best:

@Provides(type=MAP_VALUE, key="Foo") Handler provideFooPlugin() { ... }

Strings are enough for everything I've had to do. More sophisticated apps can use strings and create their own adapter bindings like this:

@Provides Map<Identifier, Handler> provideFooPlugin(Map<String, Handler> stringMap) { // use stringMap to create the other map }

Upsides

  • No confusion between qualifier annotations and map key annotations
  • Statically checkable
  • Fewer types and less code

— Reply to this email directly or view it on GitHubhttps://github.com/square/dagger/issues/257#issuecomment-18387972 .

codefromthecrypt avatar May 24 '13 15:05 codefromthecrypt

I'm trying to think of whether there are possible use-cases that are common enough that couldn't support a string key. I am generally in favour of reducing the complexity, and forcing static string keys would definitely do so. But it would also reduce any and all dynamism, in a way that's even more restrictive than Set binder.

My problem with this is that then we are managing the content of the graph, not the structure... though arguably we decided to do that with making contributions to sets as well. But with set binder we've already effectively decided that we're ok with a run-time exception for duplicates. I wonder if we shouldn't be ok with that here. The value of this is reduced, I think, if people can't dynamically create keys and values, just like the value of the setbinder is reduced if people can't generate set contents at runtime.

I don't know... I'm suddenly quite wishy-washy on this whole topic. :( I think I'm just worried about creating a feature that will have limited utility if we make it too dumb. But I'm also not sitting looking at specific use-cases. Maybe we should just keep this open, and ask for more use-cases before deciding. So two questions.

  1. Are there any use-cases that simply couldn't use string keys, or string-mapped-to-something the way Jesse suggests above?
  2. Are static string keys sufficient? (That is, are there use-cases which would require dynamically generated keys, that we are willing to support?)

And let's not pre-judge them and say, "too complicated." Let's see if any (or sufficient numbers) exist, and whether or not they could be re-cast to work with this approach.

cgruber avatar May 24 '13 17:05 cgruber

There's a lot of power you can achieve with multibindings. Guice's servlet module uses multibindings-like magic to accomplish a lot of flexibility with very little code. Imagine this:

List<Object> modules = list(
  new UsModule(),
  new GenericCountryModule("canada", "CAD", "Beavers", "en-US"),
  new GenericCountryModule("france", "EUR", "Frogs", "fr-FR"),
);

swankjesse avatar May 24 '13 17:05 swankjesse

I've been playing around quite a bit, and one thing that seems quite easy (in principle) to do with map bindings, but not in set bindings, is to override by key. Here's the current workaround I am using. I'm totally cool with the first 3 providers, as I can simply override in a module if I choose to use a different impl per key. As the boilerplates to achieve this are so error-prone, I'm considering code gen, if we make a strict decision to never support map binding. I understand the "just fork and do whatever" mantra, just that I still don't want to maintain my own DI library just because it makes map binding really really hard to achieve. It would be a better pain to codegen the boilerplate, I guess, as the responsibility is less.

        @Provides
        @Named("CloudIdentity")
        Decoder cloudIdentityDecoder() {
            return elementsAreGroups(
                    "^.*token[\":{\\s]+id\":\"([^\"]+)\"(.*cloudDNS\"[^\\]]+publicURL\":\\s*\"([^\"]+)\")?", 3, 1);
        }

        @Provides
        @Named("CloudDNS#nameToIds()")
        Decoder nameToIdsDecoder() {
            return multimapEntriesAreGroups("\"name\"[:\\s]+\"([^\"]+)\"[,:\\s]+\"id\"[:\\s]+([0-9]+)", 1, 2);
        }

        @Provides
        @Named("CloudDNS#idNameType(String)")
        Decoder idNameTypeDecoder() {
            return cellsAreGroups(
                    "\"name\"[:\\s]+\"([^\"]+)\"[,:\\s]+\"id\"[:\\s]++\"([^\"]+)\"[,:\\s]+\"type\"[:\\s]++\"([^\"]+)\"",
                    2, 1, 3);
        }

        // workaround until map binding is added to dagger
        @Provides(type = SET)
        Config<Decoder> cloudIdentityDecoder(@Named("CloudIdentity") Decoder cloudIdentityDecoder) {
            return Config.create("CloudIdentity", cloudIdentityDecoder);
        }

        @Provides(type = SET)
        Config<Decoder> nameToIdsDecoder(@Named("CloudDNS#nameToIds()") Decoder nameToIdsDecoder) {
            return Config.create("CloudDNS#nameToIds()", nameToIdsDecoder);
        }

        @Provides(type = SET)
        Config<Decoder> addIdNameTypeDecoder(@Named("CloudDNS#idNameType(String)") Decoder idNameTypeDecoder) {
            return Config.create("CloudDNS#idNameType(String)", idNameTypeDecoder);
        }

codefromthecrypt avatar Jun 09 '13 15:06 codefromthecrypt

+1. I am in the midst of needing this feature, and luckily its already here as an enhancement.

To me, having compile-time key-analysis is certainly nice to have, but not a necessity. The first run could/would throw an error on first injection if the created map has duplicate keys. Perhaps that could be a Module configuration option to throw an error or not, or based on existing "overrides=true" to not throw the error.

bentatham avatar Sep 19 '13 17:09 bentatham