ppkarwasz commented on code in PR #3195: URL: https://github.com/apache/logging-log4j2/pull/3195#discussion_r1853398829
########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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; Review Comment: ```suggestion import org.junit.jupiter.api.Test; ``` Please use JUnit 5. Our JUnit 4 tests are being currently rewritten to JUnit 5. When using JUnit 5, the recommended style is to make all classes and methods package-private instead of `public`. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java: ########## @@ -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, Review Comment: I think that `ERROR` would be fine here. Just make sure that the message contains a mention about the `@SuppressWarnings` annotation, so third-party Log4j plugin developers don't get crazy if they can not create a setter. ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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() { + // 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()); + + JavaCompiler.CompilationTask task = compiler.getTask( + null, + fileManager, + diagnosticCollector, + Arrays.asList("-proc:only", "-processor", PluginProcessor.class.getName()), + null, + compilationUnits); + task.call(); + + 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() { Review Comment: ```suggestion public void ignoreWarningWhenSuppressWarningsIsPresent() { ``` ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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() { + // 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()); + + JavaCompiler.CompilationTask task = compiler.getTask( + null, + fileManager, + diagnosticCollector, + Arrays.asList("-proc:only", "-processor", PluginProcessor.class.getName()), + null, + compilationUnits); + task.call(); + + 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() { + // 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(); Review Comment: ```suggestion assertThat(sourceFile).exists(); ``` ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java: ########## @@ -0,0 +1,107 @@ +/* + * 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() { + // 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(); Review Comment: ```suggestion assertThat(sourceFile).exists(); ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java: ########## @@ -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))); + } Review Comment: I think we could be stricter here and use a case-sensitive comparison, for example add a helper method that: 1. Removes an `is` prefix from the field name. 2. Capitalizes the first letter. 3. Compares with the method name with `set` or `with` removed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
