javax-inject / javax-inject

JSR-330 Dependency Injection standard for Java
http://javax-inject.github.io/javax-inject/
118 stars 34 forks source link

Should we support multiple qualifier annotations? #2

Closed cgruber closed 8 years ago

cgruber commented 8 years ago

From crazybob...@gmail.com on June 16, 2009 13:07:16

Options:

a) Only allow 0 or 1 b) Allow injectors to support more than one but require that they support 0 or 1 c) Require injectors to support N qualifiers

Original issue: http://code.google.com/p/atinject/issues/detail?id=2

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 16, 2009 13:26:48

IMO, there are some complex cases which just work out better with multiple qualifiers.

class Robot {

 @Metal Head head;
 @Left @Metal Leg leftLeg;
 @Right @Metal Leg rightLeg;

}

this example does not read better if we force you to use @LegKind(side=LEFT, material=METAL).

Qualifiers are adjectives. Even English lets you say things like "big red truck".

cgruber commented 8 years ago

From crazybob...@gmail.com on June 16, 2009 16:45:21

There's no doubt that having multiple qualifier support is very nice when you need it.

But here's the rub. Most of the time, you don't use a qualifier at all; the type alone is sufficient. The cases where you need one qualifier are relatively rare--I'd estimate less than 10% of cases, but Jesse Wilson might be able to provide a more exact number based on usage at Google. The cases where you need more than one qualifier are much more rare.

Supporting multiple qualifiers complexifies our core model and makes all code, even code that doesn't actually use multiple qualifiers, more difficult to reason about.

If we support just zero or one qualifiers, the core model is a simple 1:1 mapping from a type T and an optional qualifier Q to a provider of T:

[T, Q] -> Provider

If a programmer looks at an injection point of type "@ReadOnly DataSource", they just need to look up the unique entry corresponding to the key [DataSource, @ReadOnly] to find out what gets injected.

If we support multiple qualifiers, we'd move to a more complex query-based model. Instead of finding the one unique mapping to a [T, Q], the programmer will find all dependencies that match the type T and that have a superset of the qualifiers at the injection point. We'll need to determine what to do when you have multiple matches. You might generate an error. I don't like that adding new dependencies to your configuration can break client code that worked before.

While multiple qualifier support is very easy to specify, and implement and tool, it does create more work for programmers. When writing and maintaining code, the programmer has to understand and run through the resolution algorithm in their head. The simpler it is, the better. They'll have to look out for multiple qualifier annotations at injection points. They may not get an error if they accidentally apply more than one qualifier annotation when they only intended to apply one.

(Note: These same issues come up if we plan to resolve subtypes vs. only matching exact types.)

I just want us to fully consider these implications and tradeoffs before adding a feature that addresses such a small set of use cases. I don't think multiple qualifiers carries its own weight. I'd also like to maintain the simplicity of the 1:1 mapping-based model if at all possible.

Of course, if I'm wrong, we can easily add multiple qualifier support in a later release, but once we've done so, we can't take it back out. I'm not happy about the inconsistency of some injectors supporting only one qualifier and other supporting multiple qualifiers, but I really don't want to support multiple qualifiers in my own framework.

That said, I think supporting only one qualifier is the right choice for my users, but I'm OK with the annotations allowing for multiple qualifiers if others feel strongly about supporting it in their own frameworks.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 16, 2009 17:07:13

"The cases where you need one qualifier are relatively rare--I'd estimate less than 10% of cases"

Actually I think the case of one qualifier is much more common than that, since they are used with many very common infrastructure objects, e.g. Datasource, Connection, EntityManager, Topic, etc. But it is true that for application-defined types, one qualifier is rarer.

"The cases where you need more than one qualifier are much more rare."

Agreed. Which is why I don't buy the argument that they create much complexity for the user.

"Supporting multiple qualifiers complexifies our core model and makes all code, even code that doesn't actually use multiple qualifiers, more difficult to reason about."

I don't see how that can be true. If you mean it complexifies the implementation of the container, I don't care. If you mean it complexifies reasoning about cases with zero or one qualifier, I don't see how it can be true.

"If we support multiple qualifiers, we'd move to a more complex query-based model. Instead of finding the one unique mapping to a [T, Q], the programmer will find all dependencies that match the type T and that have a superset of the qualifiers at the injection point. We'll need to determine what to do when you have multiple matches. You might generate an error. I don't like that adding new dependencies to your configuration can break client code that worked before."

I don't understand. The problem you just described already exists as soon as you allow one qualifier.

Suppose I have a [Foo, -] and a [Foo, @Bar]. How do the issues you just described not already apply?

What is the difference between

(1) [Foo, -] + [Foo, @Bar], as already exists, and (2) [Foo, {@Bar}] + [Foo, {@Bar, @Baz}]

precisely the same ambiguities exist.

It sounds like you're just expressing doubt over the issue of "subset matching" vs. "set equality matching", but it sounds like 330 is using set equality matching already, so what is the big deal?

"When writing and maintaining code, the programmer has to understand and run through the resolution algorithm in their head."

I don't see why they need to "understand and run through" anything except when they decide to use multiple qualifiers. And they have to "understand and run through" exactly the same thing if they have one vs. zero qualifiers, as I argued above.

And I don't accept that it's harder to reason about this:

@Left @Metal Leg leftLeg;

than about this:

@LegQualifier(side=LEFT, material=METAL) Leg leftLeg;

the only real difference is that one is uglier.

"(Note: These same issues come up if we plan to resolve subtypes vs. only matching exact types.)"

Nope the subtypes vs. exact type matching debate is exactly analogous to the set equality vs. subset matching debate for qualifiers.

FTR I have zero problem with 330 going for a model of:

Since the alternative model of:

can easily be layered over the top. (You just re-conceptualize it as multiple bindings and you're done.)

"I'd also like to maintain the simplicity of the 1:1 mapping-based model if at all possible."

So specify set equality matching. Done.

"Of course, if I'm wrong, we can easily add multiple qualifier support in a later release"

299 is supporting this in this release, and so we need the JavaDoc of @Qualifier to reflect that.

cgruber commented 8 years ago

From limpbizkit on June 17, 2009 01:09:10

I searched through Google's corpus of Guice client code. For every 10 occurrences of @Inject, there are 2 occurrences of @Named.

If I (wildly) assume that each @Inject is used to inject 3 dependencies and that @Named represents half of all binding annotations, then 13% of the injected dependences are marked by a binding annotation. This is consistent with my experience.

cgruber commented 8 years ago

From limpbizkit on June 17, 2009 01:29:15

I don't mind multiple qualifiers. But subset matching is lame.

As is, even Guice gets this somewhat wrong. If the user injects '@Named("foo") Integer' and no such binding exists, we fall back to the parameterless version '@Named() Integer'. With such fault-tolerance my app works with this dependency: @Inject @Named("six") Integer lucky; ...and these bindings: bind(Integer.class).annotatedWith(Named.class).toInstance(5); bind(Integer.class).annotatedWith(Names.named("six")).toInstance(6); If I remove either of these bindings, my app still works! Horrible! That's utterly lame, and we should avoid making the same mistake in the new spec. Best-match injections are clumsy.

I'm not fussy about how many annotations we support, although in my experience one has always been enough.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 17, 2009 09:36:46

"@Named represents half of all binding annotations"

Gee a really hope not. Use of @Named as a qualifier is just lame and totally non-typesafe.

"If the user injects '@Named("foo") Integer' and no such binding exists, we fall back to the parameterless version '@Named() Integer'."

That's not subset matching. (Or at least it's not what I call subset matching.) Rather, it's just broken and sounds like a bug in Guice.

"That's utterly lame, and we should avoid making the same mistake in the new spec."

Right. But nobody is arguing for the behavior you just described.

cgruber commented 8 years ago

From roberto....@sun.com on June 18, 2009 16:19:18

I'm not convinced of the need for multiple qualifiers.

The argument by the English language is not a very good one, and neither is the example.

Adjectives in English (or any natural language, for that matter) form a tree structure, where annotations don't.

In the example, left and right legs are certainly not substitutable for each other, whereas a (@Wooden (@Left Leg)) could replace a (@Metal (@Left Leg)), albeit with diminished durability. This would call for separate LeftLeg and RightLeg types, if anything. The example would then include declarations like @Metal LeftLeg leftLeg; i.e. one qualifier.

The arguments by analogy with JPA or Bean Validation are not convincing either, since those annotations have completely different semantics.

Thus, I'm inclined to accept that types do the most in discriminating between things to inject, and one qualifier should be enough to deal with the rest.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 18, 2009 16:37:26

"Adjectives in English (or any natural language, for that matter) form a tree structure"

huh? I have never heard this claim before. Is this an actual scientific fact of descriptive linguistics, or are you just making this up? Link to peer-reviewed source, please.

We are not saying anything about substitutability here. "@Inject @Big @Red Truck truck" says "inject a truck that is big and red". Clearly that is a meaningful thing to say in both Java and English.

"The arguments by analogy with JPA or Bean Validation are not convincing either, since those annotations have completely different semantics."

I never made any such argument. The analogy I draw is with event processing. Clearly in event processing I want the ability to filter by multiple predicates. Example:

void protectedDocumentUpdated(@Observes @Updated @Protected Document doc) { ... }

As with injection, we need the ability to select between multiple objects of the same type based upon qualifiers (e.g. adjectives).

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 18, 2009 16:50:18

"@Inject @Big @Red Truck truck" says "inject a truck that is big and red".

And, of course, English let's me also say "inject a big red truck".

cgruber commented 8 years ago

From roberto....@sun.com on June 18, 2009 16:55:57

"Adjectives in English (or any natural language, for that matter) form a tree structure" huh? I have never heard this claim before. Is this an actual scientific fact of descriptive linguistics, or are you just making this up? Link to peer-reviewed source, please.

Chomsky's Universal Grammar. Here's an introduction: http://www.amazon.com/Chomskys-Universal-Grammar-Vivian- Cook/dp/0631195564.

Your observer example is interesting, but it looks odd too: using a Document as an event object? I don't think that annotations are a good way to build a query language in the first place.

cgruber commented 8 years ago

From jason.th...@gmail.com on June 18, 2009 17:26:52

Roberto,

I don't think phrase trees are really applicable here as we only have a "noun phrase", which of course has a set (not a tree) of modifiers (qualifiers).

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 18, 2009 17:41:56

Look, before the discussion gets dragged off into a debate over whether there is really enough experimental evidence for belief in the "universal grammar", can we just admit that Chomsky's theory doesn't really tell us much about the topic we're discussing?

The fact is that "big red truck" and "left metal leg" are perfectly valid noun phrases in both Java and English. And you can't argue from Chomsky that I should not be able to write down something in Java that I can write down in English. Java has a well-defined grammar, and this construct is allowed according to the grammar of Java.

Or are you seriously trying to tell me that we should require the user to create a new annotation named @PublicStaticInjectLeftMetal?

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 18, 2009 18:05:50

Jason, I think I've figured out that what Roberto was trying to argue was not that there is some kind of crazy tree structure that Chomsky discovered as part of his work on universal grammars, but just that the order of adjectives in a noun phrase is sometimes significant in natural languages (in some contexts). Which, of course, is true, and we don't need Chomsky to tell us this.

But Java developers all know that annotation order is not significant in Java, just as adjective order is often not significant in a natural language. It all depends on context, of course, but if I say "give me a big hot plate of fish", you immediately understand that a hot big plate of fish would also be perfectly satisfactory.

So I don't buy that any user will ever be confused into thinking that "@Left @Metal Leg" means something different to "@Metal @Left Leg". I've never heard of a Java developer thinking that "public static" means something different to "static public".

cgruber commented 8 years ago

From roberto....@sun.com on June 18, 2009 19:07:27

Gavin,

I think you are misunderstanding my point.

Initially, you said "qualifiers are adjectives", as if that proved anything. Obviously an annotation reads well in English when it's named after an adjective. At the same time, annotations are a coarse feature in the Java language: no subtyping, no ordering, no associativity (since you don't seem to like talking about tree structures...). Java types are a lot more expressive.

So I'm not at all surprised that anecdotal information on the usage of Guice shows that people are happy taking advantage of the richness of types and seldom need even one qualifier. The examples you produced don't look like something developers would actually write in those circumstances.

cgruber commented 8 years ago

From roberto....@sun.com on June 18, 2009 19:14:16

I've been trying to understand why multiple qualifiers seem so controversial.

My conclusion is that it all goes back to the different ways that injectors are configured in Guice and JSR-299.

In 299, injectors are configured automagically based on the beans that are available, whereas in Guice they are not. So I expect Guice developers to use the most concise description of a dependency that could possibly work, since they'll decide on how to satisfy it when they define their modules.

But in JSR-299 a developer may well find that the same dependency is ambiguous as written, due to too many eligible beans floating around. In order to disambiguate it, they'll have to add one more qualifier to the injection site. Hence the need for multiple qualifiers.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 18, 2009 19:24:56

I don't understand how English adjectives possibly have subtype relationships as part of the grammar of the language. The subtyping is purely at the semantic level, just like with Java annotations.

cgruber commented 8 years ago

From crazybob...@gmail.com on June 23, 2009 16:54:30

I have a compromise that I hope we can all live with. I removed mention of certain restrictions from the @Qualifier docs: http://atinject.googlecode.com/svn/trunk/javadoc/javax/inject/Qualifier.html Now, other specs and implementations should feel free to use qualifiers in other contexts and in different ways.

I added a section about qualifiers to the @Inject docs: http://atinject.googlecode.com/svn/trunk/javadoc/javax/inject/Inject.html It should now be clear that the single-qualifier restriction only applies to injection as defined by JSR-330. This should not inhibit other specifications and implementations from defining and supporting completely different injection approaches which may or may not utilize qualifiers.

Please reopen if you're not satisfied.

Status: Fixed

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 23, 2009 18:34:14

"I added a section about qualifiers to the @Inject docs:"

Well, that doesn't really solve the problem, since we were planning to adopt @Inject for injection in 299. So there is still an inconsistency with what 299 will say is possible.

Why not just say "injectors are not required to support multiple qualifiers per injection point".

cgruber commented 8 years ago

From tpeie...@gmail.com on June 23, 2009 19:49:53

Why not just say "injectors are not required to support multiple qualifiers per injection point".

Under that wording, if JSR-330-compliant implementation A does support multiple qualifiers, and someone writes code that takes advantage of that and later tries to switch to a JSR-330-compliant implementation B that does not support multiple qualifiers ... what happens?

cgruber commented 8 years ago

From crazybob...@gmail.com on June 24, 2009 15:15:34

Status: Accepted

cgruber commented 8 years ago

From michael....@gmail.com on June 25, 2009 08:44:22

The goal here is to draw the lines around these annotations so that both 299 and 330 can be compliant. This does not mean that one or the other needs to stick to compliance and cannot offer additional features, but that there is a standard of portability defined.

In this case we should simply define it so that it is clear that portable applications may only use one qualifier. This does not preclude an injector from being able to support multiple.

Can we agree on this?

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 09:08:49

"Can we agree on this?"

It's not my preference, but I can definitely live with it.

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 11:43:17

It sounds like everyone is satisfied.

Status: Fixed

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 11:49:59

Bob, what is the language you propose to express this?

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 12:04:12

Does the current language need to change? I think it accomplishes what Mike described.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 12:12:30

Currently it says:

"Qualifiers are optional and when used with @Inject, no more than one qualifier should annotate a single field or parameter."

It would be OK if it said:

"Qualifiers are optional and injectors are not required to support use of more than one qualifier with @Inject."

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 12:44:27

If a user wants to write code that's portable across 330 injectors, they must use no more than one qualifier.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 12:55:26

That's not what it says. It says "no more than one qualifier should annotate a single field or parameter", without any qualification.

Since we are sharing this annotation between multiple specs (this is going into common annotations), and since use of multiple qualifiers is supported and portable in some of these specs (299), it must be clear in precisely what circumstances this restriction applies.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 12:56:14

I suppose we need to reopen this issue.

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 13:01:13

For future reference, you're a project member and should be able to reopen issues.

Status: Accepted

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 13:44:00

Gavin said:

That's not what it says. It says "no more than one qualifier should annotate a single field or parameter", without any qualification.

We moved this sentence from @Qualifier to @Inject in r4 and added the following qualification: "when used with @Inject, no more than one qualifier should annotate a single field or parameter."

This instruction is directed at users who want to write code that's portable across 330 injectors. It doesn't preclude injectors from extending the spec and supporting more than one qualifier (though I would strongly advise against doing so for the reasons mentioned above).

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 14:12:11

I understand the reluctance to support multiple qualifiers with @Inject, but loosening the spec doesn't fix the real problem. Regardless of what the spec says, an injector (like 299) that supports multiple qualifiers will encourage users to write code that's subtly incompatible with other JSR-330 injectors; confusion will ensue.

I'm not sure how 330 can better accommodate 299. Despite Gavin's claims, we're more certain than ever of the soundness and simplicity of using only one qualifier. Perhaps the 299 EG can reconsider their decision to support multiple qualifiers. While supporting multiple qualifiers may have been the right decision when 299 was a standalone spec, the EG may determine that the feature isn't worth fragmenting the platform, especially considering that it can be re-added in a later release.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 25, 2009 16:43:03

I've already had this discussion with the 299 experts, and there is very little support for imposing this unnecessary restriction. Whatever the JavaDoc for @Inject says, 299 is going to support this.

cgruber commented 8 years ago

From crazybob...@gmail.com on June 25, 2009 18:02:55

I'm disappointed that the 299 EG would deliberately fragment the platform, reduce portability, and increase complexity by forcing such a rarely-needed and non-critical feature, but I think we've explored this issue to the extent possible within the scope of JSR-330.

Status: Fixed

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 26, 2009 09:40:39

Hrm, must admit that I don't understand this response. The 299 EG is agreeing to adopt, wholesale, the use of the annotations proposed by the 330 folks, including @Inject, @Qualifier, etc, when 299 already had a set of annotations that covered the same territory, purely to avoid any platform fragmentation. All we've asked for are a set of very minimal changes, in order to ensure that we don't lose useful functionality from 299.

Now, of course, the 299 EG could, quite reasonably, have demanded that the 330 group, as it arrives on the scene 3 years later, adopt the annotations that are already defined the the 299 PFD. We've not done that, because we decided that it would be OK for us to swallow some pride in the interests of the greater good. But we made that decision with strings attached: namely that we needed some minor changes, so that we would not lose functionality, or otherwise vandalize our own specification. It seems to me that we've done a lot of "giving" here, and it's now your turn, Bob. Do you really care about not "fragmenting" the platform, or is that just empty rhetoric, covering up an ego issue?

cgruber commented 8 years ago

From crazybob...@gmail.com on June 26, 2009 15:39:55

Gavin, please focus on the issue at hand. Ad hominem arguments will not be tolerated in this JSR.

Multiple qualifier support is not a minor change. The rest of the EG seems satisfied with the current state of the spec. Let's move on.

cgruber commented 8 years ago

From gavin.k...@gmail.com on June 26, 2009 17:09:02

Bob, don't be absurd. You accused my group of wanting to split the platform, when we are the ones making all the compromises here, and you're the one closing issues before consensus has been reached. I'm sorry, but if you close issues without the agreement of other experts, you simply are going to have to tolerate the fact that people will be unhappy about that. Unfortunately, that's what you signed up for when you took on the job of spec lead.

cgruber commented 8 years ago

From crazybob...@gmail.com on June 26, 2009 18:17:17

Gavin, if you have something new to add, reopen issues and express your unhappiness, but keep it technical. Personal attacks are verboten. No 330 EG member will have to tolerate or endure them.

If some injectors support only one qualifier and others support multiple qualifiers, the platform will indeed be split--code written for one injector won't necessarily work with another. The 330 spec doesn't preclude 299 from making this split, but I am advising the 299 EG against it. I'd do so more directly, but 299's mailing list is private.

cgruber commented 8 years ago

From michael....@gmail.com on June 26, 2009 18:44:15

Time out, guys. I thought earlier on that we had reached a conclusion that you both agreed with, which was that portable applications would be restricted to one qualifier. This means that only a small change to the wording would need to be made. Bob, will you agree to make a small change to the sentence to reflect this? It currently says:

"Qualifiers are optional and when used with @Inject, no more than one qualifier should annotate a single field or parameter."

but could be changed to:

"Qualifiers are optional and applications that wish to be portable across injectors should use no more than one qualifier when annotating a single field or parameter."

Would you be willing to do this?

cgruber commented 8 years ago

From crazybob...@gmail.com on June 27, 2009 09:28:38

Done in r10 . Please review and reopen if this isn't satisfactory.

cgruber commented 8 years ago

From tpeie...@gmail.com on June 27, 2009 14:17:49

Nice, Bob -- I like that wording.

Is there a natural place to put non-normative advice for implementers on what to do when faced with multiple qualifiers? Options are (1) punt completely with config errors, (2) pick one qualifier and issue a warning about the ignored remainder, or (3) support all of them. (Right?)

cgruber commented 8 years ago

From crazybob...@gmail.com on June 28, 2009 08:50:34

Thanks, Tim!

I went ahead and created a wiki page ImplementationAdvice , to be filled in later.

bar commented 8 years ago

@cgruber OMG what have I done?