You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/02/18 11:33:04 UTC
[GitHub] [logging-log4j2] kennymacleod opened a new pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
kennymacleod opened a new pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761
This change will make `PropertiesConfiguration` tolerant of attempts to resolve a non-existent Appender. See JIRA ticket for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] kennymacleod commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
kennymacleod commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r809919399
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
##########
@@ -436,6 +436,10 @@ public Appender parseAppender(final Properties props, final String appenderName)
final String layoutPrefix = prefix + ".layout";
final String filterPrefix = APPENDER_PREFIX + appenderName + ".filter.";
final String className = OptionConverter.findAndSubst(prefix, props);
+ if (className == null) {
+ LOGGER.debug("Appender \"" + appenderName + "\" does not exist.");
+ return null;
+ }
Review comment:
Without this null-check, there will be a NPE from the next line down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] kennymacleod commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
kennymacleod commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r809919901
##########
File path: log4j-1.2-api/src/test/java/org/apache/log4j/config/PropertiesConfigurationTest.java
##########
@@ -125,6 +125,15 @@ public void testConsoleAppenderLevelRangeFilter() throws Exception {
}
}
+ @Test
+ public void testConfigureAppenderDoesNotExist() throws Exception {
+ // Verify that we tolerate a logger which specifies an appender that does not exist.
+ try (LoggerContext loggerContext = TestConfigurator.configure("target/test-classes/LOG4J2-3407.properties")) {
+ final Configuration configuration = loggerContext.getConfiguration();
+ assertNotNull(configuration);
+ }
Review comment:
This test fails with an NPE without the fix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory merged pull request #761: LOG4J2-3407 Log4j 1.2 bridge Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
garydgregory merged pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] kerysgang937 commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
kerysgang937 commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r810068934
##########
File path: log4j-1.2-api/src/test/resources/LOG4J2-3407.properties
##########
@@ -0,0 +1,26 @@
+#
+# 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.
+#
+
+log4j.appender.CONSOLE=org.apache.log4j.ConsoleAppender
+log4j.appender.CONSOLE.filter.1=org.apache.log4j.config.NeutralFilterFixture
+log4j.appender.CONSOLE.filter.1.onMatch=neutral
+log4j.appender.CONSOLE.Target=System.out
+log4j.appender.CONSOLE.layout=org.apache.log4j.PatternLayout
+log4j.appender.CONSOLE.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
+
+log4j.logger.org.apache.log4j.xml=trace, A1
+log4j.rootLogger=trace, CONSOLE
Review comment:
Kerstintdurden:release-2.2Ftownship.onelink.me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] kerysgang937 commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
kerysgang937 commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r810069282
##########
File path: log4j-1.2-api/src/test/resources/LOG4J2-3407.properties
##########
@@ -0,0 +1,26 @@
+#
+# 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.
+#
+
+log4j.appender.CONSOLE=org.apache.log4j.ConsoleAppender
+log4j.appender.CONSOLE.filter.1=org.apache.log4j.config.NeutralFilterFixture
+log4j.appender.CONSOLE.filter.1.onMatch=neutral
+log4j.appender.CONSOLE.Target=System.out
+log4j.appender.CONSOLE.layout=org.apache.log4j.PatternLayout
+log4j.appender.CONSOLE.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
+
+log4j.logger.org.apache.log4j.xml=trace, A1
+log4j.rootLogger=trace, CONSOLE
Review comment:
log4j-1.2-api/src/test/resources/LOG4J2-3407.properties
##########
File path: log4j-1.2-api/src/test/resources/LOG4J2-3407.properties
##########
@@ -0,0 +1,26 @@
+#
+# 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.
+#
+
+log4j.appender.CONSOLE=org.apache.log4j.ConsoleAppender
+log4j.appender.CONSOLE.filter.1=org.apache.log4j.config.NeutralFilterFixture
+log4j.appender.CONSOLE.filter.1.onMatch=neutral
+log4j.appender.CONSOLE.Target=System.out
+log4j.appender.CONSOLE.layout=org.apache.log4j.PatternLayout
+log4j.appender.CONSOLE.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
+
+log4j.logger.org.apache.log4j.xml=trace, A1
+log4j.rootLogger=trace, CONSOLE
Review comment:
Kerstintdurden:release-2.2Ftownship.onelink.me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] kennymacleod commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
kennymacleod commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r809919607
##########
File path: log4j-1.2-api/src/test/resources/LOG4J2-3407.properties
##########
@@ -0,0 +1,26 @@
+#
+# 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.
+#
+
+log4j.appender.CONSOLE=org.apache.log4j.ConsoleAppender
+log4j.appender.CONSOLE.filter.1=org.apache.log4j.config.NeutralFilterFixture
+log4j.appender.CONSOLE.filter.1.onMatch=neutral
+log4j.appender.CONSOLE.Target=System.out
+log4j.appender.CONSOLE.layout=org.apache.log4j.PatternLayout
+log4j.appender.CONSOLE.layout.ConversionPattern=%d{yyyy-MM-dd HH:mm:ss} %-5p %c{1}:%L - %m%n
+
+log4j.logger.org.apache.log4j.xml=trace, A1
Review comment:
Here we have a logger referring to an appender (A1) that does not exist
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r810286544
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
##########
@@ -436,6 +436,10 @@ public Appender parseAppender(final Properties props, final String appenderName)
final String layoutPrefix = prefix + ".layout";
final String filterPrefix = APPENDER_PREFIX + appenderName + ".filter.";
final String className = OptionConverter.findAndSubst(prefix, props);
+ if (className == null) {
+ LOGGER.debug("Appender \"" + appenderName + "\" does not exist.");
+ return null;
+ }
Review comment:
I would move this behavior to the `BuilderManager` (so we solve the problem at the source). The `BuilderManager` should return `new AppenderWrapper(null)` so that line 445 does not throw.
Remark also that Log4j 1.x behaves differently
- whenever **any** component has an empty property the error "Could not find value for key ..." is logged (cf. [source code](https://github.com/apache/logging-log4j1/blob/v1_2_17/src/main/java/org/apache/log4j/helpers/OptionConverter.java#L120).
- then there is a message "Could not instantiate appender named ..." (cf. [source code](https://github.com/apache/logging-log4j1/blob/de9f0ea504db9d1178db2cf45323d8e182a5df0a/src/main/java/org/apache/log4j/PropertyConfigurator.java#L790)). This should probably be checked just before the return statement. Remark that appender instantiation can also fail if some other properties are missing (e.g. `File` for the file appender).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] ppkarwasz commented on a change in pull request #761: LOG4J2-3407 Check for non-existent appender when parsing properties
Posted by GitBox <gi...@apache.org>.
ppkarwasz commented on a change in pull request #761:
URL: https://github.com/apache/logging-log4j2/pull/761#discussion_r810286544
##########
File path: log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java
##########
@@ -436,6 +436,10 @@ public Appender parseAppender(final Properties props, final String appenderName)
final String layoutPrefix = prefix + ".layout";
final String filterPrefix = APPENDER_PREFIX + appenderName + ".filter.";
final String className = OptionConverter.findAndSubst(prefix, props);
+ if (className == null) {
+ LOGGER.debug("Appender \"" + appenderName + "\" does not exist.");
+ return null;
+ }
Review comment:
Remark also that Log4j 1.x behaves differently
- whenever **any** component has an empty property the error "Could not find value for key ..." is logged (cf. [source code](https://github.com/apache/logging-log4j1/blob/v1_2_17/src/main/java/org/apache/log4j/helpers/OptionConverter.java#L120).
- then there is a message "Could not instantiate appender named ..." (cf. [source code](https://github.com/apache/logging-log4j1/blob/de9f0ea504db9d1178db2cf45323d8e182a5df0a/src/main/java/org/apache/log4j/PropertyConfigurator.java#L790)). This should probably be checked just before the return statement. Remark that appender instantiation can also fail if some other properties are missing (e.g. `File` for the file appender).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org