ppkarwasz commented on code in PR #3195:
URL: https://github.com/apache/logging-log4j2/pull/3195#discussion_r1860169416
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java:
##########
@@ -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;
Review Comment:
This is the reason the OSGi builds fail.
We don't have `commons-lang3` as direct dependency. It is brought in the
project by the **optional** `commons-compress` dependency.
BND does not automatically mark optional Maven dependencies as optional OSGi
dependencies and for good reasons (see bndtools/bnd#2713 for a discussion), so
the OSGi tests detected that the **required** `org.apache.commons.lang3`
package is missing. Please don't use Commons Lang 3, we are a
bottom-of-the-stack library with no required dependencies.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java:
##########
@@ -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) {
Review Comment:
Please make this `private`, we only expose public methods, when we need to.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java:
##########
@@ -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'
Review Comment:
```suggestion
return fieldName.isEmpty() ? fieldName :
Character.toUpperCase(fieldName.charAt(0)) + fieldName.substring(1);
```
In this case, we don't need `StringUtils.isEmpty()`, since the argument is
certainly non null. In the cases we need it, we have a copy in
`o.a.l.l.util.Strings.isEmpty()`.
--
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]
