DefaultMergeStrategy - problem merging filters

[JWT007]

DefaultMergeStrategy (Log4j 2.24.1)

In DefaultMergeStrategy#updateFilterNode I think there is a problem when merging two configurations with filters and the target does not yet contain a CompositeFilter. (in the code below this is the elsecase).

private void updateFilterNode(
        final Node target,
        final Node targetChildNode,
        final Node sourceChildNode,
        final PluginManager pluginManager) {
    if (CompositeFilter.class.isAssignableFrom(targetChildNode.getType().getPluginClass())) {
        final Node node = new Node(targetChildNode, sourceChildNode.getName(), sourceChildNode.getType());
        node.getChildren().addAll(sourceChildNode.getChildren());
        node.getAttributes().putAll(sourceChildNode.getAttributes());
        targetChildNode.getChildren().add(node);
    } else {
        final PluginType pluginType = pluginManager.getPluginType(FILTERS);
        final Node filtersNode = new Node(targetChildNode, FILTERS, pluginType);
        final Node node = new Node(filtersNode, sourceChildNode.getName(), sourceChildNode.getType());
        node.getAttributes().putAll(sourceChildNode.getAttributes());
        final List<Node> children = filtersNode.getChildren();
        children.add(targetChildNode);
        children.add(node);
        final List<Node> nodes = target.getChildren();
        nodes.remove(targetChildNode);
        nodes.add(filtersNode);
    }
}

If I am not mistaken, there is a step missing to add the children of the 'sourceChildNode' to the new node.

I think it should be (see line commented with "<==== ADDED").

else {
        final PluginType pluginType = pluginManager.getPluginType(FILTERS);
        final Node filtersNode = new Node(targetChildNode, FILTERS, pluginType);
        final Node node = new Node(filtersNode, sourceChildNode.getName(), sourceChildNode.getType());
        node.getChildren().addAll(sourceChildeNode.getChildren());   // <==== ADDED
        node.getAttributes().putAll(sourceChildNode.getAttributes());
        final List<Node> children = filtersNode.getChildren();
        children.add(targetChildNode);
        children.add(node);
        final List<Node> nodes = target.getChildren();
        nodes.remove(targetChildNode);
        nodes.add(filtersNode);
    }
}

For example if merging a secondary configuration (source) containing the following filter to a base configuration (target) when the target contains a filter (but not a CompositeFilter):

<Configuration>
  <DynamicThresholdFilter key="loginId" defaultThreshold="ERROR"> 
    <KeyValuePair key="alice" value="DEBUG"/> 
    <KeyValuePair key="bob" value="INFO"/> 
  </DynamicThresholdFilter>
</Configuration>

I believe all the KeyValuePair children would be dropped during merge with the current code.

In addition, the current code does not account for the possibilty of the sourceChildNode being a CompositeFilter which would result in nested composite-filters. (this applies to both the ifand elsebranches of the original code.

<Configuration>
  <CompositeFilter>
     <Filter1/>
     <Filter2/>
  </CompositeFilter>
</Configuration>
<Configuration>
  <CompositeFilter>
     <Filter3/>
     <Filter4/>
  </CompositeFilter>
</Configuration>

Would result in:

<Configuration>
  <CompositeFilter>
     <Filter1/>
     <Filter2/>
     <CompositeFilter>
       <Filter3/>
       <Filter4/>
    </CompositeFilter>
  </CompositeFilter>
</Configuration>

But should probably be:

<Configuration>
  <CompositeFilter>
     <Filter1/>
     <Filter2/>
     <Filter3/>
     <Filter4/>
  </CompositeFilter>
</Configuration>

[JWT007]

@ppkarwasz - Hi Piotr, can you maybe take a quick look at the mergeConfigurations implementation and see if you also maybe see a similar problem as described in this issue with merging Scripts?

The problem I think that exists is that the name attribute on a Script/ScriptRef/ScriptFile is optional. So if for whatever reason, multiple scripts with no name exist, they will continue to overwrite each other during merge so that only the last unnamed script is retained.

switch (toRootLowerCase(targetChildNode.getName())) {
    case PROPERTIES:
    case SCRIPTS:
    case APPENDERS: {
        for (final Node node : sourceChildNode.getChildren()) {
            for (final Node targetNode : targetChildNode.getChildren()) {
                if (Objects.equals(  // <=== two unnamed scripts (NAME == null) would match and remove the existing
                        targetNode.getAttributes().get(NAME),
                        node.getAttributes().get(NAME))) {
                    targetChildNode.getChildren().remove(targetNode);
                    break;
                }
            }
            targetChildNode.getChildren().add(node);
        }
        isMerged = true;
        break;
    }

If you agree that this might be problematic I can create a separate bug for that... but since name is optional I am not sure how the merge strategy should work - or how those scripts are even referenced? (why is name optional? :) )

[ppkarwasz]

@JWT007,

Thank you for stress-testing the DefaultMergeStrategy. I is a publicized, but not commonly used feature of Log4j Core.

If I am not mistaken, there is a step missing to add the children of the 'sourceChildNode' to the new node.

It appears so. In general the class lacks some helper methods that would make the code easier to read, like cloneNode, getChildByName, isAssignableFrom and so on.

In addition, the current code does not account for the possibilty of the sourceChildNode being a CompositeFilter which would result in nested composite-filters. (this applies to both the if and else branches of the original code).

Filter composition is probably something that should be discussed separately, you can post a thread on the dev@logging mailing list about it.
The filtering three-valued logic is a little bit ad hoc and I am not sure you can get a reasonable result by just appending the filters from multiple configurations.

Some initial thoughts:

• If there is a filter that never returns NEUTRAL, it must be unique and must be the last filter of a pipeline. Users should be able to override such a filter.
• Filters that accept NEUTRAL and another result should probably be sorted. E.g. first the ALLOW filters, then the DENY filters.

The problem I think that exists is that the name attribute on a Script/ScriptRef/ScriptFile is optional. So if for whatever reason, multiple scripts with no name exist, they will continue to overwrite each other during merge so that only the last unnamed script is retained.

Scripts without a name in the global <Scripts> container are useless, since you can not reference them. They should probably be just removed during the merge procedure.
I also opened #3176 to add some validation code to the ScriptsPlugin, so that users are warned about such a condition.

[JWT007]

Thanks @ppkarwasz,

Unfortunately I feel I lack the deeper understanding of the filter logic to lead such a discussion. :) I am decent at finding "holes" in a code-review but the implications and caveats of filter compisition are not all known to me.

My concern about Scripts with no name is probably moot.

All scripts with a null-name (missing a name attriubute) are assigned a default name in toString() of AbstractScript constructor - see comments on #3176 .

[JWT007]

@ppkarwasz

One other thing I noticed in my testing, the DefaultMergeStrategy uses the root node of the first configuration to merge all the others - this results in the first configuration being modified (regardless of type).

For example, if the first is a XmlConffiguration and a second XmlConfiguration is merged - the internal node structure of the first is modified.

I was wondering why, when a "reconfigure" is started on a CompositeConfiguration, isn't a new empty root-node used as the target for the merges?

[ppkarwasz]

One other thing I noticed in my testing, the DefaultMergeStrategy uses the root node of the first configuration to merge all the others - this results in the first configuration being modified (regardless of type).

For example, if the first is a XmlConffiguration and a second XmlConfiguration is merged - the internal node structure of the first is modified.

I was wondering why, when a "reconfigure" is started on a CompositeConfiguration, isn't a new empty root-node used as the target for the merges?

That it probably a mistake, the target should be the root node of the CompositeConfiguration.

[JWT007]

I had to work around it myself by creating a custom AbstractConfiguration implementation with the prepared empty child nodes for Loggers, Appenders, etc. and ensure that is always first in my CompositeConfigurations.

Like the BuiltConfiguration it breaks down if you don't do a minimum prepopulation of child nodes / components - i.e. BuiltConfiguration NPEs if you don't prepopulate the root component with all child components.