apache
DefaultConfigurationBuilder - is not final but cannot be practically extended due to (in)visibility of private fields
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<T extends BuiltConfiguration> implements ConfigurationBuilder<T> {
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<T> 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<T> clazz) {
if (clazz == null) {
throw new IllegalArgumentException("A Configuration class must be provided");
}
this.clazz = clazz;
final List<Component> 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<T> 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<T> 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<T> 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.
@ppkarwasz - Hi Piotr, I am working on a PR for this - but I have run into a problem.
I added some convenience methods to the ConfigurationBuilder interface and this is now being reported as a Major change by the baseline plugin.
[ERROR] * org.apache.logging.log4j.core.config.builder.api PACKAGE MAJOR 2.25.0 2.20.1 3.0.0 2.21.0
[ERROR] MAJOR PACKAGE org.apache.logging.log4j.core.config.builder.api
[ERROR] MAJOR INTERFACE org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] ADDED METHOD addProperty(org.apache.logging.log4j.core.config.builder.api.PropertyComponentBuilder)
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED RETURN org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] ADDED METHOD setMonitorInterval(int)
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED RETURN org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] ADDED METHOD setShutdownTimeout(java.lang.String,java.util.concurrent.TimeUnit)
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED RETURN org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] ADDED METHOD setShutdownTimeout(long)
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED RETURN org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] ADDED METHOD setStatusLevel(java.lang.String)
[ERROR] ADDED ACCESS abstract
[ERROR] ADDED RETURN org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilder
[ERROR] MICRO ANNOTATED org.osgi.annotation.versioning.Version
[ERROR] REMOVED PROPERTY value='2.20.1'
[ERROR] ADDED PROPERTY value='2.25.0'
[ERROR] REMOVED VERSION 2.20.1
[ERROR] ADDED VERSION 2.25.0
The only "in-library" implementation of this interface is the DefaultConfigurationBuilder which of course I have updated; however (although unlikely) there could be an implementation of this interface in the wild.
In the BUILDING.adoc it said to talk to the development team... :)
Hi @JWT007,
I added some convenience methods to the
ConfigurationBuilderinterface and this is now being reported as a Major change by the baseline plugin.
It is a MAJOR change because the methods are abstract. You can fix it by:
- adding those methods as
defaultmethods, which is the less invasive change. - or marking the
ConfigurationBuilderclass using the@ProviderType. This means that the type is not meant to be implemented by most users (the "Consumers"), which will not suffer from the change. Only "Providers" will need to update their code. See@ConsumerType/@ProviderType
Hi @ppkarwasz
ok thanks! I will take a look - defaults don' seem sensible in that case but I will look at the annotations. :)
Am fighting with some problems with some flaky tests (unrelated to my changes)... all sort of the same scenario where collection sizes don't match. I don't know if the tests are running too fast?
[ERROR] Failures:
[ERROR] SyslogAppenderCustomLayoutTest>SyslogAppenderTest.testUDPAppender:79->SyslogAppenderTestBase.sendAndCheckLegacyBsdMessage:74->SyslogAppenderTestBase.checkTheNumberOfSentAndReceivedMessages:112 The number of received messages should be equal with the number of sent messages ==> expected: <1> but was: <0>
[ERROR] SyslogAppenderCustomLayoutTest>SyslogAppenderTest.testUDPStructuredAppender:88->SyslogAppenderTestBase.sendAndCheckStructuredMessage:93->SyslogAppenderTestBase.checkTheNumberOfSentAndReceivedMessages:112 The number of received messages should be equal with the number of sent messages ==> expected: <1> but was: <0>
[ERROR] TlsSyslogAppenderTest>SyslogAppenderTest.testTCPAppender:56->SyslogAppenderTestBase.sendAndCheckLegacyBsdMessage:74->SyslogAppenderTestBase.checkTheNumberOfSentAndReceivedMessages:112 The number of received messages should be equal with the number of sent messages ==> expected: <1> but was: <0>
[ERROR] RollingAppenderDeleteAccumulatedSizeTest.testAppender:64 [target\rolling-with-delete-accum-size\test\test-10.log, target\rolling-with-delete-accum-size\test\test-6.log, target\rolling-with-delete-accum-size\test\test-7.log, target\rolling-with-delete-accum-size\test\test-8.log, target\rolling-with-delete-accum-size\test\test-9.log] expected:<4> but was:<5>
[ERROR] RollingAppenderDirectCronTest.testAppender(LoggerContext, RollingFileAppender)
Hi @ppkarwasz,
I created a PR - hopefully everything correct.
Tried to avoid any binary incompatilitty with the exception of the Interface changes mentioned above.
There were some shifts in behavior and design - see PR - please review carefully and let me know if changes need to be made.