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.

[jaykataria1111]

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:

This is true, I have never worked on an annotation processor, good learning opportunity! Thanks for listing out the changes that I need this provides a good starting point appreciate it a lot :) You are awesome! :) 💯

[jaykataria1111]

So my original idea was something of this sorts, This would be during the build of the field, which would be runtime

In Plugin Builder.java

   for (final ConstraintValidator<?> validator : validators) {
                if (!validator.isValid(field, builder)) {
                    if (!reason.isEmpty()) {
                        reason += ", ";
                    }

                    if (validator instanceof PublicSetterValidator) {
                        reason += "field '" + field.getName() + " does not have a public setter";
                    } else {
                        reason += "field '" + field.getName() + "' has invalid value '" + value + "'";
                    }
                }
            }
        }
        return reason;

Implementing a constraint Validator, and changing the interface.

public interface ConstraintValidator<A extends Annotation> {

...

    /**
     * Indicates if the given value is valid.
     *
     * @param name the name to use for error reporting
     * @param value the value to validate.
     * @return {@code true} if the given value is valid.
     */
    default boolean isValid(String name, Object value) {
        return true;
    }

    default boolean isValid(Field field, final Builder<?> builder) throws IllegalAccessException {
        final Object value = field.get(builder);
        return isValid(field.getName(), value);
    }
}

public class PublicSetterValidator implements ConstraintValidator<PluginBuilderAttribute> {

    private static final Logger LOGGER = StatusLogger.getLogger();

    private PluginBuilderAttribute annotation;

    /**
     * @param annotation the annotation value this validator will be validating.
     */
    @Override
    public void initialize(PluginBuilderAttribute annotation) {
        this.annotation = annotation;
    }

    @Override
    public boolean isValid(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.
    }

}

I do see this has a lots of flaws though already for that purpose, and its too drastic

Doing it at compile time makes more sense. Thanks a lot for giving me a direction on this.

[ppkarwasz]

Doing it at compile time makes more sense. Thanks a lot for giving me a direction on this.

Yes, it does. Validators should be used for something that the user can fix. This can not be fixed by the user, only by the developer.

[jaykataria1111]

Hi @ppkarwasz

Made some good progress but needed your input on something:

I am not sure if we should add type checking here:

if (methodName.startsWith("set")
                        && methodElement.getParameters().size() == 1) {
                    // Check if this is the right setter and if it matches return
                }

I added this code:

 if ((methodName.toLowerCase(Locale.ROOT).startsWith("set") // Typical pattern for setters
                            || methodName.toLowerCase(Locale.ROOT).startsWith("with")) // Typical pattern for setters
                            && methodElement.getParameters().size() == 1 // It is a weird pattern to not have public setter
                    ) {

                        // Check if the method parameter matches the type.
                        boolean methodParamTypeMatchesBoxedFieldType = false;
                        TypeMirror fieldType = element.asType();
                        TypeMirror methodParamType = methodElement.getParameters().get(0).asType();
                        Types typeUtils = processingEnv.getTypeUtils();

                        if (fieldType.getKind().isPrimitive() && !methodParamType.getKind().isPrimitive()) {
                            methodParamTypeMatchesBoxedFieldType = typeUtils.isSameType(methodParamType, typeUtils.boxedClass((PrimitiveType) fieldType).asType());
                        } else if (methodParamType.getKind().isPrimitive() && !fieldType.getKind().isPrimitive()) {
                            methodParamTypeMatchesBoxedFieldType = typeUtils.isSameType(fieldType, typeUtils.boxedClass((PrimitiveType) methodParamType).asType());
                        }
                        boolean methodParamTypeMatchesFieldType = typeUtils.isSameType(methodParamType, fieldType) || methodParamTypeMatchesBoxedFieldType;

                        // Check if method is public
                        boolean isPublicMethod = methodElement.getModifiers().contains(Modifier.PUBLIC);

                      boolean foundPublicSetter = methodParamTypeMatchesFieldType && isPublicMethod;
                      if (foundPublicSetter) {
                          // Hurray we found a public setter for the field!
                          return;
                      }
                    }

But the issue that I face is with such kind of setters, we are throwing an error in such cases due to type checking :

 public static final class Builder implements org.apache.logging.log4j.core.util.Builder<IfLastModified> {
        @PluginBuilderAttribute
        @Required(message = "No age provided for IfLastModified")
        private org.apache.logging.log4j.core.appender.rolling.action.Duration age;

        public Builder setAge(final Duration age) {
            this.age = org.apache.logging.log4j.core.appender.rolling.action.Duration.ofMillis(age.toMillis());
            return this;
        }

As you can see the setAge method has a different parameter type than the fields type.

if we do have a hard check on the naming convention "setFieldName" (This becomes more restrictive, do not recommend, I have seen people get really creative for no reason) that has a number of problems as well.

I do believe we should do some sort of checks, just checking for methodName.startsWith("set") && methodElement.getParameters().size() == 1 to identify

Hence I have come up with the following plan:

1. method name starts with "set" or "with"
2. method has 1 param
3. method param type is the same as the field type (includes boxed comparisons) 3.a if the above is not being followed then as a fall back we can check if there is a method that follows the naming convention like in the above case

after that I believe we can give up, because then people are doing something really creative with their code.

Hence building on the above statement, I have the following logic, which seems to work :) :

   if ((methodName.toLowerCase(Locale.ROOT).startsWith("set") // Typical pattern for setters
                            || methodName.toLowerCase(Locale.ROOT).startsWith("with")) // Typical pattern for setters
                            && methodElement.getParameters().size() == 1 // It is a weird pattern to not have public setter
                    ) {

                        // Check if the method parameter matches the type.
                        boolean methodParamTypeMatchesBoxedFieldType = false;
                        TypeMirror fieldType = element.asType();
                        TypeMirror methodParamType = methodElement.getParameters().get(0).asType();
                        Types typeUtils = processingEnv.getTypeUtils();

                        if (fieldType.getKind().isPrimitive() && !methodParamType.getKind().isPrimitive()) {
                            methodParamTypeMatchesBoxedFieldType = typeUtils.isSameType(methodParamType, typeUtils.boxedClass((PrimitiveType) fieldType).asType());
                        } else if (methodParamType.getKind().isPrimitive() && !fieldType.getKind().isPrimitive()) {
                            methodParamTypeMatchesBoxedFieldType = typeUtils.isSameType(fieldType, typeUtils.boxedClass((PrimitiveType) methodParamType).asType());
                        }
                        boolean methodParamTypeMatchesFieldType = typeUtils.isSameType(methodParamType, fieldType) || methodParamTypeMatchesBoxedFieldType;

                        boolean followsNamePattern = methodName.toLowerCase(Locale.ROOT).equals(String.format("set%s",fieldName.toLowerCase(Locale.ROOT)))
                                || methodName.toLowerCase(Locale.ROOT).equals(String.format("with%s",fieldName.toLowerCase(Locale.ROOT)));
                        // Check if method is public
                        boolean isPublicMethod = methodElement.getModifiers().contains(Modifier.PUBLIC);

                      boolean foundPublicSetter = (methodParamTypeMatchesFieldType || followsNamePattern) && isPublicMethod;
                      if (foundPublicSetter) {
                          // Hurray we found a public setter for the field!
                          return;
                      }
                    }
                }
            }

If this approach to identify setters looks good to you I can add tests and open a PR :) lmk what are your thoughts

[ppkarwasz]

@jaykataria1111,

But the issue that I face is with such kind of setters, we are throwing an error in such cases due to type checking:

private org.apache.logging.log4j.core.appender.rolling.action.Duration age;
public Builder setAge(final Duration age) { ... }

Checking the type of the setter parameter is not necessary. All the configuration attributes in Log4j are provided as Strings anyway (e.g. in an XML configuration file). As long as there is a setter, the PluginBuilder can handle the conversion. Besides, the main utility of having setters is to be able to change the type of the field, without breaking backward compatibility. :wink:

Instead of checking the type of the parameter, it would be more useful to check the return type of the setter: the return types of our setters are not always equal to the type of the builder (e.g. they can be type variables for abstract builders), but the return type should be assignable to the builder type. I believe that Types.isAssignable() should handle that, although I am not sure how well it manages parameterized types.

[ppkarwasz]

Note: I noticed that some of our builders have complex conventions for the name of the setter: the setter for an isUnicode field should be called setUnicode and not setIsUnicode.

[jaykataria1111]

Hi @ppkarwasz

Note: I noticed that some of our builders have complex conventions for the name of the setter: the setter for an isUnicode field should be called setUnicode and not setIsUnicode.

+1. I removed the type check and kept the naming check and fix the compilation issues for the purpose. I had to change the name of some fields due to this. It should be a backwards compatible change.

Instead of checking the type of the parameter, it would be more useful to check the return type of the setter: the return types of our setters are not always equal to the type of the builder (e.g. they can be type variables for abstract builders), but the return type should be assignable to the builder type. I believe that Types.isAssignable() should handle that, although I am not sure how well it manages parameterized types

I did incorporate that in code:

Some of the methods had a void return type I fixed that for example changed SocketPerformancePreferences

    public SocketPerformancePreferences setBandwidth(final int bandwidth) {
        this.bandwidth = bandwidth;
        return this;
    }

    public SocketPerformancePreferences setConnectionTime(final int connectionTime) {
        this.connectionTime = connectionTime;
        return this;
    }

    public SocketPerformancePreferences setLatency(final int latency) {
        this.latency = latency;
        return this;
    }

The issue is that, the check-api-comp build task fails for the purpose when I do the code change, should we ignore it? All of my changes so far are backwards compatible, but the tool thinks that these are "MAJOR" changes.

I have created a dummy PR in my own repo: Code Changes to show you the code changes and how they should be backward compatible, please let me know if that works, also I am new to github, so learning its quirks, My company uses its own system, so workflows code reviewing are a bit different. So please bear with me if it is not a fancy PR and just my repo, as long as you can see the commit and the diff that is all that matters to me :)

Besides, the main utility of having setters is to be able to change the type of the field, without breaking backward compatibility.

😃 , yeah it is interesting to see, this sort of usage of setters, I must say I wasn't acquainted it with it, which just gives me more breadth of knowledge.

[ppkarwasz]

@jaykataria1111,

Great job! :100:

Note: I noticed that some of our builders have complex conventions for the name of the setter: the setter for an isUnicode field should be called setUnicode and not setIsUnicode.

+1. I removed the type check and kept the naming check and fix the compilation issues for the purpose. I had to change the name of some fields due to this. It should be a backwards compatible change.

Unfortunately, changing the name of @PluginBuilderAttribute-annotated fields is a breaking change, because those names are used in the configuration file. What you should do in this case is deprecate the old setter and create a new one.

In PR #3174 I fixed the setter problems I found.

Some of the methods had a void return type I fixed that for example changed SocketPerformancePreferences

The issue is that, the check-api-comp build task fails for the purpose when I do the code change, should we ignore it? All of my changes so far are backwards compatible, but the tool thinks that these are "MAJOR" changes.

Nice catch! Unfortunately changing any public signature is technically a breaking change. Of course the ultimate classification is up to the developers. We have two choices here:

1. Add a configuration option to the processor to change the behavior for mismatched return types from ERROR to WARNING.
2. Tell BND Baseline to ignore those changes.

I am quite strict on binary compatibility, even if it concerns code that probably nobody uses. @vy, @JWT007, what do you think? Is fixing those broken setters in SocketPerformancePreferences worth the breaking change?

[JWT007]

Hi @ppkarwasz hmmm I am not sure - what is the real impact?

How many really create the SocketPerformancePreferences programmatically? :/

I think if someone is creating a SocketPerformancePreferences programmatically they are doing it like this:

SocketPerformancePreferences socketPerformancePreferences = SocketPerformancePreferences.newBuilder();
socketPerformancePreferences .setBandwidth(200);
socketPerformancePreferences .setConnectionTime(50);
socketPerformancePreferences.setLatency(5);
SocketPerformancePreferences clonedSPP = socketPerformancePreferences.build();

In this case changing the return type would not really be a breaking-change because no one is currently expecting a return type.

The only difference would be that one could chain the commands after the change:

SocketPerformancePreferences socketPerformancePreferences = 
  SocketPerformancePreferences
       .newBuilder()
       .setBandwidth(200)
       .setConnectionTime(50)
       .setLatency(5)
       .build();

Worse would be going from a return type to void. :)

[ppkarwasz]

Hi @JWT007,

How many really create the SocketPerformancePreferences programmatically? :/

IIRC, you are using the plugin builders quite a lot. Do you plan to also support SocketPerformancePreferences?

I think if someone is creating a SocketPerformancePreferences programmatically they are doing it like this:

SocketPerformancePreferences socketPerformancePreferences = SocketPerformancePreferences.newBuilder();
socketPerformancePreferences .setBandwidth(200);
socketPerformancePreferences .setConnectionTime(50);
socketPerformancePreferences.setLatency(5);
SocketPerformancePreferences clonedSPP = socketPerformancePreferences.build();

In this case changing the return type would not really be a breaking-change because no one is currently expecting a return type.

The change will not break source compatibility, but binary compatibility will be broken and the code needs to be recompiled.

[JWT007]

@ppkarwasz in my special case we won't support the SocketAppender at all so this wouldn't affect me at all :)

Hmm ok binary compatibility sure - but only for those programmatically configuring - I assume they would need to recompile when upgrading the library version anyways.

[JWT007]

you could just deprecate the old signatures and add new ones for "with***"? the method lookup above would need to prefer the non-deprecated variants though.

[jaykataria1111]

you could just deprecate the old signatures and add new ones for "with***"? the method lookup above would need to prefer the non-deprecated variants though.

I am sorry, got confused. Do we want to deprecate the setters all together?

1. Add a configuration option to the processor to change the behavior for mismatched return types from ERROR to WARNING.
2. Tell BND Baseline to ignore those changes.

This option was also mentioned. Just was thinking of the direction.

@ppkarwasz @JWT007

re, had a little dumb question:

Hmm ok binary compatibility sure - but only for those programmatically configuring - I assume they would need to recompile when upgrading the library version anyways.

I see but wouldn't the expectation be to not recompile? Is there a possibility of this being used by other folks? If that is the case then, this makes more sense:

you could just deprecate the old signatures and add new ones for "with***"? the method lookup above would need to prefer the non-deprecated variants though.

@ppkarwasz @JWT007 That is the action I am going to take.

Also just fyi thanks a lot @ppkarwasz , I am getting to learn a lot! Did not know about Semantic Versioning and how to properly deprecate this is awesome 🎉

[ppkarwasz]

you could just deprecate the old signatures and add new ones for "with***"? the method lookup above would need to prefer the non-deprecated variants though.

@ppkarwasz @JWT007 That is the action I am going to take.

I would just annotate those three methods with a custom @SuppressWarnings and make the annotation processor ignore them.

[jaykataria1111]

I would just annotate those three methods with a custom @SuppressWarnings and make the annotation processor ignore them.

Okay sounds good let me do that then! I will add tests and raise a PR.

[jaykataria1111]

Hi @ppkarwasz , seems like the @SuppressWarnings annotation won't work, what do you recommend among these:

1.Add a configuration option to the processor to change the behavior for mismatched return types from ERROR to WARNING. 2.Tell BND Baseline to ignore those changes.

[ppkarwasz]

Hi @ppkarwasz , seems like the @SuppressWarnings annotation won't work, what do you recommend among these:

Can you elaborate on that? You should be able to check if a method is annotated with @SuppressWarnings or not.

[jaykataria1111]

@ppkarwasz

I would just annotate those three methods with a custom @SuppressWarnings and make the annotation processor ignore them.

I am sorry for the confusion are you asking to add something of this sorts, on the methods to ignore the warnings? Because @SuppressWarnings annotation is not going to work

   @BaselineIgnore("2.x")
    public SocketPerformancePreferences setBandwidth(final int bandwidth) {
        this.bandwidth = bandwidth;
        return this;
    }

    @BaselineIgnore("2.x")
    public SocketPerformancePreferences setConnectionTime(final int connectionTime) {
        this.connectionTime = connectionTime;
        return this;
    }

    @BaselineIgnore("2.x")
    public SocketPerformancePreferences setLatency(final int latency) {
        this.latency = latency;
        return this;
    }

[ppkarwasz]

I thought rather to keep the methods as they are and add logic to PluginProcessor to issue a WARNING instead of an error in this case:

   // This setter has the wrong return type
   @SuppressWarnings("log4j.return.type")
    public void setBandwidth(final int bandwidth) {
        this.bandwidth = bandwidth;
    }

    @SuppressWarnings("log4j.return.type")
    public void setConnectionTime(final int connectionTime) {
        this.connectionTime = connectionTime;
    }

     @SuppressWarnings("log4j.return.type")
    public void setLatency(final int latency) {
        this.latency = latency;
    }

This way:

• we don't get compilation errors for these 3 known problematic setters.
• we still get compilation errors if we accidentally add a void setter.

[jaykataria1111]

@ppkarwasz , that makes more sense let me do that! :)

[jaykataria1111]

Hi @ppkarwasz, I have updated the code, it is late here in my time zone, so have not written the unit tests yet, but just leaving the code here, so that you can peruse through it once, to see what I have done and if it looks good, will add the unit tests and raise the PR by tomorrow soon 😺

Code

[jaykataria1111]

Hi @ppkarwasz here is a pull request, I have built on top of your change because without your change I will get compilation issues: https://github.com/apache/logging-log4j2/pull/3195

[jaykataria1111]

Hi @ppkarwasz, had a general question about how Pull Requests and Code reviews work in this repo, do I have to reach out to folks to drive it or, someone volunteering will just pickup the review on their own time.

Also for 3.x I will start looking at the OpenRewrite rule.

[ppkarwasz]

@jaykataria1111,

Hi @ppkarwasz, had a general question about how Pull Requests and Code reviews work in this repo, do I have to reach out to folks to drive it or, someone volunteering will just pickup the review on their own time.

We don't have a lot of active maintainers (more maintainers are always welcome, see becoming a committer on the ASF website), but we'll look at your PR as soon as we have time.

[jaykataria1111]

Hi @ppkarwasz , it would be awesome I can contribute more, let me see the steps to become a contributor, this is awesome thanks a lot 🙂

[jaykataria1111]

Hi @ppkarwasz, I am writing an OpenRewrite recipe for the general setters, going to take a dependency for OpenRewrite for the purpose, wanted to share as an FYI. hope taking dependencies is fine.

[ppkarwasz]

@jaykataria1111,

I am writing an OpenRewrite recipe for the general setters, going to take a dependency for OpenRewrite for the purpose, wanted to share as an FYI. hope taking dependencies is fine.

That is great! Note that the general problem of moving an annotation from a field to a getter or setter is not Log4j-specific: most dependency injection frameworks (JAXB, JPA, JSR 330, Spring) allow users to annotate fields, getters or setters. The best place for such a rule would be OpenRewrite itself, probably the openrewrite/rewrite-migrate-java repository. We will apply the rule as a one off operation.

@timtebeek, do you have any recommendations on how to rewrite:

public static class Builder implements Supplier<ConsoleAppender> {

    @PluginBuilderAttribute
    private boolean follow;

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

to

public static class Builder implements Supplier<ConsoleAppender> {

    private boolean follow;

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

[jaykataria1111]

@ppkarwasz , that would be awesome if there is a pre-written recipe that we could use! 🎉

Thanks for sharing this @ppkarwasz , it was pretty cool to learn about a openRewrite and what a better way to get up to speed with an actual application! Getting to learn a lot!

We will apply the rule as a one off operation.

Yeah, so run the rule locally, and just not add dependency file changes in the commit?

[ppkarwasz]

We will apply the rule as a one off operation.

Yeah, so run the rule locally, and just not add dependency file changes in the commit?

I would say that implementing such a change should consist of several steps:

1. The source code of the OpenRewrite rule should be published. If not in OpenRewrite itself, at least in Log4j Tools.
2. You create a PR with two commits.
   • In the first commit you add a rewrite.yml file to the repo and update the rewrite profile in the main pom.xml. We did have a rewrite.yml file in the past, you can take the last version and add your rule below.
   • In the second commit you just add the modifications generated by ./mvnw -Prewrite.

Such a procedure would help reviewing the PR. A reviewer can just pull your first commit and verify that the second commit contains only changes generated by OpenRewrite. Changes generated by OpenRewrite are massive and we need to show the world that PRs are reviewed with due diligence.

[jaykataria1111]

@timtebeek, regarding this:

@timtebeek, do you have any recommendations on how to rewrite:

public static class Builder implements Supplier {

@PluginBuilderAttribute
private boolean follow;

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

} to

public static class Builder implements Supplier {

private boolean follow;

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

}

I tried to lookup if there are any prewritten recipes for the purpose which would help us achieve the above case ^^ and that does not seem to be the case, will we have to write our own recipe? Would that be the way to go?

[jaykataria1111]

Hi @ppkarwasz, trying to understand this statement better:

The source code of the OpenRewrite rule should be published. If not in OpenRewrite itself, at least in Log4j Tools.

If we go with a custom recipe, i.e. a custom rule that we make, then I need to add that rule in this repo: https://github.com/apache/logging-log4j-tools ?

[ppkarwasz]

I tried to lookup if there are any prewritten recipes for the purpose which would help us achieve the above case ^^ and that does not seem to be the case, will we have to write our own recipe? Would that be the way to go?

Yes I believe we need to write a new OpenRewrite recipe in Java.

The source code of the OpenRewrite rule should be published. If not in OpenRewrite itself, at least in Log4j Tools.

If we go with a custom recipe, i.e. a custom rule that we make, then I need to add that rule in this repo: https://github.com/apache/logging-log4j-tools ?

It is probably better to write a more generic rule that, e.g. moves annotation @A from a field to a setter and contribute it to the OpenRewrite project. IIRC the OpenRewrite project has a very short release cycle (around 2 weeks). If that doesn't work, you can make a PR in logging-log4j-tools.

[jaykataria1111]

Thanks @ppkarwasz , awesome, let me write the rule and contribute it to OpenRewrite project! Would be fun!