-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix(#2769): Modify the annotation processor in 2.x to give a warning if a plugin builder attribute does not have a public setter. #3195
base: 2.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! I added some minor remarks below.
If you haven't done it already, please sign the ASF ICLA.
.../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
Outdated
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
Outdated
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
Outdated
Show resolved
Hide resolved
.../org/apache/logging/log4j/core/config/plugins/processor/PluginProcessorPublicSetterTest.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/org/apache/logging/log4j/core/config/plugins/processor/PluginProcessor.java
Outdated
Show resolved
Hide resolved
Sounds good @ppkarwasz let me fix and rebase this! 🙂 |
50b1e54
to
d6e4898
Compare
…rning if a plugin builder attribute does not have a public setter.
d6e4898
to
024654e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
} | ||
} | ||
|
||
private void processBuilderAttributes(final VariableElement element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void processBuilderAttributes(final VariableElement element) { | |
private void processBuilderAttribute(final VariableElement element) { |
Small typo.
I sent you an invitation to the Apache Slack to the address you use in your commits. |
Hi @ppkarwasz the build was failing, hence I put the pr in draft 🥲. Thanks for approving it, but I will have to make a revision. I am not sure I got the slack invitation, was the email [email protected]? |
No, I used the one from you University/Alma Mater that you use for your commits. |
Hi @ppkarwasz i am trying to move away from that email, because I don't have access to that anymore 🥲 will change it in my commits as well! I apologize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jaykataria1111,
The OSGi fail is due to an "unauthorized" usage of Commons Lang 3. All optional Maven dependencies, must be explicitly marked as optional in OSGi, if we need them. In this case we don't.
import javax.tools.Diagnostic; | ||
import javax.tools.FileObject; | ||
import javax.tools.StandardLocation; | ||
import org.apache.commons.lang3.StringUtils; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make this private
, we only expose public methods, when we need to.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' | |
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()
.
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")
attribute can be used to ignore this compilation warning incase it is a known issue.Checklist
2.x
branch if you are targeting Log4j 2; usemain
otherwise./mvnw verify
succeeds (if it fails due to code formatting issues reported by Spotless, simply run./mvnw spotless:apply
and retry)src/changelog/.2.x.x
directory