You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by pk...@apache.org on 2022/03/31 19:43:32 UTC
[logging-log4j2] 03/15: Improves behavior of failing Log4j 1.x builders
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 dcd648e0b8afd579ed91627add456b2cfdeb278e
Author: Piotr P. Karwasz <pi...@karwasz.org>
AuthorDate: Mon Feb 21 08:07:30 2022 +0100
Improves behavior of failing Log4j 1.x builders
If a Builder fails due to a missing property, it return `null`. This
activates the fallback _"instantiate by class name"_ mechanism that will
fail with `NoClassDefFoundException` in the best scenario or will
instantiate a native Log4j 1.x component in the worst scenario.
A better approach, IMHO, is to an empty wrapper, which deactivates the
fallback mechanism, but will be unwrapped to `null` soon after and
removed from the configuration.
This PR also increases the level of status logger messages to `ERROR` if
the condition prevents the instantiation of the component.
Conflicts:
log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java
---
.../org/apache/log4j/builders/BuilderManager.java | 48 ++++++++++++++-----
.../builders/appender/AsyncAppenderBuilder.java | 8 +++-
.../appender/DailyRollingFileAppenderBuilder.java | 2 +-
.../builders/appender/FileAppenderBuilder.java | 2 +-
.../builders/appender/RewriteAppenderBuilder.java | 12 +++--
.../appender/RollingFileAppenderBuilder.java | 2 +-
.../builders/filter/StringMatchFilterBuilder.java | 11 ++++-
.../log4j/config/PropertiesConfiguration.java | 5 +-
.../apache/log4j/builders/BuilderManagerTest.java | 54 ++++++++++++++++++++++
9 files changed, 122 insertions(+), 22 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 b524dfe..249ca94 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
@@ -18,6 +18,10 @@ package org.apache.log4j.builders;
import org.apache.log4j.Appender;
import org.apache.log4j.Layout;
+import org.apache.log4j.bridge.AppenderWrapper;
+import org.apache.log4j.bridge.FilterWrapper;
+import org.apache.log4j.bridge.LayoutWrapper;
+import org.apache.log4j.bridge.RewritePolicyWrapper;
import org.apache.log4j.builders.appender.AppenderBuilder;
import org.apache.log4j.builders.filter.FilterBuilder;
import org.apache.log4j.builders.layout.LayoutBuilder;
@@ -48,6 +52,10 @@ public class BuilderManager {
/** Plugin category. */
public static final String CATEGORY = "Log4j Builder";
+ public static final Appender INVALID_APPENDER = new AppenderWrapper(null);
+ public static final Filter INVALID_FILTER = new FilterWrapper(null);
+ public static final Layout INVALID_LAYOUT = new LayoutWrapper(null);
+ public static final RewritePolicy INVALID_REWRITE_POLICY = new RewritePolicyWrapper(null);
public static final Key<PluginManager> PLUGIN_MANAGER_KEY = new @Named(CATEGORY) Key<>() {};
private static final Logger LOGGER = StatusLogger.getLogger();
private static final Class<?>[] CONSTRUCTOR_PARAMS = new Class[] { String.class, Properties.class };
@@ -96,41 +104,59 @@ public class BuilderManager {
return (PluginType<T>) pluginType;
}
- private <T extends Builder<U>, U> U newInstance(final PluginType<T> plugin, final Function<T, U> consumer) {
+ private <T extends Builder<U>, U> U newInstance(final PluginType<T> plugin, final Function<T, U> consumer,
+ final U invalidValue) {
if (plugin != null) {
final T builder = injector.getInstance(plugin.getPluginClass());
if (builder != null) {
- return consumer.apply(builder);
+ final U result = consumer.apply(builder);
+ // returning an empty wrapper is short for "we support this legacy class, but it has validation errors"
+ return result != null ? result : invalidValue;
}
}
return null;
}
- public <P extends Parser<T>, T> T parse(final String className, final String prefix, final Properties props, final PropertiesConfiguration config) {
+ public <P extends Parser<T>, T> T parse(final String className, final String prefix, final Properties props,
+ final PropertiesConfiguration config, T invalidValue) {
final P parser = createBuilder(getPlugin(className), prefix, props);
- return parser != null ? parser.parse(config) : null;
+ if (parser != null) {
+ final T value = parser.parse(config);
+ return value != null ? value : invalidValue;
+ }
+ return null;
}
- public Appender parseAppender(final String className, final Element appenderElement, final XmlConfiguration config) {
- return newInstance(this.<AppenderBuilder<Appender>>getPlugin(className), b -> b.parseAppender(appenderElement, config));
+ public Appender parseAppender(final String className, final Element appenderElement,
+ final XmlConfiguration config) {
+ return newInstance(this.<AppenderBuilder<Appender>>getPlugin(className),
+ b -> b.parseAppender(appenderElement, config), INVALID_APPENDER);
}
public Appender parseAppender(final String name, final String className, final String prefix, final String layoutPrefix, final String filterPrefix,
final Properties props, final PropertiesConfiguration config) {
final AppenderBuilder<Appender> builder = createBuilder(getPlugin(className), prefix, props);
- return builder != null ? builder.parseAppender(name, prefix, layoutPrefix, filterPrefix, props, config) : null;
+ if (builder != null) {
+ final Appender appender = builder.parseAppender(name, prefix, layoutPrefix, filterPrefix, props, config);
+ return appender != null ? appender : INVALID_APPENDER;
+ }
+ return null;
}
public Filter parseFilter(final String className, final Element filterElement, final XmlConfiguration config) {
- return newInstance(this.<FilterBuilder>getPlugin(className), b -> b.parse(filterElement, config));
+ return newInstance(this.<FilterBuilder>getPlugin(className), b -> b.parse(filterElement, config),
+ INVALID_FILTER);
}
public Layout parseLayout(final String className, final Element layoutElement, final XmlConfiguration config) {
- return newInstance(this.<LayoutBuilder>getPlugin(className), b -> b.parse(layoutElement, config));
+ return newInstance(this.<LayoutBuilder>getPlugin(className), b -> b.parse(layoutElement, config),
+ INVALID_LAYOUT);
}
- public RewritePolicy parseRewritePolicy(final String className, final Element rewriteElement, final XmlConfiguration config) {
- return newInstance(this.<RewritePolicyBuilder>getPlugin(className), b -> b.parse(rewriteElement, config));
+ public RewritePolicy parseRewritePolicy(final String className, final Element rewriteElement,
+ final XmlConfiguration config) {
+ return newInstance(this.<RewritePolicyBuilder>getPlugin(className), b -> b.parse(rewriteElement, config),
+ INVALID_REWRITE_POLICY);
}
}
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java
index 1801dfa..5aece66 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java
@@ -118,12 +118,12 @@ public class AsyncAppenderBuilder extends AbstractBuilder implements AppenderBui
final String level = getProperty(THRESHOLD_PARAM);
final int bufferSize = getIntegerProperty(BUFFER_SIZE_PARAM, 1024);
if (appenderRef == null) {
- LOGGER.warn("No appender references configured for AsyncAppender {}", name);
+ LOGGER.error("No appender references configured for AsyncAppender {}", name);
return null;
}
final Appender appender = configuration.parseAppender(props, appenderRef);
if (appender == null) {
- LOGGER.warn("Cannot locate Appender {}", appenderRef);
+ LOGGER.error("Cannot locate Appender {}", appenderRef);
return null;
}
return createAppender(name, level, new String[]{appenderRef}, blocking, bufferSize, includeLocation, filter,
@@ -133,6 +133,10 @@ public class AsyncAppenderBuilder extends AbstractBuilder implements AppenderBui
private <T extends Log4j1Configuration> Appender createAppender(final String name, final String level,
final String[] appenderRefs, final boolean blocking, final int bufferSize, final boolean includeLocation,
final Filter filter, final T configuration) {
+ if (appenderRefs.length == 0) {
+ LOGGER.error("No appender references configured for AsyncAppender {}", name);
+ return null;
+ }
final org.apache.logging.log4j.Level logLevel = OptionConverter.convertLevel(level,
org.apache.logging.log4j.Level.TRACE);
final AppenderRef[] refs = new AppenderRef[appenderRefs.length];
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java
index a0dacac..c0704a1 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java
@@ -152,7 +152,7 @@ public class DailyRollingFileAppenderBuilder extends AbstractBuilder implements
}
final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter);
if (fileName == null) {
- LOGGER.warn("Unable to create File Appender, no file name provided");
+ LOGGER.error("Unable to create DailyRollingFileAppender, no file name provided");
return null;
}
final String filePattern = fileName + "%d{" + datePattern + "}";
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java
index 0327218..65bd489 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java
@@ -136,7 +136,7 @@ public class FileAppenderBuilder extends AbstractBuilder implements AppenderBuil
}
final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter);
if (fileName == null) {
- LOGGER.warn("Unable to create File Appender, no file name provided");
+ LOGGER.error("Unable to create FileAppender, no file name provided");
return null;
}
return new AppenderWrapper(FileAppender.newBuilder()
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java
index d8b61ba..9e1b3b5 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java
@@ -33,6 +33,7 @@ import org.apache.log4j.bridge.AppenderWrapper;
import org.apache.log4j.bridge.RewritePolicyAdapter;
import org.apache.log4j.bridge.RewritePolicyWrapper;
import org.apache.log4j.builders.AbstractBuilder;
+import org.apache.log4j.builders.BuilderManager;
import org.apache.log4j.config.Log4j1Configuration;
import org.apache.log4j.config.PropertiesConfiguration;
import org.apache.log4j.helpers.OptionConverter;
@@ -105,15 +106,16 @@ public class RewriteAppenderBuilder extends AbstractBuilder implements AppenderB
final Filter filter = configuration.parseAppenderFilters(props, filterPrefix, name);
final String policyPrefix = appenderPrefix + ".rewritePolicy";
final String className = getProperty(policyPrefix);
- final RewritePolicy policy = configuration.getBuilderManager().parse(className, policyPrefix, props, configuration);
+ final RewritePolicy policy = configuration.getBuilderManager()
+ .parse(className, policyPrefix, props, configuration, BuilderManager.INVALID_REWRITE_POLICY);
final String level = getProperty(THRESHOLD_PARAM);
if (appenderRef == null) {
- LOGGER.warn("No appender references configured for AsyncAppender {}", name);
+ LOGGER.error("No appender references configured for RewriteAppender {}", name);
return null;
}
final Appender appender = configuration.parseAppender(props, appenderRef);
if (appender == null) {
- LOGGER.warn("Cannot locate Appender {}", appenderRef);
+ LOGGER.error("Cannot locate Appender {}", appenderRef);
return null;
}
return createAppender(name, level, new String[] {appenderRef}, policy, filter, configuration);
@@ -121,6 +123,10 @@ public class RewriteAppenderBuilder extends AbstractBuilder implements AppenderB
private <T extends Log4j1Configuration> Appender createAppender(final String name, final String level,
final String[] appenderRefs, final RewritePolicy policy, final Filter filter, final T configuration) {
+ if (appenderRefs.length == 0) {
+ LOGGER.error("No appender references configured for RewriteAppender {}", name);
+ return null;
+ }
final org.apache.logging.log4j.Level logLevel = OptionConverter.convertLevel(level,
org.apache.logging.log4j.Level.TRACE);
final AppenderRef[] refs = new AppenderRef[appenderRefs.length];
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java
index 9664756..4cd8bb4 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java
@@ -157,7 +157,7 @@ public class RollingFileAppenderBuilder extends AbstractBuilder implements Appen
}
final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter);
if (fileName == null) {
- LOGGER.warn("Unable to create File Appender, no file name provided");
+ LOGGER.error("Unable to create RollingFileAppender, no file name provided");
return null;
}
final String filePattern = fileName + ".%i";
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java
index 616e9f8..cce3344 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java
@@ -19,6 +19,7 @@ package org.apache.log4j.builders.filter;
import static org.apache.log4j.builders.BuilderManager.CATEGORY;
import static org.apache.log4j.xml.XmlConfiguration.forEachElement;
+import java.util.Properties;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
@@ -43,6 +44,14 @@ public class StringMatchFilterBuilder extends AbstractBuilder<Filter> implements
private static final String STRING_TO_MATCH = "StringToMatch";
private static final String ACCEPT_ON_MATCH = "AcceptOnMatch";
+ public StringMatchFilterBuilder() {
+ super();
+ }
+
+ public StringMatchFilterBuilder(String prefix, Properties props) {
+ super(prefix, props);
+ }
+
@Override
public Filter parse(Element filterElement, XmlConfiguration config) {
final AtomicBoolean acceptOnMatch = new AtomicBoolean();
@@ -72,7 +81,7 @@ public class StringMatchFilterBuilder extends AbstractBuilder<Filter> implements
private Filter createFilter(String text, boolean acceptOnMatch) {
if (text == null) {
- LOGGER.warn("No text provided for StringMatchFilter");
+ LOGGER.error("No text provided for StringMatchFilter");
return null;
}
org.apache.logging.log4j.core.Filter.Result onMatch = acceptOnMatch
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
index 5eb9686..f5774f1 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
@@ -36,6 +36,7 @@ import org.apache.log4j.PatternLayout;
import org.apache.log4j.bridge.AppenderAdapter;
import org.apache.log4j.bridge.AppenderWrapper;
import org.apache.log4j.bridge.FilterAdapter;
+import org.apache.log4j.builders.BuilderManager;
import org.apache.log4j.helpers.OptionConverter;
import org.apache.log4j.spi.ErrorHandler;
import org.apache.log4j.spi.Filter;
@@ -498,7 +499,7 @@ public class PropertiesConfiguration extends Log4j1Configuration {
if (layoutClass == null) {
return null;
}
- Layout layout = manager.parse(layoutClass, layoutPrefix, props, this);
+ Layout layout = manager.parse(layoutClass, layoutPrefix, props, this, BuilderManager.INVALID_LAYOUT);
if (layout == null) {
layout = buildLayout(layoutPrefix, layoutClass, appenderName, props);
}
@@ -574,7 +575,7 @@ public class PropertiesConfiguration extends Log4j1Configuration {
final String clazz = props.getProperty(entry.getKey());
Filter filter = null;
if (clazz != null) {
- filter = manager.parse(clazz, entry.getKey(), props, this);
+ filter = manager.parse(clazz, entry.getKey(), props, this, BuilderManager.INVALID_FILTER);
if (filter == null) {
LOGGER.debug("Filter key: [{}] class: [{}] props: {}", entry.getKey(), clazz, entry.getValue());
filter = buildFilter(clazz, appenderName, entry.getValue());
diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java
new file mode 100644
index 0000000..7203b19
--- /dev/null
+++ b/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java
@@ -0,0 +1,54 @@
+/*
+ * 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.
+ */
+package org.apache.log4j.builders;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.Properties;
+
+import org.apache.log4j.Appender;
+import org.apache.log4j.FileAppender;
+import org.apache.log4j.config.PropertiesConfiguration;
+import org.apache.log4j.spi.Filter;
+import org.apache.log4j.varia.StringMatchFilter;
+import org.apache.logging.log4j.plugins.di.DI;
+import org.junit.jupiter.api.Test;
+
+public class BuilderManagerTest {
+
+ /**
+ * This test ensures that instantiation failures due to missing parameters
+ * always return an empty wrapper instead of null, hence disabling the
+ * <i>"instantiate by classname"</i> fallback mechanism for supported components.
+ */
+ @Test
+ public void testReturnInvalidValueOnError() {
+ final PropertiesConfiguration config = new PropertiesConfiguration(null, null);
+ final BuilderManager manager = new BuilderManager(DI.createInjector());
+ final Properties props = new Properties();
+ props.setProperty("FILE", FileAppender.class.getName());
+ props.setProperty("FILE.filter.1", StringMatchFilter.class.getName());
+ // Parse an invalid StringMatchFilter
+ final Filter filter = manager.parse(StringMatchFilter.class.getName(), "FILE.filter", props, config,
+ BuilderManager.INVALID_FILTER);
+ assertEquals(BuilderManager.INVALID_FILTER, filter);
+ // Parse an invalid FileAppender
+ final Appender appender = manager.parseAppender("FILE", FileAppender.class.getName(), "FILE", "FILE.layout",
+ "FILE.filter.", props, config);
+ assertEquals(BuilderManager.INVALID_APPENDER, appender);
+ }
+}