-
Notifications
You must be signed in to change notification settings - Fork 29
Add Log4j 1 to Log4j 2 configuration file conversion #202
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
base: main
Are you sure you want to change the base?
Add Log4j 1 to Log4j 2 configuration file conversion #202
Conversation
This PR uses the [Log4j Configuration Converter API](https://logging.staged.apache.org/log4j/transform/log4j-converter-config.html) to add a `Log4j1ConfigurationToLog4jCore2` rule, which converts `log4j.properties` and `log4j.xml` configuration files into the equivalent `log4j2.xml` configuration files. It also refactors the `Log4j1toLog4j2` recipe into three parts: - `Log4j1toLog4jAPI` performs the migration from the "Log4j 1 API" to Log4j API 2, as described in [Log4j 1 API migration](https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#api-migration). This recipe performs almost exclusively Java code changes and is usually not required in applications that use JCL or SLF4J as logging interface. - `Log4j1ConfigurationToLog4jCore2` migrates configuration files, as described in the [Log4j 1 Configuration file migration](https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#configuration-file-migration) section. - `Log4j1ToLog4jCore2` migrates the runtime dependencies to use Log4j Core 2 instead of Log4j 1, as described in [Log4j 1 Backend migration](https://logging.apache.org/log4j/2.x/migrate-from-log4j1.html#backend-migration) and also calls the previous recipe. This is probably the most useful recipe for users that no longer user Log4j 1 directly, but use it as logging implementation. Closes openrewrite#154. Related to apache/logging-log4j2#3220
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/logging/ConvertConfigurationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/logging/ConvertConfigurationTest.java
Outdated
Show resolved
Hide resolved
|
Thanks @ppkarwasz ! The bot suggestions can be a little confusing at times; it wants you to place the static imports last if that was unclear before. I'll try to review in the coming days! Right now caught up midway through a release and some meetings. |
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/logging/ConvertConfigurationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/java/logging/ConvertConfiguration.java
Outdated
Show resolved
Hide resolved
src/test/java/org/openrewrite/java/logging/ConvertConfigurationTest.java
Outdated
Show resolved
Hide resolved
They are indeed confusing, thanks for the explanation.
I am also working on the |
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.
Had a closer look already and applied some polish & suggestions. Thanks for working on these recipes ahead of the release! Should help adoption I imagine. Do let us know if you'd like to coordinate a merge and patch release of rewrite-logging-frameworks at some point.
|
|
||
| implementation("org.apache.logging.log4j:log4j-core:2.+") | ||
| implementation("org.slf4j:slf4j-api:2.+") | ||
| implementation("org.apache.logging.log4j:log4j-converter-config:0.3.0-SNAPSHOT") |
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.
Indeed likely best to merge this once released; thanks for creating the recipe before then!
| implementation("org.apache.logging.log4j:log4j-converter-config:0.3.0-SNAPSHOT") | |
| implementation("org.apache.logging.log4j:log4j-converter-config:latest.release") |
| repositories { | ||
| mavenCentral() | ||
| mavenLocal() | ||
| // Temporarily add Apache Snapshot repository for Log4j artifacts | ||
| maven { | ||
| setUrl("https://repository.apache.org/snapshots") | ||
| mavenContent { | ||
| // Excessive 404 result codes in `repository.apache.org` will ban the worker IP | ||
| // https://infra.apache.org/infra-ban.html | ||
| snapshotsOnly() | ||
| excludeGroupAndSubgroups("org.openrewrite") | ||
| } | ||
| } | ||
| } | ||
|
|
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.
Similarly best removed before a merge.
| repositories { | |
| mavenCentral() | |
| mavenLocal() | |
| // Temporarily add Apache Snapshot repository for Log4j artifacts | |
| maven { | |
| setUrl("https://repository.apache.org/snapshots") | |
| mavenContent { | |
| // Excessive 404 result codes in `repository.apache.org` will ban the worker IP | |
| // https://infra.apache.org/infra-ban.html | |
| snapshotsOnly() | |
| excludeGroupAndSubgroups("org.openrewrite") | |
| } | |
| } | |
| } |
|
|
||
| testImplementation("org.openrewrite:rewrite-kotlin:${rewriteVersion}") | ||
| testImplementation("org.openrewrite:rewrite-maven") | ||
| testImplementation("org.openrewrite:rewrite-properties:${rewriteVersion}") |
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 rewrite-bom manages the version here, so we can leave that out already.
| testImplementation("org.openrewrite:rewrite-properties:${rewriteVersion}") | |
| testImplementation("org.openrewrite:rewrite-properties") |
| converter.convert(inputStream, inputFormat, outputStream, outputFormat); | ||
| String utf8 = new String(outputStream.toByteArray(), StandardCharsets.UTF_8); | ||
|
|
||
| return PlainTextParser.convert(sourceFile) |
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.
It looks like the output files are actually XML; should we use the XML parser here to read the converted contents such that subsequent recipes can optionally target their contents as part of the same recipe run?
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 exactly the main question I had for you. The log4j-converter-config artifact can convert to multiple formats (right now log4j2.xml, log4j2.json, log4j2.yaml and log4j2.properties are supported).
In the "Log4j 1 to Log4j Core 2" recipe I only use the XML output, but the ConvertConfiguration recipe could be used by users for other conversions. Should I make a switch here on outputFormat and use PlainText only if the format is not one of the recognized ones?
|
This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
I didn't have time to release Apache Log4j Transformer yet, but this PR is on my TODO list. |
|
This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
I still plan to deliver this, although the ETA is not certain. |
|
This PR is stale because it has been open for 90 days with no activity. Remove |
|
I was still unable to make a |
What's changed?
This PR uses the Log4j Configuration Converter API to add a
Log4j1ConfigurationToLog4jCore2rule, which convertslog4j.propertiesandlog4j.xmlconfiguration files into the equivalentlog4j2.xmlconfiguration files.It also refactors the
Log4j1toLog4j2recipe into three parts:Log4j1toLog4jAPIperforms the migration from the "Log4j 1 API" to Log4j API 2, as described in Log4j 1 API migration. This recipe performs almost exclusively Java code changes and is usually not required in applications that use JCL or SLF4J as logging interface.Log4j1ConfigurationToLog4jCore2migrates configuration files, as described in the Log4j 1 Configuration file migration section.Log4j1ToLog4jCore2migrates the runtime dependencies to use Log4j Core 2 instead of Log4j 1, as described in Log4j 1 Backend migration and also calls the previous recipe. This is probably the most useful recipe for users that no longer user Log4j 1 directly, but use it as logging implementation.What's your motivation?
This PR includes the last missing ingredient for a complete Log4j 1 to Log4j API/Core 2 migration: the conversion of configuration files.
Anything in particular you'd like reviewers to focus on?
This PR is a draft, since the "Log4j Configuration Converter API" it uses hasn't been published yet. We will publish
log4j-converter-configas soon as we are reasonably sure that it can be used in OpenRewrite without modifications.The configuration converter recipe is special, since not only it rewrites the content of a file, but it also can change its type (e.g. from
Properties.FiletoXML.Document). Currently the recipe:SourceFileof typePlainTextregardless of the actual type of the converted file and encodes the file using UTF-8.Is there a way to skip the decoding/encoding of the converted configuration file? I tried
Binary, but in that caseprintAllAsBytes()fails.Anyone you would like to review specifically?
@timtebeek
Checklist