logging-log4j2 icon indicating copy to clipboard operation
logging-log4j2 copied to clipboard

Replace `@NameProvider` and `@AliasesProvider` registration of strategies in `ConfigurableInstanceFactory`

Open jvz opened this issue 5 months ago • 5 comments

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.

jvz avatar Aug 05 '25 16:08 jvz

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();

jvz avatar Aug 05 '25 19:08 jvz

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?

ppkarwasz avatar Aug 06 '25 09:08 ppkarwasz

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 avatar Aug 08 '25 18:08 jvz

@jvz, I am not able to follow the proposal here. Would you please mind explaining the proposal using simple pseudo-code snippets revealing

  1. How does it look like now? What are the problematic lines/parts?
  2. How will it look like when this proposal is delivered? What are the improvements?

vy avatar Aug 09 '25 07:08 vy

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 value element, but we can maintain some flexibility here.
  • The annotations always model this as either String value() or String[] value().
  • For a name provider, this takes either the String value 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 @Named which works for both names and aliases).

Thus, the whole scheme can be simplified into a couple updates:

  • @NameProvider needs to know which element of the annotation to get the name from (default is value as all our existing annotations use that one)
  • @AliasesProvider needs to know which element of the annotation to get the aliases from (default is also value), 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.

jvz avatar Aug 11 '25 16:08 jvz