Builder definitions in final Appender implementations unncessarily use Generics on Builder class

[JWT007]

Log4j 2.24

Not technically a bug??? But not a feature-request either...just pointing it out...if no interest in changing this from L4J team go ahead and close it

The appender Builders (ConsoleAppender.Builder, FileAppender.Builder, etc) have been generically defiined.

For example:

    /**
     * Builds ConsoleAppender instances.
     * @param <B> The type to build
     */
    public static class Builder<B extends Builder<B>> 
            extends AbstractOutputStreamAppender.Builder<B>
            implements org.apache.logging.log4j.core.util.Builder<ConsoleAppender> {
            ...
    }

This means when not chaining you need to handle the generics:

ConsoleAppender.Builder<?> builder = ConsoleAppender.newBuilder();

This is OK for the abstract Builders that are inherited, but a wee bit non-standard for concrete builder implementations.

Since the appender classes are final one might assume its builders can also be final and don't need the generics on the class itself.

I think it could be simplified::

For example ConsoleAppender:

  public static final class Builder 
      extends AbstractOutputStreamAppender.Builder<Builder> 
      implements org.apache.logging.log4j.core.util.Builder<ConsoleAppender>

ConsoleAppender.Builder builder = ConsoleAppender.newBuilder();

From a coding perspective a trivial change but more of a binary compatibility problem.

[ppkarwasz]

From a coding perspective a trivial change but more of a binary compatibility problem.

Removing the type variable B should be binary compatible (the erasure of B is Builder, so the descriptor of the methods remains the same). The only problem would be adding the final modifier to the Builder class (and removing the public constructor).

It is probably OK to do that. @vy, what do you think?

[vy]

I would expect there to be more compatibility implications. Nevertheless, these classes should never be accessed programmatically — that is how we advise users in docs too. Hence, I am fine with making appender builders final and removing their type param. Op ma 6 jan 2025 om 16:12 schreef Piotr P. Karwasz ***@***.***

…

From a coding perspective a trivial change but more of a binary compatibility problem. Removing the type variable B should be binary compatible (the erasure of B is Builder, so the descriptor of the methods remains the same). The only problem would be adding the final modifier to the Builder class (and removing the public constructor). It is probably OK to do that. @vy <https://github.com/vy>, what do you think? — Reply to this email directly, view it on GitHub <#3368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARTSLAK2VIURAKM3LTJY32JKMPBAVCNFSM6AAAAABUVWVDLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNZTGMYTKMRQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[JWT007]

I am surprised to hear that these should never be accessed programmatically?

We have been doing this for years to create dynamic runtime-only appenders and loggers - since Log4j 1.x. Since Log4j 2.x we have a CompositeConfiguration with a nestted custom runtime-configuration for these runtime only service-appenders/loggers.

The new Log4j 2.x docs also clearly state "Log4j Core can be configured programmatically too."

The warning you are referring to is probably this one: "We strongly advise against programmatically modifying components of a configuration! This section will explain what it is, and why you should avoid it." - but this would not be directly relevant to calling the builder methods.

Unfortunately in our case we have an old legacy monolilth which provided admin functions to add/remove/update loggers/appenders - functionality the users don't want to give up :( - and since the Log4j configuration objects cannot be used to recreate an XML file from an existing configuration - we have had to get ... creative. :/

Also the Log4j Plugin concept sort of breaks down in many spots because many other configuration classes are final , have private attributes, or have package-private functionality, (i.e. providing an alternate "BurstFilter" implementation - trivial example).

O_o

[JWT007]

similar scenario on LoggerConfig.Builder but there its a little bit different because of the nested RootLogger... but I think that doesn't need the generics either...

public static class Builder<B extends Builder<B>>
                implements org.apache.logging.log4j.core.util.Builder<LoggerConfig> {

[ppkarwasz]

I am surprised to hear that these should never be accessed programmatically?

The opinions on this matter diverge in the PMC.

We agree that the best way to modify a configuration is to create a new one and replace the old one. This prevents subtle bugs like adding an appender to a running configuration without previously starting it.

We do not agree, which is the best way to create a new configuration:

• @vy prefers to use the Configuration Builder API, which has the advantage of being equivalent to the file configuration. Since we didn't change the configuration format in 3.x, your code will be binary compatible with Log4j Core 3 too. If we remove some configuration attributes in 3.x, your code will still work, although the values of the attributes will be ignored (and cause warnings).
• Personally I prefer to directly instantiate the plugins using the Builder classes, since it allows to provide the attributes in a type safe way instead of using Strings as in the Configuration Builder API. This approach however needs code changes to migrate to Log4j Core 3, e.g. due to Replace withers with setters in main #1206.

The choice is between binary compatibility between major releases and type safety.

[JWT007]

Thanks @ppkarwasz , I don't know how many people are using Log4j via the Builders...

For me, wither/setter doesn't matter to me as long as it works (personally I prefer withers in builders - its feels more "builder" than "bean" :P). But yeah from a purist perspective the configuration builders are currently all over the place - withers/setters, sometimes conversion (ie. String -> Level) must happen before calling builder, sometimes not - some filters have "createNewFilter" others use Builders....

I guess I wouldn't be as critical about binary compatibility between major releases - at least not on a non-public API (core) "edge-case" :). But I am (thankfully) not responsibile for a major logging library .D

[vy]

Unfortunately in our case we have an old legacy monolilth which provided admin functions to add/remove/update loggers/appenders - functionality the users don't want to give up :( - and since the Log4j configuration objects cannot be used to recreate an XML file from an existing configuration - we have had to get ... creative. :/

@JWT007, why do you have to do that by programmatically accessing the components, directly? If you have the initial, say, XML configuration, you can update the XML with the changed appends/loggers, feed it again to Configurator.reconfigure(). This way you will only be manipulating configuration files – the only API Log4j Core supports (besides Log4j API, obviously), AFAICT.

The choice is between binary compatibility between major releases and type safety.

@ppkarwasz, Configuration Builder API (aka. the glorified DOM builder) is as type-safe as XML/YAML/JSON/Properties configurations files. I don't think type-safety is a tie breaker here. If it would be, we would have seen users asking for a type-safe logging configuration, and we don't.

There is life-cycle related magic happening when a component gets realized from a configuration. Programmatically making that happen is walking on a minefield.

I think the problem mostly emanates from a disagreement between maintainers on what is the API boundary for Log4j Core. For sure everyone agrees that the configuration file format is one such API – we shouldn't break it. What else? Shall we make FileAppender public too? Some users, even though they represent a tiny fraction of the total, will certainly find that helpful. But why do we stop there? What about FileManager, AbstractManager, and OutputStreamManager too? Those are handy too! My point is, if we want to accept FileAppender as a programmatically consumable public API of Log4j Core, we need to involve dozens and dozens more auxiliary support classes into that batch. This not only explodes the maintenance responsibility, severely hinders the evolution of the project. Mistakes will not be able to decently get corrected due to backward compatibility concerns. Since Log4j was invented at a time JPMS did not exist, now we treat everything public in Log4j Core as an API surface. It is at such a bad level, some maintainers even insist we should include test modules (e.g., log4j-core-test) in that group too. Can you pause for a second an try to grasp the API surface supported by 2 F/OSS developers in their spare time?

In overall, I don't know if Log4j 3, 4, 5, etc. will support org.apache.logging.log4j.core.util.Builder. Though I know they all will support the XML configuration file you have today.

[JWT007]

Hi @vy ,

as I mentioned, we have a few things going on.

Here I will try and provide some rough details but it is not "relevant" to this ticket directly.

1. we are using a CompositeConfigurration - we have a sort of framework based on WAR overlays and our distribution can be extended in projects - including the Log4j configuration. (So some XML comes from the WARs (read-only), some is dynamic (no XML), and finally the last is quasi the "editable" external-configuration).

2. In our CompositeConfiguration hierarchy, we have a runtime-only (no XML) configuration which generates service-appenders/loggers on-the-fly if a service is added/removed.

3. We need to present the original configuration (as-is) to our client UI - which we cannot extract from a constructed Configuration object - hidden fields, converted attributes, etc. We also need to provide the merged view - i.e. performing the DefaultMergeStrategy behaviour on the "XML" configuration instead of the runtime configuration.

---

I am a big fan of fluent APIs - the ConfigurationBuilder is OK but unwieldy for a lot of content or for dynamic changes.

Also, it doesn't really document the behaviour very well in some cases:

For example, this will throw NPEs (trying to represent an undefined onResult/onMismatchResult).

var filterBuilder =
        configBuilder.newFilter("BurstFilter",
                                (Filter.Result) null,
                                (Filter.Result) null);

...but this will probably not - (but it will put null attrribute values in the backing map - don't know if that causes problems downstream):

var filterBuilder =
        configBuilder.newFilter("BurstFilter",
                                (String) null,
                                (String) null);

Of course, this would be easier if I could take any configuration X and use it to re-generate the XML representation at runtime - but that unfortunately is not possible.

[vy]

@JWT007, if I understand you right, you have

1. an XML configuration bundled with the deployment unit (JAR, WAR, etc.)
2. an XML configuration containing user-provided customizations (quasi the "editable" external-configuration)
3. a configuration containing admin UI appenders/loggers

You're combining first three into the bootstrap configuration using CompositeConfiguration, later on programmatically changing this with new loggers/appenders through your admin UI, right? Wouldn't it be possible to construct your admin UI configuration using ConfigurationBuilder, and create a new CompositeConfiguration containing all three?

[ppkarwasz]

I think the problem mostly emanates from a disagreement between maintainers on what is the API boundary for Log4j Core. For sure everyone agrees that the configuration file format is one such API – we shouldn't break it. What else? Shall we make FileAppender public too? Some users, even though they represent a tiny fraction of the total, will certainly find that helpful. But why do we stop there? What about FileManager, AbstractManager, and OutputStreamManager too? Those are handy too! My point is, if we want to accept FileAppender as a programmatically consumable public API of Log4j Core, we need to involve dozens and dozens more auxiliary support classes into that batch. This not only explodes the maintenance responsibility, severely hinders the evolution of the project. Mistakes will not be able to decently get corrected due to backward compatibility concerns. Since Log4j was invented at a time JPMS did not exist, now we treat everything public in Log4j Core as an API surface. It is at such a bad level, some maintainers even insist we should include test modules (e.g., log4j-core-test) in that group too. Can you pause for a second an try to grasp the API surface supported by 2 F/OSS developers in their spare time?

Basically we don't consider those classes as part of the API, but we don't introduce breaking changes in it either. 😉
Sure, if we were to write Log4j 2 today, most of the Log4j plugins (FileAppender, etc.) would be internal classes.

For example, this will throw NPEs (trying to represent an undefined onResult/onMismatchResult).

var filterBuilder =
        configBuilder.newFilter("BurstFilter",
                                (Filter.Result) null,
                                (Filter.Result) null);

This is a known bug (#2791 and LOG4J2-3453). In general the Configuration Builder API has many bugs such as this, because it is rarely used. Its main usage is the Java properties configuration format that nobody on the dev team uses.
Thanks for auditing it.