From e1715dc9f3132c70f1a87a6a75058d02a0b69d63 Mon Sep 17 00:00:00 2001 From: Jay Katariya <32147410+jaykataria1111@users.noreply.github.com> Date: Thu, 28 Nov 2024 23:13:39 -0800 Subject: [PATCH] Warn if plugin attribute has no public setter (#3195) The following commit modifies the annotation processor in 2.x to shell out a warning if the plugin builder attribute does not have a public setter. A `@SuppressWarnings("log4j.public.setter")` annotation can be used to ignore this compilation warning in case it is a known issue. --- .../config/plugins/processor/FakePlugin.java | 5 + .../FakePluginPublicSetter.java.source | 89 ++++++++++++++ .../processor/GraalVmProcessorTest.java | 1 + .../PluginProcessorPublicSetterTest.java | 110 ++++++++++++++++++ .../core/appender/db/jdbc/JdbcAppender.java | 2 + .../plugins/processor/PluginProcessor.java | 97 +++++++++++++++ .../net/SocketPerformancePreferences.java | 3 + .../log4j/web/appender/ServletAppender.java | 1 + .../log4j/web/appender/ServletAppender.java | 1 + ...69_pluginAttribute_publicSetterWarning.xml | 8 ++ 10 files changed, 317 insertions(+) create mode 100644 log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePluginPublicSetter.java.source create mode 100644 log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java create mode 100644 src/changelog/.2.x.x/2769_pluginAttribute_publicSetterWarning.xml 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 index babf3a16ae8..0343f3bb358 100644 --- 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 @@ -60,8 +60,13 @@ public static Builder newBuilder() { public static class Builder implements org.apache.logging.log4j.core.util.Builder { @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") private int attribute; + @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") + private int attributeWithoutPublicSetterButWithSuppressAnnotation; + @PluginElement("layout") private Layout layout; diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePluginPublicSetter.java.source b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/FakePluginPublicSetter.java.source new file mode 100644 index 00000000000..045f03f0fcf --- /dev/null +++ b/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 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 { + + @PluginBuilderAttribute + private int attribute; + + @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") + private int attributeWithoutPublicSetterButWithSuppressAnnotation; + + @PluginElement("layout") + private Layout layout; + + @PluginConfiguration + private Configuration config; + + @PluginNode + private Node node; + + @PluginLoggerContext + private LoggerContext loggerContext; + + @PluginValue("value") + private String value; + + @Override + public FakePluginPublicSetter build() { + return null; + } + } +} 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 index 2c0133835db..9f8b51a7b75 100644 --- 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 new file mode 100644 index 00000000000..587490e2c92 --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java @@ -0,0 +1,110 @@ +/* + * 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 diagnosticCollector; + private List> 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 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)); + File pluginsDatFile = Paths.get("Log4j2Plugins.dat").toFile(); + if (pluginsDatFile.exists()) { + pluginsDatFile.delete(); + } + } + + @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")); + } +} 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 index 48b1f4820aa..523994cea55 100644 --- 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> extends AbstractDatabaseAppend private ConnectionSource connectionSource; @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") private boolean immediateFail; @PluginBuilderAttribute @@ -80,6 +81,7 @@ public static class Builder> 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 index cb4b9444e98..cf268c22dd8 100644 --- 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; /** @@ -60,6 +67,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 +92,12 @@ public boolean process(final Set annotations, final Round final Set elements = roundEnv.getElementsAnnotatedWith(Plugin.class); collectPlugins(elements); processedElements.addAll(elements); + + // process plugin builder Attributes + final Set pluginAttributeBuilderElements = + roundEnv.getElementsAnnotatedWith(PluginBuilderAttribute.class); + processBuilderAttribute(pluginAttributeBuilderElements); + processedElements.addAll(pluginAttributeBuilderElements); } // Write the cache file if (roundEnv.processingOver() && !processedElements.isEmpty()) { @@ -107,6 +122,88 @@ public boolean process(final Set annotations, final Round return false; } + private void processBuilderAttribute(final Iterable elements) { + for (final Element element : elements) { + if (element instanceof VariableElement) { + processBuilderAttribute((VariableElement) element); + } + } + } + + private void processBuilderAttribute(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 + */ + private static String expectedFieldNameInASetter(String fieldName) { + if (fieldName.startsWith("is")) fieldName = fieldName.substring(2); + + return fieldName.isEmpty() ? fieldName : Character.toUpperCase(fieldName.charAt(0)) + fieldName.substring(1); + } + private void collectPlugins(final Iterable elements) { final Elements elementUtils = processingEnv.getElementUtils(); final ElementVisitor 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 index 61f711348bf..fc99f2470d8 100644 --- 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 index e9e14fcd43b..f61571f2b3d 100644 --- 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> extends AbstractAppender.Build implements org.apache.logging.log4j.core.util.Builder { @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") private boolean logThrowables; @Override diff --git a/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java b/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java index b3f5e674a79..d5795c8cd3f 100644 --- a/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java +++ b/log4j-web/src/main/java/org/apache/logging/log4j/web/appender/ServletAppender.java @@ -40,6 +40,7 @@ public static class Builder> extends AbstractAppender.Build implements org.apache.logging.log4j.core.util.Builder { @PluginBuilderAttribute + @SuppressWarnings("log4j.public.setter") // The setter is not assignable. 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 new file mode 100644 index 00000000000..e55e1740375 --- /dev/null +++ b/src/changelog/.2.x.x/2769_pluginAttribute_publicSetterWarning.xml @@ -0,0 +1,8 @@ + + + + Adding a compilation warning for Plugin Builder Attributes that do not have a public setter. + \ No newline at end of file