avaje-inject icon indicating copy to clipboard operation
avaje-inject copied to clipboard

Weird map keys for @QualifiedMap

Open kliushnichenko opened this issue 2 months ago • 8 comments

Consider next configuration

@Qualifier
@Retention(RUNTIME)
@Target({TYPE})
public @interface Accepts {
    PaymentMethod value();
}
@Singleton
@Accepts(PaymentMethod.VISA)
public class VisaStore implements Store {
}
@Singleton
@Accepts(PaymentMethod.MASTERCARD)
public class MastercardStore implements Store {
}
@Singleton
class PaymentGateway {

   private final Map<String, Store> paymentMethods;

   @Inject
   PaymentGateway (@QualifiedMap Map<String, Store> paymentMethods) {
     this.paymentMethods = paymentMethods;
   }
 }

The resulting map keys for the injected qualified map paymentMethods looks like:

accepts(value=visa) : <VisaStore>,
accepts(value=mastercard): <MastercardStore>

I would expect keys like VISA and MASTERCARD.
Or my configuration is wrong somewhere?

kliushnichenko avatar Apr 19 '24 10:04 kliushnichenko

yeah all qualifiers currently generate with lowercase. @rbygrave do you want to keep it that way or change for 10.0?

SentryMan avatar Apr 19 '24 13:04 SentryMan

just for clarity, it is not only about the case. accepts(value=visa) is literally the String in the map key.
Also, it would be nice to support an enum as a map key, support EnumMap in other words.

kliushnichenko avatar Apr 19 '24 14:04 kliushnichenko

just for clarity, it is not only about the case. accepts(value=visa) is literally the String in the map key.

That's because the @Accepts annotation itself is also qualifier.

For testing to work we convert the annotation into a string key to match the output of the runtime java.lang.Annotation#toString. The issue with enums is that toString() will not print out the full enum type. It will only have the enum type name itself.

Hence why PaymentMethod.VISA is printed out as VISA before we lowercase it to visa.

Also, it would be nice to support an enum as a map key, support EnumMap in other words.

Would be nice indeed but such a thing is impossible at the current time. Inject test works via reading the annotation at runtime as a basic java.lang.Annotation instance.

Unfortunately Annotation doesn't have any way except that stunted toString to retrieve annotation values. So at the current time strings are the best we can do.

Edit: mention that the annotation itself is a qualifier

SentryMan avatar Apr 19 '24 15:04 SentryMan

So pondering, could we get to:

   @Inject
   PaymentGateway (@QualifiedMap @Accepts EnumMap<PaymentMethod, Store> paymentMethods) {
     this.paymentMethods = paymentMethods;
   }

I think what this means is that internally we'd be getting avaje-inject to make the assumption that the qualifier argument is an Enum - now for me I think that is actually ok in that my gut is telling me this could be the 99% case of using this type of qualifier - one that takes a single enum parameter is the 99% case.

So if this is the 99% case, then can we get avaje-inject to support this?

Currently Builder has:

  /**
   * Return a map of dependencies for the generic type keyed by qualifier name.
   */
  <T> Map<String, T> map(Type type);

And we'd need to add something like:

  /**
   * Return a map of dependencies for the generic type keyed by qualifier name.
   */
  <E,T> EnumMap<E extends Enum, T> enumMap(Type type, String topQualifier);

... and for this example the generated code would call the builder like:

EnumMap<PaymentMethod, Store> enumMap = builder.map(Store.class, "Accepts");
var bean = new PaymentGateway(enumMap);

My thinking is that the internal string values for the qualifiers is accepts(value=visa) and accepts(value=mastercard) ... and internally the builder enumMap implementation is going to make some assumptions on the string format along the likes:

  • The internal string qualifier startsWith("Accepts") // toLowerCase?
  • The (value=___) is parsed to determine the "visa" and "mastercard" values
  • It can perform this parsing and build the EnumMap

all qualifiers currently generate with lowercase. @rbygrave do you want to keep it that way or change for 10.0?

I'm definitely open to removing the toLowerCase() performed on all qualifier names ... and I'm thinking that we might have to do that if we want to support this case.

To a large extent lowercase was used to avoid any issue/variance when people migrate from using @Named like @Named("red") and migrate to @Red and have a difference there with red vs Red.

rbygrave avatar Apr 20 '24 00:04 rbygrave

To a large extent lowercase was used to avoid any issue/variance when people migrate from using @Named like @Named("red") and migrate to @Red and have a difference there with red vs Red.

Why would one expect it to behave the same when the annotation class has a different case? If anything it should be strange that it works.

SentryMan avatar Apr 20 '24 02:04 SentryMan

  /**
   * Return a map of dependencies for the generic type keyed by qualifier name.
   */
  <E extends Enum<E>,T> EnumMap<E , T> enumMap(Type type, String topQualifier);

idk, it seems like a method like this would have undefined behavior for a bunch of valid cases. For example if the annotation has more than one member, or if the annotation doesn't happen to be using an enum (strings also would be a fairly common case)

SentryMan avatar Apr 20 '24 02:04 SentryMan

So be clear we can't use @Accepts so instead it would look more like:

  @Inject
   PaymentGateway (@QualifiedMap EnumMap<PaymentMethod, Store> paymentMethods) {
     this.paymentMethods = paymentMethods;
   }

So this must assume that:

  • all the qualifiers use @Accepts,
  • there is only 1 member which is the PaymentMethod enum.

if the annotation has more than one member, or if the annotation doesn't happen to be using an enum

These would not be supported - ideally an exception is thrown for those cases.

regarding case sensitive ... when the annotation class has a different case?

I expect many people start by using @Named often using lower case and then later convert over to using a typed qualifier and so it then goes from "red" to "Red" and in a big enough code base there can even be both so ideally these match (hence I think that DContextEntryBean uses equalsIgnoreCase in matching qualifier names.

rbygrave avatar Apr 23 '24 11:04 rbygrave

To me, supporting a @QualifierMap of EnumMap comes with a few restrictions. It seems that the value of the EnumMap is marginal with say only 2 implementations [we could instead just inject the 2 known implementations using the full qualifiers] and EnumMap gets to be valuable when there are more entries / more implementations like say 10.

An alternative approach would be more that all the implementations have a method that returns the enum (e.g. VisaStore has a supportsPayment() method that returns PaymentType.VISA) ... and code iterates all the implementations and puts them into an EnumMap that way.

Still, EnumMap can be done if we are prepared to accept the restrictions on its use.

rbygrave avatar Apr 23 '24 11:04 rbygrave