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

Author: Jay Katariya

log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePlugin.java

@@ -60,8 +60,13 @@ public static Builder newBuilder() {
     public static class Builder implements org.apache.logging.log4j.core.util.Builder<FakePlugin> {
 
         @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
         private int attribute;
 
+        @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
+        private int attributeWithoutPublicSetterButWithSuppressAnnotation;
+
         @PluginElement("layout")
         private Layout<? extends Serializable> layout;
 

log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePluginPublicSetter.java.source

@@ -0,0 +1,89 @@
+/*
+ * 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 java.io.Serializable;
+import org.apache.logging.log4j.core.Layout;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.config.Configuration;
+import org.apache.logging.log4j.core.config.Node;
+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.PluginAttribute;
+import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute;
+import org.apache.logging.log4j.core.config.plugins.PluginConfiguration;
+import org.apache.logging.log4j.core.config.plugins.PluginElement;
+import org.apache.logging.log4j.core.config.plugins.PluginFactory;
+import org.apache.logging.log4j.core.config.plugins.PluginLoggerContext;
+import org.apache.logging.log4j.core.config.plugins.PluginNode;
+import org.apache.logging.log4j.core.config.plugins.PluginValue;
+
+/**
+ * Test plugin class for unit tests.
+ */
+@Plugin(name = "Fake", category = "Test")
+@PluginAliases({"AnotherFake", "StillFake"})
+public class FakePluginPublicSetter {
+
+    @Plugin(name = "Nested", category = "Test")
+    public static class Nested {}
+
+    @PluginFactory
+    public static FakePluginPublicSetter newPlugin(
+            @PluginAttribute("attribute") int attribute,
+            @PluginElement("layout") Layout<? extends Serializable> layout,
+            @PluginConfiguration Configuration config,
+            @PluginNode Node node,
+            @PluginLoggerContext LoggerContext loggerContext,
+            @PluginValue("value") String value) {
+        return null;
+    }
+
+    public static Builder newBuilder() {
+        return new Builder();
+    }
+
+    public static class Builder implements org.apache.logging.log4j.core.util.Builder<FakePluginPublicSetter> {
+
+        @PluginBuilderAttribute
+        private int attribute;
+
+        @PluginBuilderAttribute
+        @SuppressWarnings("log4j.public.setter")
+        private int attributeWithoutPublicSetterButWithSuppressAnnotation;
+
+        @PluginElement("layout")
+        private Layout<? extends Serializable> layout;
+
+        @PluginConfiguration
+        private Configuration config;
+
+        @PluginNode
+        private Node node;
+
+        @PluginLoggerContext
+        private LoggerContext loggerContext;
+
+        @PluginValue("value")
+        private String value;
+
+        @Override
+        public FakePluginPublicSetter build() {
+            return null;
+        }
+    }
+}

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"),

log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java

@@ -0,0 +1,106 @@
+/*
+ * 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 static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+import java.io.File;
+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.apache.commons.io.FileUtils;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.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()
+                    + "PublicSetter.java.source";
+
+    private File createdFile;
+    private DiagnosticCollector<JavaFileObject> diagnosticCollector;
+    private List<Diagnostic<? extends JavaFileObject>> errorDiagnostics;
+
+    @BeforeEach
+    void setup() {
+        // Instantiate the tooling
+        final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
+        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(sourceFile).exists();
+
+        final File orig = sourceFile.toFile();
+        createdFile = new File(orig.getParentFile(), "FakePluginPublicSetter.java");
+        assertDoesNotThrow(() -> FileUtils.copyFile(orig, createdFile));
+
+        // get compilation units
+        Iterable<? extends JavaFileObject> compilationUnits = fileManager.getJavaFileObjects(createdFile);
+
+        JavaCompiler.CompilationTask task = compiler.getTask(
+                null,
+                fileManager,
+                diagnosticCollector,
+                Arrays.asList("-proc:only", "-processor", PluginProcessor.class.getName()),
+                null,
+                compilationUnits);
+        task.call();
+
+        errorDiagnostics = diagnosticCollector.getDiagnostics().stream()
+                .filter(diagnostic -> diagnostic.getKind() == Diagnostic.Kind.ERROR)
+                .collect(Collectors.toList());
+    }
+
+    @AfterEach
+    void tearDown() {
+        assertDoesNotThrow(() -> FileUtils.delete(createdFile));
+    }
+
+    @Test
+    void warnWhenPluginBuilderAttributeLacksPublicSetter() {
+
+        assertThat(errorDiagnostics).anyMatch(errorMessage -> errorMessage
+                .getMessage(Locale.ROOT)
+                .contains("The field `attribute` does not have a public setter"));
+    }
+
+    @Test
+    void ignoreWarningWhenSuppressWarningsIsPresent() {
+        assertThat(errorDiagnostics)
+                .allMatch(
+                        errorMessage -> !errorMessage
+                                .getMessage(Locale.ROOT)
+                                .contains(
+                                        "The field `attributeWithoutPublicSetterButWithSuppressAnnotation` does not have a public setter"));
+    }
+}

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

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,20 @@
 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.commons.lang3.StringUtils;
 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;
 
 /**
@@ -60,6 +68,8 @@ public class PluginProcessor extends AbstractProcessor {
 
     private static final Element[] EMPTY_ELEMENT_ARRAY = {};
 
+    private static final String SUPPRESS_WARNING_PUBLIC_SETTER_STRING = "log4j.public.setter";
+
     /**
      * The location of the plugin cache data file. This file is written to by this processor, and read from by
      * {@link org.apache.logging.log4j.core.config.plugins.util.PluginManager}.
@@ -83,6 +93,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 +123,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(SUPPRESS_WARNING_PUBLIC_SETTER_STRING)) {
+            // 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.equals(
+                                        String.format("set%s", expectedFieldNameInASetter(fieldName)))
+                                || methodName.equals(String.format("with%s", expectedFieldNameInASetter(fieldName)));
+
+                        // 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.ERROR,
+                            String.format(
+                                    "The field `%s` does not have a public setter, Note that @SuppressWarnings(\"%s\"), can be used on the field to suppress the compilation error. ",
+                                    fieldName, SUPPRESS_WARNING_PUBLIC_SETTER_STRING),
+                            element);
+        }
+    }
+
+    /**
+     *  Helper method to get the expected Method name in a field.
+     *  For example if the field name is 'isopen', then the expected setter would be 'setOpen' or 'withOpen'
+     *  This method is supposed to return the capitalized 'Open', fieldName which is expected in the setter.
+     * @param fieldName who's setter we are checking.
+     * @return The expected fieldName that will come after withxxxx or setxxxx
+     */
+    public static String expectedFieldNameInASetter(String fieldName) {
+        if (fieldName.startsWith("is")) fieldName = fieldName.substring(2);
+
+        if (StringUtils.isEmpty(fieldName)) return ""; // Return empty string if the field is a blank String.
+
+        return String.format(
+                "%s%s",
+                fieldName.substring(0, 1).toUpperCase(Locale.ROOT),
+                fieldName.substring(1)); // we do not need to Lowercase here examples like field name 'ignoreExceptions'
+    }
+
     private void collectPlugins(final Iterable<? extends Element> elements) {
         final Elements elementUtils = processingEnv.getElementUtils();
         final ElementVisitor<PluginEntry, Plugin> pluginVisitor = new PluginElementVisitor(elementUtils);