apache / apache/logging-log4j2

DefaultConfigurationBuilder - is not final but cannot be practically extended due to (in)visibility of private fields

Open
#3,441 4 comments 1 reaction 0 assignees View on GitHub
Dominant language
Java
Stars
3.6k
Forks
1.7k
Avg merge
21h 30m
Merged PRs (30d)
27

Description

Log4j 2.24..3
----
The DefaultConfigurationBuilder is not final but it practically not extensible because access to the root and other components is private.

This implementation could benefit from a bit more adherence to the Open/Closed Principle: - **Open for Extension, Closed for Modification**

```
public class DefaultConfigurationBuilder implements ConfigurationBuilder {

private static final String INDENT = " ";

private final Component root = new Component();
private Component loggers;
private Component appenders;
private Component filters;
private Component properties;
private Component customLevels;
private Component scripts;
private final Class clazz;
private ConfigurationSource source;
private int monitorInterval;
private Level level;
private String destination;
private String packages;
private String shutdownFlag;
private long shutdownTimeoutMillis;
private String advertiser;
private LoggerContext loggerContext;
private String name;
```

Since the components (and other attributes) are private, it is not possible to extend this (for example to add a custom component type) - this would be important because a BuiltConfiguration only gets the root component as an argument in the constructor instantiated by reflection.

It also might be useful if this inittialization of the root component was moved out of the constructor to a protected / overridable method.

```
public DefaultConfigurationBuilder(final Class clazz) {
if (clazz == null) {
throw new IllegalArgumentException("A Configuration class must be provided");
}
this.clazz = clazz;
final List components = root.getComponents();
properties = new Component("Properties");
components.add(properties);
scripts = new Component("Scripts");
components.add(scripts);
customLevels = new Component("CustomLevels");
components.add(customLevels);
filters = new Component("Filters");
components.add(filters);
appenders = new Component("Appenders");
components.add(appenders);
loggers = new Component("Loggers");
components.add(loggers);
}
```

i.e.
* `protected Component getRootComponent()`
* `protected void initializeComponents()`

maybe a method like this would help?

```
protected Component getOrCreateComponent(String name) {
// Retrieve an existing component or create a new one
return this.root.components.computeIfAbsent(name, key -> new Component(key));
}
```

This would significantly simplify the constructor.
```
public DefaultConfigurationBuilder(final Class clazz) {

if (clazz == null) {
throw new IllegalArgumentException("A Configuration class must be provided");
}

this.clazz = clazz;

properties = this.getOrCreateComponent("Properties");
scripts = this.getOrCreateComponent("Scripts");
customLevels = this.getOrCreateComponent("CustomLevels");
filters = this.getOrCreateComponent("Filters);
appenders = this.getOrCreateComponents("Appenders");
loggers = this.getOrCreateComponentts("Loggers");

}
```

Another nice-to-have would be moving the instantation of the Configuratio to a separate method so that a custom constructor could be called.

For example, from:

```
@Override
public T build(final boolean initialize) {
T configuration;
try {
...
final Constructor constructor =
clazz.getConstructor(LoggerContext.class, ConfigurationSource.class, Component.class);
configuration = constructor.newInstance(loggerContext, source, root);
}
...
}
```

```
@Override
public T build(final boolean initialize) {
T configuration;
try {
...
configuration = constructor.newInstance(loggerContext, source, root);
}
...
}

protected T createNewConfiguration() {
final Constructor constructor =
MyCustomClass.getConstructor(LoggerContext.class, ConfigurationSource.class, Component.class, FooBar.class);
return constructor..newInstance(loggerContext, source, root, this.foobar);
}

```
This would allow passing custom information to a custom configuration's constructor.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.