ramanathan1504 commented on code in PR #4329: URL: https://github.com/apache/logging-log4j2/pull/4329#discussion_r4066384994
########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/CompositeConfigurationPostConfigureTest.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.logging.log4j.core.config; + +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; + +import java.io.ByteArrayInputStream; +import java.nio.charset.StandardCharsets; +import java.util.Arrays; +import org.apache.logging.log4j.core.Appender; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.appender.NullAppender; +import org.apache.logging.log4j.core.config.composite.CompositeConfiguration; +import org.apache.logging.log4j.core.config.xml.XmlConfiguration; +import org.junit.Test; Review Comment: Can we use JUnit 5 here? `org.junit.jupiter.api.Test` and `Assertions`, with the message as the last argument. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java: ########## @@ -141,6 +141,18 @@ public void setup() { } } + @Override + protected void doConfigure() { + super.doConfigure(); + // The node-level merge above cannot capture elements a child's doConfigure() adds programmatically, + // so give each contributing configuration a chance to apply them to the merged configuration. This + // runs on the initial build and, because reconfigure() rebuilds a CompositeConfiguration, on every + // reconfiguration as well. Review Comment: Same here. The Javadoc on `postConfigure` covers it, remove the comments. ```suggestion ``` ########## log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java: ########## @@ -801,6 +801,25 @@ protected void doConfigure() { setParents(); } + /** + * Invoked by a {@link org.apache.logging.log4j.core.config.composite.CompositeConfiguration} after it has + * configured the merged node tree, to let this contributing configuration add programmatic elements + * (appenders, loggers, filters) to the effective {@code target} configuration. + * <p> + * A {@link CompositeConfiguration} merges the child configurations at the node level and runs its own + * {@link #doConfigure()} on the merged tree; it never calls a child's {@code doConfigure()}. As a result any + * element a custom {@code Configuration} adds programmatically in its {@code doConfigure()} override is lost + * under a composite configuration, on both the initial build and every reconfiguration. Such a + * configuration overrides this method to re-apply those elements to {@code target}. The default is a no-op. + * </p> + * + * @param target the effective (composite) configuration to contribute to + * @since 2.27.0 + */ + public void postConfigure(final Configuration target) { + // no-op by default; a custom Configuration overrides this to contribute programmatic elements. Review Comment: The Javadoc already says it is a no-op. Not needed, remove the comment. ```suggestion ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
