Correct script resolution order dependency (#3718)

Alters AbstractConfiguration to process the Scripts element before other components. This allows ScriptRef elements to be resolved correctly, regardless of their order in the configuration file.

Co-authored-by: Volkan Yazıcı <[email protected]>

Author: jhl221123

log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/AbstractConfigurationTest.java

@@ -24,14 +24,19 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.filter.CompositeFilter;
+import org.apache.logging.log4j.core.filter.ScriptFilter;
 import org.apache.logging.log4j.core.lookup.Interpolator;
 import org.apache.logging.log4j.core.lookup.InterpolatorTest;
 import org.apache.logging.log4j.core.lookup.StrSubstitutor;
+import org.apache.logging.log4j.core.test.junit.LoggerContextSource;
+import org.apache.logging.log4j.test.junit.SetTestProperty;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.junitpioneer.jupiter.Issue;
 
+@SetTestProperty(key = "log4j2.scriptEnableLanguages", value = "groovy")
 class AbstractConfigurationTest {
 
     @Test
@@ -60,6 +65,26 @@ void substitutorHasConfigurationAndLoggerContext(final boolean hasProperties) {
         }
     }
 
+    @Test
+    @LoggerContextSource("log4j2-script-order-test.xml")
+    void scriptRefShouldBeResolvedWhenScriptsElementIsLast(final Configuration config) {
+        assertThat(config.getFilter())
+                .as("Top-level filter should be a CompositeFilter")
+                .isInstanceOf(CompositeFilter.class);
+        final CompositeFilter compositeFilter = (CompositeFilter) config.getFilter();
+
+        assertThat(compositeFilter.getFilters())
+                .as("CompositeFilter should contain one filter")
+                .hasSize(1);
+        final ScriptFilter scriptFilter =
+                (ScriptFilter) compositeFilter.getFilters().get(0);
+
+        assertThat(scriptFilter).isNotNull();
+        assertThat(scriptFilter.toString())
+                .as("Script name should match the one in the config")
+                .isEqualTo("GLOBAL_FILTER");
+    }
+
     private static class TestConfiguration extends AbstractConfiguration {
 
         private final Map<String, String> map;

log4j-core-test/src/test/resources/log4j2-script-order-test.xml

@@ -0,0 +1,31 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  ~ 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.
+  -->
+<Configuration status="WARN">
+    <Filters>
+        <ScriptFilter onMatch="ACCEPT" onMisMatch="DENY">
+            <ScriptRef ref="GLOBAL_FILTER"/>
+        </ScriptFilter>
+    </Filters>
+
+    <Scripts>
+        <Script name="GLOBAL_FILTER" language="groovy"><![CDATA[
+        // simple script
+        return true
+    ]]></Script>
+    </Scripts>
+</Configuration>

log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java

@@ -682,13 +682,14 @@ protected void doConfigure() {
         preConfigure(rootNode);
         configurationScheduler.start();
         // Find the "Properties" node first
+        final List<Node> children = rootNode.getChildren();
         boolean hasProperties = false;
-        for (final Node node : rootNode.getChildren()) {
-            if ("Properties".equalsIgnoreCase(node.getName())) {
+        for (final Node child : children) {
+            if ("Properties".equalsIgnoreCase(child.getName())) {
                 hasProperties = true;
-                createConfiguration(node, null);
-                if (node.getObject() != null) {
-                    final StrLookup lookup = node.getObject();
+                createConfiguration(child, null);
+                if (child.getObject() != null) {
+                    final StrLookup lookup = child.getObject();
                     runtimeStrSubstitutor.setVariableResolver(lookup);
                     configurationStrSubstitutor.setVariableResolver(lookup);
                 }
@@ -705,28 +706,36 @@ protected void doConfigure() {
             configurationStrSubstitutor.setVariableResolver(interpolator);
         }
 
+        for (final Node child : children) {
+            if ("Scripts".equalsIgnoreCase(child.getName())) {
+                createConfiguration(child, null);
+                if (child.getObject() != null) {
+                    for (final AbstractScript script : child.getObject(AbstractScript[].class)) {
+                        if (script instanceof ScriptRef) {
+                            LOGGER.error(
+                                    "Script reference to {} not added. Scripts definition cannot contain script references",
+                                    script.getName());
+                        } else if (scriptManager != null) {
+                            scriptManager.addScript(script);
+                        }
+                    }
+                }
+                break;
+            }
+        }
+
         boolean setLoggers = false;
         boolean setRoot = false;
-        for (final Node child : rootNode.getChildren()) {
-            if ("Properties".equalsIgnoreCase(child.getName())) {
+        for (final Node child : children) {
+            if ("Properties".equalsIgnoreCase(child.getName()) || "Scripts".equalsIgnoreCase(child.getName())) {
                 // We already used this node
                 continue;
             }
             createConfiguration(child, null);
             if (child.getObject() == null) {
                 continue;
             }
-            if ("Scripts".equalsIgnoreCase(child.getName())) {
-                for (final AbstractScript script : child.getObject(AbstractScript[].class)) {
-                    if (script instanceof ScriptRef) {
-                        LOGGER.error(
-                                "Script reference to {} not added. Scripts definition cannot contain script references",
-                                script.getName());
-                    } else if (scriptManager != null) {
-                        scriptManager.addScript(script);
-                    }
-                }
-            } else if ("Appenders".equalsIgnoreCase(child.getName())) {
+            if ("Appenders".equalsIgnoreCase(child.getName())) {
                 appenders = child.getObject();
             } else if (child.isInstanceOf(Filter.class)) {
                 addFilter(child.getObject(Filter.class));

src/changelog/.2.x.x/3336_script_resolution_order_fix.xml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xsi:schemaLocation="
+           https://logging.apache.org/xml/ns
+           https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+    <issue id="3336" link="https://github.com/apache/logging-log4j2/issues/3336"/>
+    <description format="asciidoc">
+        Fix script resolution failure when the `Scripts` element is placed after a `ScriptRef` in the configuration.
+    </description>
+</entry>