CompositeConfiguration merges into the root node of the first configuration instead of using its own root-node

[JWT007]

Log4j 2.24..3

The composite configuration uses the root-node of the first configuration in the provided list as the basis for the merge of all configurations.

        rootNode = configurations.get(0).getRootNode();
       ...
        for (final AbstractConfiguration config : configurations) {
            mergeStrategy.mergeRootProperties(rootNode, config);
        }

As @ppkarwasz mentioned in the comments of (#3173) this is an error because it changes the original source configuration instead of populating the composite.

The fix would be to use the composite configuration's rootNode (protected access via 'AbstractConfiguration'). The rootNode is created empty by the super-constructor.

        for (final AbstractConfiguration config : configurations) {
            mergeStrategy.mergeRootProperties(rootNode, config);
        }

[JWT007]

@ppkarwasz / @rgoers - I am ready to create a PR for this, but I noticed something else - if you agree I will adjust the description and include the change in the PR.

---

In the current CompositeConfiguration#reconfigure() it currently relies on directly trying to reinstantiate the child configurations via the ConfigurationSource URI.

@Override
public Configuration reconfigure() {
    LOGGER.debug("Reconfiguring composite configuration");
    final List<AbstractConfiguration> configs = new ArrayList<>();
    final ConfigurationFactory factory = ConfigurationFactory.getInstance();
    for (final AbstractConfiguration config : configurations) {
        final ConfigurationSource source = config.getConfigurationSource();
        final URI sourceURI = source.getURI();
        Configuration currentConfig = config;
        if (sourceURI == null) {
            LOGGER.warn(
                    "Unable to determine URI for configuration {}, changes to it will be ignored",
                    config.getName());
        } else {
            currentConfig = factory.getConfiguration(getLoggerContext(), config.getName(), sourceURI);
            if (currentConfig == null) {
                LOGGER.warn("Unable to reload configuration {}, changes to it will be ignored", config.getName());
             }
        }
        configs.add((AbstractConfiguration) currentConfig);
    }
    return new CompositeConfiguration(configs);
}

This will not work for custom Configurations that cannot be instantiated from the ConfigurationFactory(i.e. not the defaults from Log4j / custom extensions).

I was wondering if taking advantage of the Reconfigurable interface might be a more consistent approach for the reconfigure() design?

@Override
public Configuration reconfigure() {
    LOGGER.debug("Reconfiguring composite configuration");
    final List<AbstractConfiguration> configs = new ArrayList<>();
    final ConfigurationFactory factory = ConfigurationFactory.getInstance();
    for (final AbstractConfiguration config : configurations) {
        Configuration currentConfig = config;
        if (currentConfig instanceof Reconfigurable) {
            try {
                Configuration reloadedConfiguration = ((Reconfigurable) currentConfig).reconfigure();
                if (reloadedConfiguration != null) {
                   currentConfig = reloadedConfiguration;
                } else {
                    LOGGER.warn("Unable to reload configuration {}, changes to it will be ignored", config.getName());
                    continue; // continue loop so that null configuration doesn't get added
                }
            } catch (final Exception ex) {
                LOGGER.error("Unable to reconfigure configuration {}, changes to it will be ignored.",
                             config.getName(), ex);
             }
        }

        configs.add((AbstractConfiguration) currentConfig);
    }

    return new CompositeConfiguration(configs);
}

One more thing - the original and my suggestion above both drop the original configuration from the new Configuration if the reconfigure fails - this results in the configurattion (and its configuration source) being lost.

For example if at runtime an XML configuration file is unavailable or locked for some reason, it will fail the reconfigure and be removed from the composite - a future reconfigure will not attempt to recover. Should the original configuration be kept as-is in this case?

IMHO it would generally be better if the 'Reconfigurable' interface never expected null to be returned - for example the XmlConfigurattion returns an empty configuration that keeps its ConfigurationSource instead of null - then it can be added to the composite and attempted again in the next reconfigure.

Please give me a short feedback so I can finish up and submit a PR. :) Thanks!

@rgoers (I included you because I think the original code is from you?)