Replace `@NameProvider` and `@AliasesProvider` registration of strategies in `ConfigurableInstanceFactory`
During the 3.x DI system overhaul, one major change made to most of the plugin annotations was migrating the strategy code for interpreting annotations. Previously, it would be specified via plugin visitor classes, and the annotation would be meta-annotated to mark the strategy class to use. This is excess implementation detail that leaks into the annotations of plugins, and that already raised issues in the past when log4j-plugins had to be separated from log4j-core in order for the annotation processor to work in a modular environment.
The SPI to replace this should allow for registering strategies on how to extract a qualifier name and aliases from an injection point. The resulting plugin annotations should therefore no longer be meta-annotated with implementation details.
This particular issue is blocked at the moment due to how Key makes use of Keys for getting the name of things when constructing a key based on some annotated element. Fixing up how Key works overlaps a bit with https://github.com/apache/logging-log4j2/issues/1967. This is going to touch a lot of log4j-plugins files to remove unnecessary generics and casting.
If Key is refactored into a record class, then the Key<T> KEY = new Key<>() {}; pattern of creating typed keys will no longer be valid. It might work better to emulate the ResolvableType API in Spring here in that we don't want to rely on reflection on generic parameters to find type info when we can just be a little more explicit about it. For a more concrete example:
public static final Key<PluginNamespace> PLUGIN_NAMESPACE_KEY =
new @Namespace(NAMESPACE) Key<>() {};
This could be made more explicit to avoid reflection here, and assuming the type parameter is removed, the declaration would look like this (using the existing Key.Builder DSL):
public static final Key PLUGIN_NAMESPACE_KEY =
Key.builder(PluginNamespace.class).setNamespace(NAMESPACE).get();
The SPI to replace this should allow for registering strategies on how to extract a qualifier name and aliases from an injection point. The resulting plugin annotations should therefore no longer be meta-annotated with implementation details.
A SPI for registering strategies to extract a qualifier name and aliases from an injection point sounds like a solid approach. Right now, plugin annotations are still tightly coupled to the providers that interpret them. That link not only made it harder to split log4j-plugins from log4j-core, but also makes it impossible to create a log4j-plugins-annotation module containing only the annotations, independent from log4j-plugins. (Validators have the same issue.)
I’m not entirely clear on the connection between the meta-annotated annotations and Key. I agree that anonymous Key subtypes make GraalVM support more cumbersome (since each one has to be listed in reflect-config.json) and I don’t see a tangible benefit over using the Key builder. From a compile-time perspective, though, they should still be discoverable via an annotation processor using RoundEnvironment.getRootElements(), correct?
Branch opened in https://github.com/apache/logging-log4j2/compare/main...feature/3.x/name-alias-provider?expand=1 that's not complete, but has worked toward removing unnecessary use of Keys::getName. What remains to be done there include unrolling the other Keys::getName methods from InjectionPoint and moving that logic to DefaultInstanceFactory where the name and alias providers are tracked.
@jvz, I am not able to follow the proposal here. Would you please mind explaining the proposal using simple pseudo-code snippets revealing
- How does it look like now? What are the problematic lines/parts?
- How will it look like when this proposal is delivered? What are the improvements?
Alright, this turned out to be a bit different. I noticed some key insights into how the names and aliases are extracted:
- All name and alias extractors operate on a single element of an annotation (i.e., only one method to call).
- In fact, this is always the
valueelement, but we can maintain some flexibility here. - The annotations always model this as either
String value()orString[] value(). - For a name provider, this takes either the
Stringvalue or the first element of the array. - For an alias provider, this either uses the whole
String[] value()element (as in the case of@PluginAliases) or it uses all but the first element of the array (as in the case of@Namedwhich works for both names and aliases).
Thus, the whole scheme can be simplified into a couple updates:
@NameProviderneeds to know which element of the annotation to get the name from (default isvalueas all our existing annotations use that one)@AliasesProviderneeds to know which element of the annotation to get the aliases from (default is alsovalue), and it needs to know what offset to use in that array (default of 0)
This reduces the maximum reflection usage here to only calling the value method (or whatever is specified in the annotation) on the annotated annotation. As a bonus, this makes it possible to use the annotation processor to index this information ahead of time and avoid reflection entirely, something that will be useful for GraalVM usage.
As for how this changes things, it removes the need for using AnnotatedElementNameProvider and AnnotatedElementAliasesProvider entirely, removes those implementations, and simplifies the @NameProvider and @AliasesProvider sites to use default arguments most of the time.