Bug Fix: Script Resolution fix

[Suvrat1629]

#3336
Refactored the ScriptFilter class to enhance script resolution and error handling. This update ensures that ScriptRef scripts are resolved properly using the ScriptManager. Improved logging for scenarios where scripts fail to execute or resolve.

Checklist

• .Base your changes on 2.xbranch if you are targeting Log4j 2; usemain` otherwise
• ./mvnw verify succeeds (if it fails due to code formatting issues reported by Spotless, simply run ./mvnw spotless:apply and retry)
• Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
• Tests for the changes are provided
• Commits are signed (optional, but highly recommended)

[ppkarwasz]

@Suvrat1629,

Thank you for the PR.

Unfortunately it does not solve #3336, since we have a lot of elements that use scripting (ScriptFilter was just an example):

• ScriptPatternSelector
• Routes
• ScriptFilter
• ScriptAppenderSelector
• ScriptCondition
• ScriptArbiter
• RoutingAppender

To fix the resolution of scripts in all of these, I think we should take a different approach: we could alter the order in which the children of a <Configuration> element are processed. Right now we process them in order of appearance, with a single exception: the <Properties> element must be processed first to allow the values of the remaining elements to be properly interpolated:

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

Lines 668 to 688 in 6e3f2e2

	for (final Node node : rootNode.getChildren()) {
	if ("Properties".equalsIgnoreCase(node.getName())) {
	hasProperties = true;
	createConfiguration(node, null);
	if (node.getObject() != null) {
	final StrLookup lookup = node.getObject();
	runtimeStrSubstitutor.setVariableResolver(lookup);
	configurationStrSubstitutor.setVariableResolver(lookup);
	}
	break;
	}
	}
	if (!hasProperties) {
	final Map<String, String> map = this.getComponent(CONTEXT_PROPERTIES);
	final StrLookup lookup = map == null ? null : new PropertiesLookup(map);
	final Interpolator interpolator = new Interpolator(lookup, pluginPackages);
	interpolator.setConfiguration(this);
	interpolator.setLoggerContext(loggerContext.get());
	runtimeStrSubstitutor.setVariableResolver(interpolator);
	configurationStrSubstitutor.setVariableResolver(interpolator);
	}

We should probably process the <Scripts> element just after <Properties>.

Note: the ScriptArbiter should probably be processed differently. Arbiters are special plugins that modify the structure of the configuration file before it is processed. Therefore the ScriptArbiter should probably just signal an ERROR (or throw), if it contains a ScriptRef. At the stage, when the arbiter is used, the Scripts element is not yet processed.

[ppkarwasz]

so should i just revert back the changes i made to the ScriptFilter.java file and and make changes to the processRootNode method in the AbstractConfiguration and update the logic?

The AbstractConfiguration.doConfigure() method seems to be a better choice.

The logic would be:

1. Process first, as it currently does.
2. Process script-related components (ScriptPatternSelector, Routes, ScriptFilter, etc.) after to ensure their dependencies are resolved.
3. Process all remaining components afterward.

It is hard to identify the script-related components. It is easier to make sure that the Scripts container is processed at the beginning, so that the scripts are already registered, when ScriptFilter and company are instantiated. This means:

1. Process the nodes of type Arbiter to modify the configuration tree. This is what doConfigure() already does.
2. Process the "Properties" plugin first, as the current code does. If the "Properties" plugin is not present, the current code uses something else to initialize the interpolator.
3. Process the "Scripts" plugin next, to register all the scripts.
4. Process the remaining components (skipping "Properties" and "Scripts").

is this how i should keep my approach?

It is one of the possible approaches, probably the easiest to implement.

[Suvrat1629]

i have made the necessary fix to the code please review the code.

[ppkarwasz]

@Suvrat1629,

Please review the files you are committing before you commit them. A lot of garbage/temporary files made their way into your PR.

Also make sure to push the minimal amount of changes, e.g. configure you IDE not to reformat comments or the lines you didn't modify.

In this project we use the Palantir Java Formatter, which is available as IntelliJ IDEA plugin, but you can also run it using:

./mvnw spotless:apply

Regarding Java imports, we don't use wildcard (*) imports and we follow the order:

all `import static` statements
<empty line>
all `import` statements

The changes to AbstractConfiguration look almost OK, but the unit test does not currently compile.

[Suvrat1629]

I have tried to fix the changes you have asked me and keep the pr as clean as possible however for the test cases I am not so sure because I lack knowledge about testing but if there are anymore further changes please let me know.

Thank you.