Replace field injection with setter injection in `3.x`

[ppkarwasz]

As issue #2766 shows, there are still some Log4j plugin builders that don't have setters for all their configuration attributes.

Since field injection becomes more problematic in newer Java version, I believe we should add the missing setters and make sure all new attributes come with a public setter.

Therefore I propose:

• to modify the annotation processor in 2.x to fail if a plugin builder attribute does not have a public setter (or at least a wither).
• fix the compilation errors in 2.x.

Regarding 3.x I would prefer to remove field injection completely from log4j-plugins and use builders and setter injection everywhere. IMHO our DI subsystem does not need to have all the features of a fully-fledged DI.
Obviously we might still need field injection if we want to support all the existing 2.x plugins, but I would still love a 200 KiB dependency injection system.

[ppkarwasz]

Removing field injection in 3.x has an advantage for JPMS users: they no longer need to open the package to org.apache.logging.log4j.plugins, since setter injection only requires public reflection. JPMS grants public reflection on all exported packages.

Basically this feature request consists in replacing code like:

public static class Builder implements Supplier<ConsoleAppender> {

    @PluginBuilderAttribute
    private boolean follow;

    public Builder setFollow(boolean follow) {
        this.follow = follow;
        return this;
    }
}

with

public static class Builder implements Supplier<ConsoleAppender> {

    private boolean follow;

    public Builder setFollow(@PluginAttribute boolean follow) {
        this.follow = follow;
        return this;
    }
}

so I would leave it as "good first issue". I believe that the process can be done automatically by writing an OpenRewrite rule.

[vy]

Question: Can we implement this change in the following manner?

1. Support plugins using field injection and compiled with Log4j 2 annotation processor
2. Drop field injection support from the Log4j 3 annotation processor

That is, effectively support legacy and stop with field injection in Log4j 3 and onwards.

[jaykataria1111]

Hi @vy and @ppkarwasz,

If this issue has not been picked up yet, can I contribute to this, do I have to be assigned to this issue or I can just start working on it?

[ppkarwasz]

@jaykataria1111,

Thank you for your help. I have assigned you to the issue.

[jaykataria1111]

Hi @ppkarwasz ,

re: to modify the annotation processor in 2.x to fail if a plugin builder attribute does not have a public setter (or at least a wither).

I was considering the changes needed here, and have already started working on some, I did have a question though, when it comes to identifying if a Field has a public setter, we cannot look at the implementation of the class' methods (not sure if I could find any other way of finding setters looking at the reflection API), and hence to check if a field has a public setter I have the following proposal:

1. We go to through methods of the class and then check which one takes 1 parameter
2. That parameter has to be same as the type of the Field that we want to check has a public setter.
3. The method starts with set. We could technically not have this check, I understand a concern here, because people can have unconventional naming.
4. The method contains the fieldName somewhere. We could technically not have this check, I understand a concern here, because people can have unconventional naming.

For point 3 and 4, if someone is not using the naming standard for setters, then there is really a problem there overall the logic looks something of this sorts:

    public boolean hasPublicSetter(Field field, Builder<?> builder) {
        Class<?> declaringClass = field.getDeclaringClass();
        Class<?> fieldType = field.getType();

        Method[] methods = declaringClass.getMethods();
        for (Method method : methods) {
            if (method.getName().toLowerCase(Locale.ROOT).contains(field.getName().toLowerCase(Locale.ROOT))
                    && method.getName().toLowerCase(Locale.ROOT).startsWith("set")
                    && method.getParameterCount() == 1
                    && method.getParameterTypes()[0].equals(fieldType)
                    && java.lang.reflect.Modifier.isPublic(method.getModifiers())) {
                return true; // Found a valid public setter according to convention.
            }
        }
        return false; // Could not find a method with a valid public setter.
    }

This would be sort of our best effort to check if a field has a public setter.

[ppkarwasz]

Hi @jaykataria1111,

not sure if I could find any other way of finding setters looking at the reflection API

The usage of the Reflection API suggests to me that you want to do this check at runtime, right? I would rather do this check in our PluginProcessor class, which is used during the compilation process and provides access to more advanced APIs (like the language model in the javax.lang.model.element package.

Note: If you never wrote an annotation processor, its API has a steep learning curve, but the feature only requires a small part of this API:

1. You need to add PluginBuilderAttribute to the list of annotations handled by PluginProcessor.
2. You can easily recover all the Java language Elements using the RoundEnvironment:

   roundEnv.getElementsAnnotatedWith(PluginBuilderAttribute.class);

3. The annotated elements should be of type VariableElement (which corresponds to fields and method parameters). You can then loop through all the methods (ExecutableElement) in the containing class to see if a setter is defined:

   private void processBuilderAttributes(final VariableElement element) {
       final String fieldName = element.getSimpleName().toString();
       final Element enclosingElement = element.getEnclosingElement();
       // `element is a field
       if (enclosingElement instanceof TypeElement) {
           final TypeElement typeElement = (TypeElement) enclosingElement;
           // Check the siblings of the field
           for (final Element enclosedElement : typeElement.getEnclosedElements()) {
               if (enclosedElement instanceof ExecutableElement) {
                   final ExecutableElement methodElement = (ExecutableElement) enclosedElement;
                   final String methodName = methodElement.getSimpleName().toString();
                   if (methodName.startsWith("set")
                           && methodElement.getParameters().size() == 1) {
                       // Check if this is the right setter and if it matches return
                   }
               }
           }
           // If the setter wan not found generate a compiler error.
           processingEnv.getMessager().printMessage(
               Diagnostic.Kind.ERROR,
               String.format("The field `%s` does not have a setter", fieldName),
               element
           );
       }
   }

[ppkarwasz]

To test the annotation processor, we have some examples in logging-log4j-tools like DescriptorGeneratorTest. The test runs a compiler programmatically and check if there are any compilation errors.