This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 38d4392df0039e0dec730227ebbe7a36a73417fa Author: Gary Gregory <[email protected]> AuthorDate: Sat Feb 19 18:12:56 2022 -0500 Allow for whitespace in property files. Tests are from PR #762 by ppkarwasz. Conflicts: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java log4j-1.2-api/src/test/java/org/apache/log4j/config/Log4j1ConfigurationFactoryTest.java log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java --- .../org/apache/log4j/builders/BuilderManager.java | 105 ++++++++++++--------- .../log4j/config/Log4j1ConfigurationParser.java | 11 ++- .../config/Log4j1ConfigurationFactoryTest.java | 23 +++++ .../log4j/config/PropertiesConfigurationTest.java | 51 ++++++++-- .../config-1.2/log4j-untrimmed.properties | 28 ++++++ 5 files changed, 156 insertions(+), 62 deletions(-) diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java index 6672c8e..d0a8531 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java @@ -36,7 +36,7 @@ import org.apache.logging.log4j.status.StatusLogger; import org.apache.logging.log4j.util.LoaderUtil; import org.w3c.dom.Element; -import java.lang.reflect.Constructor; +import java.util.Locale; import java.util.Map; import java.util.Objects; import java.util.Properties; @@ -58,21 +58,48 @@ public class BuilderManager { plugins = injector.getInstance(PLUGIN_MANAGER_KEY).getPlugins(); } + @SuppressWarnings("unchecked") + private <T extends Builder> T createBuilder(PluginType<T> plugin, String prefix, Properties props) { + try { + Class<T> clazz = plugin.getPluginClass(); + if (AbstractBuilder.class.isAssignableFrom(clazz)) { + return clazz.getConstructor(CONSTRUCTOR_PARAMS).newInstance(prefix, props); + } + Object builder = LoaderUtil.newInstanceOf(clazz); + // Reasonable message instead of `ClassCastException` + if (!Builder.class.isAssignableFrom(clazz)) { + LOGGER.warn("Unable to load plugin: builder {} does not implement {}", clazz, Builder.class); + return null; + } + return (T) builder; + } catch (ReflectiveOperationException ex) { + LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); + return null; + } + } + + @SuppressWarnings("unchecked") + private <T> PluginType<T> getPlugin(String className) { + Objects.requireNonNull(plugins, "plugins"); + Objects.requireNonNull(className, "className"); + return (PluginType<T>) plugins.get(className.toLowerCase(Locale.ROOT).trim()); + } + public Appender parseAppender(String className, Element appenderElement, XmlConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<AppenderBuilder> plugin = getPlugin(className); if (plugin != null) { - final Class<? extends AppenderBuilder> pluginClass = plugin.getPluginClass().asSubclass(AppenderBuilder.class); - final AppenderBuilder builder = injector.getInstance(pluginClass); - return builder.parseAppender(appenderElement, config); + try { + return LoaderUtil.newInstanceOf(plugin.getPluginClass()).parseAppender(appenderElement, config); + } catch (ReflectiveOperationException ex) { + LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); + } } return null; } - public Appender parseAppender(String name, String className, String prefix, String layoutPrefix, - String filterPrefix, Properties props, PropertiesConfiguration config) { - Objects.requireNonNull(plugins, "plugins"); - Objects.requireNonNull(className, "className"); - PluginType<?> plugin = plugins.get(className.toLowerCase()); + public Appender parseAppender(String name, String className, String prefix, String layoutPrefix, String filterPrefix, Properties props, + PropertiesConfiguration config) { + PluginType<AppenderBuilder> plugin = getPlugin(className); if (plugin != null) { AppenderBuilder builder = createBuilder(plugin, prefix, props); if (builder != null) { @@ -83,17 +110,19 @@ public class BuilderManager { } public Filter parseFilter(String className, Element filterElement, XmlConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<FilterBuilder> plugin = getPlugin(className); if (plugin != null) { - final Class<? extends FilterBuilder> pluginClass = plugin.getPluginClass().asSubclass(FilterBuilder.class); - final FilterBuilder builder = injector.getInstance(pluginClass); - return builder.parseFilter(filterElement, config); + try { + return LoaderUtil.newInstanceOf(plugin.getPluginClass()).parseFilter(filterElement, config); + } catch (ReflectiveOperationException ex) { + LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); + } } return null; } public Filter parseFilter(String className, String filterPrefix, Properties props, PropertiesConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<FilterBuilder> plugin = getPlugin(className); if (plugin != null) { FilterBuilder builder = createBuilder(plugin, filterPrefix, props); if (builder != null) { @@ -104,17 +133,19 @@ public class BuilderManager { } public Layout parseLayout(String className, Element layoutElement, XmlConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<LayoutBuilder> plugin = getPlugin(className); if (plugin != null) { - final Class<? extends LayoutBuilder> pluginClass = plugin.getPluginClass().asSubclass(LayoutBuilder.class); - final LayoutBuilder builder = injector.getInstance(pluginClass); - return builder.parseLayout(layoutElement, config); + try { + return LoaderUtil.newInstanceOf(plugin.getPluginClass()).parseLayout(layoutElement, config); + } catch (ReflectiveOperationException ex) { + LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); + } } return null; } public Layout parseLayout(String className, String layoutPrefix, Properties props, PropertiesConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<LayoutBuilder> plugin = getPlugin(className); if (plugin != null) { LayoutBuilder builder = createBuilder(plugin, layoutPrefix, props); if (builder != null) { @@ -125,17 +156,19 @@ public class BuilderManager { } public RewritePolicy parseRewritePolicy(String className, Element rewriteElement, XmlConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<RewritePolicyBuilder> plugin = getPlugin(className); if (plugin != null) { - final Class<? extends RewritePolicyBuilder> pluginClass = plugin.getPluginClass().asSubclass(RewritePolicyBuilder.class); - final RewritePolicyBuilder builder = injector.getInstance(pluginClass); - return builder.parseRewritePolicy(rewriteElement, config); + try { + return LoaderUtil.newInstanceOf(plugin.getPluginClass()).parseRewritePolicy(rewriteElement, config); + } catch (ReflectiveOperationException ex) { + LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); + } } return null; } public RewritePolicy parseRewritePolicy(String className, String policyPrefix, Properties props, PropertiesConfiguration config) { - PluginType<?> plugin = plugins.get(className.toLowerCase()); + PluginType<RewritePolicyBuilder> plugin = getPlugin(className); if (plugin != null) { RewritePolicyBuilder builder = createBuilder(plugin, policyPrefix, props); if (builder != null) { @@ -145,26 +178,4 @@ public class BuilderManager { return null; } - @SuppressWarnings("unchecked") - private <T extends Builder> T createBuilder(PluginType<?> plugin, String prefix, Properties props) { - try { - Class<?> clazz = plugin.getPluginClass(); - if (AbstractBuilder.class.isAssignableFrom(clazz)) { - Constructor<T> constructor = - (Constructor<T>) clazz.getConstructor(CONSTRUCTOR_PARAMS); - return constructor.newInstance(prefix, props); - } - Object builder = LoaderUtil.newInstanceOf(clazz); - // Reasonable message instead of `ClassCastException` - if (!Builder.class.isAssignableFrom(clazz)) { - LOGGER.warn("Unable to load plugin: builder {} does not implement {}", clazz, Builder.class); - return null; - } - return (T) builder; - } catch (ReflectiveOperationException ex) { - LOGGER.warn("Unable to load plugin: {} due to: {}", plugin.getKey(), ex.getMessage()); - return null; - } - } - } diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java index 5425244..d345ba0 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java @@ -143,13 +143,13 @@ public class Log4j1ConfigurationParser { for (final Map.Entry<Object, Object> entry : properties.entrySet()) { final Object keyObj = entry.getKey(); if (keyObj != null) { - final String key = keyObj.toString(); + final String key = keyObj.toString().trim(); if (key.startsWith(prefix)) { if (key.indexOf('.', preLength) < 0) { final String name = key.substring(preLength); final Object value = entry.getValue(); if (value != null) { - map.put(name, value.toString()); + map.put(name, value.toString().trim()); } } } @@ -394,13 +394,13 @@ public class Log4j1ConfigurationParser { for (final Map.Entry<Object, Object> entry : properties.entrySet()) { final Object keyObj = entry.getKey(); if (keyObj != null) { - final String key = keyObj.toString(); + final String key = keyObj.toString().trim(); if (key.startsWith(prefix)) { final String name = key.substring(preLength); final Object value = entry.getValue(); if (value != null) { // a Level may be followed by a list of Appender refs. - final String valueStr = value.toString(); + final String valueStr = value.toString().trim(); final String[] split = valueStr.split(COMMA_DELIMITED_RE); final String level = getLevelString(split, null); if (level == null) { @@ -429,7 +429,8 @@ public class Log4j1ConfigurationParser { private String getProperty(final String key) { final String value = properties.getProperty(key); - return OptionConverter.substVars(value, properties); + final String substVars = OptionConverter.substVars(value, properties); + return substVars == null ? null : substVars.trim(); } private String getProperty(final String key, final String defaultValue) { diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/config/Log4j1ConfigurationFactoryTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/config/Log4j1ConfigurationFactoryTest.java index e2638ec..26c000a 100644 --- a/log4j-1.2-api/src/test/java/org/apache/log4j/config/Log4j1ConfigurationFactoryTest.java +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/config/Log4j1ConfigurationFactoryTest.java @@ -16,21 +16,25 @@ */ package org.apache.log4j.config; +import static org.junit.Assert.fail; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.io.Serializable; import java.net.URISyntaxException; import java.net.URL; import org.apache.log4j.layout.Log4j1XmlLayout; import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.core.Appender; import org.apache.logging.log4j.core.Layout; import org.apache.logging.log4j.core.appender.ConsoleAppender; import org.apache.logging.log4j.core.appender.ConsoleAppender.Target; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.LoggerConfig; +import org.apache.logging.log4j.core.layout.PatternLayout; import org.junit.Test; public class Log4j1ConfigurationFactoryTest extends AbstractLog4j1ConfigurationTest { @@ -150,4 +154,23 @@ public class Log4j1ConfigurationFactoryTest extends AbstractLog4j1ConfigurationT public void testDefaultValues() throws Exception { super.testDefaultValues(); } + + @Test + public void testUntrimmedValues() throws Exception { + try { + final Configuration config = getConfiguration("config-1.2/log4j-untrimmed"); + final LoggerConfig rootLogger = config.getRootLogger(); + assertEquals(Level.DEBUG, rootLogger.getLevel()); + final Appender appender = config.getAppender("Console"); + assertTrue(appender instanceof ConsoleAppender); + final Layout<? extends Serializable> layout = appender.getLayout(); + assertTrue(layout instanceof PatternLayout); + assertEquals("%level - %m%n", ((PatternLayout)layout).getConversionPattern()); + // No filter support + config.start(); + config.stop(); + } catch (NoClassDefFoundError e) { + fail(e.getMessage()); + } + } } diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java index 09208cd..0c3ddc0 100644 --- a/log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java @@ -16,6 +16,20 @@ */ package org.apache.log4j.config; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.Serializable; +import java.net.URISyntaxException; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; + import org.apache.log4j.ListAppender; import org.apache.log4j.LogManager; import org.apache.log4j.Logger; @@ -24,26 +38,22 @@ import org.apache.log4j.bridge.FilterAdapter; import org.apache.log4j.spi.LoggingEvent; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.core.Appender; +import org.apache.logging.log4j.core.Filter; +import org.apache.logging.log4j.core.Layout; import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.ConsoleAppender; import org.apache.logging.log4j.core.appender.FileAppender; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationSource; +import org.apache.logging.log4j.core.config.LoggerConfig; import org.apache.logging.log4j.core.filter.CompositeFilter; +import org.apache.logging.log4j.core.filter.DenyAllFilter; import org.apache.logging.log4j.core.filter.Filterable; import org.apache.logging.log4j.core.filter.LevelRangeFilter; +import org.apache.logging.log4j.core.layout.PatternLayout; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.io.File; -import java.io.IOException; -import java.io.InputStream; -import java.net.URISyntaxException; -import java.nio.file.Path; -import java.util.List; -import java.util.Map; - -import static org.junit.jupiter.api.Assertions.*; - /** * Test configuration from Properties. */ @@ -271,4 +281,25 @@ public class PropertiesConfigurationTest extends AbstractLog4j1ConfigurationTest public void testMultipleFilters(final @TempDir Path tmpFolder) throws Exception { super.testMultipleFilters(tmpFolder); } + + @Test + public void testUntrimmedValues() throws Exception { + try { + final Configuration config = getConfiguration("config-1.2/log4j-untrimmed"); + final LoggerConfig rootLogger = config.getRootLogger(); + assertEquals(Level.DEBUG, rootLogger.getLevel()); + final Appender appender = config.getAppender("Console"); + assertTrue(appender instanceof ConsoleAppender); + final Layout<? extends Serializable> layout = appender.getLayout(); + assertTrue(layout instanceof PatternLayout); + assertEquals("%level - %m%n", ((PatternLayout)layout).getConversionPattern()); + final Filter filter = ((Filterable) appender).getFilter(); + assertTrue(filter instanceof DenyAllFilter); + config.start(); + config.stop(); + } catch (NoClassDefFoundError e) { + fail(e.getMessage()); + } + } + } diff --git a/log4j-1.2-api/src/test/resources/config-1.2/log4j-untrimmed.properties b/log4j-1.2-api/src/test/resources/config-1.2/log4j-untrimmed.properties new file mode 100644 index 0000000..66b2fba --- /dev/null +++ b/log4j-1.2-api/src/test/resources/config-1.2/log4j-untrimmed.properties @@ -0,0 +1,28 @@ +# +# 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. +# + +### +# Warning: this file contains INTENTIONAL trailing spaces on all properties +# + +log4j.threshold = INFO + +log4j.appender.Console = org.apache.log4j.ConsoleAppender +log4j.appender.Console.layout = org.apache.log4j.SimpleLayout +log4j.appender.Console.filter.1 = org.apache.log4j.varia.DenyAllFilter + +log4j.rootLogger = DEBUG , Console \ No newline at end of file
