You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by ta924 <gi...@git.apache.org> on 2017/07/17 14:13:35 UTC
[GitHub] logging-log4j2 pull request #93: modification to resolver to discover jar as...
GitHub user ta924 opened a pull request:
https://github.com/apache/logging-log4j2/pull/93
modification to resolver to discover jar assests without the manipula…
…tion of asset paths.
This PR is to address issue https://issues.apache.org/jira/browse/LOG4J2-1852. If the resource submitted for scanning is of Jar protocol it utilizes a JarURLConnection to scan for classes matching the test, it will no longer manipulate the URI for Jars. In the event the protocol is file it will proceed with the existing approach.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ta924/logging-log4j2 PATCH-LOG4J2-1852
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/logging-log4j2/pull/93.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #93
----
commit f7fdc3a3edae62f936233935968bfa0cdd0486e0
Author: Tanner Altares <ta...@walmart.com>
Date: 2017-07-17T14:01:04Z
modification to resolver to discover jar assests without the manipulation of asset paths.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
Sorry Gary had some things pop up on the work front that have eaten up most of my time. I will get you an example as soon as things free up.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #93: modification to resolver to discover jar as...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on a diff in the pull request:
https://github.com/apache/logging-log4j2/pull/93#discussion_r127725532
--- Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/ResolverUtil.java ---
@@ -329,6 +331,49 @@ private void loadImplementationsInJar(final Test test, final String parent, fina
}
/**
+ * Finds matching classes within a jar files that contains a folder structure matching the package structure. If the
+ * File is not a JarFile or does not exist a warning will be logged, but no error will be raised.
+ *
+ * @param test
+ * a Test used to filter the classes that are discovered
+ * @param parent
+ * the parent package under which classes must be in order to be considered
+ * @param connection
+ * the URL Connection to be examined for classes
+ */
+ private void loadImplementationsInJar(final ResolverUtil.Test test, final String parent, final URLConnection connection) {
+
+ JarFile jarFile = null;
+ try {
+ if (connection != null && connection instanceof JarURLConnection) {
+ JarURLConnection jarURLConnection = (JarURLConnection) connection;
+ jarFile = jarURLConnection.getJarFile();
+ final Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String name = entry.getName();
+
+ if (!entry.isDirectory() && name.startsWith(parent) && this.isTestApplicable(test, name)) {
+ this.addIfMatching(test, name);
+ }
+ }
+ }
+ }catch (IOException e){
+ LOGGER.error("Could not search JAR file '{}' for classes matching criteria {}, file not found", jarFile,
+ test, e);
+ }finally {
+ if (jarFile != null){
+ try {
--- End diff --
Is it possible to simplify with a try-with-resources block?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #93: modification to resolver to discover jar as...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on a diff in the pull request:
https://github.com/apache/logging-log4j2/pull/93#discussion_r127726686
--- Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/ResolverUtil.java ---
@@ -210,11 +210,13 @@ public void findInPackage(final Test test, String packageName) {
} else if (BUNDLE_RESOURCE.equals(url.getProtocol())) {
loadImplementationsInBundle(test, packageName);
} else {
- final File file = new File(urlPath);
- if (file.isDirectory()) {
- loadImplementationsInDirectory(test, packageName, file);
- } else {
- loadImplementationsInJar(test, packageName, file);
+ if (JAR.equals(url.getProtocol())){
+ this.loadImplementationsInJar(test, packageName, url.openConnection());
--- End diff --
Nevermind, the openConnection() does not allocate an actual connection.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
I will evaluate the test logic in place to see if it is an option to create a sample jar of the same structure.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
Submitted code change to introduce try-with-resources block, thank you for the catch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
Hi. Were you able to create a sample jar for testing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
After some additional research I found that an internal library was excluding all log4jplugins.dat files to ensure that only specific packages were scanned. This was accomplished by a custom classloader. This forced the PluginManager to to attempt resolution based off relocated package names from the shade plugin. This modification made scanning and resolution possible for the custom wrappers. I noticed that Spring Boot includes a log4jplugins.dat file, which was getting loaded by default while trying to create a sample app. That being said this becomes much more of a one off situation and doesn't fit the normal operating patterns of log4j. I think the approach is a good fall back in the event log4jplugins.dat file is not present and provided a resolution mechanism for core plugins.
Again sorry for the late turn around on correspondence on the PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
Hi @ta924 Any news?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #93: modification to resolver to discover jar as...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on a diff in the pull request:
https://github.com/apache/logging-log4j2/pull/93#discussion_r127734669
--- Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/ResolverUtil.java ---
@@ -329,6 +331,49 @@ private void loadImplementationsInJar(final Test test, final String parent, fina
}
/**
+ * Finds matching classes within a jar files that contains a folder structure matching the package structure. If the
+ * File is not a JarFile or does not exist a warning will be logged, but no error will be raised.
+ *
+ * @param test
+ * a Test used to filter the classes that are discovered
+ * @param parent
+ * the parent package under which classes must be in order to be considered
+ * @param connection
+ * the URL Connection to be examined for classes
+ */
+ private void loadImplementationsInJar(final ResolverUtil.Test test, final String parent, final URLConnection connection) {
+
+ JarFile jarFile = null;
+ try {
+ if (connection != null && connection instanceof JarURLConnection) {
+ JarURLConnection jarURLConnection = (JarURLConnection) connection;
+ jarFile = jarURLConnection.getJarFile();
+ final Enumeration<JarEntry> entries = jarFile.entries();
+
+ while (entries.hasMoreElements()) {
+ JarEntry entry = entries.nextElement();
+ String name = entry.getName();
+
+ if (!entry.isDirectory() && name.startsWith(parent) && this.isTestApplicable(test, name)) {
+ this.addIfMatching(test, name);
+ }
+ }
+ }
+ }catch (IOException e){
+ LOGGER.error("Could not search JAR file '{}' for classes matching criteria {}, file not found", jarFile,
+ test, e);
+ }finally {
+ if (jarFile != null){
+ try {
--- End diff --
Yes agreed this can be cleaned up with try-with-resources block. I will make those modifications.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 issue #93: modification to resolver to discover jar assests w...
Posted by ta924 <gi...@git.apache.org>.
Github user ta924 commented on the issue:
https://github.com/apache/logging-log4j2/pull/93
Hi. Sorry for the delay had to prep for a few presentations the last couple days. I will shoot to have one ready by the end of the week.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] logging-log4j2 pull request #93: modification to resolver to discover jar as...
Posted by garydgregory <gi...@git.apache.org>.
Github user garydgregory commented on a diff in the pull request:
https://github.com/apache/logging-log4j2/pull/93#discussion_r127725284
--- Diff: log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/ResolverUtil.java ---
@@ -210,11 +210,13 @@ public void findInPackage(final Test test, String packageName) {
} else if (BUNDLE_RESOURCE.equals(url.getProtocol())) {
loadImplementationsInBundle(test, packageName);
} else {
- final File file = new File(urlPath);
- if (file.isDirectory()) {
- loadImplementationsInDirectory(test, packageName, file);
- } else {
- loadImplementationsInJar(test, packageName, file);
+ if (JAR.equals(url.getProtocol())){
+ this.loadImplementationsInJar(test, packageName, url.openConnection());
--- End diff --
Thank you for the patch. Who closes this connection?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---