Fix(apache#2769): modify the annotation processor in 2.x to fail if a plugin builder attribute does not have a public setter.

Jay Katariya · Jay Katariya · commit 885ab1640e62 · 2024-11-09T02:18:50.000-08:00
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePlugin.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePlugin.java
@@ -62,6 +62,10 @@ public static class Builder implements org.apache.logging.log4j.core.util.Builde
         @PluginBuilderAttribute
         private int attribute;
 
+        @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
+        private int attributeWithoutPublicSetterButWithSuppressAnnotation;
+
         @PluginElement("layout")
         private Layout<? extends Serializable> layout;
 
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/GraalVmProcessorTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/GraalVmProcessorTest.java
@@ -65,6 +65,7 @@ class GraalVmProcessorTest {
             "fields",
             asList(
                     asMap("name", "attribute"),
+                    asMap("name", "attributeWithoutPublicSetterButWithSuppressAnnotation"),
                     asMap("name", "config"),
                     asMap("name", "layout"),
                     asMap("name", "loggerContext"),
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.config.plugins.processor;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Locale;
+import java.util.stream.Collectors;
+import javax.tools.Diagnostic;
+import javax.tools.DiagnosticCollector;
+import javax.tools.JavaCompiler;
+import javax.tools.JavaFileObject;
+import javax.tools.StandardJavaFileManager;
+import javax.tools.ToolProvider;
+import org.junit.Test;
+
+public class PluginProcessorPublicSetterTest {
+
+    private static final String FAKE_PLUGIN_CLASS_PATH =
+            "src/test/java/org/apache/logging/log4j/core/config/plugins/processor/" + FakePlugin.class.getSimpleName()
+                    + ".java";
+
+    @Test
+    public void warnWhenPluginBuilderAttributeLacksPublicSetter() throws Exception {
+        // Instantiate the tooling
+        final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+        final DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>();
+        final StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, Locale.ROOT, UTF_8);
+
+        // Get the source files
+        Path sourceFile = Paths.get(FAKE_PLUGIN_CLASS_PATH);
+
+        assertThat(Files.exists(sourceFile)).isTrue();
+        Iterable<? extends JavaFileObject> compilationUnits = fileManager.getJavaFileObjects(sourceFile.toFile());
+
+        // Compile the sources with the plugin processor
+        JavaCompiler.CompilationTask task = compiler.getTask(
+                null,
+                fileManager,
+                diagnosticCollector,
+                Arrays.asList("-proc:only", "-processor", PluginProcessor.class.getName()),
+                null,
+                compilationUnits);
+        task.call();
+
+        // Check for warnings about missing public setter
+        List<Diagnostic<? extends JavaFileObject>> warningDiagnostics = diagnosticCollector.getDiagnostics().stream()
+                .filter(diagnostic -> diagnostic.getKind() == Diagnostic.Kind.WARNING)
+                .collect(Collectors.toList());
+
+        assertThat(warningDiagnostics).anyMatch(warningMessage -> warningMessage
+                .getMessage(Locale.ROOT)
+                .contains("The field `attribute` does not have a public setter"));
+    }
+
+    @Test
+    public void IgnoreWarningWhenSuppressWarningsIsPresent() throws Exception {
+        // Instantiate the tooling
+        final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+        final DiagnosticCollector<JavaFileObject> diagnosticCollector = new DiagnosticCollector<>();
+        final StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, Locale.ROOT, UTF_8);
+
+        // Get the source files
+        Path sourceFile = Paths.get(FAKE_PLUGIN_CLASS_PATH);
+        System.out.println(FAKE_PLUGIN_CLASS_PATH);
+        assertThat(Files.exists(sourceFile)).isTrue();
+        Iterable<? extends JavaFileObject> compilationUnits = fileManager.getJavaFileObjects(sourceFile.toFile());
+
+        // Compile the sources with the plugin processor
+        JavaCompiler.CompilationTask task = compiler.getTask(
+                null,
+                fileManager,
+                diagnosticCollector,
+                Arrays.asList("-proc:only", "-processor", PluginProcessor.class.getName()),
+                null,
+                compilationUnits);
+        task.call();
+
+        // Check for warnings about missing public setter
+        List<Diagnostic<? extends JavaFileObject>> warningDiagnostics = diagnosticCollector.getDiagnostics().stream()
+                .filter(diagnostic -> diagnostic.getKind() == Diagnostic.Kind.WARNING)
+                .collect(Collectors.toList());
+
+        assertThat(warningDiagnostics)
+                .allMatch(
+                        warningMessage -> !warningMessage
+                                .getMessage(Locale.ROOT)
+                                .contains(
+                                        "The field `attributeWithoutPublicSetterButWithSuppressAnnotation` does not have a public setter"));
+    }
+}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcAppender.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/db/jdbc/JdbcAppender.java
@@ -60,6 +60,7 @@ public static class Builder<B extends Builder<B>> extends AbstractDatabaseAppend
         private ConnectionSource connectionSource;
 
         @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
         private boolean immediateFail;
 
         @PluginBuilderAttribute
@@ -80,6 +81,7 @@ public static class Builder<B extends Builder<B>> extends AbstractDatabaseAppend
 
         // TODO Consider moving up to AbstractDatabaseAppender.Builder.
         @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
         private long reconnectIntervalMillis = DEFAULT_RECONNECT_INTERVAL_MILLIS;
 
         @Override
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
@@ -25,9 +25,11 @@
 import java.io.PrintWriter;
 import java.io.StringWriter;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
@@ -39,14 +41,19 @@
 import javax.lang.model.SourceVersion;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ElementVisitor;
+import javax.lang.model.element.ExecutableElement;
+import javax.lang.model.element.Modifier;
 import javax.lang.model.element.TypeElement;
+import javax.lang.model.element.VariableElement;
 import javax.lang.model.util.Elements;
 import javax.lang.model.util.SimpleElementVisitor7;
+import javax.lang.model.util.Types;
 import javax.tools.Diagnostic;
 import javax.tools.FileObject;
 import javax.tools.StandardLocation;
 import org.apache.logging.log4j.core.config.plugins.Plugin;
 import org.apache.logging.log4j.core.config.plugins.PluginAliases;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
 import org.apache.logging.log4j.util.Strings;
 
 /**
@@ -83,6 +90,12 @@ public boolean process(final Set<? extends TypeElement> annotations, final Round
             final Set<? extends Element> elements = roundEnv.getElementsAnnotatedWith(Plugin.class);
             collectPlugins(elements);
             processedElements.addAll(elements);
+
+            // process plugin builder Attributes
+            final Set<? extends Element> pluginAttributeBuilderElements =
+                    roundEnv.getElementsAnnotatedWith(PluginBuilderAttribute.class);
+            processBuilderAttributes(pluginAttributeBuilderElements);
+            processedElements.addAll(pluginAttributeBuilderElements);
         }
         // Write the cache file
         if (roundEnv.processingOver() && !processedElements.isEmpty()) {
@@ -107,6 +120,93 @@ public boolean process(final Set<? extends TypeElement> annotations, final Round
         return false;
     }
 
+    private void processBuilderAttributes(final Iterable<? extends Element> elements) {
+        for (final Element element : elements) {
+            if (element instanceof VariableElement) {
+                processBuilderAttributes((VariableElement) element);
+            }
+        }
+    }
+
+    private void processBuilderAttributes(final VariableElement element) {
+        final String fieldName = element.getSimpleName().toString(); // Getting the name of the field
+        SuppressWarnings suppress = element.getAnnotation(SuppressWarnings.class);
+        if (suppress != null && Arrays.asList(suppress.value()).contains("log4j.public.setter")) {
+            // Suppress the warning due to annotation
+            return;
+        }
+        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()) {
+                // `enclosedElement is a method or constructor
+                if (enclosedElement instanceof ExecutableElement) {
+                    final ExecutableElement methodElement = (ExecutableElement) enclosedElement;
+                    final String methodName = methodElement.getSimpleName().toString();
+
+                    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
+                    ) {
+
+                        Types typeUtils = processingEnv.getTypeUtils();
+
+                        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)));
+
+                        if (fieldName.toLowerCase(Locale.ROOT).startsWith("is")) {
+                            followsNamePattern = methodName
+                                            .toLowerCase(Locale.ROOT)
+                                            .equals(String.format(
+                                                    "set%s",
+                                                    fieldName
+                                                            .toLowerCase(Locale.ROOT)
+                                                            .substring(2)))
+                                    || methodName
+                                            .toLowerCase(Locale.ROOT)
+                                            .equals(String.format(
+                                                    "with%s",
+                                                    fieldName
+                                                            .toLowerCase(Locale.ROOT)
+                                                            .substring(2)));
+                        }
+
+                        // Check if method is public
+                        boolean isPublicMethod = methodElement.getModifiers().contains(Modifier.PUBLIC);
+
+                        // Check if the return type of the method element is Assignable.
+                        // Assuming it is a builder class the type of it should be assignable to its parent
+                        boolean checkForAssignable = typeUtils.isAssignable(
+                                methodElement.getReturnType(),
+                                methodElement.getEnclosingElement().asType());
+
+                        boolean foundPublicSetter = followsNamePattern && checkForAssignable && isPublicMethod;
+                        if (foundPublicSetter) {
+                            // Hurray we found a public setter for the field!
+                            return;
+                        }
+                    }
+                }
+            }
+            // If the setter was not found generate a compiler warning.
+            processingEnv
+                    .getMessager()
+                    .printMessage(
+                            Diagnostic.Kind.WARNING,
+                            String.format("The field `%s` does not have a public setter", fieldName),
+                            element);
+        }
+    }
+
     private void collectPlugins(final Iterable<? extends Element> elements) {
         final Elements elementUtils = processingEnv.getElementUtils();
         final ElementVisitor<PluginEntry, Plugin> pluginVisitor = new PluginElementVisitor(elementUtils);
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SocketPerformancePreferences.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/net/SocketPerformancePreferences.java
@@ -40,14 +40,17 @@ public static SocketPerformancePreferences newBuilder() {
 
     @PluginBuilderAttribute
     @Required
+    @SuppressWarnings("log4j.public.setter")
     private int bandwidth;
 
     @PluginBuilderAttribute
     @Required
+    @SuppressWarnings("log4j.public.setter")
     private int connectionTime;
 
     @PluginBuilderAttribute
     @Required
+    @SuppressWarnings("log4j.public.setter")
     private int latency;
 
     public void apply(final Socket socket) {
diff --git a/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java b/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java
@@ -40,6 +40,7 @@ public static class Builder<B extends Builder<B>> extends AbstractAppender.Build
             implements org.apache.logging.log4j.core.util.Builder<ServletAppender> {
 
         @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
         private boolean logThrowables;
 
         @Override
diff --git a/src/changelog/.2.x.x/2769_pluginAttribute_publicSetterWarning.xml b/src/changelog/.2.x.x/2769_pluginAttribute_publicSetterWarning.xml
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+    <issue id="2769" link="https://github.com/apache/logging-log4j2/issues/2769"/>
+    <description format="asciidoc">Adding a compilation warning for Plugin Builder Attributes that do not have a public setter.</description>
+</entry>
